-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: print backtrace on fatal error #6734
src: print backtrace on fatal error #6734
Conversation
@@ -0,0 +1,5 @@ | |||
#include "src/base/logging.cc" |
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.
#include "src/base/logging.h"
works too, at least locally for me. Any particular reason for including the source file itself?
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.
And CI is failing on smartos with ld: fatal: symbol 'v8::base::DumpBacktrace()' is multiply-defined
: https://ci.nodejs.org/job/node-test-commit-smartos/2478/nodes=smartos14-32/console
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.
I included the .cc so it works with shared V8 builds (think Fedora, Debian, Ubuntu, etc.,or linking against a .so.)
I'll ifdef out smartos. That user base is a rounding error compared to the aforementioned distros.
LGTM if CI is green |
Oh my gosh, +1000 |
Looks like plinux doesn't like it.
|
LGTM, but yea, was just about to say what @Fishrock123 did. |
/cc @mhdawson re plinux error |
887c8cb
to
9c63535
Compare
Different approach that should work on all platforms: https://ci.nodejs.org/job/node-test-pull-request/2637/ |
@bnoordhuis Looks like that failed on
|
God, smartos is like a time machine back to the '90s. Okay, loosened the const-ness. CI: https://ci.nodejs.org/job/node-test-pull-request/2638/ |
@bnoordhuis smartos still 🚒 |
Looks like something went wrong. Again: https://ci.nodejs.org/job/node-test-pull-request/2639/ |
Finally, green except for flaky |
Works on my machine. This might be a silly question, but does the CI actually exercise this at all? |
Not intentionally, I don't think. I thought about adding a test but it would be rather anti-social when core dumps are enabled. |
@bnoordhuis I added a minimal test in cjihrig@3ce55fa. The |
4ab11e0
to
f15eb7b
Compare
@cjihrig Thanks, added to the PR. Anyone wants to take one more quick look? |
I guess the test should be skipped on Windows? Other than that still LGTM. |
assert.strictEqual(child.stdout.toString().trim(), ''); | ||
assert.ok(frames.length > 0); | ||
assert.ok(frames.every((frame, i) => { | ||
const re = new RegExp(`\w*${i + 1}: .+( \[.+\])?`); |
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.
Eh, the RE should probably start with ^\s*
and end with $
, otherwise the optional parts here are pointless
f15eb7b
to
e52e6d2
Compare
Updated the test, PTAL. CI: https://ci.nodejs.org/job/node-test-pull-request/2741/ |
LGTM if CI is green |
LGTM. Seems to be an issue with the CI though. |
`python tools/test.py abort` won't work without one. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
The --abort-on-uncaught-exception can terminate the process with either a SIGABRT or a SIGILL signal but the test only expected SIGABRT. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
There is no real need and it causes endless grief on Windows with some of the upcoming changes. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Print a C backtrace on fatal errors to make it easier to debug issues. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This commit adds a test that validates backtraces which are printed on fatal errors. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Don't inline calls to node::DumpBacktrace() and fflush(), it makes the generated code bigger. A secondary benefit of moving it to a function is that it gives you something to put a breakpoint on. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Print a C backtrace on fatal errors to make it easier to debug issues. PR-URL: #6734 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bnoordhuis should this be backported? |
Ideally, yes. |
due to the AIX + compiler failures I'm going to hold off on this change for the v4.5.0 release @bnoordhuis would you be willing to backport this PR along with #7508, #7544, and any other potential commits that need to land with this? |
ping @bnoordhuis sorry about the other message. Would you be willing to backport a working version of this as mentioned above |
There is no real need and it causes endless grief on Windows with some of the upcoming changes. PR-URL: nodejs#6734 Backport-PR-URL: nodejs#16550 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Print a C backtrace on fatal errors to make it easier to debug issues
like #6727.
CI: https://ci.nodejs.org/job/node-test-pull-request/2629/