From 7f23ba5504b5a5f12d7b22227262985e25e2f955 Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Mon, 24 Aug 2026 13:58:32 -0400 Subject: [PATCH 01/12] Extract shared signal-handler helpers: SighandlerTidScope + tickInitWindowIfNeeded 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. --- ddprof-lib/src/main/cpp/ctimer_linux.cpp | 48 ++++---- ddprof-lib/src/main/cpp/guards.h | 27 +++++ ddprof-lib/src/main/cpp/itimer.cpp | 23 ++-- ddprof-lib/src/main/cpp/jvmThread.h | 17 +++ ddprof-lib/src/main/cpp/wallClock.cpp | 143 ++++++++++++----------- 5 files changed, 149 insertions(+), 109 deletions(-) diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp index 6cf46b2a7d..515f4ee298 100644 --- a/ddprof-lib/src/main/cpp/ctimer_linux.cpp +++ b/ddprof-lib/src/main/cpp/ctimer_linux.cpp @@ -230,9 +230,7 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { } int tid = 0; - if (JVMThread::current() == nullptr - && current->inInitWindow()) { - current->tickInitWindow(); + if (tickInitWindowIfNeeded(current)) { errno = saved_errno; return; } @@ -240,17 +238,17 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { current->noteCPUSample(Profiler::instance()->recordingEpoch()); tid = current->tid(); - Shims::instance().setSighandlerTid(tid); - - ExecutionEvent event; - event._execution_mode = getThreadExecutionMode(); - // Opted into JVMTI delegation; drop the sample if the JVM rejects the - // request (WRONG_PHASE if JFR is not recording, NOT_AVAILABLE if - // jdk.StackTraceRequest is disabled). recordSampleDelegated() bumps the - // failure counters; there is no fallback to ASGCT in this engine. - Profiler::instance()->recordSampleDelegated(ucontext, _interval, tid, - BCI_CPU, &event); - Shims::instance().setSighandlerTid(-1); + { + SighandlerTidScope sighandlerTid(tid); + ExecutionEvent event; + event._execution_mode = getThreadExecutionMode(); + // Opted into JVMTI delegation; drop the sample if the JVM rejects the + // request (WRONG_PHASE if JFR is not recording, NOT_AVAILABLE if + // jdk.StackTraceRequest is disabled). recordSampleDelegated() bumps the + // failure counters; there is no fallback to ASGCT in this engine. + Profiler::instance()->recordSampleDelegated(ucontext, _interval, tid, + BCI_CPU, &event); + } errno = saved_errno; } @@ -276,32 +274,30 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { + errno = saved_errno; return; // Another critical section is active, defer profiling } // we want to ensure memory order because of the possibility the instance gets // cleared if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { + errno = saved_errno; return; } assert(!current->isDeepCrashHandler()); - // Guard against the race window between Profiler::registerThread() and - // thread_native_entry setting JVM TLS (PROF-13072): skip at most one signal - // per thread. Pure native threads (where JVMThread::current() is always null) - // are allowed through once the one-shot window expires. - if (JVMThread::current() == nullptr && current->inInitWindow()) { - current->tickInitWindow(); + if (tickInitWindowIfNeeded(current)) { errno = saved_errno; return; } current->noteCPUSample(Profiler::instance()->recordingEpoch()); int tid = current->tid(); - Shims::instance().setSighandlerTid(tid); - ExecutionEvent event; - event._execution_mode = getThreadExecutionMode(); - Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, - &event); - Shims::instance().setSighandlerTid(-1); + { + SighandlerTidScope sighandlerTid(tid); + ExecutionEvent event; + event._execution_mode = getThreadExecutionMode(); + Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, + &event); + } // we need to avoid spoiling the value of errno (tsan report) errno = saved_errno; } diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index f92de48c35..90f68351d1 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -23,6 +23,7 @@ #include #include "counters.h" +#include "debugSupport.h" class ProfiledThread; @@ -250,4 +251,30 @@ class SignalBlocker { SignalBlocker& operator=(const SignalBlocker&) = delete; }; +/** + * RAII guard around the span of a signal handler during which the current + * thread is the one being sampled. Sets Shims::instance().setSighandlerTid(tid) + * on construction and resets it to -1 on destruction, guaranteeing the reset + * happens on every return path out of the guarded scope. + * + * Must be scoped narrowly around exactly the existing set/reset span (right + * before recordSample and right after) rather than wrapped around the whole + * handler: widening the scope would change the window during which the + * sighandler tid is observably set for other consumers of Shims (e.g. + * crash-handler / re-entrant stack-walking code). + */ +class SighandlerTidScope { +public: + explicit SighandlerTidScope(int tid) { + Shims::instance().setSighandlerTid(tid); + } + ~SighandlerTidScope() { + Shims::instance().setSighandlerTid(-1); + } + + // Non-copyable + SighandlerTidScope(const SighandlerTidScope&) = delete; + SighandlerTidScope& operator=(const SighandlerTidScope&) = delete; +}; + #endif // _GUARDS_H diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index e4c02dcafd..7392310474 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -117,24 +117,23 @@ void ITimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { errno = saved_errno; return; } - if (JVMThread::current() == nullptr - && current->inInitWindow()) { - current->tickInitWindow(); + if (tickInitWindowIfNeeded(current)) { errno = saved_errno; return; } int tid = current->tid(); current->noteCPUSample(Profiler::instance()->recordingEpoch()); - Shims::instance().setSighandlerTid(tid); - ExecutionEvent event; - event._execution_mode = getThreadExecutionMode(); - // setitimer(ITIMER_PROF) delivers SIGPROF to an arbitrary thread chosen by - // the OS, so ucontext may be from a JVM-internal thread. Pass nullptr to - // force the JVM into safepoint-based stack walking instead. - Profiler::instance()->recordSampleDelegated(nullptr, _interval, tid, - BCI_CPU, &event); - Shims::instance().setSighandlerTid(-1); + { + SighandlerTidScope sighandlerTid(tid); + ExecutionEvent event; + event._execution_mode = getThreadExecutionMode(); + // setitimer(ITIMER_PROF) delivers SIGPROF to an arbitrary thread chosen by + // the OS, so ucontext may be from a JVM-internal thread. Pass nullptr to + // force the JVM into safepoint-based stack walking instead. + Profiler::instance()->recordSampleDelegated(nullptr, _interval, tid, + BCI_CPU, &event); + } errno = saved_errno; } diff --git a/ddprof-lib/src/main/cpp/jvmThread.h b/ddprof-lib/src/main/cpp/jvmThread.h index 2f5bd69104..8929c1bfbc 100644 --- a/ddprof-lib/src/main/cpp/jvmThread.h +++ b/ddprof-lib/src/main/cpp/jvmThread.h @@ -10,6 +10,7 @@ #include #include "threadLocal.h" +#include "threadLocalData.h" /** * JVMThread represents a native JVM thread that is JVM implementation agnostic @@ -53,4 +54,20 @@ class JVMThread { static void* currentThreadSlow(); }; +// Shared init-window guard used by the CPU/wall profiling signal handlers. +// Guards against the race window between Profiler::registerThread() and +// thread_native_entry setting JVM TLS (PROF-13072): a pure native thread +// (where JVMThread::current() is always null) is allowed through once its +// one-shot init window has ticked down. Returns true iff 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 +// call sites save errno the same way. +static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { + if (JVMThread::current() == nullptr && current->inInitWindow()) { + current->tickInitWindow(); + return true; + } + return false; +} + #endif // _JVMTHREAD_H diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index 2afd90aa1c..d2cc2de7bf 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -232,18 +232,16 @@ void WallClockASGCT::sharedSignalHandler(int signo, siginfo_t *siginfo, void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, u64 last_sample, ProfiledThread* current) { + int saved_errno = errno; assert(current != nullptr); // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { + errno = saved_errno; return; // Another critical section is active, defer profiling } - // Guard against the race window between Profiler::registerThread() and - // thread_native_entry setting JVM TLS (PROF-13072): skip at most one signal - // per thread. Pure native threads (where JVMThread::current() is always null) - // are allowed through once the one-shot window expires. - if (JVMThread::current() == nullptr && current->inInitWindow()) { - current->tickInitWindow(); + if (tickInitWindowIfNeeded(current)) { + errno = saved_errno; return; } // Once-per-run filter (wallprecheck=true): for untraced threads, exact @@ -254,51 +252,55 @@ void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext // sampling instead of arming sampled_this_run. WallPrecheckResult precheck = prepareWallPrecheck(current, _precheck); if (precheck.suppress) { + errno = saved_errno; return; } int tid = current->tid(); - Shims::instance().setSighandlerTid(tid); - u64 call_trace_id = 0; - if (_collapsing) { - StackFrame frame(ucontext); - u64 spanId = 0, rootSpanId = 0; - // contextValid is not redundant with (spanId==0 && rootSpanId==0): a cleared - // context has spanId=0 and contextValid=true, while an uninitialized/mid-write - // thread has spanId=0 and contextValid=false. lookupWallclockCallTraceId uses - // contextValid to decide whether to update the sidecar _otel_local_root_span_id. - bool contextValid = ContextApi::get(spanId, rootSpanId); - call_trace_id = current->lookupWallclockCallTraceId( - (u64)frame.pc(), (u64)frame.sp(), - Profiler::instance()->recordingEpoch(), - contextValid, spanId, rootSpanId); - if (call_trace_id != 0) { - Counters::increment(SKIPPED_WALLCLOCK_UNWINDS); + + { + SighandlerTidScope sighandlerTid(tid); + u64 call_trace_id = 0; + if (_collapsing) { + StackFrame frame(ucontext); + u64 spanId = 0, rootSpanId = 0; + // contextValid is not redundant with (spanId==0 && rootSpanId==0): a cleared + // context has spanId=0 and contextValid=true, while an uninitialized/mid-write + // thread has spanId=0 and contextValid=false. lookupWallclockCallTraceId uses + // contextValid to decide whether to update the sidecar _otel_local_root_span_id. + bool contextValid = ContextApi::get(spanId, rootSpanId); + call_trace_id = current->lookupWallclockCallTraceId( + (u64)frame.pc(), (u64)frame.sp(), + Profiler::instance()->recordingEpoch(), + contextValid, spanId, rootSpanId); + if (call_trace_id != 0) { + Counters::increment(SKIPPED_WALLCLOCK_UNWINDS); + } } - } - ExecutionEvent event; - OSThreadState state = - precheck.observed_state_valid ? precheck.observed_state : getOSThreadState(); - ExecutionMode mode = getThreadExecutionMode(); - if (state == OSThreadState::UNKNOWN) { - if (inSyscall(ucontext)) { - state = OSThreadState::SYSCALL; - mode = ExecutionMode::SYSCALL; - } else { - state = OSThreadState::RUNNABLE; + ExecutionEvent event; + OSThreadState state = + precheck.observed_state_valid ? precheck.observed_state : getOSThreadState(); + ExecutionMode mode = getThreadExecutionMode(); + if (state == OSThreadState::UNKNOWN) { + if (inSyscall(ucontext)) { + state = OSThreadState::SYSCALL; + mode = ExecutionMode::SYSCALL; + } else { + state = OSThreadState::RUNNABLE; + } } + event._thread_state = state; + event._execution_mode = mode; + event._weight = precheck.unowned_weight; + u64 recorded_call_trace_id = 0; + bool recorded = Profiler::instance()->recordSample(ucontext, last_sample, tid, + BCI_WALL, call_trace_id, + &event, + &recorded_call_trace_id); + finishWallPrecheck(precheck, recorded, recorded_call_trace_id); + emitUnownedBlockedTailForWallPrecheck(tid, precheck); } - event._thread_state = state; - event._execution_mode = mode; - event._weight = precheck.unowned_weight; - u64 recorded_call_trace_id = 0; - bool recorded = Profiler::instance()->recordSample(ucontext, last_sample, tid, - BCI_WALL, call_trace_id, - &event, - &recorded_call_trace_id); - finishWallPrecheck(precheck, recorded, recorded_call_trace_id); - emitUnownedBlockedTailForWallPrecheck(tid, precheck); - Shims::instance().setSighandlerTid(-1); + errno = saved_errno; } Error BaseWallClock::start(Arguments &args) { @@ -448,9 +450,7 @@ void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, } int saved_errno = errno; - if (JVMThread::current() == nullptr - && current->inInitWindow()) { - current->tickInitWindow(); + if (tickInitWindowIfNeeded(current)) { errno = saved_errno; return; } @@ -461,32 +461,33 @@ void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, return; } int tid = current->tid(); - Shims::instance().setSighandlerTid(tid); - - ExecutionEvent event; - OSThreadState state = - precheck.observed_state_valid ? precheck.observed_state : getOSThreadState(); - ExecutionMode mode = getThreadExecutionMode(); - if (state == OSThreadState::UNKNOWN) { - if (inSyscall(ucontext)) { - state = OSThreadState::SYSCALL; - mode = ExecutionMode::SYSCALL; - } else { - state = OSThreadState::RUNNABLE; + + { + SighandlerTidScope sighandlerTid(tid); + ExecutionEvent event; + OSThreadState state = + precheck.observed_state_valid ? precheck.observed_state : getOSThreadState(); + ExecutionMode mode = getThreadExecutionMode(); + if (state == OSThreadState::UNKNOWN) { + if (inSyscall(ucontext)) { + state = OSThreadState::SYSCALL; + mode = ExecutionMode::SYSCALL; + } else { + state = OSThreadState::RUNNABLE; + } } + event._thread_state = state; + event._execution_mode = mode; + event._weight = precheck.unowned_weight; + // Pass nullptr ucontext so the JVM uses safepoint-based stack walking. + // Passing the signal-frame PC causes the extension to reject samples where + // the thread is currently inside JVM-internal (non-Java) code. + // JVMTI-delegated samples carry a correlation_id, not a call_trace_id, so + // unowned tail flushing remains limited to the ASGCT wall engine. + bool recorded = Profiler::instance()->recordSampleDelegated( + nullptr, last_sample, tid, BCI_WALL, &event); + finishWallPrecheck(precheck, recorded); } - event._thread_state = state; - event._execution_mode = mode; - event._weight = precheck.unowned_weight; - // Pass nullptr ucontext so the JVM uses safepoint-based stack walking. - // Passing the signal-frame PC causes the extension to reject samples where - // the thread is currently inside JVM-internal (non-Java) code. - // JVMTI-delegated samples carry a correlation_id, not a call_trace_id, so - // unowned tail flushing remains limited to the ASGCT wall engine. - bool recorded = Profiler::instance()->recordSampleDelegated( - nullptr, last_sample, tid, BCI_WALL, &event); - finishWallPrecheck(precheck, recorded); - Shims::instance().setSighandlerTid(-1); errno = saved_errno; } From 00c8d23ecbba9db15ab7db75d3258d42239109ac Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Mon, 24 Aug 2026 17:01:32 -0400 Subject: [PATCH 02/12] Fix 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. Move the save to the top of the handler, matching WallClockASGCT. --- ddprof-lib/src/main/cpp/wallClock.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index d2cc2de7bf..dce9bd9e96 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -443,12 +443,13 @@ void WallClockJvmti::sharedSignalHandler(int signo, siginfo_t *siginfo, void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, u64 last_sample, ProfiledThread* current) { + int saved_errno = errno; assert(current != nullptr); CriticalSection cs(current); if (!cs.entered()) { + errno = saved_errno; return; } - int saved_errno = errno; if (tickInitWindowIfNeeded(current)) { errno = saved_errno; From 13292f158b78ee60fe922ae98b0ddcaf06ded7f9 Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Tue, 25 Aug 2026 13:39:08 -0400 Subject: [PATCH 03/12] Apply signal-handler boilerplate (errno save/restore, SighandlerTidScope, 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. --- ddprof-lib/src/main/cpp/itimer.cpp | 24 +++++++++++++------- ddprof-lib/src/main/cpp/perfEvents_linux.cpp | 13 ++++++++--- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index 7392310474..decaeb2015 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -26,6 +26,7 @@ #include "threadLocalData.inline.h" #include "threadState.inline.h" #include "guards.h" +#include #include bool ITimer::_enabled = false; @@ -33,7 +34,8 @@ long ITimer::_interval; CStack ITimer::_cstack; void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - SIGNAL_HANDLER_GUARD_OR_DROP(); + int saved_errno = errno; + SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); // NOTE: ITimer uses setitimer(ITIMER_PROF) which delivers signals with // si_code==SI_KERNEL — no sival payload is available. The signal-origin // check implemented in CTimer/WallClock cannot be applied here. ITimer @@ -41,25 +43,31 @@ void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // feature addresses. Use CTimer (the default) when signal-origin // validation is required. InflightGuard inflight; - if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) + if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { + errno = saved_errno; return; + } ProfiledThread *current = SIGNAL_HANDLER_CURRENT_THREAD(); + assert(current != nullptr); // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { + errno = saved_errno; return; // Another critical section is active, defer profiling } current->noteCPUSample(Profiler::instance()->recordingEpoch()); int tid = current->tid(); - Shims::instance().setSighandlerTid(tid); - ExecutionEvent event; - event._execution_mode = getThreadExecutionMode(); - Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, - &event); - Shims::instance().setSighandlerTid(-1); + { + SighandlerTidScope sighandlerTid(tid); + ExecutionEvent event; + event._execution_mode = getThreadExecutionMode(); + Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, + &event); + } + errno = saved_errno; } Error ITimer::check(Arguments &args) { diff --git a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp index 711a76f4c9..5cf1155206 100644 --- a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp +++ b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp @@ -764,9 +764,14 @@ class PerfFdRearmGuard { public: PerfFdRearmGuard(int fd, int tid) : _fd(fd), _tid(tid) {} ~PerfFdRearmGuard() { + // Constructed first among signalHandler's locals, so this destructs + // last -- after any errno restore the handler body performs. Save and + // restore errno here too, otherwise these calls silently clobber it. + int saved_errno = errno; PerfEvents::resetBuffer(_tid); ioctl(_fd, PERF_EVENT_IOC_RESET, 0); ioctl(_fd, PERF_EVENT_IOC_REFRESH, 1); + errno = saved_errno; } PerfFdRearmGuard(const PerfFdRearmGuard &) = delete; PerfFdRearmGuard &operator=(const PerfFdRearmGuard &) = delete; @@ -777,12 +782,13 @@ class PerfFdRearmGuard { }; void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { + int saved_errno = errno; if (siginfo->si_code <= 0) { // Looks like an external signal; don't treat as a profiling event return; } PerfFdRearmGuard rearm(siginfo->si_fd, OS::threadId()); - SIGNAL_HANDLER_GUARD_OR_DROP(); + SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); InflightGuard inflight; // A thread with no ProfiledThread attached must never enter the critical @@ -797,20 +803,21 @@ void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { + errno = saved_errno; return; // Another critical section is active, defer profiling } current->noteCPUSample(Profiler::instance()->recordingEpoch()); int tid = current->tid(); if (__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { - Shims::instance().setSighandlerTid(tid); + SighandlerTidScope sighandlerTid(tid); u64 counter = readCounter(siginfo, ucontext); ExecutionEvent event; event._execution_mode = getThreadExecutionMode(); Profiler::instance()->recordSample(ucontext, counter, tid, BCI_CPU, 0, &event); - Shims::instance().setSighandlerTid(-1); } + errno = saved_errno; } Error PerfEvents::check(Arguments &args) { From 2c7e7c43971d1d97edc01b1b243a77ec0a2da864 Mon Sep 17 00:00:00 2001 From: yaronguro-datadog Date: Thu, 27 Aug 2026 12:56:52 -0400 Subject: [PATCH 04/12] Apply suggestion from @jbachorik Co-authored-by: Jaroslav Bachorik --- ddprof-lib/src/main/cpp/jvmThread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ddprof-lib/src/main/cpp/jvmThread.h b/ddprof-lib/src/main/cpp/jvmThread.h index 8929c1bfbc..9e13383aa8 100644 --- a/ddprof-lib/src/main/cpp/jvmThread.h +++ b/ddprof-lib/src/main/cpp/jvmThread.h @@ -58,7 +58,7 @@ class JVMThread { // Guards against the race window between Profiler::registerThread() and // thread_native_entry setting JVM TLS (PROF-13072): a pure native thread // (where JVMThread::current() is always null) is allowed through once its -// one-shot init window has ticked down. Returns true iff the caller should +// 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 // call sites save errno the same way. From 1ec7509a4fb461291d59e506577b555b72d87412 Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Tue, 1 Sep 2026 14:27:03 -0400 Subject: [PATCH 05/12] Introduce ErrnoPreserver RAII guard, replace manual errno save/restore 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. --- ddprof-lib/src/main/cpp/ctimer_linux.cpp | 17 ++----- ddprof-lib/src/main/cpp/guards.h | 50 +++++++++++++++----- ddprof-lib/src/main/cpp/itimer.cpp | 15 ++---- ddprof-lib/src/main/cpp/os_linux.cpp | 6 +-- ddprof-lib/src/main/cpp/perfEvents_linux.cpp | 13 ++--- ddprof-lib/src/main/cpp/wallClock.cpp | 12 +---- 6 files changed, 54 insertions(+), 59 deletions(-) diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp index 515f4ee298..919d82b45f 100644 --- a/ddprof-lib/src/main/cpp/ctimer_linux.cpp +++ b/ddprof-lib/src/main/cpp/ctimer_linux.cpp @@ -206,7 +206,7 @@ Error CTimerJvmti::start(Arguments &args) { } void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - int saved_errno = errno; + ErrnoPreserver errno_preserver; if (!OS::shouldProcessSignal(siginfo, SI_TIMER, SignalCookie::cpu())) { Counters::increment(CTIMER_SIGNAL_FOREIGN); OS::forwardForeignSignal(signo, siginfo, ucontext); @@ -214,24 +214,21 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { } Counters::increment(CTIMER_SIGNAL_OWN); - SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); + SIGNAL_HANDLER_GUARD_OR_DROP(); InflightGuard inflight; ProfiledThread *current = SIGNAL_HANDLER_CURRENT_THREAD(); assert(!current->isDeepCrashHandler()); CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; } if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { - errno = saved_errno; return; } int tid = 0; if (tickInitWindowIfNeeded(current)) { - errno = saved_errno; return; } @@ -249,11 +246,10 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { Profiler::instance()->recordSampleDelegated(ucontext, _interval, tid, BCI_CPU, &event); } - errno = saved_errno; } void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - int saved_errno = errno; + ErrnoPreserver errno_preserver; // Reject signals that did not originate from our timer_create timers. // This guards against Go's process-wide setitimer(ITIMER_PROF) and other @@ -266,7 +262,7 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { } Counters::increment(CTIMER_SIGNAL_OWN); - SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); + SIGNAL_HANDLER_GUARD_OR_DROP(); InflightGuard inflight; ProfiledThread* current = SIGNAL_HANDLER_CURRENT_THREAD(); assert(current != nullptr); @@ -274,18 +270,15 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; // Another critical section is active, defer profiling } // we want to ensure memory order because of the possibility the instance gets // cleared if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { - errno = saved_errno; return; } assert(!current->isDeepCrashHandler()); if (tickInitWindowIfNeeded(current)) { - errno = saved_errno; return; } current->noteCPUSample(Profiler::instance()->recordingEpoch()); @@ -298,8 +291,6 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, &event); } - // we need to avoid spoiling the value of errno (tsan report) - errno = saved_errno; } #endif // __linux__ diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 90f68351d1..59bb1baac8 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -97,24 +98,20 @@ class SignalHandlerScope { bool _active; }; -// Shared drop-path body for the SIGNAL_HANDLER_GUARD_OR_DROP* macros below. -// extra_stmt runs after the dropped-sample counter increment and before the -// return, so both macros stay in lockstep as the drop-accounting logic -// evolves. -#define SIGNAL_HANDLER_GUARD_OR_DROP_IMPL(extra_stmt) \ +// Declare a scope guard local that increments the depth on entry and +// decrements on scope exit. Use as the first statement after any +// foreign-signal-origin rejection check, before any profiling-owned work. +// If the handler also needs to preserve errno across its early returns, +// declare an ErrnoPreserver before this macro -- it must be the +// first-declared local in the handler so it destructs last, after every +// other guard (including this one) has had a chance to touch errno. +#define SIGNAL_HANDLER_GUARD_OR_DROP() \ SignalHandlerScope _signal_handler_scope(true); \ if (!_signal_handler_scope.isActive()) { \ Counters::increment(SAMPLES_DROPPED_THREAD_LOCAL); \ - extra_stmt; \ return; \ } -// Declare a scope guard local that increments the depth on entry and -// decrements on scope exit. Use as the first statement after any -// foreign-signal-origin rejection check, before any profiling-owned work -#define SIGNAL_HANDLER_GUARD_OR_DROP() SIGNAL_HANDLER_GUARD_OR_DROP_IMPL((void)0) -#define SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(err) SIGNAL_HANDLER_GUARD_OR_DROP_IMPL(errno = err) - // Declare a scope guard local that increments the depth on entry and // decrements on scope exit. Use as the first statement of non-profiling @@ -277,4 +274,33 @@ class SighandlerTidScope { SighandlerTidScope& operator=(const SighandlerTidScope&) = delete; }; +/** + * RAII guard that saves errno on construction and restores it on + * destruction, regardless of which return path is taken in between. + * + * Replaces the previous pattern of `int saved_errno = errno;` at handler + * entry paired with a manual `errno = saved_errno;` before every return -- + * a pattern that is easy to miss on a newly added return and, even when + * done consistently, still leaves a gap: any destructor that runs after + * the manual restore (e.g. a signal-scope guard making a syscall) can + * clobber errno again before the handler actually exits. + * + * To close that gap, declare the ErrnoPreserver as the *first* local in + * the guarded function: C++ destroys locals in reverse declaration order, + * so it destructs last, after every other guard's destructor has already + * run. + */ +class ErrnoPreserver { +public: + ErrnoPreserver() : _errno(errno) { } + ~ErrnoPreserver() { errno = _errno; } + + // Non-copyable + ErrnoPreserver(const ErrnoPreserver&) = delete; + ErrnoPreserver& operator=(const ErrnoPreserver&) = delete; + +private: + int _errno; +}; + #endif // _GUARDS_H diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index decaeb2015..985e8b067f 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -34,8 +34,8 @@ long ITimer::_interval; CStack ITimer::_cstack; void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - int saved_errno = errno; - SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); + ErrnoPreserver errno_preserver; + SIGNAL_HANDLER_GUARD_OR_DROP(); // NOTE: ITimer uses setitimer(ITIMER_PROF) which delivers signals with // si_code==SI_KERNEL — no sival payload is available. The signal-origin // check implemented in CTimer/WallClock cannot be applied here. ITimer @@ -44,7 +44,6 @@ void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // validation is required. InflightGuard inflight; if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { - errno = saved_errno; return; } @@ -54,7 +53,6 @@ void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; // Another critical section is active, defer profiling } current->noteCPUSample(Profiler::instance()->recordingEpoch()); @@ -67,7 +65,6 @@ void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, &event); } - errno = saved_errno; } Error ITimer::check(Arguments &args) { @@ -110,23 +107,20 @@ bool ITimerJvmti::_enabled = false; long ITimerJvmti::_interval = 0; void ITimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - int saved_errno = errno; - SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); + ErrnoPreserver errno_preserver; + SIGNAL_HANDLER_GUARD_OR_DROP(); ProfiledThread *current = SIGNAL_HANDLER_CURRENT_THREAD(); assert(current != nullptr); InflightGuard inflight; CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; } if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { - errno = saved_errno; return; } if (tickInitWindowIfNeeded(current)) { - errno = saved_errno; return; } int tid = current->tid(); @@ -142,7 +136,6 @@ void ITimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { Profiler::instance()->recordSampleDelegated(nullptr, _interval, tid, BCI_CPU, &event); } - errno = saved_errno; } Error ITimerJvmti::check(Arguments &args) { diff --git a/ddprof-lib/src/main/cpp/os_linux.cpp b/ddprof-lib/src/main/cpp/os_linux.cpp index bc01fbc40b..4ef679e9c8 100644 --- a/ddprof-lib/src/main/cpp/os_linux.cpp +++ b/ddprof-lib/src/main/cpp/os_linux.cpp @@ -32,6 +32,7 @@ #include #include "common.h" #include "counters.h" +#include "guards.h" #include "log.h" #include "os.h" @@ -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 // value without this guard. - int saved_errno = errno; + ErrnoPreserver errno_preserver; if (signo <= 0 || signo >= MAX_SIGNALS) { - errno = saved_errno; return; } // Acquire-load the valid flag — synchronises with the release-store in // installSignalHandler so we only touch the oldaction struct after it // has been fully written. if (!__atomic_load_n(&installed_oldaction_valid[signo], __ATOMIC_ACQUIRE)) { - errno = saved_errno; return; } // ASYNC-SIGNAL-SAFE CONSTRAINT: forwardForeignSignal is called from signal @@ -576,7 +575,6 @@ void OS::forwardForeignSignal(int signo, siginfo_t* siginfo, void* ucontext) { if (need_mask) { syscall(__NR_rt_sigprocmask, SIG_SETMASK, &saved_mask, nullptr, _NSIG / 8); } - errno = saved_errno; } bool OS::signalOriginCheckEnabled() { diff --git a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp index 5cf1155206..a5da093f2b 100644 --- a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp +++ b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp @@ -764,14 +764,11 @@ class PerfFdRearmGuard { public: PerfFdRearmGuard(int fd, int tid) : _fd(fd), _tid(tid) {} ~PerfFdRearmGuard() { - // Constructed first among signalHandler's locals, so this destructs - // last -- after any errno restore the handler body performs. Save and - // restore errno here too, otherwise these calls silently clobber it. - int saved_errno = errno; + // These calls must not leak an errno change to the caller. + ErrnoPreserver errno_preserver; PerfEvents::resetBuffer(_tid); ioctl(_fd, PERF_EVENT_IOC_RESET, 0); ioctl(_fd, PERF_EVENT_IOC_REFRESH, 1); - errno = saved_errno; } PerfFdRearmGuard(const PerfFdRearmGuard &) = delete; PerfFdRearmGuard &operator=(const PerfFdRearmGuard &) = delete; @@ -782,13 +779,13 @@ class PerfFdRearmGuard { }; void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - int saved_errno = errno; + ErrnoPreserver errno_preserver; if (siginfo->si_code <= 0) { // Looks like an external signal; don't treat as a profiling event return; } PerfFdRearmGuard rearm(siginfo->si_fd, OS::threadId()); - SIGNAL_HANDLER_GUARD_OR_DROP_WITH_ERRNO(saved_errno); + SIGNAL_HANDLER_GUARD_OR_DROP(); InflightGuard inflight; // A thread with no ProfiledThread attached must never enter the critical @@ -803,7 +800,6 @@ void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; // Another critical section is active, defer profiling } current->noteCPUSample(Profiler::instance()->recordingEpoch()); @@ -817,7 +813,6 @@ void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { Profiler::instance()->recordSample(ucontext, counter, tid, BCI_CPU, 0, &event); } - errno = saved_errno; } Error PerfEvents::check(Arguments &args) { diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index dce9bd9e96..562203aa5b 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -232,16 +232,14 @@ void WallClockASGCT::sharedSignalHandler(int signo, siginfo_t *siginfo, void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, u64 last_sample, ProfiledThread* current) { - int saved_errno = errno; + ErrnoPreserver errno_preserver; assert(current != nullptr); // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; // Another critical section is active, defer profiling } if (tickInitWindowIfNeeded(current)) { - errno = saved_errno; return; } // Once-per-run filter (wallprecheck=true): for untraced threads, exact @@ -252,7 +250,6 @@ void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext // sampling instead of arming sampled_this_run. WallPrecheckResult precheck = prepareWallPrecheck(current, _precheck); if (precheck.suppress) { - errno = saved_errno; return; } int tid = current->tid(); @@ -300,7 +297,6 @@ void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext finishWallPrecheck(precheck, recorded, recorded_call_trace_id); emitUnownedBlockedTailForWallPrecheck(tid, precheck); } - errno = saved_errno; } Error BaseWallClock::start(Arguments &args) { @@ -443,22 +439,19 @@ void WallClockJvmti::sharedSignalHandler(int signo, siginfo_t *siginfo, void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, u64 last_sample, ProfiledThread* current) { - int saved_errno = errno; + ErrnoPreserver errno_preserver; assert(current != nullptr); CriticalSection cs(current); if (!cs.entered()) { - errno = saved_errno; return; } if (tickInitWindowIfNeeded(current)) { - errno = saved_errno; return; } WallPrecheckResult precheck = prepareWallPrecheck(current, _precheck); if (precheck.suppress) { - errno = saved_errno; return; } int tid = current->tid(); @@ -489,7 +482,6 @@ void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, nullptr, last_sample, tid, BCI_WALL, &event); finishWallPrecheck(precheck, recorded); } - errno = saved_errno; } void WallClockJvmti::initialize(Arguments &args) { From 886c00fd564170ff5ec28fa7c238a3cb34f9a0ac Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Wed, 2 Sep 2026 14:11:03 -0400 Subject: [PATCH 06/12] Address Sphinx Review comments on PR #756 - 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 ` in itimer.cpp. - Correct forwardForeignSignal()'s errno rationale comment, which misdescribed CTimer::signalHandler's save ordering. --- ddprof-lib/src/main/cpp/ctimer_linux.cpp | 3 +- ddprof-lib/src/main/cpp/itimer.cpp | 1 - ddprof-lib/src/main/cpp/jvmThread.h | 7 +- ddprof-lib/src/main/cpp/os_linux.cpp | 5 +- ddprof-lib/src/main/cpp/perfEvents_linux.cpp | 5 +- ddprof-lib/src/main/cpp/wallClock.cpp | 4 +- ddprof-lib/src/test/cpp/jvmThread_ut.cpp | 94 ++++++++++++++++++++ 7 files changed, 106 insertions(+), 13 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/jvmThread_ut.cpp diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp index 919d82b45f..0ad7907c28 100644 --- a/ddprof-lib/src/main/cpp/ctimer_linux.cpp +++ b/ddprof-lib/src/main/cpp/ctimer_linux.cpp @@ -226,14 +226,13 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { return; } - int tid = 0; if (tickInitWindowIfNeeded(current)) { return; } current->noteCPUSample(Profiler::instance()->recordingEpoch()); - tid = current->tid(); + int tid = current->tid(); { SighandlerTidScope sighandlerTid(tid); diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index 985e8b067f..d0d219a25f 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -26,7 +26,6 @@ #include "threadLocalData.inline.h" #include "threadState.inline.h" #include "guards.h" -#include #include bool ITimer::_enabled = false; diff --git a/ddprof-lib/src/main/cpp/jvmThread.h b/ddprof-lib/src/main/cpp/jvmThread.h index 9e13383aa8..421d02028c 100644 --- a/ddprof-lib/src/main/cpp/jvmThread.h +++ b/ddprof-lib/src/main/cpp/jvmThread.h @@ -17,6 +17,7 @@ */ class JVMThread { private: + friend class JVMThreadTestAccessor; static jfieldID _tid; static ThreadLocal _jvm_thread; @@ -59,9 +60,9 @@ class JVMThread { // thread_native_entry setting JVM TLS (PROF-13072): a pure native thread // (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 -// call sites save errno the same way. +// tick-and-return, in which case the tick has already happened. All call +// sites are signal handlers holding an ErrnoPreserver, so no manual errno +// handling is needed on this return path. static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { if (JVMThread::current() == nullptr && current->inInitWindow()) { current->tickInitWindow(); diff --git a/ddprof-lib/src/main/cpp/os_linux.cpp b/ddprof-lib/src/main/cpp/os_linux.cpp index 4ef679e9c8..361f4284d9 100644 --- a/ddprof-lib/src/main/cpp/os_linux.cpp +++ b/ddprof-lib/src/main/cpp/os_linux.cpp @@ -472,9 +472,8 @@ bool OS::shouldProcessSignal(siginfo_t* siginfo, int expected_si_code, void* exp void OS::forwardForeignSignal(int signo, siginfo_t* siginfo, void* ucontext) { // Preserve errno: syscall(rt_sigprocmask) on the slow path (and any - // chained handler) may set errno. Callers that save errno AFTER - // forwardForeignSignal (e.g. CTimer::signalHandler) would see a clobbered - // value without this guard. + // chained handler) may set errno. Without this guard, that clobbered + // value would leak to the code that was interrupted by the signal. ErrnoPreserver errno_preserver; if (signo <= 0 || signo >= MAX_SIGNALS) { return; diff --git a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp index a5da093f2b..242716e089 100644 --- a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp +++ b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp @@ -764,8 +764,9 @@ class PerfFdRearmGuard { public: PerfFdRearmGuard(int fd, int tid) : _fd(fd), _tid(tid) {} ~PerfFdRearmGuard() { - // These calls must not leak an errno change to the caller. - ErrnoPreserver errno_preserver; + // 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); ioctl(_fd, PERF_EVENT_IOC_RESET, 0); ioctl(_fd, PERF_EVENT_IOC_REFRESH, 1); diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index 562203aa5b..d43b6ce587 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -205,6 +205,7 @@ bool BaseWallClock::inSyscall(void *ucontext) { void WallClockASGCT::sharedSignalHandler(int signo, siginfo_t *siginfo, void *ucontext) { + ErrnoPreserver errno_preserver; // Reject any SIGVTALRM that did not originate from our rt_tgsigqueueinfo // send. Defends against stray in-process tgkill / external sigqueue that // would otherwise drive our wallclock sampling path. @@ -232,7 +233,6 @@ void WallClockASGCT::sharedSignalHandler(int signo, siginfo_t *siginfo, void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, u64 last_sample, ProfiledThread* current) { - ErrnoPreserver errno_preserver; assert(current != nullptr); // Atomically try to enter critical section - prevents all reentrancy races CriticalSection cs(current); @@ -411,6 +411,7 @@ void WallClockASGCT::timerLoop() { void WallClockJvmti::sharedSignalHandler(int signo, siginfo_t *siginfo, void *ucontext) { + ErrnoPreserver errno_preserver; // Reject any SIGVTALRM that did not originate from our rt_tgsigqueueinfo // send (mirrors WallClockASGCT). Defends against stray in-process tgkill or // external sigqueue driving the JVMTI RequestStackTrace path. @@ -439,7 +440,6 @@ void WallClockJvmti::sharedSignalHandler(int signo, siginfo_t *siginfo, void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext, u64 last_sample, ProfiledThread* current) { - ErrnoPreserver errno_preserver; assert(current != nullptr); CriticalSection cs(current); if (!cs.entered()) { diff --git a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp new file mode 100644 index 0000000000..309d3e31f1 --- /dev/null +++ b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp @@ -0,0 +1,94 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +// Unit tests for tickInitWindowIfNeeded() (jvmThread.h). +// +// This gtest binary has no live JVM attached, so JVMThread::_jvm_thread's +// pthread key is never established by the normal path (see jvmSupport_ut.cpp +// for why JVMThread::initialize() can never succeed here) and +// JVMThread::current() asserts if called with that key still invalid. +// JVMThreadTestAccessor below fabricates a valid key via the same +// ThreadLocal::initialize() scan the real JVM startup path uses, +// backed by a plain pthread key this test controls directly -- letting +// JVMThread::current() be driven to both null and non-null without a JVM. + +#include +#include +#include "jvmThread.h" +#include "threadLocalData.inline.h" + +class JVMThreadTestAccessor { +public: + static bool initializeKey(void* current_thread_marker) { + return JVMThread::_jvm_thread.initialize(current_thread_marker); + } +}; + +class TickInitWindowTest : public ::testing::Test { +protected: + void SetUp() override { + ProfiledThread::initCurrentThread(); + _pt = ProfiledThread::current(); + ASSERT_NE(nullptr, _pt); + + ASSERT_EQ(0, pthread_key_create(&_key, nullptr)); + void* marker = reinterpret_cast(this); + ASSERT_EQ(0, pthread_setspecific(_key, marker)); + ASSERT_TRUE(JVMThreadTestAccessor::initializeKey(marker)); + } + + void TearDown() override { + pthread_key_delete(_key); + ProfiledThread::release(); + } + + // JVMThread::current() reads whatever this test last stored in _key. + void setJvmThreadCurrent(void* value) { + ASSERT_EQ(0, pthread_setspecific(_key, value)); + } + + ProfiledThread* _pt = nullptr; + pthread_key_t _key = 0; +}; + +// JVMThread::current() == nullptr, not in window -> false. +TEST_F(TickInitWindowTest, NoJvmThreadNotInWindowReturnsFalse) { + setJvmThreadCurrent(nullptr); + ASSERT_FALSE(_pt->inInitWindow()); + + EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); +} + +// JVMThread::current() == nullptr, in window -> true, and the window ticks +// down. This is the pure-native-thread case tickInitWindowIfNeeded exists +// for. +TEST_F(TickInitWindowTest, NoJvmThreadInWindowReturnsTrueAndTicks) { + setJvmThreadCurrent(nullptr); + _pt->startInitWindow(); + ASSERT_TRUE(_pt->inInitWindow()); + + EXPECT_TRUE(tickInitWindowIfNeeded(_pt)); + EXPECT_FALSE(_pt->inInitWindow()); // one-shot window exhausted by the tick +} + +// JVMThread::current() != nullptr, not in window -> false. +TEST_F(TickInitWindowTest, HasJvmThreadNotInWindowReturnsFalse) { + setJvmThreadCurrent(reinterpret_cast(this)); + ASSERT_FALSE(_pt->inInitWindow()); + + EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); +} + +// JVMThread::current() != nullptr, in window -> still false: a thread the +// JVM already knows about must never take the init-window bypass. This is +// the case that would break if && were mutated to ||. +TEST_F(TickInitWindowTest, HasJvmThreadInWindowReturnsFalse) { + setJvmThreadCurrent(reinterpret_cast(this)); + _pt->startInitWindow(); + ASSERT_TRUE(_pt->inInitWindow()); + + EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); + EXPECT_TRUE(_pt->inInitWindow()); // not consumed: the call was a no-op +} From c87b241ad79d21aa2c0af9c54fadf53dea3ce732 Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Thu, 3 Sep 2026 08:44:43 -0400 Subject: [PATCH 07/12] Don't delete the fabricated pthread key in jvmThread_ut TearDown JVMThread::_jvm_thread is a static that keeps using whatever key it last scanned; deleting it here (per Copilot review on PR #756) leaves that static holding a dangling key between tests, fighting the class's own never-deleted key contract. Leak it instead, like the real JVM-owned key it stands in for. --- ddprof-lib/src/test/cpp/jvmThread_ut.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp index 309d3e31f1..fe762505dc 100644 --- a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp +++ b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp @@ -40,7 +40,11 @@ class TickInitWindowTest : public ::testing::Test { } void TearDown() override { - pthread_key_delete(_key); + // Deliberately not calling pthread_key_delete(_key): JVMThread::_jvm_thread + // is a static that keeps using whatever key the next test's SetUp scans + // into it, mirroring the real (JVM-owned, never-deleted) key it normally + // reflects. Deleting it here would leave that static holding a dangling + // key for the brief window before the next SetUp re-scans it. ProfiledThread::release(); } From c7a8d57ea23abd8feb016addc6e2cb97c1c6b90a Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Thu, 3 Sep 2026 14:03:12 -0400 Subject: [PATCH 08/12] Address remaining PR #756 review comments - guards.h: clarify SIGNAL_HANDLER_GUARD_OR_DROP() doc to note that guards needing cleanup on the drop path (e.g. PerfFdRearmGuard) must be declared before the macro too. - jvmThread_ut.cpp: assert the key-discovery scan finds the right key, and fix the resulting flaky test by using a monotonically unique marker instead of `this` (whose address gets reused across test instances, colliding with never-deleted stale keys from earlier tests). - jvmThread.h: drop the heavy threadLocalData.h include added only to support tickInitWindowIfNeeded(); move that helper to threadLocalData.inline.h instead, where ProfiledThread is already available, keeping jvmThread.h forward-declaration-only again. --- ddprof-lib/src/main/cpp/ctimer_linux.cpp | 1 - ddprof-lib/src/main/cpp/guards.h | 14 +++++--- ddprof-lib/src/main/cpp/itimer.cpp | 1 - ddprof-lib/src/main/cpp/jvmThread.h | 17 ---------- .../src/main/cpp/threadLocalData.inline.h | 16 +++++++++ ddprof-lib/src/main/cpp/wallClock.cpp | 1 - ddprof-lib/src/test/cpp/jvmThread_ut.cpp | 33 +++++++++++++++---- 7 files changed, 52 insertions(+), 31 deletions(-) diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp index 0ad7907c28..4ce5a6d5a3 100644 --- a/ddprof-lib/src/main/cpp/ctimer_linux.cpp +++ b/ddprof-lib/src/main/cpp/ctimer_linux.cpp @@ -22,7 +22,6 @@ #include "ctimer.h" #include "signalInflight.h" #include "debugSupport.h" -#include "jvmThread.h" #include "libraries.h" #include "log.h" #include "profiler.h" diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 97dc1c8bf3..25fe0209c0 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -102,12 +102,16 @@ class SignalHandlerScope { }; // Declare a scope guard local that increments the depth on entry and -// decrements on scope exit. Use as the first statement after any -// foreign-signal-origin rejection check, before any profiling-owned work. -// If the handler also needs to preserve errno across its early returns, -// declare an ErrnoPreserver before this macro -- it must be the +// decrements on scope exit. In the common case, use as the first statement +// after any foreign-signal-origin rejection check, before any profiling-owned +// work. If the handler also needs to preserve errno across its early +// returns, declare an ErrnoPreserver before this macro -- it must be the // first-declared local in the handler so it destructs last, after every -// other guard (including this one) has had a chance to touch errno. +// other guard (including this one) has had a chance to touch errno. The same +// ordering applies to any other guard whose cleanup must still run on the +// drop path below (e.g. PerfEvents::signalHandler's PerfFdRearmGuard): +// declare it before this macro too, so its destructor still fires when +// SIGNAL_HANDLER_GUARD_OR_DROP() returns early. #define SIGNAL_HANDLER_GUARD_OR_DROP() \ SignalHandlerScope _signal_handler_scope(true); \ if (!_signal_handler_scope.isActive()) { \ diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index d0d219a25f..2ddf18c2e9 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -18,7 +18,6 @@ #include "itimer.h" #include "counters.h" #include "debugSupport.h" -#include "jvmThread.h" #include "os.h" #include "profiler.h" #include "signalInflight.h" diff --git a/ddprof-lib/src/main/cpp/jvmThread.h b/ddprof-lib/src/main/cpp/jvmThread.h index 421d02028c..196ef1153e 100644 --- a/ddprof-lib/src/main/cpp/jvmThread.h +++ b/ddprof-lib/src/main/cpp/jvmThread.h @@ -10,7 +10,6 @@ #include #include "threadLocal.h" -#include "threadLocalData.h" /** * JVMThread represents a native JVM thread that is JVM implementation agnostic @@ -55,20 +54,4 @@ class JVMThread { static void* currentThreadSlow(); }; -// Shared init-window guard used by the CPU/wall profiling signal handlers. -// Guards against the race window between Profiler::registerThread() and -// thread_native_entry setting JVM TLS (PROF-13072): a pure native thread -// (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. All call -// sites are signal handlers holding an ErrnoPreserver, so no manual errno -// handling is needed on this return path. -static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { - if (JVMThread::current() == nullptr && current->inInitWindow()) { - current->tickInitWindow(); - return true; - } - return false; -} - #endif // _JVMTHREAD_H diff --git a/ddprof-lib/src/main/cpp/threadLocalData.inline.h b/ddprof-lib/src/main/cpp/threadLocalData.inline.h index 856b7facc7..20ebf39683 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.inline.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.inline.h @@ -7,6 +7,7 @@ #define THREADLOCALDATA_INLINE_H #include "guards.h" +#include "jvmThread.h" #include "os.h" #include "threadLocalData.h" #include "threadLocalDataPool.h" @@ -51,5 +52,20 @@ inline bool ProfiledThread::claimAcquire(int tid) { return false; } +// Shared init-window guard used by the CPU/wall profiling signal handlers. +// Guards against the race window between Profiler::registerThread() and +// thread_native_entry setting JVM TLS (PROF-13072): a pure native thread +// (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. All call +// sites are signal handlers holding an ErrnoPreserver, so no manual errno +// handling is needed on this return path. +static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { + if (JVMThread::current() == nullptr && current->inInitWindow()) { + current->tickInitWindow(); + return true; + } + return false; +} #endif // THREADLOCALDATA_INLINE_H diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index d43b6ce587..67413dc4f3 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -11,7 +11,6 @@ #include "context.h" #include "context_api.h" #include "debugSupport.h" -#include "jvmThread.h" #include "libraries.h" #include "log.h" #include "otel_context.h" diff --git a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp index fe762505dc..7e1b49d71a 100644 --- a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp +++ b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -// Unit tests for tickInitWindowIfNeeded() (jvmThread.h). +// Unit tests for tickInitWindowIfNeeded() (threadLocalData.inline.h). // // This gtest binary has no live JVM attached, so JVMThread::_jvm_thread's // pthread key is never established by the normal path (see jvmSupport_ut.cpp @@ -16,9 +16,24 @@ #include #include +#include +#include #include "jvmThread.h" #include "threadLocalData.inline.h" +namespace { +// A marker value guaranteed unique across the whole test binary run, so the +// initializeKey() scan below can never land on a stale key from an earlier +// test. Using `this` here would not be safe: successive TickInitWindowTest +// instances are allocated and freed by gtest between tests, so the allocator +// commonly hands the same address back out, and an earlier test's key (never +// deleted -- see TearDown) would still hold that now-reused address. +void* nextUniqueMarker() { + static std::atomic counter{1}; + return reinterpret_cast(counter.fetch_add(1)); +} +} // namespace + class JVMThreadTestAccessor { public: static bool initializeKey(void* current_thread_marker) { @@ -34,9 +49,14 @@ class TickInitWindowTest : public ::testing::Test { ASSERT_NE(nullptr, _pt); ASSERT_EQ(0, pthread_key_create(&_key, nullptr)); - void* marker = reinterpret_cast(this); - ASSERT_EQ(0, pthread_setspecific(_key, marker)); - ASSERT_TRUE(JVMThreadTestAccessor::initializeKey(marker)); + _marker = nextUniqueMarker(); + ASSERT_EQ(0, pthread_setspecific(_key, _marker)); + ASSERT_TRUE(JVMThreadTestAccessor::initializeKey(_marker)); + // initializeKey() scans all live pthread keys for one holding `marker`; + // guard against it landing on some other stale slot that happens to + // hold the same pointer value, which would make current()/ + // setJvmThreadCurrent() disagree on which key they're touching. + ASSERT_EQ(_key, JVMThread::key()); } void TearDown() override { @@ -55,6 +75,7 @@ class TickInitWindowTest : public ::testing::Test { ProfiledThread* _pt = nullptr; pthread_key_t _key = 0; + void* _marker = nullptr; }; // JVMThread::current() == nullptr, not in window -> false. @@ -79,7 +100,7 @@ TEST_F(TickInitWindowTest, NoJvmThreadInWindowReturnsTrueAndTicks) { // JVMThread::current() != nullptr, not in window -> false. TEST_F(TickInitWindowTest, HasJvmThreadNotInWindowReturnsFalse) { - setJvmThreadCurrent(reinterpret_cast(this)); + setJvmThreadCurrent(_marker); ASSERT_FALSE(_pt->inInitWindow()); EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); @@ -89,7 +110,7 @@ TEST_F(TickInitWindowTest, HasJvmThreadNotInWindowReturnsFalse) { // JVM already knows about must never take the init-window bypass. This is // the case that would break if && were mutated to ||. TEST_F(TickInitWindowTest, HasJvmThreadInWindowReturnsFalse) { - setJvmThreadCurrent(reinterpret_cast(this)); + setJvmThreadCurrent(_marker); _pt->startInitWindow(); ASSERT_TRUE(_pt->inInitWindow()); From 7d661e568d6a0ef8f7e483ce239310cdcf8a7293 Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Thu, 3 Sep 2026 15:28:01 -0400 Subject: [PATCH 09/12] jvmThread_ut: reuse one pthread key across tests instead of per-test TickInitWindowTest::SetUp() created a fresh pthread_key_create every test and never deleted it, diverging from the binary-lifetime-static key convention in threadLocal_ut.cpp and risking pthread-key exhaustion under a harness that repeats fixtures in-process (e.g. --gtest_repeat). Fix by creating one shared key once via sharedTestKey(), reused for the life of the binary; each test still gets its own unique marker value to drive/verify the key-discovery scan. --- ddprof-lib/src/test/cpp/jvmThread_ut.cpp | 35 ++++++++++++++++-------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp index 7e1b49d71a..84a0fdbb80 100644 --- a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp +++ b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp @@ -22,12 +22,26 @@ #include "threadLocalData.inline.h" namespace { +// One pthread key for the whole binary, created on first use and never +// deleted -- mirroring the ThreadLocal instances in threadLocal_ut.cpp, +// which are kept alive for the binary's lifetime rather than +// created/deleted per test to avoid exhausting pthread keys. Sharing the key +// across tests is safe here: each test still drives it with its own unique +// marker (see nextUniqueMarker()) to verify the scan below finds it. +pthread_key_t sharedTestKey() { + static pthread_key_t key = [] { + pthread_key_t k = 0; + if (pthread_key_create(&k, nullptr) != 0) { + ADD_FAILURE() << "pthread_key_create failed"; + } + return k; + }(); + return key; +} + // A marker value guaranteed unique across the whole test binary run, so the -// initializeKey() scan below can never land on a stale key from an earlier -// test. Using `this` here would not be safe: successive TickInitWindowTest -// instances are allocated and freed by gtest between tests, so the allocator -// commonly hands the same address back out, and an earlier test's key (never -// deleted -- see TearDown) would still hold that now-reused address. +// initializeKey() scan below can never land on a stale value from an +// earlier test that happens to still be sitting in sharedTestKey(). void* nextUniqueMarker() { static std::atomic counter{1}; return reinterpret_cast(counter.fetch_add(1)); @@ -48,7 +62,7 @@ class TickInitWindowTest : public ::testing::Test { _pt = ProfiledThread::current(); ASSERT_NE(nullptr, _pt); - ASSERT_EQ(0, pthread_key_create(&_key, nullptr)); + _key = sharedTestKey(); _marker = nextUniqueMarker(); ASSERT_EQ(0, pthread_setspecific(_key, _marker)); ASSERT_TRUE(JVMThreadTestAccessor::initializeKey(_marker)); @@ -60,11 +74,10 @@ class TickInitWindowTest : public ::testing::Test { } void TearDown() override { - // Deliberately not calling pthread_key_delete(_key): JVMThread::_jvm_thread - // is a static that keeps using whatever key the next test's SetUp scans - // into it, mirroring the real (JVM-owned, never-deleted) key it normally - // reflects. Deleting it here would leave that static holding a dangling - // key for the brief window before the next SetUp re-scans it. + // _key is the binary-lifetime sharedTestKey(), not deleted here (see + // its comment). JVMThread::_jvm_thread also keeps using whatever key + // was last scanned into it, which is fine since that's always this + // same shared key. ProfiledThread::release(); } From 629af9beb4c7613c5da5bbd2559c1bcc2bd0d2d3 Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Thu, 3 Sep 2026 18:24:21 -0400 Subject: [PATCH 10/12] Replace the init-window test's pthread-key rig with a plain branch test The unit test for tickInitWindowIfNeeded() had grown a rig -- a binary-lifetime pthread key, a unique marker generator, a JVMThreadTestAccessor, and a `friend` declaration in jvmThread.h -- whose only purpose was to cover one line: tickInitWindowIfNeededImpl(JVMThread::current() != nullptr, current) That trade doesn't hold up. The Impl split already exists so the branch logic is testable without a JVM; paying for the seam *and* for the fakery the seam was meant to avoid is paying twice. The rig also mutated process-global state (JVMThread::_jvm_thread ended up pointing at a test-owned key for the binary's lifetime) and leaned on ThreadLocal::initialize()'s scan over every live pthread key, which is what forced the unique-marker machinery in the first place. Keep four tests over tickInitWindowIfNeededImpl() -- all four has_jvm_thread x inInitWindow() combinations, including the one that catches && mutated to ||, plus the one-shot property -- and drop everything else. The derivation from JVMThread::current() now rests on compilation and inspection; no integration test exercises the init-window path either, so the header says that outright rather than implying coverage that doesn't exist. jvmThread.h is byte-identical to origin/main again: the PR no longer touches a production header for test access. Renamed the test to tickInitWindow_ut.cpp to match what it tests (tickInitWindowIfNeededImpl lives in threadLocalData.inline.h, not jvmThread.h) and fixed the stale filename pointer in threadLocalData.inline.h. Also adds a direct #include to itimer.cpp, found while reviewing: the assert(current != nullptr) added to ITimer::signalHandler earlier in this branch compiled only via the threadLocalData.inline.h -> jvmThread.h -> threadLocal.h chain this same branch introduced, so trimming either include would have broken the build. ctimer_linux.cpp and wallClock.cpp already include it directly. --- ddprof-lib/src/main/cpp/itimer.cpp | 1 + ddprof-lib/src/main/cpp/jvmThread.h | 1 - .../src/main/cpp/threadLocalData.inline.h | 17 ++- ddprof-lib/src/test/cpp/jvmThread_ut.cpp | 132 ------------------ ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp | 75 ++++++++++ 5 files changed, 88 insertions(+), 138 deletions(-) delete mode 100644 ddprof-lib/src/test/cpp/jvmThread_ut.cpp create mode 100644 ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index 2ddf18c2e9..409a4583f9 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -25,6 +25,7 @@ #include "threadLocalData.inline.h" #include "threadState.inline.h" #include "guards.h" +#include #include bool ITimer::_enabled = false; diff --git a/ddprof-lib/src/main/cpp/jvmThread.h b/ddprof-lib/src/main/cpp/jvmThread.h index 196ef1153e..2f5bd69104 100644 --- a/ddprof-lib/src/main/cpp/jvmThread.h +++ b/ddprof-lib/src/main/cpp/jvmThread.h @@ -16,7 +16,6 @@ */ class JVMThread { private: - friend class JVMThreadTestAccessor; static jfieldID _tid; static ThreadLocal _jvm_thread; diff --git a/ddprof-lib/src/main/cpp/threadLocalData.inline.h b/ddprof-lib/src/main/cpp/threadLocalData.inline.h index 20ebf39683..60b1cdfb13 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.inline.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.inline.h @@ -52,6 +52,17 @@ inline bool ProfiledThread::claimAcquire(int tid) { return false; } +// Core logic of tickInitWindowIfNeeded(), split out so unit tests can drive +// has_jvm_thread directly with a plain bool instead of needing a live +// JVMThread::current(). See tickInitWindow_ut.cpp. +static inline bool tickInitWindowIfNeededImpl(bool has_jvm_thread, ProfiledThread* current) { + if (!has_jvm_thread && current->inInitWindow()) { + current->tickInitWindow(); + return true; + } + return false; +} + // Shared init-window guard used by the CPU/wall profiling signal handlers. // Guards against the race window between Profiler::registerThread() and // thread_native_entry setting JVM TLS (PROF-13072): a pure native thread @@ -61,11 +72,7 @@ inline bool ProfiledThread::claimAcquire(int tid) { // sites are signal handlers holding an ErrnoPreserver, so no manual errno // handling is needed on this return path. static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { - if (JVMThread::current() == nullptr && current->inInitWindow()) { - current->tickInitWindow(); - return true; - } - return false; + return tickInitWindowIfNeededImpl(JVMThread::current() != nullptr, current); } #endif // THREADLOCALDATA_INLINE_H diff --git a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp b/ddprof-lib/src/test/cpp/jvmThread_ut.cpp deleted file mode 100644 index 84a0fdbb80..0000000000 --- a/ddprof-lib/src/test/cpp/jvmThread_ut.cpp +++ /dev/null @@ -1,132 +0,0 @@ -/* - * Copyright 2026, Datadog, Inc. - * SPDX-License-Identifier: Apache-2.0 - */ - -// Unit tests for tickInitWindowIfNeeded() (threadLocalData.inline.h). -// -// This gtest binary has no live JVM attached, so JVMThread::_jvm_thread's -// pthread key is never established by the normal path (see jvmSupport_ut.cpp -// for why JVMThread::initialize() can never succeed here) and -// JVMThread::current() asserts if called with that key still invalid. -// JVMThreadTestAccessor below fabricates a valid key via the same -// ThreadLocal::initialize() scan the real JVM startup path uses, -// backed by a plain pthread key this test controls directly -- letting -// JVMThread::current() be driven to both null and non-null without a JVM. - -#include -#include -#include -#include -#include "jvmThread.h" -#include "threadLocalData.inline.h" - -namespace { -// One pthread key for the whole binary, created on first use and never -// deleted -- mirroring the ThreadLocal instances in threadLocal_ut.cpp, -// which are kept alive for the binary's lifetime rather than -// created/deleted per test to avoid exhausting pthread keys. Sharing the key -// across tests is safe here: each test still drives it with its own unique -// marker (see nextUniqueMarker()) to verify the scan below finds it. -pthread_key_t sharedTestKey() { - static pthread_key_t key = [] { - pthread_key_t k = 0; - if (pthread_key_create(&k, nullptr) != 0) { - ADD_FAILURE() << "pthread_key_create failed"; - } - return k; - }(); - return key; -} - -// A marker value guaranteed unique across the whole test binary run, so the -// initializeKey() scan below can never land on a stale value from an -// earlier test that happens to still be sitting in sharedTestKey(). -void* nextUniqueMarker() { - static std::atomic counter{1}; - return reinterpret_cast(counter.fetch_add(1)); -} -} // namespace - -class JVMThreadTestAccessor { -public: - static bool initializeKey(void* current_thread_marker) { - return JVMThread::_jvm_thread.initialize(current_thread_marker); - } -}; - -class TickInitWindowTest : public ::testing::Test { -protected: - void SetUp() override { - ProfiledThread::initCurrentThread(); - _pt = ProfiledThread::current(); - ASSERT_NE(nullptr, _pt); - - _key = sharedTestKey(); - _marker = nextUniqueMarker(); - ASSERT_EQ(0, pthread_setspecific(_key, _marker)); - ASSERT_TRUE(JVMThreadTestAccessor::initializeKey(_marker)); - // initializeKey() scans all live pthread keys for one holding `marker`; - // guard against it landing on some other stale slot that happens to - // hold the same pointer value, which would make current()/ - // setJvmThreadCurrent() disagree on which key they're touching. - ASSERT_EQ(_key, JVMThread::key()); - } - - void TearDown() override { - // _key is the binary-lifetime sharedTestKey(), not deleted here (see - // its comment). JVMThread::_jvm_thread also keeps using whatever key - // was last scanned into it, which is fine since that's always this - // same shared key. - ProfiledThread::release(); - } - - // JVMThread::current() reads whatever this test last stored in _key. - void setJvmThreadCurrent(void* value) { - ASSERT_EQ(0, pthread_setspecific(_key, value)); - } - - ProfiledThread* _pt = nullptr; - pthread_key_t _key = 0; - void* _marker = nullptr; -}; - -// JVMThread::current() == nullptr, not in window -> false. -TEST_F(TickInitWindowTest, NoJvmThreadNotInWindowReturnsFalse) { - setJvmThreadCurrent(nullptr); - ASSERT_FALSE(_pt->inInitWindow()); - - EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); -} - -// JVMThread::current() == nullptr, in window -> true, and the window ticks -// down. This is the pure-native-thread case tickInitWindowIfNeeded exists -// for. -TEST_F(TickInitWindowTest, NoJvmThreadInWindowReturnsTrueAndTicks) { - setJvmThreadCurrent(nullptr); - _pt->startInitWindow(); - ASSERT_TRUE(_pt->inInitWindow()); - - EXPECT_TRUE(tickInitWindowIfNeeded(_pt)); - EXPECT_FALSE(_pt->inInitWindow()); // one-shot window exhausted by the tick -} - -// JVMThread::current() != nullptr, not in window -> false. -TEST_F(TickInitWindowTest, HasJvmThreadNotInWindowReturnsFalse) { - setJvmThreadCurrent(_marker); - ASSERT_FALSE(_pt->inInitWindow()); - - EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); -} - -// JVMThread::current() != nullptr, in window -> still false: a thread the -// JVM already knows about must never take the init-window bypass. This is -// the case that would break if && were mutated to ||. -TEST_F(TickInitWindowTest, HasJvmThreadInWindowReturnsFalse) { - setJvmThreadCurrent(_marker); - _pt->startInitWindow(); - ASSERT_TRUE(_pt->inInitWindow()); - - EXPECT_FALSE(tickInitWindowIfNeeded(_pt)); - EXPECT_TRUE(_pt->inInitWindow()); // not consumed: the call was a no-op -} diff --git a/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp b/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp new file mode 100644 index 0000000000..0bebc58136 --- /dev/null +++ b/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp @@ -0,0 +1,75 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +// Unit tests for tickInitWindowIfNeededImpl() (threadLocalData.inline.h). +// +// tickInitWindowIfNeeded() takes "does the JVM already know this thread" from +// JVMThread::current(); tickInitWindowIfNeededImpl() takes it as a plain bool. +// The tests below drive that bool directly, covering all four +// has_jvm_thread x inInitWindow() combinations with no JVM involved. +// +// The one-line derivation in tickInitWindowIfNeeded() itself is deliberately +// not unit tested. JVMThread::current() reads a pthread key established by +// JVM startup, and this gtest binary has no JVM attached; fabricating that +// key means mutating process-global state and leaning on +// ThreadLocal::initialize()'s scan over every live pthread key, +// which costs far more than a single `!= nullptr` argument expression that +// compilation and inspection already cover. + +#include +#include "threadLocalData.inline.h" + +class TickInitWindowImplTest : public ::testing::Test { +protected: + void SetUp() override { + ProfiledThread::initCurrentThread(); + _pt = ProfiledThread::current(); + ASSERT_NE(nullptr, _pt); + } + + void TearDown() override { + ProfiledThread::release(); + } + + ProfiledThread* _pt = nullptr; +}; + +// !has_jvm_thread, not in window -> false. +TEST_F(TickInitWindowImplTest, NoJvmThreadNotInWindowReturnsFalse) { + ASSERT_FALSE(_pt->inInitWindow()); + + EXPECT_FALSE(tickInitWindowIfNeededImpl(false, _pt)); +} + +// !has_jvm_thread, in window -> true, and the window ticks down. This is the +// pure-native-thread case tickInitWindowIfNeeded exists for. +TEST_F(TickInitWindowImplTest, NoJvmThreadInWindowReturnsTrueAndTicks) { + _pt->startInitWindow(); + ASSERT_TRUE(_pt->inInitWindow()); + + EXPECT_TRUE(tickInitWindowIfNeededImpl(false, _pt)); + EXPECT_FALSE(_pt->inInitWindow()); // one-shot window exhausted by the tick + + // The window is one-shot: a second call must return false, not tick again. + EXPECT_FALSE(tickInitWindowIfNeededImpl(false, _pt)); +} + +// has_jvm_thread, not in window -> false. +TEST_F(TickInitWindowImplTest, HasJvmThreadNotInWindowReturnsFalse) { + ASSERT_FALSE(_pt->inInitWindow()); + + EXPECT_FALSE(tickInitWindowIfNeededImpl(true, _pt)); +} + +// has_jvm_thread, in window -> still false: a thread the JVM already knows +// about must never take the init-window bypass. This is the case that would +// break if && were mutated to ||. +TEST_F(TickInitWindowImplTest, HasJvmThreadInWindowReturnsFalse) { + _pt->startInitWindow(); + ASSERT_TRUE(_pt->inInitWindow()); + + EXPECT_FALSE(tickInitWindowIfNeededImpl(true, _pt)); + EXPECT_TRUE(_pt->inInitWindow()); // not consumed: the call was a no-op +} From 6268c57538f5885e904cfece4ef0fe1d7168eb6c Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Fri, 4 Sep 2026 08:59:42 -0400 Subject: [PATCH 11/12] Address PR #756 review round: naming, scoping, comments, ErrnoPreserver tests - Move "int tid" into the SighandlerTidScope block in the four handlers flagged; rename the guard local to snake_case at all seven sites. - Add the missing assert(current != nullptr) to CTimerJvmti::signalHandler. - guards.h: note the siglongjmp caveat and SighandlerTidScope's non-nesting precondition; trim the ErrnoPreserver comment to its forward contract. - perfEvents: declare ErrnoPreserver below the si_code <= 0 fast bail (still ahead of PerfFdRearmGuard) so external signals don't touch errno. - threadLocalData.inline.h: plain "inline" instead of "static inline", assert the non-null precondition, drop the ticket ID, state the ErrnoPreserver contract as a caller requirement, and short-circuit on inInitWindow() so the TLS lookup stays off the hot path. - Record at the ITimer/PerfEvents sites why the init-window guard is absent. - Add guards_ut.cpp for ErrnoPreserver; give tickInitWindow_ut.cpp the standard gtest crash-handler rig. --- ddprof-lib/src/main/cpp/ctimer_linux.cpp | 9 +-- ddprof-lib/src/main/cpp/guards.h | 39 ++++++----- ddprof-lib/src/main/cpp/itimer.cpp | 11 +-- ddprof-lib/src/main/cpp/perfEvents_linux.cpp | 13 +++- .../src/main/cpp/threadLocalData.inline.h | 29 +++++--- ddprof-lib/src/main/cpp/wallClock.cpp | 4 +- ddprof-lib/src/test/cpp/guards_ut.cpp | 69 +++++++++++++++++++ ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp | 5 ++ 8 files changed, 139 insertions(+), 40 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/guards_ut.cpp diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp index 4ce5a6d5a3..61e7cf62c8 100644 --- a/ddprof-lib/src/main/cpp/ctimer_linux.cpp +++ b/ddprof-lib/src/main/cpp/ctimer_linux.cpp @@ -216,6 +216,7 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { SIGNAL_HANDLER_GUARD_OR_DROP(); InflightGuard inflight; ProfiledThread *current = SIGNAL_HANDLER_CURRENT_THREAD(); + assert(current != nullptr); assert(!current->isDeepCrashHandler()); CriticalSection cs(current); @@ -231,10 +232,10 @@ void CTimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { } current->noteCPUSample(Profiler::instance()->recordingEpoch()); - int tid = current->tid(); { - SighandlerTidScope sighandlerTid(tid); + int tid = current->tid(); + SighandlerTidScope sighandler_tid(tid); ExecutionEvent event; event._execution_mode = getThreadExecutionMode(); // Opted into JVMTI delegation; drop the sample if the JVM rejects the @@ -280,10 +281,10 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { return; } current->noteCPUSample(Profiler::instance()->recordingEpoch()); - int tid = current->tid(); { - SighandlerTidScope sighandlerTid(tid); + int tid = current->tid(); + SighandlerTidScope sighandler_tid(tid); ExecutionEvent event; event._execution_mode = getThreadExecutionMode(); Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index 25fe0209c0..708eab8f3e 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -313,14 +313,20 @@ class SignalBlocker { /** * RAII guard around the span of a signal handler during which the current * thread is the one being sampled. Sets Shims::instance().setSighandlerTid(tid) - * on construction and resets it to -1 on destruction, guaranteeing the reset - * happens on every return path out of the guarded scope. + * on construction and resets it to -1 on destruction, so the reset happens on + * every normal return path out of the guarded scope. A siglongjmp that unwinds + * past this frame (see the chained-handler note in Profiler::segvHandler) + * bypasses the destructor and leaves the tid pinned, matching the behaviour of + * the manual set/reset statements this guard replaces. * - * Must be scoped narrowly around exactly the existing set/reset span (right - * before recordSample and right after) rather than wrapped around the whole - * handler: widening the scope would change the window during which the - * sighandler tid is observably set for other consumers of Shims (e.g. - * crash-handler / re-entrant stack-walking code). + * Not nesting-safe: the destructor restores a hardcoded -1 rather than the + * previous tid. Every current call site sits inside a CriticalSection, which + * rules out a second live guard on the same thread. + * + * Must be scoped narrowly around the recordSample call rather than wrapped + * around the whole handler: widening the scope would change the window during + * which the sighandler tid is observably set for other consumers of Shims + * (e.g. crash-handler / re-entrant stack-walking code). */ class SighandlerTidScope { public: @@ -338,19 +344,14 @@ class SighandlerTidScope { /** * RAII guard that saves errno on construction and restores it on - * destruction, regardless of which return path is taken in between. - * - * Replaces the previous pattern of `int saved_errno = errno;` at handler - * entry paired with a manual `errno = saved_errno;` before every return -- - * a pattern that is easy to miss on a newly added return and, even when - * done consistently, still leaves a gap: any destructor that runs after - * the manual restore (e.g. a signal-scope guard making a syscall) can - * clobber errno again before the handler actually exits. + * destruction, regardless of which normal return path is taken in between. + * (A siglongjmp past this frame bypasses the destructor, as it does for any + * RAII guard here.) * - * To close that gap, declare the ErrnoPreserver as the *first* local in - * the guarded function: C++ destroys locals in reverse declaration order, - * so it destructs last, after every other guard's destructor has already - * run. + * Declare it as the *first* local in the guarded function, ahead of every + * other guard whose cleanup may touch errno: C++ destroys locals in reverse + * declaration order, so it then destructs last and its restore is the final + * word on errno. */ class ErrnoPreserver { public: diff --git a/ddprof-lib/src/main/cpp/itimer.cpp b/ddprof-lib/src/main/cpp/itimer.cpp index 409a4583f9..623cd6a0f1 100644 --- a/ddprof-lib/src/main/cpp/itimer.cpp +++ b/ddprof-lib/src/main/cpp/itimer.cpp @@ -54,11 +54,14 @@ void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { if (!cs.entered()) { return; // Another critical section is active, defer profiling } + // The init-window guard (tickInitWindowIfNeeded()) that the CTimer/WallClock + // handlers run here is deliberately not applied: this engine never had it, + // and adding it would be a behaviour change beyond a boilerplate extraction. current->noteCPUSample(Profiler::instance()->recordingEpoch()); - int tid = current->tid(); { - SighandlerTidScope sighandlerTid(tid); + int tid = current->tid(); + SighandlerTidScope sighandler_tid(tid); ExecutionEvent event; event._execution_mode = getThreadExecutionMode(); Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, @@ -122,11 +125,11 @@ void ITimerJvmti::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { if (tickInitWindowIfNeeded(current)) { return; } - int tid = current->tid(); current->noteCPUSample(Profiler::instance()->recordingEpoch()); { - SighandlerTidScope sighandlerTid(tid); + int tid = current->tid(); + SighandlerTidScope sighandler_tid(tid); ExecutionEvent event; event._execution_mode = getThreadExecutionMode(); // setitimer(ITIMER_PROF) delivers SIGPROF to an arbitrary thread chosen by diff --git a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp index 242716e089..5d59f69b72 100644 --- a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp +++ b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp @@ -780,11 +780,15 @@ class PerfFdRearmGuard { }; void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { - ErrnoPreserver errno_preserver; if (siginfo->si_code <= 0) { - // Looks like an external signal; don't treat as a profiling event + // Looks like an external signal; don't treat as a profiling event. + // Nothing profiler-owned has run yet, so errno cannot have been touched + // and this path needs no ErrnoPreserver. return; } + // Must precede PerfFdRearmGuard so it destructs after it and restores the + // errno that the guard's ioctl()/resetBuffer() calls clobber. + ErrnoPreserver errno_preserver; PerfFdRearmGuard rearm(siginfo->si_fd, OS::threadId()); SIGNAL_HANDLER_GUARD_OR_DROP(); InflightGuard inflight; @@ -803,10 +807,13 @@ void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { if (!cs.entered()) { return; // Another critical section is active, defer profiling } + // The init-window guard (tickInitWindowIfNeeded()) that the CTimer/WallClock + // handlers run here is deliberately not applied: this engine never had it, + // and adding it would be a behaviour change beyond a boilerplate extraction. current->noteCPUSample(Profiler::instance()->recordingEpoch()); int tid = current->tid(); if (__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE)) { - SighandlerTidScope sighandlerTid(tid); + SighandlerTidScope sighandler_tid(tid); u64 counter = readCounter(siginfo, ucontext); ExecutionEvent event; diff --git a/ddprof-lib/src/main/cpp/threadLocalData.inline.h b/ddprof-lib/src/main/cpp/threadLocalData.inline.h index 60b1cdfb13..3a1f99732c 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.inline.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.inline.h @@ -11,6 +11,7 @@ #include "os.h" #include "threadLocalData.h" #include "threadLocalDataPool.h" +#include inline ProfiledThread* ProfiledThread::current() { if (!isThreadKeyValid()) { @@ -55,7 +56,9 @@ inline bool ProfiledThread::claimAcquire(int tid) { // Core logic of tickInitWindowIfNeeded(), split out so unit tests can drive // has_jvm_thread directly with a plain bool instead of needing a live // JVMThread::current(). See tickInitWindow_ut.cpp. -static inline bool tickInitWindowIfNeededImpl(bool has_jvm_thread, ProfiledThread* current) { +// `current` must not be null. +inline bool tickInitWindowIfNeededImpl(bool has_jvm_thread, ProfiledThread* current) { + assert(current != nullptr); if (!has_jvm_thread && current->inInitWindow()) { current->tickInitWindow(); return true; @@ -65,13 +68,23 @@ static inline bool tickInitWindowIfNeededImpl(bool has_jvm_thread, ProfiledThrea // Shared init-window guard used by the CPU/wall profiling signal handlers. // Guards against the race window between Profiler::registerThread() and -// thread_native_entry setting JVM TLS (PROF-13072): a pure native thread -// (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. All call -// sites are signal handlers holding an ErrnoPreserver, so no manual errno -// handling is needed on this return path. -static inline bool tickInitWindowIfNeeded(ProfiledThread* current) { +// thread_native_entry setting JVM TLS: a pure native thread (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. +// +// Preconditions: `current` must not be null, and the caller must be a signal +// handler with an ErrnoPreserver live in its own frame or an enclosing one -- +// this return path does no manual errno handling. (For the wallclock engines +// that guard lives one frame up, in sharedSignalHandler.) +inline bool tickInitWindowIfNeeded(ProfiledThread* current) { + // Cheap per-thread byte first: the window is closed for the rest of the + // thread's life after the first signal, so this keeps the pthread TLS + // lookup off the hot path. The full condition is a conjunction, so the + // ordering is semantically neutral. + if (!current->inInitWindow()) { + return false; + } return tickInitWindowIfNeededImpl(JVMThread::current() != nullptr, current); } diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index 67413dc4f3..4d39c73a4b 100644 --- a/ddprof-lib/src/main/cpp/wallClock.cpp +++ b/ddprof-lib/src/main/cpp/wallClock.cpp @@ -254,7 +254,7 @@ void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext int tid = current->tid(); { - SighandlerTidScope sighandlerTid(tid); + SighandlerTidScope sighandler_tid(tid); u64 call_trace_id = 0; if (_collapsing) { StackFrame frame(ucontext); @@ -456,7 +456,7 @@ void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, int tid = current->tid(); { - SighandlerTidScope sighandlerTid(tid); + SighandlerTidScope sighandler_tid(tid); ExecutionEvent event; OSThreadState state = precheck.observed_state_valid ? precheck.observed_state : getOSThreadState(); diff --git a/ddprof-lib/src/test/cpp/guards_ut.cpp b/ddprof-lib/src/test/cpp/guards_ut.cpp new file mode 100644 index 0000000000..e2314de119 --- /dev/null +++ b/ddprof-lib/src/test/cpp/guards_ut.cpp @@ -0,0 +1,69 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +// Unit tests for ErrnoPreserver (guards.h): errno is restored when the +// guarded scope exits, and -- because the guard is declared first and so +// destructs last -- the restore is the final word even when a later guard's +// destructor clobbers errno on its way out. + +#include +#include "guards.h" +#include "gtest_crash_handler.h" +#include + +static constexpr char GUARDS_TEST_NAME[] = "GuardsTest"; + +class GuardsTest : public ::testing::Test { +protected: + void SetUp() override { + installGtestCrashHandler(); + } + void TearDown() override { + restoreDefaultSignalHandlers(); + } +}; + +namespace { +// Stand-in for a guard whose destructor makes a syscall (e.g. +// PerfFdRearmGuard's ioctl()) and therefore leaves errno clobbered. +class ErrnoClobberingGuard { +public: + explicit ErrnoClobberingGuard(int value) : _value(value) {} + ~ErrnoClobberingGuard() { errno = _value; } +private: + int _value; +}; +} + +TEST_F(GuardsTest, ErrnoPreserverRestoresOnScopeExit) { + errno = EAGAIN; + { + ErrnoPreserver errno_preserver; + errno = EINVAL; + } + EXPECT_EQ(EAGAIN, errno); +} + +TEST_F(GuardsTest, ErrnoPreserverRestoresOnEarlyReturn) { + errno = EAGAIN; + [] { + ErrnoPreserver errno_preserver; + errno = EINVAL; + return; + }(); + EXPECT_EQ(EAGAIN, errno); +} + +// The ordering invariant the handlers rely on: declared first, the preserver +// destructs last, after the clobbering guard has already run. +TEST_F(GuardsTest, ErrnoPreserverDeclaredFirstOutlivesClobberingGuard) { + errno = EAGAIN; + { + ErrnoPreserver errno_preserver; + ErrnoClobberingGuard clobber(EPIPE); + errno = EINVAL; + } + EXPECT_EQ(EAGAIN, errno); +} diff --git a/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp b/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp index 0bebc58136..8ad77fddff 100644 --- a/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp +++ b/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp @@ -20,10 +20,14 @@ #include #include "threadLocalData.inline.h" +#include "gtest_crash_handler.h" + +static constexpr char TICKINITWINDOW_TEST_NAME[] = "TickInitWindowImplTest"; class TickInitWindowImplTest : public ::testing::Test { protected: void SetUp() override { + installGtestCrashHandler(); ProfiledThread::initCurrentThread(); _pt = ProfiledThread::current(); ASSERT_NE(nullptr, _pt); @@ -31,6 +35,7 @@ class TickInitWindowImplTest : public ::testing::Test { void TearDown() override { ProfiledThread::release(); + restoreDefaultSignalHandlers(); } ProfiledThread* _pt = nullptr; From c81d4ad36a7a3ab0ad804175f571adf362af7e2d Mon Sep 17 00:00:00 2001 From: "yaron.gurovich" Date: Fri, 4 Sep 2026 09:05:08 -0400 Subject: [PATCH 12/12] Assert the non-null precondition in tickInitWindowIfNeeded() too The short-circuit on inInitWindow() dereferences current in the wrapper, ahead of the assert in tickInitWindowIfNeededImpl(), so production call paths lost the debug/ASan diagnostic that assert was added for. --- ddprof-lib/src/main/cpp/threadLocalData.inline.h | 1 + 1 file changed, 1 insertion(+) diff --git a/ddprof-lib/src/main/cpp/threadLocalData.inline.h b/ddprof-lib/src/main/cpp/threadLocalData.inline.h index 3a1f99732c..e129f25a76 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.inline.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.inline.h @@ -78,6 +78,7 @@ inline bool tickInitWindowIfNeededImpl(bool has_jvm_thread, ProfiledThread* curr // this return path does no manual errno handling. (For the wallclock engines // that guard lives one frame up, in sharedSignalHandler.) inline bool tickInitWindowIfNeeded(ProfiledThread* current) { + assert(current != nullptr); // Cheap per-thread byte first: the window is closed for the rest of the // thread's life after the first signal, so this keeps the pthread TLS // lookup off the hot path. The full condition is a conjunction, so the