diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp index 6cf46b2a7..61e7cf62c 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" @@ -206,7 +205,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,48 +213,42 @@ 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 != nullptr); 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 (JVMThread::current() == nullptr - && current->inInitWindow()) { - current->tickInitWindow(); - errno = saved_errno; + if (tickInitWindowIfNeeded(current)) { return; } 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); - errno = saved_errno; + + { + 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 + // 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); + } } 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 @@ -268,7 +261,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); @@ -284,26 +277,19 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { 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(); - errno = saved_errno; + if (tickInitWindowIfNeeded(current)) { 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); - // we need to avoid spoiling the value of errno (tsan report) - errno = saved_errno; + + { + int tid = current->tid(); + SighandlerTidScope sighandler_tid(tid); + ExecutionEvent event; + event._execution_mode = getThreadExecutionMode(); + Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, + &event); + } } #endif // __linux__ diff --git a/ddprof-lib/src/main/cpp/guards.h b/ddprof-lib/src/main/cpp/guards.h index e4c881582..708eab8f3 100644 --- a/ddprof-lib/src/main/cpp/guards.h +++ b/ddprof-lib/src/main/cpp/guards.h @@ -20,11 +20,13 @@ #include #include #include +#include #include #include #include "common.h" #include "counters.h" +#include "debugSupport.h" class ProfiledThread; @@ -99,24 +101,24 @@ class SignalHandlerScope { DEBUG_ONLY(int _signal_depth;) }; -// 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. 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. 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()) { \ 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 @@ -308,4 +310,60 @@ 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, 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. + * + * 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: + explicit SighandlerTidScope(int tid) { + Shims::instance().setSighandlerTid(tid); + } + ~SighandlerTidScope() { + Shims::instance().setSighandlerTid(-1); + } + + // Non-copyable + SighandlerTidScope(const SighandlerTidScope&) = delete; + SighandlerTidScope& operator=(const SighandlerTidScope&) = delete; +}; + +/** + * RAII guard that saves errno on construction and restores it on + * 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.) + * + * 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: + 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 e4c02dcaf..623cd6a0f 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" @@ -26,6 +25,7 @@ #include "threadLocalData.inline.h" #include "threadState.inline.h" #include "guards.h" +#include #include bool ITimer::_enabled = false; @@ -33,6 +33,7 @@ long ITimer::_interval; CStack ITimer::_cstack; void ITimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { + 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 @@ -41,25 +42,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)) { 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()) { 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(); - Shims::instance().setSighandlerTid(tid); - - ExecutionEvent event; - event._execution_mode = getThreadExecutionMode(); - Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, - &event); - Shims::instance().setSighandlerTid(-1); + + { + int tid = current->tid(); + SighandlerTidScope sighandler_tid(tid); + ExecutionEvent event; + event._execution_mode = getThreadExecutionMode(); + Profiler::instance()->recordSample(ucontext, _interval, tid, BCI_CPU, 0, + &event); + } } Error ITimer::check(Arguments &args) { @@ -102,40 +109,35 @@ 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 (JVMThread::current() == nullptr - && current->inInitWindow()) { - current->tickInitWindow(); - errno = saved_errno; + if (tickInitWindowIfNeeded(current)) { 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); - errno = saved_errno; + + { + 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 + // 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); + } } 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 bc01fbc40..361f4284d 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" @@ -471,19 +472,16 @@ 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. - int saved_errno = errno; + // 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) { - 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 +574,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 711a76f4c..5d59f69b7 100644 --- a/ddprof-lib/src/main/cpp/perfEvents_linux.cpp +++ b/ddprof-lib/src/main/cpp/perfEvents_linux.cpp @@ -764,6 +764,9 @@ class PerfFdRearmGuard { public: PerfFdRearmGuard(int fd, int tid) : _fd(fd), _tid(tid) {} ~PerfFdRearmGuard() { + // Errno changes made here are caught by the handler-level + // ErrnoPreserver, which is declared before this guard and therefore + // destructs after it. PerfEvents::resetBuffer(_tid); ioctl(_fd, PERF_EVENT_IOC_RESET, 0); ioctl(_fd, PERF_EVENT_IOC_REFRESH, 1); @@ -778,9 +781,14 @@ class PerfFdRearmGuard { void PerfEvents::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) { 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; @@ -799,17 +807,19 @@ 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)) { - Shims::instance().setSighandlerTid(tid); + SighandlerTidScope sighandler_tid(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); } } diff --git a/ddprof-lib/src/main/cpp/threadLocalData.inline.h b/ddprof-lib/src/main/cpp/threadLocalData.inline.h index 856b7facc..e129f25a7 100644 --- a/ddprof-lib/src/main/cpp/threadLocalData.inline.h +++ b/ddprof-lib/src/main/cpp/threadLocalData.inline.h @@ -7,9 +7,11 @@ #define THREADLOCALDATA_INLINE_H #include "guards.h" +#include "jvmThread.h" #include "os.h" #include "threadLocalData.h" #include "threadLocalDataPool.h" +#include inline ProfiledThread* ProfiledThread::current() { if (!isThreadKeyValid()) { @@ -51,5 +53,40 @@ 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. +// `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; + } + 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: 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) { + 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 + // ordering is semantically neutral. + if (!current->inInitWindow()) { + return false; + } + return tickInitWindowIfNeededImpl(JVMThread::current() != nullptr, current); +} #endif // THREADLOCALDATA_INLINE_H diff --git a/ddprof-lib/src/main/cpp/wallClock.cpp b/ddprof-lib/src/main/cpp/wallClock.cpp index 2afd90aa1..4d39c73a4 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" @@ -205,6 +204,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. @@ -238,12 +238,7 @@ void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext if (!cs.entered()) { 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)) { return; } // Once-per-run filter (wallprecheck=true): for untraced threads, exact @@ -257,48 +252,50 @@ void WallClockASGCT::signalHandler(int signo, siginfo_t *siginfo, void *ucontext 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 sighandler_tid(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); } Error BaseWallClock::start(Arguments &args) { @@ -413,6 +410,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. @@ -446,48 +444,43 @@ void WallClockJvmti::signalHandler(int signo, siginfo_t *siginfo, if (!cs.entered()) { return; } - int saved_errno = errno; - if (JVMThread::current() == nullptr - && current->inInitWindow()) { - current->tickInitWindow(); - errno = saved_errno; + if (tickInitWindowIfNeeded(current)) { return; } WallPrecheckResult precheck = prepareWallPrecheck(current, _precheck); if (precheck.suppress) { - errno = saved_errno; 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 sighandler_tid(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; } void WallClockJvmti::initialize(Arguments &args) { 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 000000000..e2314de11 --- /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 new file mode 100644 index 000000000..8ad77fddf --- /dev/null +++ b/ddprof-lib/src/test/cpp/tickInitWindow_ut.cpp @@ -0,0 +1,80 @@ +/* + * 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" +#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); + } + + void TearDown() override { + ProfiledThread::release(); + restoreDefaultSignalHandlers(); + } + + 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 +}