-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add stacktrace to ASSERTs #38139
base: main
Are you sure you want to change the base?
Add stacktrace to ASSERTs #38139
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Looks like legit failures. /wait |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Looks like I'm not able to make that function call consistently be not-inlined enough to show up in an opt build's stack trace. Not sure if I could reasonably do "only test that in non-opt builds" somehow? Could potentially force the function to stay a function by putting it into a different compilation unit, but that seems like overkill. |
Also baffling, @ggreenway says that a Regardless of that though, this change seems like it's useful in general because a stack trace on an ASSERT triggered by a test would be useful, and as this test shows, we don't get one before this change. |
You can try adding ifdef DEBUG and exit the test if it is not present /wait |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
That worked for gcc, coverage and asan, but looks like msan is still inlining it. |
Commit Message: Add stacktrace to ASSERTs
Additional Description: Before this change, ASSERTs provoke a stacktrace, but it is always the useless two-line stacktrace
as demonstrated by the test case added in this PR not passing before the change.
It's a bit odd that after this change there will be two stacktraces output on assert, but at least one of them will be useful.
Risk Level: Minimal, it's just output from asserts so if it does anything it's during a crash anyway.
Testing: Added a test case, and used to debug my production issue which I couldn't with the original version.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a