impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <>
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. ( )

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

Patch Set 1:

File be/src/codegen/llvm-codegen.h:
PS1, Line 783:   class DiagnosticState {
> Based on, 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.
File be/src/codegen/
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.
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
PS1, Line 1621:     DiagnosticPrinterRawOStream diagnostic_printer(llvm::errs());
> We talked about this interface offline and whether it was possible to get t
PS1, Line 1622:     info.print(diagnostic_printer);
> Does this make it to our logs?
File tests/query_test/
PS1, Line 45:   def test_codegen_diagnostic_handler(self, vector):
> You could probably test this in be/src/codegen/ with co
I tried moving this to 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

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

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Bikramjeet Vig <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Tue, 10 Oct 2017 16:56:14 +0000
Gerrit-HasComments: Yes

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