impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Date Tue, 10 Oct 2017 16:56:14 GMT
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8233 )

Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................


Patch Set 1:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.h@783
PS1, Line 783:   class DiagnosticState {
> Based on http://llvm.org/doxygen/DiagnosticHandler_8h_source.html, it looks
that DiagnosticHandler was only added recently (20 days ago) and is not available in the llvm
version we use. currently the only way is to set a handler callback function.


http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.cc@186
PS1, Line 186:       &this->diagnostic_state_, true);
> Could you comment on what this third argument does?
Thanks for highlighting this. There are some cases where the diagnostic handler will not be
called for some remarks based on filters set using commandline flags.

I had initially set this to true to receive all diagnostic messages regardless of filters,
but since we are only interested in errors at the moment, we can do away with not receiving
those remarks. hence I will revert this back to the default value of false.


http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.cc@304
PS1, Line 304:   if (error || diagnostic_state_.EncounteredError()) {
> So do the diagnostic messages get printed anywhere?
Done, also removed this condition from the if statement because it seems that error is true
(Linker::linkModules returns true) if a diagnostic handler is set and an error is encountered


http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.cc@1621
PS1, Line 1621:     DiagnosticPrinterRawOStream diagnostic_printer(llvm::errs());
> We talked about this interface offline and whether it was possible to get t
Done


http://gerrit.cloudera.org:8080/#/c/8233/1/be/src/codegen/llvm-codegen.cc@1622
PS1, Line 1622:     info.print(diagnostic_printer);
> Does this make it to our logs?
Done


http://gerrit.cloudera.org:8080/#/c/8233/1/tests/query_test/test_codegen.py
File tests/query_test/test_codegen.py:

http://gerrit.cloudera.org:8080/#/c/8233/1/tests/query_test/test_codegen.py@45
PS1, Line 45:   def test_codegen_diagnostic_handler(self, vector):
> You could probably test this in be/src/codegen/llvm-codegen-test.cc with co
I tried moving this to llvm-codegen-test.cc but it turns out  that a lot ugly code needs to
be added to induce a linkage error. I had 2 options there:
1) add a Gtest that calles LlvmCodeGen::LoadFunction twice on the same lib, but I need 2 different
names to the same file to induce an error because there are checks to see if a file has already
been linked. I am not sure I can do hdfs calls to create a copy of a lib through the backend
test.

2) Since the methods related to this were private to LlvmCodegen class, I wrote a Test method
in LlvmCodeGenTest to load module from a lib twice and call LLVM linker directly to those
2. but it turns out, that this adds alot of ugle code to the class LlvmCodeGenTest.

I think another alternative would be to figure out how we can induce another kind of error
through a simpler way, but I'll have to look more into it. would really appreciate any suggestions
here.
Thanks



-- 
To view, visit http://gerrit.cloudera.org:8080/8233
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 16:56:14 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message