Avoid potential deadlocks from using std::* operations in signal handlers (#293)

* Avoid potential deadlocks from using std::* operations in signal handlers

* Avoid intermittent failures in traceid generation tests

* Mark fields with concurrent access as volatile

(cherry picked from commit 8070f3c)

Author: jbachorik

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

@@ -28,29 +28,50 @@ class CallTraceHashTable;
 // Using reference parameter avoids malloc() for vector creation and copying
 typedef std::function<void(std::unordered_set<u64>&)> LivenessChecker;
 
+/**
+ * Cache-aligned hazard pointer slot to eliminate false sharing.
+ * Each slot occupies a full cache line (64 bytes) to ensure that
+ * updates by one thread do not invalidate cache lines for other threads.
+ */
+struct alignas(DEFAULT_CACHE_LINE_SIZE) HazardSlot {
+    volatile CallTraceHashTable* pointer;
+    char padding[DEFAULT_CACHE_LINE_SIZE - sizeof(pointer)];
+
+    HazardSlot() : pointer(nullptr), padding{} {
+        static_assert(sizeof(HazardSlot) == DEFAULT_CACHE_LINE_SIZE,
+                      "HazardSlot must be exactly one cache line");
+    }
+};
+
 /**
  * RAII guard for hazard pointer management.
- * 
+ *
  * This class encapsulates the hazard pointer mechanism used to protect CallTraceHashTable
  * instances from being deleted while they are being accessed by concurrent threads.
  *
  * Usage:
- *   HazardPointer guard(active_table);
- *   // active_table is now protected from deletion
+ *   HazardPointer guard(_active_table);
+ *   // _active_table is now protected from deletion
  *   // Automatic cleanup when guard goes out of scope
  */
 class HazardPointer {
 public:
     static constexpr int MAX_THREADS = 8192;
     static constexpr int MAX_PROBE_DISTANCE = 32;  // Maximum probing attempts
-    
-    static std::atomic<CallTraceHashTable*> global_hazard_list[MAX_THREADS];
-    static std::atomic<int> slot_owners[MAX_THREADS];  // Thread ID ownership verification
+    static constexpr int BITMAP_WORDS = MAX_THREADS / 64;  // 8192 / 64 = 128 words
+
+    static HazardSlot global_hazard_list[MAX_THREADS];
+    static int slot_owners[MAX_THREADS];  // Thread ID ownership verification
+
+    // Occupied slot bitmap for efficient scanning (avoids iterating 8192 empty slots)
+    // Uses raw uint64_t with GCC atomic builtins to avoid any potential malloc on musl
+    // Each bit represents whether the corresponding slot is occupied
+    static uint64_t occupied_bitmap[BITMAP_WORDS];
 
 private:
-    bool active_;
-    int my_slot_;  // This instance's assigned slot
-    
+    bool _active;
+    int _my_slot;  // This instance's assigned slot
+
     // Signal-safe slot assignment using thread ID hash
     static int getThreadHazardSlot();
 
@@ -66,7 +87,7 @@ class HazardPointer {
     HazardPointer& operator=(HazardPointer&& other) noexcept;
 
     // Check if hazard pointer is active (slot allocation succeeded)
-    bool isActive() const { return active_; }
+    bool isActive() const { return _active; }
 
     // Wait for hazard pointers pointing to specific table to clear (used during shutdown)
     static void waitForHazardPointersToClear(CallTraceHashTable* table_to_delete);
@@ -89,20 +110,21 @@ class CallTraceStorage {
     // Triple-buffered storage with atomic pointers  
     // Rotation: tmp=scratch, scratch=active, active=standby, standby=tmp
     // New active inherits preserved traces for continuity
-    std::atomic<CallTraceHashTable*> _active_storage;
-    std::atomic<CallTraceHashTable*> _standby_storage;
-    std::atomic<CallTraceHashTable*> _scratch_storage;
+    volatile CallTraceHashTable* _active_storage;
+    volatile CallTraceHashTable* _standby_storage;
+    volatile CallTraceHashTable* _scratch_storage;
 
     // Generation counter for ABA protection during table swaps
-    std::atomic<u32> _generation_counter;
+    u32 _generation_counter;
 
     // Liveness checkers - protected by simple spinlock during registration/clear
     // Using vector instead of unordered_set since std::function cannot be hashed
     std::vector<LivenessChecker> _liveness_checkers;
     SpinLock _liveness_lock;  // Simple atomic lock for rare liveness operations
 
     // Static atomic for instance ID generation - avoids function-local static initialization issues
-    static std::atomic<u64> _next_instance_id;
+    
+    static u64 _next_instance_id;
 
     // Lazy initialization helper to avoid global constructor
     static u64 getNextInstanceId();

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

@@ -9,7 +9,7 @@
 #include "thread.h"
 
 // Static bitmap storage for fallback cases
-std::atomic<uint64_t> CriticalSection::_fallback_bitmap[CriticalSection::FALLBACK_BITMAP_WORDS] = {};
+uint64_t CriticalSection::_fallback_bitmap[CriticalSection::FALLBACK_BITMAP_WORDS] = {};
 
 CriticalSection::CriticalSection() : _entered(false), _using_fallback(false), _word_index(0), _bit_mask(0) {
     ProfiledThread* current = ProfiledThread::current();
@@ -27,8 +27,7 @@ CriticalSection::CriticalSection() : _entered(false), _using_fallback(false), _w
         uint32_t bit_index = tid % 64;
         _bit_mask = 1ULL << bit_index;
 
-        // Atomically try to set the bit
-        uint64_t old_word = _fallback_bitmap[_word_index].fetch_or(_bit_mask, std::memory_order_relaxed);
+        uint64_t old_word = __atomic_fetch_or(&_fallback_bitmap[_word_index], _bit_mask, __ATOMIC_RELAXED);
         _entered = !(old_word & _bit_mask);  // Success if bit was previously 0
     }
 }
@@ -37,7 +36,7 @@ CriticalSection::~CriticalSection() {
     if (_entered) {
         if (_using_fallback) {
             // Clear the bit atomically for fallback bitmap
-            _fallback_bitmap[_word_index].fetch_and(~_bit_mask, std::memory_order_relaxed);
+            __atomic_fetch_and(&_fallback_bitmap[_word_index], ~_bit_mask, __ATOMIC_RELAXED);
         } else {
             // Release ProfiledThread flag
             ProfiledThread* current = ProfiledThread::current();

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

@@ -6,7 +6,6 @@
 #ifndef _CRITICALSECTION_H
 #define _CRITICALSECTION_H
 
-#include <atomic>
 #include <cstdint>
 #include <cstddef>
 
@@ -37,7 +36,7 @@ class CriticalSection {
     // Must be atomic because multiple signal handlers can run concurrently across
     // different threads and attempt to set/clear bits simultaneously. Compare-and-swap
     // operations ensure race-free bit manipulation even during signal interruption.
-    static std::atomic<uint64_t> _fallback_bitmap[FALLBACK_BITMAP_WORDS];
+    static uint64_t _fallback_bitmap[FALLBACK_BITMAP_WORDS];
 
     bool _entered;          // Track if this instance successfully entered
     bool _using_fallback;   // Track which storage mechanism we're using

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

@@ -26,7 +26,8 @@
 
 class CTimer : public Engine {
 private:
-  static std::atomic<bool> _enabled;
+  // This is accessed from signal handlers, so must be async-signal-safe
+  static bool _enabled;
   static long _interval;
   static CStack _cstack;
   static int _signal;
@@ -52,7 +53,7 @@ class CTimer : public Engine {
   void stop();
 
   inline void enableEvents(bool enabled) {
-    _enabled.store(enabled, std::memory_order_release);
+    __atomic_store_n(&_enabled, enabled, __ATOMIC_RELEASE);
   }
 };
 

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

@@ -83,7 +83,7 @@ long CTimer::_interval;
 int CTimer::_max_timers = 0;
 int *CTimer::_timers = NULL;
 CStack CTimer::_cstack;
-std::atomic<bool> CTimer::_enabled{false};
+bool CTimer::_enabled = false;
 int CTimer::_signal;
 
 int CTimer::registerThread(int tid) {
@@ -207,7 +207,7 @@ void CTimer::signalHandler(int signo, siginfo_t *siginfo, void *ucontext) {
   int saved_errno = errno;
   // we want to ensure memory order because of the possibility the instance gets
   // cleared
-  if (!_enabled.load(std::memory_order_acquire))
+  if (!__atomic_load_n(&_enabled, __ATOMIC_ACQUIRE))
     return;
   int tid = 0;
   ProfiledThread *current = ProfiledThread::current();

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

@@ -131,11 +131,14 @@ class ProfiledThread : public ThreadLocalData {
   void setFilterSlotId(int slotId) { _filter_slot_id = slotId; }
 
   // Signal handler reentrancy protection
-  bool tryEnterCriticalSection() { 
-    return !_in_critical_section.exchange(true, std::memory_order_acquire); 
+  bool tryEnterCriticalSection() {
+    // Uses GCC atomic builtin (no malloc, async-signal-safe)
+    bool expected = false;
+    return __atomic_compare_exchange_n(&_in_critical_section, &expected, true, false, __ATOMIC_ACQUIRE, __ATOMIC_RELAXED);
   }
-  void exitCriticalSection() { 
-    _in_critical_section.store(false, std::memory_order_release); 
+  void exitCriticalSection() {
+    // Uses GCC atomic builtin (no malloc, async-signal-safe)
+    __atomic_store_n(&_in_critical_section, false, __ATOMIC_RELEASE);
   }
 
   // Hazard pointer management for lock-free memory reclamation (signal-safe)
@@ -162,7 +165,7 @@ class ProfiledThread : public ThreadLocalData {
   // and both contexts may attempt to enter the critical section. Without atomic exchange(),
   // both could see the flag as false and both would think they successfully entered.
   // The atomic exchange() is uninterruptible, ensuring only one context succeeds.
-  std::atomic<bool> _in_critical_section{false};
+  bool _in_critical_section{false};
 
   // Hazard pointer instance for signal-safe access (not atomic since only accessed by same thread)
   void* _hazard_instance{nullptr};

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

@@ -1673,21 +1673,17 @@ TEST_F(StressTestSuite, InstanceIdTraceIdStressTest) {
     std::vector<std::thread> workers;
     for (int t = 0; t < NUM_THREADS; ++t) {
         workers.emplace_back([&, t]() {
-            std::mt19937 gen(std::random_device{}() + t);
-            // No longer need storage distribution since we use single instance
-            std::uniform_int_distribution<uint32_t> bci_dis(1, 100000);
-            std::uniform_int_distribution<uintptr_t> method_dis(0x10000, 0xFFFFFF);
-            
             for (int op = 0; op < OPERATIONS_PER_THREAD && !test_failed.load(); ++op) {
                 try {
                     // Use the single shared storage instance
                     CallTraceStorage* storage = storage_instance;
-                    
-                    // Create a unique frame to avoid hash collisions masking trace ID issues
+
+                    // Create a DETERMINISTIC unique frame - no randomness
+                    // Each (thread, operation) pair generates a unique frame
                     ASGCT_CallFrame frame;
-                    frame.bci = bci_dis(gen) + t * 1000000 + op;  // Ensure uniqueness
-                    frame.method_id = reinterpret_cast<jmethodID>(method_dis(gen) + t * 0x1000000);
-                    
+                    frame.bci = t * 1000000 + op;  // Deterministic: thread_id * 1M + op_index
+                    frame.method_id = reinterpret_cast<jmethodID>(0x10000000ULL + (u64)t * 10000000ULL + (u64)op);
+
                     // Calculate stack trace hash for analysis (simplified hash of frame data)
                     u64 stack_hash = (u64)frame.bci ^ ((u64)frame.method_id << 32);