refactor(profiling): Signal handler boilerplate - #756
Conversation
…indowIfNeeded Adds a narrowly-scoped RAII guard (guards.h) for the setSighandlerTid(tid)/setSighandlerTid(-1) span and a shared tickInitWindowIfNeeded() helper (jvmThread.h), then applies both to the five duplicated signal handlers in ctimer_linux.cpp, itimer.cpp and wallClock.cpp. Also fixes two pre-existing errno-restore gaps: CTimer::signalHandler's !cs.entered()/!_enabled early returns, and WallClockASGCT::signalHandler, which previously never saved/restored errno at all.
saved_errno was captured after the CriticalSection entry check, so the !cs.entered() bail-out path left errno unrestored. Move the save to the top of the handler, matching WallClockASGCT.
…ope, null assert) to ITimer and PerfEvents Brings ITimer::signalHandler and PerfEvents::signalHandler in line with CTimer/WallClock: save/restore errno across every return path, scope SighandlerTidScope narrowly around the recordSample span, and assert current != nullptr. ITimerJvmti was already fully migrated. Deliberately excludes tickInitWindowIfNeeded: unlike CTimer/WallClock, neither engine had this check before, and both lack the signal-origin validation those engines gate it behind, so adding it is a behavior change that needs separate review, not a boilerplate refactor.
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #33666237762 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-09-02 18:34:26 UTC |
This comment has been minimized.
This comment has been minimized.
|
One small typo to fix, otherwise looks good! |
Co-authored-by: Jaroslav Bachorik <jaroslav.bachorik@datadoghq.com>
There was a problem hiding this comment.
Pull request overview
Refactors duplicated boilerplate across multiple profiling signal handlers in the native (C++) profiler to reduce duplication and make handler cleanup (notably errno restore and sighandler TID bookkeeping) more consistent and less error-prone.
Changes:
- Introduces
SighandlerTidScope(RAII) to replace manualsetSighandlerTid(tid)/setSighandlerTid(-1)pairs in multiple handlers. - Extracts the “init-window tick-and-return” logic into
tickInitWindowIfNeeded(ProfiledThread*)and reuses it in the original CPU/wall handler set. - Expands and fixes
errnosave/restore coverage in several signal handlers and preventsPerfFdRearmGuarddestruction from clobbering restorederrno.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/wallClock.cpp | Adds errno save/restore and switches to SighandlerTidScope; uses shared init-window helper. |
| ddprof-lib/src/main/cpp/perfEvents_linux.cpp | Adds errno save/restore in the perf-events signal handler and fixes PerfFdRearmGuard destructor to preserve errno. |
| ddprof-lib/src/main/cpp/jvmThread.h | Adds tickInitWindowIfNeeded() helper and required include for ProfiledThread. |
| ddprof-lib/src/main/cpp/itimer.cpp | Adds errno save/restore and SighandlerTidScope in ITimer handler; uses shared init-window helper in JVMTI variant. |
| ddprof-lib/src/main/cpp/guards.h | Adds SighandlerTidScope RAII guard for sighandler TID management. |
| ddprof-lib/src/main/cpp/ctimer_linux.cpp | Uses shared init-window helper and SighandlerTidScope; fixes missing errno restore on early returns. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: in_progress
mergequeue-status: rejected
and removed
mergequeue-status: in_progress
labels
|
Good cleanup. For preserving |
Addresses PR feedback suggesting an RAII wrapper for errno preservation. Replaces the `int saved_errno = errno; ... errno = saved_errno;` pattern repeated before every early return in ITimer, ITimerJvmti, WallClockASGCT, WallClockJvmti, CTimer, CTimerJvmti, PerfEvents::signalHandler, PerfFdRearmGuard, and OS::forwardForeignSignal. Declaring ErrnoPreserver as the first local also fixes a latent gap: since locals destruct in reverse order, it now restores errno after every other guard's destructor (e.g. InflightGuard's clock_gettime call) has run, instead of before. Removes the now-redundant SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO macro variant.
…ataDog/java-profiler into yg/signal-handler-boilerplate-refactor
|
|
||
| void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, | ||
| u64 last_sample, ProfiledThread* current) { | ||
| ErrnoPreserver errno_preserver; |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] For the wall-clock engines the ErrnoPreserver is declared in the inner WallClockASGCT/WallClockJvmti::signalHandler helper rather than at the top of sharedSignalHandler, which is the actual installed sa_sigaction. Everything between signal entry and this line - Counters::increment, SIGNAL_HANDLER_GUARD_OR_DROP() -> SignalHandlerScope -> ProfiledThread::acquireCurrent() -> SignalBlocker -> pthread_sigmask, and BaseWallClock::eventsEnabled() - runs before errno is captured, so a clobbered errno is snapshotted and restored, leaving the interrupted user code with the wrong value. Symmetrically, sharedSignalHandler's SignalHandlerScope/InflightGuard destructors run after the inner restore. This is the same gap guards.h documents ('declare the ErrnoPreserver as the first local in the guarded function') and that the other five handlers do avoid, since there ErrnoPreserver is the first statement of the sa_sigaction itself.
Suggestion: Move the ErrnoPreserver declaration to be the first local of WallClockASGCT::sharedSignalHandler (wallClock.cpp:206) and WallClockJvmti::sharedSignalHandler (wallClock.cpp:412), and drop it from the inner signalHandler helpers. That makes it the first local of the real signal-entry frame, matching the CTimer/ITimer/PerfEvents handlers and the contract stated in guards.h.
| // (where JVMThread::current() is always null) is allowed through once its | ||
| // one-shot init window has ticked down. Returns true if the caller should | ||
| // tick-and-return, in which case the tick has already happened; the caller | ||
| // remains responsible for restoring errno at its own return, since not all |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] The new doc comment states that callers of tickInitWindowIfNeeded() must restore errno at their own return 'since not all call sites save errno the same way'. That is untrue of every call site created in this diff: all five (ctimer_linux.cpp:231/281, itimer.cpp:113, wallClock.cpp:242/449) now save errno identically via ErrnoPreserver and none performs a manual restore. The comment documents the very pattern this PR removed.
Suggestion: Replace the errno sentence with a statement of the actual convention, e.g. that all call sites are signal handlers holding an ErrnoPreserver and therefore need no manual errno handling on this return path.
| // tick-and-return, in which case the tick has already happened; the caller | ||
| // remains responsible for restoring errno at its own return, since not all | ||
| // call sites save errno the same way. | ||
| static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — MEDIUM] No test in this PR would detect changing && to || in the init-window condition of tickInitWindowIfNeeded(), which would make the function return true if either condition (JVMThread::current() == nullptr OR current->inInitWindow()) holds, instead of requiring both.
Suggestion: Add a test case that calls tickInitWindowIfNeeded() with different combinations of conditions and verifies the return value and whether tickInitWindow() is called in each case (null/in-window, null/not-in-window, not-null/in-window, not-null/not-in-window).
| public: | ||
| PerfFdRearmGuard(int fd, int tid) : _fd(fd), _tid(tid) {} | ||
| ~PerfFdRearmGuard() { | ||
| // These calls must not leak an errno change to the caller. |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] The ErrnoPreserver inside ~PerfFdRearmGuard is redundant. The PR description justifies it by claiming PerfFdRearmGuard 'is the first local constructed in PerfEvents::signalHandler, so it destructs last - after any errno = saved_errno restore in the handler body'. The same diff makes ErrnoPreserver the first local (line 782), so PerfFdRearmGuard now destructs strictly before the errno restore and cannot clobber it. PerfFdRearmGuard has a single instantiation, so there is no other caller the extra guard protects.
Suggestion: Drop the nested ErrnoPreserver (and its comment) from ~PerfFdRearmGuard, relying on the handler-level ErrnoPreserver; or, if the belt-and-braces guard is intentional, correct the PR description since its stated rationale no longer holds.
| errno = saved_errno; | ||
| return; | ||
| } | ||
| int tid = 0; |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] int tid = 0; is declared before the init-window early return and then unconditionally reassigned at line 236 without ever being read. The refactor removed the reason for the split declaration; CTimer::signalHandler just below (line 285) already uses int tid = current->tid();.
Suggestion: Delete line 229 and change line 236 to int tid = current->tid();, matching CTimer::signalHandler.
| #include "threadLocalData.inline.h" | ||
| #include "threadState.inline.h" | ||
| #include "guards.h" | ||
| #include <errno.h> |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] #include <errno.h> was added to itimer.cpp, but the diff simultaneously removed every direct use of errno from that file (only ErrnoPreserver remains, whose definition in guards.h already includes ). The include is unnecessary.
Suggestion: Remove the #include <errno.h> line; guards.h already provides for ErrnoPreserver.
| @@ -474,16 +475,14 @@ void OS::forwardForeignSignal(int signo, siginfo_t* siginfo, void* ucontext) { | |||
| // chained handler) may set errno. Callers that save errno AFTER | |||
| // forwardForeignSignal (e.g. CTimer::signalHandler) would see a clobbered | |||
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] The rationale comment above the new ErrnoPreserver names CTimer::signalHandler as a caller that 'saves errno AFTER forwardForeignSignal'. It does not - ctimer_linux.cpp:252 declares ErrnoPreserver as the handler's first statement, before the forwardForeignSignal call, as does CTimerJvmti at line 209. The real justification for the guard is that the interrupted code must not observe an errno clobbered by rt_sigprocmask or the chained handler.
Suggestion: Restate the rationale in terms of not leaking errno changes from the chained handler / rt_sigprocmask to the interrupted code, and drop the now-incorrect CTimer::signalHandler example.
| #include <jvmti.h> | ||
|
|
||
| #include "threadLocal.h" | ||
| #include "threadLocalData.h" |
There was a problem hiding this comment.
🗿 🤖 🔴
[Sphinx Review — LOW] Adding #include "threadLocalData.h" to jvmThread.h drags a heavy header (which transitively pulls context.h, otel_context.h, os.h, unwindStats.h, , , jvmti.h) into all 17 TUs that include jvmThread.h, and transitively into everything including hotspot/vmStructs.h, solely to support a 5-line helper used by 5 call sites that all already include threadLocalData.h themselves.
Suggestion: Consider putting tickInitWindowIfNeeded in a lighter, purpose-specific header (or in threadLocalData.inline.h, which already has ProfiledThread available), keeping jvmThread.h's forward-declaration-only footprint.
- Move ErrnoPreserver to the top of WallClock{ASGCT,Jvmti}::sharedSignalHandler
(the real signal-entry points), not the inner signalHandler helpers.
- Fix stale tickInitWindowIfNeeded() doc comment describing errno handling
the refactor removed; state the actual ErrnoPreserver convention instead.
- Add tickInitWindowIfNeeded() unit tests covering all 4 combinations of its
guard condition, catching a `&&` -> `||` mutation.
- Drop the redundant nested ErrnoPreserver in ~PerfFdRearmGuard now that the
handler-level guard is declared first and destructs after it.
- Remove dead pre-init of `tid` in CTimerJvmti::signalHandler.
- Remove unused `#include <errno.h>` in itimer.cpp.
- Correct forwardForeignSignal()'s errno rationale comment, which
misdescribed CTimer::signalHandler's save ordering.
…ataDog/java-profiler into yg/signal-handler-boilerplate-refactor
There was a problem hiding this comment.
🟡 Changes recommended
The new unit test deletes a pthread key that JVMThread’s ThreadLocal intentionally treats as process-immortal, which can invalidate JVMThread::current() for later tests, and there is a PR-description/implementation mismatch around PerfFdRearmGuard errno preservation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
| void TearDown() override { | ||
| pthread_key_delete(_key); | ||
| ProfiledThread::release(); | ||
| } |
| ~PerfFdRearmGuard() { | ||
| // Errno changes made here are caught by the handler-level | ||
| // ErrnoPreserver, which is declared before this guard and therefore | ||
| // destructs after it. | ||
| PerfEvents::resetBuffer(_tid); |
What does this PR do?:
Extracts the boilerplate duplicated across the CPU/wall profiling signal
handlers (
CTimer::signalHandler,CTimerJvmti::signalHandler,ITimerJvmti::signalHandler,WallClockASGCT::signalHandler,WallClockJvmti::signalHandler) into two shared helpers, fixes severallatent errno-restore gaps found along the way, and extends the same
treatment to two handlers outside PROF-14748's original five
(
ITimer::signalHandler,PerfEvents::signalHandler):SighandlerTidScope(guards.h), a narrowly-scoped RAII guard aroundShims::instance().setSighandlerTid(tid)/setSighandlerTid(-1),replacing the manual set/reset pairs in all seven handlers.
tickInitWindowIfNeeded()(jvmThread.h), a shared helper for the4-line init-window guard that was copied verbatim into the original five
handlers. Deliberately not applied to
ITimer::signalHandlerorPerfEvents::signalHandler(see Additional Notes).CTimer::signalHandler's!_enabledearly return, which returned without restoringerrno(thethree Jvmti handlers already did this correctly on the equivalent path).
saved_errnosave/restore toWallClockASGCT::signalHandler,which previously didn't save/restore errno at all, unlike its sibling
WallClockJvmti::signalHandler.WallClockJvmti::signalHandler:saved_errnowas captured after theCriticalSectionentry check, so the!cs.entered()bail-out path lefterrnounrestored. The save now happensfirst, matching
WallClockASGCT.saved_errnosave/restore toITimer::signalHandlerandPerfEvents::signalHandler, neither of which had it before, wraps theirrecordSamplespan inSighandlerTidScope(replacing the manualsetSighandlerTid/-1pair), and addsassert(current != nullptr)ahead of the
tidcomputation, mirroring the original five handlers.ITimerJvmti::signalHandlerwas already fully migrated and needed nochanges here.
PerfFdRearmGuard::~PerfFdRearmGuard()(
perfEvents_linux.cpp): it is the first local constructed inPerfEvents::signalHandler, so it destructs last — after anyerrno = saved_errnorestore in the handler body — and itsioctl()/resetBuffer()calls were silently overwriting the restored value. Itsdestructor now saves/restores
errnoaround its own side effects.Motivation:
PROF-14748 : these five handlers were identified during review of
the jvmtistacks addition as sharing identical boilerplate with no shared
abstraction.
Additional Notes:
ITimer::signalHandlerandPerfEvents::signalHandlerwere not named in PROF-14748'soriginal five. Both had the identical manual
set/reset-
setSighandlerTidpattern as their siblings and are now giventhe same
SighandlerTidScopetreatment, plus errno save/restore and acurrent != nullptrassert.tickInitWindowIfNeeded()was deliberately not wired into ITimer nor PerfEventshandlers, unlike the original five. Neither engine had this check before,
and unlike
CTimer/WallClock— which gate it behind signal-originvalidation (
si_code/sivalpayload checks) —ITimercan't do thatvalidation at all (
setitimer(ITIMER_PROF)deliversSI_KERNEL, with nopayload to check) and
PerfEventsgates on a different, coarsersi_code <= 0"external signal" check. Adding the init-window dropwithout that same origin guard would be a behavior change with different
risk characteristics for these two engines, so it's left out of this
boilerplate-extraction PR and would need separate review.
PerfEvents::signalHandler's existing control flow (the_enabledcheckgates only the
SighandlerTidScope/recordSampleblock, afternoteCPUSamplealready ran unconditionally) was left as-is; only errnohandling and the
SighandlerTidScope/assert changes were added.resolveThreadId(a candidate shared helper for thecurrent ? current->tid() : OS::threadId()duplication) was not extracted.The underlying duplication was eliminated directly instead: the original
five handlers now assert
current != nullptrbefore computingtid,making the ternary dead code, so it was removed rather than factored out.
(
ITimer::signalHandlerandPerfEvents::signalHandleralready computedtidfromcurrent->tid()directly, with no ternary to remove.) Thepattern still exists in
javaApi.cpp, a non-signal-handler context outsidethis ticket's scope.
critical-section semantics in any handler.
How to test the change?:
./gradlew :ddprof-lib:compileDebugcompiles cleanly on macOS and linux.ordering) and don't alter sampling logic or control flow beyond exit-path
cleanup.
tickInitWindowIfNeeded()is not wired intoITimer/PerfEvents(see Additional Notes), so no runtime sampling behavior changes for those
two engines. No new automated test was added; existing CPU/wall sampler
correctness and signal-handler integration tests should be run to confirm
no regression, particularly around
CTimerandWallClockJvmtigiven theerrno-ordering change in the latter, and around
ITimer/PerfEventsgiven the new
SighandlerTidScope/assert/errno-restore paths.