impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Date Fri, 06 Oct 2017 23:22:29 GMT
Philip Zeyliger has posted comments on this change. (

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

Patch Set 1:


I'm excited by anything that makes LLVM debugging easier!
File be/src/codegen/llvm-codegen.h:
PS1, Line 783:   class DiagnosticState {
Based on, 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.
File be/src/codegen/
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.
PS1, Line 304:   if (error || diagnostic_state_.EncounteredError()) {
So do the diagnostic messages get printed anywhere?
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 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
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: Philip Zeyliger <>
Gerrit-Comment-Date: Fri, 06 Oct 2017 23:22:29 +0000
Gerrit-HasComments: Yes

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