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