You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2017/10/06 22:41:15 UTC
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8233
Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................
IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.
Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.
Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M tests/query_test/test_codegen.py
3 files changed, 67 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/8233/1
--
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: newchange
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 16:56:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@781
PS2, Line 781: /// Ensures that an attempt to read diagnostic state would reset the object.
> Comment is kinda cryptic. Maybe needs some reference to the LLVM Diagnostic
Done
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@784
PS2, Line 784:
> nit: extra blank line.
removed
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@785
PS2, Line 785: DiagnosticHandler() : encountered_error_(false) {}
> Let's initialise the member variables inline.
Removed
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@787
PS2, Line 787: /// Returns the error string if an error was encountered and resets the state in
> Can you give some hint about when a caller should call this? I.e. after an
Done
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@789
PS2, Line 789: string
> std::string in a header (I guess there's a rogue "using std::string" somewh
Done
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@791
PS2, Line 791: /// Handler function that sets the state on an instance of this class
> I think the comments could make the relationship between these two function
Done
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@796
PS2, Line 796: string error_str_;
> Can you briefly document the member variables and the relationship between
removed
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@796
PS2, Line 796: string
> std::string.
Done
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@799
PS2, Line 799: /// Maintains state set by diagnostic handler.
> Comment is kinda cryptic. It may not be necessary to have both a class and
Done
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc@1621
PS2, Line 1621: DiagnosticHandler* diagnostic_handler = reinterpret_cast<DiagnosticHandler*>(context);
> Can context be a nullptr in any scenarios? We should check for that before
I dont think there is any scenario as far as how we are using this handler, will a DCHECK(context != nullptr) suffice instead?
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc@1627
PS2, Line 1627: error_msg.flush();
> I feel like we should also log the error at LOG(INFO) level so that it does
Done
--
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: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 00:05:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
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 <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Oct 2017 23:22:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1339/
--
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: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 01:02:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke 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 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc@1621
PS2, Line 1621: DiagnosticHandler* diagnostic_handler = reinterpret_cast<DiagnosticHandler*>(context);
Can context be a nullptr in any scenarios? We should check for that before dereferencing it.
--
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: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 19:02:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 3:
(1 comment)
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
Done
--
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: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Oct 2017 22:57:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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:
(1 comment)
Agree with Phil's comments, and have a further question.
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@1621
PS1, Line 1621: DiagnosticPrinterRawOStream diagnostic_printer(llvm::errs());
We talked about this interface offline and whether it was possible to get the error as a string. I took a look at the LLVM headers and it looks like raw_string_ostream is a subclass of raw_ostream and could be passed in here, unless I am missing some caveat. It would be nice to bundle this into a Status and propagate it normally.
--
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 <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Oct 2017 17:55:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@308
PS4, Line 308: if (!diagnostic_err.empty()) ss <<" "<< diagnostic_err;
> nit: Do we do spacing around "<<" operators?
Done
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@1625
PS4, Line 1625: diagnostic_printer <<"LLVM diagnostic error: ";
> nit: space around "<<"?
Done
--
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: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Oct 2017 20:29:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
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:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@308
PS3, Line 308: }
nit: This might look like "... to main module.Bla bla." I.e., no space after the period.
You might want to add '<< " "' in here to make it look nicer.
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1635
PS3, Line 1635:
I might be confused about the C++11 stuff, but given that you just std::move'd it, I don't think you need to clear it.
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):
> I tried moving this to llvm-codegen-test.cc but it turns out that a lot ug
I don't know LLVM well enough to suggest how to get at these errors.
That said, LoadModuleFromFile() takes a regular file, I think, and not one on HDFS. You might be able to just cp ./llvm-ir/test-loop.bc to a temporary file to induce this. (Or create a second test-loop that has a link-time conflict: presumably name collisions cause errors?)
--
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 <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 17:43:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@781
PS2, Line 781: /// Ensures that an attempt to read diagnostic state would reset the object.
Comment is kinda cryptic. Maybe needs some reference to the LLVM DiagnosticHandler API - I guess the idea of the class is to implement LLVM's DiagnosticHandler API and save the error information from callbacks.?
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@784
PS2, Line 784:
nit: extra blank line.
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@785
PS2, Line 785: DiagnosticHandler() : encountered_error_(false) {}
Let's initialise the member variables inline.
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@787
PS2, Line 787: /// Returns the error string if an error was encountered and resets the state in
Can you give some hint about when a caller should call this? I.e. after an LLVM API call that can fail but returns error info via this mechanism. We probably want to add a TODO and file a follow-up JIRA to make sure that we're checking for errors everywhere that we need to.
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@789
PS2, Line 789: string
std::string in a header (I guess there's a rogue "using std::string" somewhere in another header).
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@791
PS2, Line 791: /// Handler function that sets the state on an instance of this class
I think the comments could make the relationship between these two functions more obvious. E.g. GetErrorString() returns the last error that was reported via DiagnosticHandlerFn()
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@796
PS2, Line 796: string error_str_;
Can you briefly document the member variables and the relationship between them? Since we return an empty string to indicate an error above, is 'encountered_error_' redundant? I.e. does {true, ""} behave differently to {false, ""}?
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@796
PS2, Line 796: string
std::string.
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.h@799
PS2, Line 799: /// Maintains state set by diagnostic handler.
Comment is kinda cryptic. It may not be necessary to have both a class and member comment since there's only one instance of the class.
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc@1627
PS2, Line 1627: error_msg.flush();
I feel like we should also log the error at LOG(INFO) level so that it doesn't get swallowed silently in places where we're not checking for errors yet. These errors should be rare enough that they won't add too much to the logs.
--
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: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Oct 2017 21:00:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8233 )
Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................
IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.
Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.
Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Reviewed-on: http://gerrit.cloudera.org:8080/8233
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/testutil/gtest-util.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
6 files changed, 93 insertions(+), 0 deletions(-)
Approvals:
Bikramjeet Vig: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1628
PS3, Line 1628: LOG(INFO) << diagnostic_handler->error_str_;
Include the query id for context, which should be accessible via state_? Will be hard to track down where the log message is coming from otherwise.
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1634
PS3, Line 1634: (std::
Should be able to drop the std:: - we import it into the namespace in common/names.h (there is other code in the codebase that is inconsistent about this).
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1635
PS3, Line 1635: error_str_.clear();
> I might be confused about the C++11 stuff, but given that you just std::mov
Yup
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):
> I don't know LLVM well enough to suggest how to get at these errors.
Yeah I think if you directly create a function or global variable with a conflicting name in the module you could induce an error that way.
--
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: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 21:51:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong 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 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@308
PS4, Line 308: if (!diagnostic_err.empty()) ss <<" "<< diagnostic_err;
> nit: Do we do spacing around "<<" operators?
yep
--
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: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Oct 2017 00:04:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8233
to look at the new patch set (#4).
Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................
IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.
Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.
Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/testutil/gtest-util.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
6 files changed, 93 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/8233/4
--
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: newpatchset
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 6: Code-Review+2
Carrying over Tim's +2
--
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: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Oct 2017 20:49:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke 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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/2/be/src/codegen/llvm-codegen.cc@1621
PS2, Line 1621: DiagnosticHandler* diagnostic_handler = reinterpret_cast<DiagnosticHandler*>(context);
> I dont think there is any scenario as far as how we are using this handler,
You are right. I confused this with something I read about LLVMContext usage online. I don't think we need to check for a null here.
--
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: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Oct 2017 21:43:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1339/
--
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: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Tue, 17 Oct 2017 21:03:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1347/
--
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: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 19:10:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
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 4: Code-Review+1
(2 comments)
Thanks!
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@308
PS4, Line 308: if (!diagnostic_err.empty()) ss <<" "<< diagnostic_err;
nit: Do we do spacing around "<<" operators?
http://gerrit.cloudera.org:8080/#/c/8233/4/be/src/codegen/llvm-codegen.cc@1625
PS4, Line 1625: diagnostic_printer <<"LLVM diagnostic error: ";
nit: space around "<<"?
--
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: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Oct 2017 23:28:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8233
to look at the new patch set (#5).
Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................
IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.
Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.
Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/testutil/gtest-util.h
M be/src/util/filesystem-util.cc
M be/src/util/filesystem-util.h
6 files changed, 93 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/8233/5
--
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: newpatchset
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, anujphadke, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8233
to look at the new patch set (#3).
Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................
IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.
Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.
Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M tests/query_test/test_codegen.py
3 files changed, 75 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/8233/3
--
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: newpatchset
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Philip Zeyliger, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8233
to look at the new patch set (#2).
Change subject: IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
......................................................................
IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Currently if llvm encounters an error it calls exit() and this kills
the impalad process. This patch adds a diagnostic handler that llvm
can use to pass the error up to impala instead of crashing it.
Testing:
Added a test which induces an error in llvm and makes sure that its
passed up to impala code and handled correctly.
Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M tests/query_test/test_codegen.py
3 files changed, 72 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/33/8233/2
--
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: newpatchset
Gerrit-Change-Id: I907ff1f8975c02d422b4a669afbfc2e536ddf1ff
Gerrit-Change-Number: 8233
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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 7: Verified+1
--
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: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 23:30:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 3:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@308
PS3, Line 308: if (!diagnostic_err.empty()) ss << diagnostic_err;
> nit: This might look like "... to main module.Bla bla." I.e., no space afte
Done
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1628
PS3, Line 1628: LOG(INFO) << diagnostic_handler->error_str_;
> Include the query id for context, which should be accessible via state_? Wi
Done
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1634
PS3, Line 1634: (std::
> Should be able to drop the std:: - we import it into the namespace in commo
Done
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1635
PS3, Line 1635: error_str_.clear();
> I might be confused about the C++11 stuff, but given that you just std::mov
Done
http://gerrit.cloudera.org:8080/#/c/8233/3/be/src/codegen/llvm-codegen.cc@1635
PS3, Line 1635: error_str_.clear();
> Yup
Done
--
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: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Mon, 16 Oct 2017 22:56:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6002: Add a LLVM diagnostic handler for LLVM linker errors
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
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 7: Code-Review+2
Rebasing.
Carrying over +2.
--
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: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Oct 2017 19:10:21 +0000
Gerrit-HasComments: No