You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/03/08 18:20:52 UTC

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error 

Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6318

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................

IMPALA-4957: Don't crash Impalad on LLVM fatal error

Whenever LLVM hits a fatal error condition, it would
call the registered callback LlvmCodegenHandleError()
which used to log the reason leading to the fatal
error and then trigger a core dump. The old behavior
is not desirable as codegen failure shouldn't lead
to Impalad crashing.

With this change, LlvmCodegenHandleError() will throw
an exception instead. a new try-catch block is also
added in LlvmCodeGen::FinalizeModule() to catch any
exceptions thrown and wind back the stack. This allows
fatal codegen error (e.g. unresolved symbols) to not
crash Impala. Instead, it just fails the query.

Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
2 files changed, 48 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/6318/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS4, Line 111:  const std::string& reason,
> Couldn't 'reason' be a reference to something allocated on the stack betwee
Yes, that doesn't seem safe.  throw(new std::string(reason)) should fix that.

Also, might want to whitelist / blacklist these strings to map to safely recoverable errors.


Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
> Good point. That's a reasonable compromise for now. I suppose we should swi
There's always longjmp()


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/6318/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS2, Line 110: [[noreturn]] 
> As far as I understand, yes.
Thanks - make sense then


http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS4, Line 111:  const std::string& reason,
Couldn't 'reason' be a reference to something allocated on the stack between this stack frame and the try/catch stack from? It doesn't seem valid to throw it directly.


Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
I'm looking at the LLVM docs and it doesn't seem like there's any guarantee that LLVM will be in a "sane" state when calling the error handler - LLVM seems to assume that the handler won't throw and it can call exit(1) after it returns:

   install_fatal_error_handler - Installs a new error handler to be used whenever a serious (non-recoverable) error is encountered by LLVM.

Is it worth the risks of trying to continue after we've got into a state that LLVM thinks is non-recoverable? I don't think LLVM code is generally written to be exception-safe, since their coding standard forbids exceptions.


Line 1026:       DCHECK(std::find(materialized_functions_.begin(), materialized_functions_.end(),
I'm concerned the N^2 loop could add to test execution time. Can we avoid that somehow? E.g. build a temporary set of the function names #ifndef NDEBUG.


Line 1094:   } catch (std::string& reason) {
Consider creating a custom exception class instead of throwing a std::string.


Line 1095:     module_->dump();
Where does the dump() output go to? Would be good to comment.


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 2:

(1 comment)

Yes, working on a backend test for it now. I validated this change by intentionally not materializing functions and I verified the output in Impalad.INFO and Impalad.ERROR.

http://gerrit.cloudera.org:8080/#/c/6318/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS2, Line 110: [[noreturn]] 
> Is noreturn valid for functions that throw??
As far as I understand, yes.

http://en.cppreference.com/w/cpp/language/attributes:

Indicates that the function does not return. 
This attribute applies to function declarations only. The behavior is undefined if the function with this attribute actually returns. 
The following standard functions have this attribute: std::_Exit, std::abort, std::exit, std::quick_exit, std::unexpected, std::terminate, std::rethrow_exception, std::throw_with_nested, std::nested_exception::rethrow_nested


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 2:

(1 comment)

Were you able to reproduce the problem? Would be good to have some tests since it seems like this would be easy to regress.

http://gerrit.cloudera.org:8080/#/c/6318/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS2, Line 110: [[noreturn]] 
Is noreturn valid for functions that throw??


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
> Good point. That's a reasonable compromise for now. I suppose we should swi
Alternately, we may just consider running LLVM in a separate process if it's hard to make recovery work.


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
> I think you're right in many cases, but they could possible have cleanup lo
Good point. That's a reasonable compromise for now. I suppose we should switch to using black-listing in the long run so as to handle more unexpected error cases.


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 5:

Where does this change stand? I think we decided this isn't safe to do, right? Should we abandon it or do you have a plan?

-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
> And even if LLVM is not in a sane state, I believe it would still allow a u
I think you're right in many cases, but they could possible have cleanup logic that we'd bypass by throwing an exception.

One example I found is WriteBitcodeToFile(), which stack-allocates a BitstreamWriter object, which has some assertions in the destructor that would be violated if it did an early exit. One of it's callees calls       report_fatal_error("Unexpected anonymous function when writing summary");

I agree in some situations that attempting recovery is better than crashing Impala. In some situations though it may be better to fail hard and restart Impala to get to a known-good state.

One compromise would be to have a white list of errors where we feel reasonably confident it's safe to attempt recovery from.


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 1:

May consider backporting this to 5.11

-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

Added a new test.

-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
> I'm looking at the LLVM docs and it doesn't seem like there's any guarantee
It's good that you raised this concern. Isn't most if not all states confined to the LLVM module though ? Are there particular things which may be not be sane after the fatal error ?

I will try to do an audit of all callsites for report_fatal_error().

A high level comment is that we should aim to make LLVM's error recoverable (e.g. malloc failure) or it'll become a major hinderance for Impala's reliability in the long run.


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 148:   llvm::install_fatal_error_handler(LlvmCodegenHandleError);
> It's good that you raised this concern. Isn't most if not all states confin
And even if LLVM is not in a sane state, I believe it would still allow a user to continue using Impala with codegen disabled. So, it's better than crashing IMHO.


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

PS4, Line 111:  const std::string& reason,
> Yes, that doesn't seem safe.  throw(new std::string(reason)) should fix tha
The throw statement should copy-initialize the string according to http://en.cppreference.com/w/cpp/language/throw

That said, I took the suggestion to create a custom class.


Line 1026:       DCHECK(std::find(materialized_functions_.begin(), materialized_functions_.end(),
> I'm concerned the N^2 loop could add to test execution time. Can we avoid t
Done


Line 1072:   try {
> this all needs some explanation.
This block is the most prone to triggering fatal error. The bitcode reader can also fail too. May be worth adding a try catch block there too.


Line 1094:   } catch (std::string& reason) {
> Consider creating a custom exception class instead of throwing a std::strin
Done


Line 1095:     module_->dump();
> Where does the dump() output go to? Would be good to comment.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#4).

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................

IMPALA-4957: Don't crash Impalad on LLVM fatal error

Whenever LLVM hits a fatal error condition, it would
call the registered callback LlvmCodegenHandleError()
which used to log the reason leading to the fatal
error and then trigger a core dump. The old behavior
is not desirable as codegen failure shouldn't lead
to Impalad crashing.

With this change, LlvmCodegenHandleError() will throw
an exception instead. A new try-catch block is also
added in LlvmCodeGen::FinalizeModule() to catch any
exceptions thrown and wind back the stack. This allows
fatal codegen error (e.g. unresolved symbols) to not
crash Impala. Instead, it just fails the query.

Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M common/thrift/generate_error_codes.py
4 files changed, 85 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/6318/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................

IMPALA-4957: Don't crash Impalad on LLVM fatal error

Whenever LLVM hits a fatal error condition, it would
call the registered callback LlvmCodegenHandleError()
which used to log the reason leading to the fatal
error and then trigger a core dump. The old behavior
is not desirable as codegen failure shouldn't lead
to Impalad crashing.

With this change, LlvmCodegenHandleError() will throw
an exception instead. a new try-catch block is also
added in LlvmCodeGen::FinalizeModule() to catch any
exceptions thrown and wind back the stack. This allows
fatal codegen error (e.g. unresolved symbols) to not
crash Impala. Instead, it just fails the query.

Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
2 files changed, 48 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/6318/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#3).

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................

IMPALA-4957: Don't crash Impalad on LLVM fatal error

Whenever LLVM hits a fatal error condition, it would
call the registered callback LlvmCodegenHandleError()
which used to log the reason leading to the fatal
error and then trigger a core dump. The old behavior
is not desirable as codegen failure shouldn't lead
to Impalad crashing.

With this change, LlvmCodegenHandleError() will throw
an exception instead. a new try-catch block is also
added in LlvmCodeGen::FinalizeModule() to catch any
exceptions thrown and wind back the stack. This allows
fatal codegen error (e.g. unresolved symbols) to not
crash Impala. Instead, it just fails the query.

Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
2 files changed, 48 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/6318/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6318/4/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 1072:   try {
this all needs some explanation.
and are these the only llvm calls that can result in the fatal error being raised?


-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#5).

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................

IMPALA-4957: Don't crash Impalad on LLVM fatal error

Whenever LLVM hits a fatal error condition, it would
call the registered callback LlvmCodegenHandleError()
which used to log the reason leading to the fatal
error and then trigger a core dump. The old behavior
is not desirable as codegen failure shouldn't lead
to Impalad crashing.

With this change, LlvmCodegenHandleError() will throw
an exception instead. A new try-catch block is also
added in LlvmCodeGen::FinalizeModule() to catch any
exceptions thrown and wind back the stack. This allows
fatal codegen error (e.g. unresolved symbols) to not
crash Impala. Instead, it just fails the query.

Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
---
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M common/thrift/generate_error_codes.py
4 files changed, 112 insertions(+), 36 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/6318/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-4957: Don't crash Impalad on LLVM fatal error

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has abandoned this change.

Change subject: IMPALA-4957: Don't crash Impalad on LLVM fatal error
......................................................................


Abandoned

Abandoned until we come up with a better approach.

-- 
To view, visit http://gerrit.cloudera.org:8080/6318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I54706e261ed223eadde347b1184fb0102e03a3d6
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>