refactor(profiling): Signal handler boilerplate

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,56 +205,50 @@ 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);
     return;
   }
   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__

ddprof-lib/src/main/cpp/guards.h

@@ -20,11 +20,13 @@
 #include <cstdint>
 #include <cstddef>
 #include <setjmp.h>
+#include <cerrno>
 #include <signal.h>
 #include <pthread.h>
 
 #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

ddprof-lib/src/main/cpp/itimer.cpp

@@ -18,21 +18,22 @@
 #include "itimer.h"
 #include "counters.h"
 #include "debugSupport.h"
-#include "jvmThread.h"
 #include "os.h"
 #include "profiler.h"
 #include "signalInflight.h"
 #include "stackWalker.h"
 #include "threadLocalData.inline.h"
 #include "threadState.inline.h"
 #include "guards.h"
+#include <cassert>
 #include <sys/time.h>
 
 bool ITimer::_enabled = false;
 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) {