-
-
Notifications
You must be signed in to change notification settings - Fork 356
feat: Capturing fatal CPPExceptions via cxa_throw #5256
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
Conversation
Use fishhook to swap the __cxa_throw C++ exception handler for getting stacktraces for unhandled C++ exceptions. This implementation is based on the code in https://github.com/kstenerud/KSCrash. Fixes GH-4517
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5256 +/- ##
=============================================
+ Coverage 85.933% 86.049% +0.115%
=============================================
Files 394 397 +3
Lines 34217 34587 +370
Branches 14798 14988 +190
=============================================
+ Hits 29404 29762 +358
- Misses 4771 4785 +14
+ Partials 42 40 -2
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ed49f0c | 1245.67 ms | 1261.15 ms | 15.48 ms |
7bc3c0d | 1212.35 ms | 1228.94 ms | 16.59 ms |
881a955 | 1214.27 ms | 1235.88 ms | 21.61 ms |
24e0744 | 1241.98 ms | 1262.44 ms | 20.46 ms |
765dd70 | 1236.16 ms | 1259.12 ms | 22.96 ms |
e8b11f8 | 1233.66 ms | 1249.74 ms | 16.08 ms |
db533ee | 1228.96 ms | 1248.23 ms | 19.28 ms |
3f366ee | 1242.28 ms | 1260.80 ms | 18.52 ms |
6604dbb | 1248.35 ms | 1256.14 ms | 7.79 ms |
596ccc1 | 1223.80 ms | 1249.44 ms | 25.64 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
ed49f0c | 21.58 KiB | 632.13 KiB | 610.55 KiB |
7bc3c0d | 20.76 KiB | 427.35 KiB | 406.59 KiB |
881a955 | 22.85 KiB | 407.63 KiB | 384.78 KiB |
24e0744 | 21.58 KiB | 709.06 KiB | 687.48 KiB |
765dd70 | 23.76 KiB | 819.58 KiB | 795.81 KiB |
e8b11f8 | 22.85 KiB | 411.92 KiB | 389.07 KiB |
db533ee | 21.58 KiB | 547.02 KiB | 525.44 KiB |
3f366ee | 20.76 KiB | 427.84 KiB | 407.08 KiB |
6604dbb | 22.84 KiB | 402.56 KiB | 379.72 KiB |
596ccc1 | 22.84 KiB | 401.44 KiB | 378.59 KiB |
Previous results on branch: fix/cpp-exceptions-v3
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
fe1b364 | 1214.59 ms | 1244.90 ms | 30.31 ms |
0143072 | 1219.62 ms | 1252.60 ms | 32.98 ms |
1a55563 | 1219.78 ms | 1245.86 ms | 26.08 ms |
05e5998 | 1235.76 ms | 1255.39 ms | 19.63 ms |
12a2c05 | 1213.24 ms | 1232.41 ms | 19.16 ms |
e102ea7 | 1239.84 ms | 1254.65 ms | 14.82 ms |
d52d3e7 | 1233.12 ms | 1255.00 ms | 21.88 ms |
6ac3949 | 1227.64 ms | 1247.69 ms | 20.06 ms |
170787d | 1225.39 ms | 1246.92 ms | 21.53 ms |
181f6e4 | 1239.90 ms | 1253.16 ms | 13.27 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
fe1b364 | 23.76 KiB | 838.03 KiB | 814.27 KiB |
0143072 | 23.76 KiB | 825.26 KiB | 801.49 KiB |
1a55563 | 23.76 KiB | 825.24 KiB | 801.48 KiB |
05e5998 | 23.76 KiB | 839.39 KiB | 815.63 KiB |
12a2c05 | 23.76 KiB | 825.23 KiB | 801.47 KiB |
e102ea7 | 23.76 KiB | 825.28 KiB | 801.51 KiB |
d52d3e7 | 23.74 KiB | 840.59 KiB | 816.84 KiB |
6ac3949 | 23.74 KiB | 840.59 KiB | 816.84 KiB |
170787d | 23.76 KiB | 839.59 KiB | 815.83 KiB |
181f6e4 | 23.76 KiB | 838.04 KiB | 814.28 KiB |
I have no clue why, but the unit tests for Mac Catalyst keep failing. I need to figure out why that is before merging this. |
a7eb148
to
e736e29
Compare
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.
Inspected the changes since the last review. Except for one copypasta everything looks sensible.
I did not get into the details of why tests failed as a result of the unswap, beyond what we already discussed offline. So, for me, the current "fix" is accepted until #5309, since the only downside is keeping the indirect symbols in memory longer than necessary (which doesn't require a significant budget, and you already consider the potential failure mode of adding duplicates when swapping multiple times).
Sources/SentryCrash/Recording/Tools/SentryCrashCxaThrowSwapper.c
Outdated
Show resolved
Hide resolved
Sources/SentryCrash/Recording/Tools/SentryCrashCxaThrowSwapper.c
Outdated
Show resolved
Hide resolved
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.
My knowledge on C/C++ is limited therefore I am not going to block this PR, but I did left a couple of comments for you to consider.
Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_CPPException.cpp
Show resolved
Hide resolved
Sources/SentryCrash/Recording/Tools/SentryCrashPlatformSpecificDefines.h
Show resolved
Hide resolved
Co-authored-by: Mischan Toosarani-Hausberger <[email protected]>
Co-authored-by: Philip Niedertscheider <[email protected]>
Co-authored-by: Philip Niedertscheider <[email protected]>
The API Stability Check is supposed to fail due to adding a new experimental options |
📜 Description
Use fishhook to swap the __cxa_throw C++ exception handler for getting
stacktraces for unhandled C++ exceptions. This implementation is based
on the code in https://github.com/kstenerud/KSCrash.
The API stability check fails because I added a new experimental option
enableUnhandledCPPExceptionsV2
.There are still a few open issues to address before making this GA; see #5309
💡 Motivation and Context
Fixes GH-4517
💚 How did you test it?
Testing Details macOS sample
Without the feature
https://sentry-sdks.sentry.io/issues/6590753526/events/d1bb9784083e40a8abfdb28ac2b9c34e/?project=5428557
With the feature
Invalid Argument
sentry-cocoa/Samples/macOS-Swift/Shared/CppSample.cpp
Lines 4 to 8 in e70c3aa
rethrowNoActiveCPPException
sentry-cocoa/Samples/macOS-Swift/Shared/CppSample.cpp
Lines 22 to 28 in e70c3aa
https://sentry-sdks.sentry.io/issues/6617441025/events/30bc9db8d17b4dcd80d6473c57a3c36f/?project=5428557
C++ Exception from BG Thread
https://sentry-sdks.sentry.io/issues/6565152294/events/691f845d8dc04305b0cf376e2d21732b/?project=5428557
NoExcept C++ Exception
sentry-cocoa/Samples/macOS-Swift/Shared/CppSample.cpp
Lines 16 to 20 in e70c3aa
https://sentry-sdks.sentry.io/issues/6565163235/events/d9dc157068994ed2879842dbaf5f477b/?project=5428557
Unhandled C++Exception from testflight iOS-Swift
https://sentry-sdks.sentry.io/issues/6623122757/events/85451edf3f324ad98a6293f1e2cc4b55/?project=5428557
📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.