-
-
Notifications
You must be signed in to change notification settings - Fork 200
fix: split inproc handler thread #1446
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
base: master
Are you sure you want to change the base?
Conversation
* use `std::nothrow` `new` consistently to keep exception-free semantics for allocation * rename static crashpad_handler to have no module-public prefix * use `nullptr` for arguments where we previously used 0 to clarify that those are pointers * eliminate the `memset()` of the `crashpad_state_t` initialization since it now contains non-trivially constructable fields (`std::atomic`) and replace it with `new` and an empty value initializer.
…ld, since libraries like libunwind.a might be packaged without PIC.
…ms with architecture prefixes (32-bit Linux)
…stack also ensure to get the first frame harmonize libunwind usage
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1446 +/- ##
==========================================
- Coverage 84.66% 84.13% -0.53%
==========================================
Files 58 59 +1
Lines 7960 8220 +260
Branches 1556 1610 +54
==========================================
+ Hits 6739 6916 +177
- Misses 1058 1111 +53
- Partials 163 193 +30 🚀 New features to boost your workflow:
|
# Conflicts: # src/backends/sentry_backend_inproc.c
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| **Breaking**: | ||
|
|
||
| - inproc(Linux): the `inproc` backend on Linux now depends on "nognu" `libunwind`. ([#1446](https://github.com/getsentry/sentry-native/pull/1446)) |
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 changelog entry seems to be part of an already released section
## 0.12.6.
Consider moving the entry to the## Unreleasedsection, please.
* introduces state-machine for in-progress crash handling vs other crashing threads * extract async-safe logging macros * adds arm64e support PAC registers (access to opaque fp, lr, sp, and pc) * skip on_crash if we handle a failing handler thread (TODO: also before_send) * handle failed pipe and semaphore signaling to handler thread * introduce even more fall backs to handling inside the signal handler where it makes sense (i.e. failed attempt to signal handler thread)
…ctually have to check return values.
…ntegration test executables with the same parametrization as the core artifacts and "example".
…thread exists as a separate library
… unstable... it works great locally and should remain for local testing and debugging though.
This is a more elaborate, long-term fix to getsentry/sentry-java#4830 than #1444.
It also finishes the work done here: #1088
And fixes the issues raised here: #1353
and here: #906
So, while the driver for this PR is a downstream issue that exposes the signal-unsafety of some parts of the current
inprocimplementation, it also addresses a much broader range of concerns that regularly affectinprocusers on all platforms.At a high level, it introduces a separate handler thread for
inproc, which the signal handler (orUEFon Windows) wakes after it exchanges crash context data.The idea is that we minimize signal handler/UEF to do the least amount of syscall stuff (or at least the subset documented in the signal-safety man-page), while the handler thread can execute functions outside that range (with limitations, since thread sync and heap allocations are still problematic). This allows us to reuse
stdiofunctionality like formatters without running squarely into UB territory or having to rewrite all utilities to async-signal-safe versions, as in #1444.There are a few considerable changes to mention:
backtrace()or any unwinder that runs from the "current" instruction address is entirely useless (ignoring the fact thatbacktrace()was always signal-unsafe to begin with, which itself was the source of crashes, hangs or just empty stack traces).inproc, which we already partially acknowledged in Using libunwind for mac, since backtrace do not expect thread context… #1088 and fix: support musl on Linux #1233.libunwind(thenognuimplementation, not thellvmone, which is a pure C++ exception unwinder), which is a breaking change (at least in the sense that users now require an additional dependency at build and runtime). This means that the "general" Linux usage is now the same as with themusl libcenvironments.arm64andx86-64, and use the system-providedlibunwindfor the default stack-trace from a call-site. It turned out that the system-providedlibunwindwasn't safe enough to use in the context of the signal handler (either led to hangs or had issues with escaping the trampoline). This means users can now useinprocon macOS again (if their code is compiled without omitting frame pointers, which is always the case by default on macOS).Further improvements/fixes (summarizing the 30 commits, which I didn't want to squash):
libunwind-based unwinder modules now also validate retrieved ucontext pointers against memory mapping (for Linux and macOS)__syncfunctions and replaced them with__atomic(especially the signal handler blocking logic and the spinlock)newwithstd::nothrowthroughout the affected backend code (including the initialization ofcrashpad_state_t, which still usedmallocandmemsetalthough it hasstd::atomicmembers)TODOs:
x86-64stackwalker formacOS(and clean up the code)libbacktracefallback at all and how to handle it.inprocinprocnot working onmacOSlibunwinddependency on Linux