Philip Zeyliger 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: (5 comments) I'm excited by anything that makes LLVM debugging easier! 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 like they recommend us subclassing their class to implement this, and that there are different "remarks" that we can get. It's tricky, but if they've got different warnings, I sure as heck would want to see them during development. 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? The llvm header says "expects enabled diagnostics", which I don't understand. 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? 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? 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 considerably less fanfare. I think the end-to-end test that we don't fail on a custom UDF is lovely, but a more targeted test makes sense too. -- 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 <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Fri, 06 Oct 2017 23:22:29 +0000 Gerrit-HasComments: Yes
