refactor(profiling): Signal handler boilerplate

[yaronguro-datadog]

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 several
latent 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):

• Adds SighandlerTidScope (guards.h), a narrowly-scoped RAII guard around
  Shims::instance().setSighandlerTid(tid) / setSighandlerTid(-1),
  replacing the manual set/reset pairs in all seven handlers.
• Adds tickInitWindowIfNeeded() (threadLocalData.inline.h), a shared
  helper for the 4-line init-window guard that was copied verbatim into the
  original five handlers. It lives there because it bridges JVMThread::current()
  and ProfiledThread::inInitWindow()/tickInitWindow(), and that file already
  owns ProfiledThread's inline members; the new #include "jvmThread.h" adds
  no external headers, since threadLocalData.h already pulled <jvmti.h>
  (which includes jni.h) and threadLocal.h. Split into
  tickInitWindowIfNeededImpl(bool has_jvm_thread, ProfiledThread*) plus a
  thin wrapper that derives the bool, so the branch logic is unit testable
  without a JVM (JVMThread::current() asserts on an invalid pthread key). Deliberately not applied to ITimer::signalHandler or
  PerfEvents::signalHandler (see Additional Notes).
• Fixes a pre-existing errno-restore gap in CTimer::signalHandler's
  !_enabled early return, which returned without restoring errno (the
  three Jvmti handlers already did this correctly on the equivalent path).
• Adds missing saved_errno save/restore to WallClockASGCT::signalHandler,
  which previously didn't save/restore errno at all, unlike its sibling
  WallClockJvmti::signalHandler.
• Fixes a second, narrower errno-restore gap in WallClockJvmti::signalHandler:
  saved_errno was captured after the CriticalSection entry check, so the
  !cs.entered() bail-out path left errno unrestored. The save now happens
  first, matching WallClockASGCT.
• Adds saved_errno save/restore to ITimer::signalHandler and
  PerfEvents::signalHandler, neither of which had it before, and wraps their
  recordSample span in SighandlerTidScope (replacing the manual
  setSighandlerTid/-1 pair). assert(current != nullptr) ahead of the
  tid computation is added to ITimer::signalHandler only, mirroring the
  original five handlers -- PerfEvents::signalHandler already had it.
  ITimer::signalHandler also gains a direct #include <cassert>: the new
  assert would otherwise have compiled only via the jvmThread.h ->
  threadLocal.h chain this PR introduced, so trimming an include would
  have broken the build. ITimerJvmti::signalHandler was already fully
  migrated and needed no changes here.
• Fixes a latent errno-clobbering bug in PerfFdRearmGuard::~PerfFdRearmGuard()
  (perfEvents_linux.cpp): its ioctl()/resetBuffer() calls could clobber
  errno on every exit path, including the drop paths, and nothing restored
  it afterwards. Fixed by declaring ErrnoPreserver in
  PerfEvents::signalHandler ahead of PerfFdRearmGuard, so it destructs
  last — after PerfFdRearmGuard — and restores whatever PerfFdRearmGuard's
  side effects clobbered. The guard itself is deliberately not self-contained:
  an earlier review pass asked for the nested ErrnoPreserver inside
  ~PerfFdRearmGuard() to be removed as redundant, and the handler-level one
  (declared just below the si_code <= 0 external-signal fast bail, so that
  path stays untouched) covers its only construction site.

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::signalHandler and PerfEvents::signalHandler were not named in PROF-14748's
  original five. Both had the identical manual
  set/reset-setSighandlerTid pattern as their siblings and are now given
  the same SighandlerTidScope treatment, plus errno save/restore and a
  current != nullptr assert.
• tickInitWindowIfNeeded() was deliberately not wired into ITimer nor
  PerfEvents handlers, unlike the original five. The reason is simply that
  neither engine ever had this check: the init-window guard closes the race
  between Profiler::registerThread() and thread_native_entry publishing
  the JVM thread TLS, which is a property of the sampled thread and unrelated
  to signal-origin validation (an earlier revision of this description claimed
  otherwise — that rationale was wrong). Adding it here would be a behaviour
  change to two engines in a PR that is otherwise a boilerplate extraction, so
  it is left for separate review. A comment at each site now records that.
• PerfEvents::signalHandler's existing control flow (the _enabled check
  gates only the SighandlerTidScope/recordSample block, after
  noteCPUSample already ran unconditionally) was left as-is; only errno
  handling and the SighandlerTidScope/assert changes were added.
• resolveThreadId (a candidate shared helper for the
  current ? current->tid() : OS::threadId() duplication) was not extracted.
  The underlying duplication was eliminated directly instead: the original
  five handlers now assert current != nullptr before computing tid,
  making the ternary dead code, so it was removed rather than factored out.
  (ITimer::signalHandler and PerfEvents::signalHandler already computed
  tid from current->tid() directly, with no ternary to remove.) The
  pattern still exists in javaApi.cpp, a non-signal-handler context outside
  this ticket's scope.
• No change to signal-origin validation, foreign-signal forwarding, or
  critical-section semantics in any handler.

How to test the change?:

• ./gradlew :ddprof-lib:compileDebug compiles cleanly on macOS and linux.
• The changes are mechanical (RAII scope-exit timing, errno save/restore
  ordering) and don't alter sampling logic or control flow beyond exit-path
  cleanup. tickInitWindowIfNeeded() is not wired into ITimer/PerfEvents
  (see Additional Notes), so no runtime sampling behavior changes for those
  two engines.
• Adds ddprof-lib/src/test/cpp/guards_ut.cpp: three unit tests over
  ErrnoPreserver -- restore on normal scope exit, restore on an early
  return, and the ordering invariant the handlers rest on (declared first, it
  destructs last and wins over a later guard whose destructor clobbers
  errno). SighandlerTidScope is not covered: reading the sighandler tid
  back needs the Shims symbol a JVM provides, and this binary has none.
• Adds ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp: four unit tests over
  tickInitWindowIfNeededImpl() covering all four
  has_jvm_thread x inInitWindow() combinations, including the case that
  would break if && were mutated to ||, plus the one-shot property (a
  second call must not tick again). The one-line derivation in
  tickInitWindowIfNeeded() itself (JVMThread::current() != nullptr) is
  deliberately not unit tested: JVMThread::current() reads a pthread key
  established by JVM startup and this gtest binary has no JVM attached, so
  covering it means mutating process-global state and leaning on
  ThreadLocal<JVMThread*>::initialize()'s scan over every live pthread key
  -- more cost and fragility than a single argument expression warrants. No
  integration test exercises the init-window path either, so that one line
  rests on compilation and inspection.
• Beyond the new unit tests, existing CPU/wall sampler
  correctness and signal-handler integration tests should be run to confirm
  no regression, particularly around CTimer and WallClockJvmti given the
  errno-ordering change in the latter, and around ITimer/PerfEvents
  given the new SighandlerTidScope/assert/errno-restore paths.

[dd-octo-sts]

Scan-Build Report

User:	runner@runnervmejwal
Working Directory:	/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:	make -j4 all
Clang Version:	Ubuntu clang version 18.1.3 (1ubuntu1)
Date:	Fri Sep 4 13:08:03 2026

Bug Summary

Bug Type	Quantity	Display?
All Bugs	1
Logic error
Dereference of null pointer	1

Reports

Bug Group	Bug Type ▾	File	Function/Method	Line	Path Length
Logic error	Dereference of null pointer	faultInjection.cpp	crashNow	24	2

[dd-octo-sts]

CI Test Results

Run: #33885297947 | Commit: ec37c49 | Duration: 15m 0s (longest job)

✅ All 32 test jobs passed

Status Overview

JDK	glibc-aarch64/debug	glibc-amd64/debug	musl-aarch64/debug	musl-amd64/debug
8	-	✅	-	-
8-ibm	-	✅	-	-
8-j9	✅	✅	-	-
8-librca	-	-	✅	✅
8-orcl	-	✅	-	-
11	-	✅	-	-
11-j9	✅	✅	-	-
11-librca	-	-	✅	✅
17	✅	✅	-	-
17-graal	✅	✅	-	-
17-j9	✅	✅	-	-
17-librca	-	-	✅	✅
21	✅	✅	-	-
21-graal	✅	✅	-	-
21-librca	-	-	✅	✅
25	✅	✅	-	-
25-graal	✅	✅	-	-
25-librca	-	-	✅	✅

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0

---

Updated: 2026-09-04 14:59:52 UTC

[jbachorik]

One small typo to fix, otherwise looks good!

[Copilot]

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 manual setSighandlerTid(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 errno save/restore coverage in several signal handlers and prevents PerfFdRearmGuard destruction from clobbering restored errno.

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.

[dd-octo-sts]

✅ All 40 integration tests passed

📊 Dashboard · 👷 Pipeline · 📦 c81d4ad3

[Copilot]

🟡 Changes recommended

The new gtest repeatedly creates pthread TLS keys without deletion, which diverges from established test conventions and can lead to pthread-key exhaustion over time.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new unit test currently generates “unique” TLS markers via integer-to-pointer casts, which is implementation-defined and can produce sanitizer/portability failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The changes touch low-level async-signal-handler behavior across multiple engines, so despite looking mechanically correct, they warrant human review and full runtime/regression testing before approval.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[rkennke]

I did a manual review pass, and only found very minor problems. I also ran an AI assisted/Sphinx review pass which found more .. but see the comments there.

[rkennke]

Looks good now, thank you! This is a great change!