-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cxxmodules] Print stacktrace before aborting on a missing exception. #921
[cxxmodules] Print stacktrace before aborting on a missing exception. #921
Conversation
Starting build on |
Build failed on centos7/gcc49. Errors:
And 2 more |
Build failed on slc6/gcc49. Errors:
And 2 more |
Build failed on slc6/gcc62. Errors:
And 1 more |
Build failed on mac1012/native. Errors:
|
Build failed on ubuntu14/native. Errors:
And 3 more |
319602a
to
66f4691
Compare
Starting build on |
Build failed on slc6/gcc49. Errors:
|
Build failed on slc6/gcc62. Errors:
|
@phsft-bot build |
Starting build on |
@phsft-bot build with flags -Dclingtest=On |
Starting build on |
Build failed on mac1012/native. Failing tests: |
66f4691
to
8b434f2
Compare
Starting build on |
Build failed on mac1012/native. Failing tests: |
Build failed on slc6/gcc49. Errors:
|
8b434f2
to
25bc54b
Compare
Starting build on |
Build failed on mac1012/native. Failing tests: |
The failing count include test is also failing in master, so this is not a regression: #962 (comment) |
@phsft-bot build with flags -Dclingtest=Off let's make sure it's not breaking master |
Starting build on |
Build failed on centos7/gcc49. Failing tests: |
Build failed on slc6/gcc62. Failing tests: |
|
||
/// \brief Asserts that the given transaction is not null, otherwise prints a | ||
/// stack trace to stderr. | ||
static void assertHasTransaction(const cling::Transaction* transaction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using just T as a variable name is a common notion for variable names of type cling::Transaction.
/// \brief Asserts that the given transaction is not null, otherwise prints a | ||
/// stack trace to stderr. | ||
static void assertHasTransaction(const cling::Transaction* transaction) { | ||
if (transaction == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more common pattern would be if (!T)
static void assertHasTransaction(const cling::Transaction* transaction) { | ||
if (transaction == nullptr) { | ||
llvm::sys::PrintStackTrace(llvm::errs()); | ||
assert(false && "Missing transaction during deserialization!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use llvm_unreachable here. If so we will need to rename the routine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code was using assert
. For me this looks like a poster child of assert
: validating a precondition is met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to some rules llvm_unreachable
is the preferred primitive. I do not have a strong opinion here, but the closer to the llvm coding guidelines we are in cling, the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points, Vassil. Thanks for explaining!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
static void assertHasTransaction(const cling::Transaction* transaction) { | ||
if (transaction == nullptr) { | ||
llvm::sys::PrintStackTrace(llvm::errs()); | ||
assert(false && "Missing transaction during deserialization!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code was using assert
. For me this looks like a poster child of assert
: validating a precondition is met.
964acb8
to
cec6da6
Compare
Added do not merge as Jenkins is restating so this only looks green. |
@phsft-bot build EDIT: Jenkins is dead... |
@phsft-bot build |
1 similar comment
@phsft-bot build |
Starting build on |
Build failed on mac1012/native. Failing tests: |
Build failed on slc6/gcc49. Failing tests: |
Build failed on slc6/gcc62. Failing tests: |
cec6da6
to
4d884e9
Compare
Starting build on |
We will probably see an increasing amount of these failures with C++ modules as we now deserialize all declarations instead of just the PCH ones. To safe us a lot of debugging time on where to push the needed transaction, let's directly print the stack trace here in the rare case that we crash here.
We will probably see an increasing amount of these failures with
C++ modules as we now deserialize all declarations instead of just
the PCH ones. To safe us a lot of debugging time on where to push
the needed transaction, let's directly print the stack trace here
in the rare case that we crash here.