refactor(profiling): Signal handler boilerplate - #756
Draft
yaronguro-datadog wants to merge 4 commits into
Draft
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.
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
Contributor
CI Test ResultsRun: #33095863748 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-27 17:28:06 UTC |
|
Collaborator
|
One small typo to fix, otherwise looks good! |
jbachorik
reviewed
Aug 27, 2026
Co-authored-by: Jaroslav Bachorik <jaroslav.bachorik@datadoghq.com>
Contributor
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.
Comment on lines
+785
to
789
| int saved_errno = errno; | ||
| if (siginfo->si_code <= 0) { | ||
| // Looks like an external signal; don't treat as a profiling event | ||
| return; | ||
| } |
Contributor
|
❌ 39 passed, 1 failed out of 40 configurations Test Matrix
Failure Detailsmusl-x64-openj9-jdk25Tracer+profiler: Links
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.