refactor(profiling): Signal handler boilerplate

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

@@ -230,27 +230,25 @@ 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;
   }
 
   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;
 }

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

@@ -23,6 +23,7 @@
 #include <pthread.h>
 
 #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

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

@@ -26,40 +26,48 @@
 #include "threadLocalData.inline.h"
 #include "threadState.inline.h"
 #include "guards.h"
+#include <errno.h>
 #include <sys/time.h>
 
 bool ITimer::_enabled = false;
 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
   // is therefore vulnerable to the foreign-SIGPROF deadlock scenario this
   // 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) {
@@ -117,24 +125,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;
 }
 

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

@@ -10,6 +10,7 @@
 #include <jvmti.h>
 
 #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 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.
+static inline bool tickInitWindowIfNeeded(ProfiledThread* current) {
+    if (JVMThread::current() == nullptr && current->inInitWindow()) {
+        current->tickInitWindow();
+        return true;
+    }
+    return false;
+}
+
 #endif // _JVMTHREAD_H

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) {