Add signal safety comments and changes

Author: ahomescu

analysis/runtime/src/handlers.rs

@@ -2,6 +2,11 @@ use crate::events::{Event, EventKind};
 use crate::mir_loc::MirLocId;
 use crate::runtime::global_runtime::RUNTIME;
 
+// WARNING! Most handlers in this file may be called from a signal handler,
+// so they and all their callees should be signal-safe.
+// Signal handlers are generally not supposed to call memory allocation
+// functions, so those do not need to be signal-safe.
+
 /// A hook function (see [`HOOK_FUNCTIONS`]).
 ///
 /// Instruments 64-bit `c2rust transpile`d `malloc`, which is similar to `libc::malloc`.

analysis/runtime/src/runtime/backend.rs

@@ -89,8 +89,9 @@ impl Backend {
             let event = match events.pop() {
                 Some(event) => event,
                 None => {
-                    // We can't use anything with a futex here since
-                    // the event sender might run inside a signal handler
+                    // We can't block on a lock/semaphore here since
+                    // it might not be safe for the event sender to wake
+                    // us from inside a signal handler
                     backoff.snooze();
                     continue;
                 }

analysis/runtime/src/runtime/global_runtime.rs

@@ -40,10 +40,15 @@ impl GlobalRuntime {
     ///
     /// It also silently drops the [`Event`] if the [`ScopedRuntime`]
     /// has been [`ScopedRuntime::finalize`]d/[`GlobalRuntime::finalize`]d.
+    ///
+    /// May be called from a signal handler, so it needs to be async-signal-safe.
     pub fn send_event(&self, event: Event) {
+        // # Async-signal-safety: OnceCell::get() is just a dereference
         match self.runtime.get() {
             None => {
                 // Silently drop the [`Event`] as the [`ScopedRuntime`] isn't ready/initialized yet.
+                //
+                // # Async-signal-safety: `skip_event(_, BeforeMain)` is async-signal-safe.
                 skip_event(event, SkipReason::BeforeMain);
             }
             Some(runtime) => {

analysis/runtime/src/runtime/scoped_runtime.rs

@@ -103,6 +103,8 @@ impl ExistingRuntime for MainThreadRuntime {
         self.backend.lock().unwrap().flush();
     }
 
+    // # Async-signal-safety: NOT SAFE!!!
+    // Do not use this with programs that install signal handlers.
     fn send_event(&self, event: Event) {
         self.backend.lock().unwrap().write(event);
     }
@@ -122,6 +124,10 @@ pub struct BackgroundThreadRuntime {
 
 impl BackgroundThreadRuntime {
     fn push_event(&self, mut event: Event, can_sleep: bool) {
+        // # Async-signal-safety: This needs `can_sleep == false` if called from
+        // a signal handler; in that case, it spins instead of sleeping
+        // which should be safe. `ArrayQueue::push` is backed by a fixed-size
+        // array so it does not allocate.
         let backoff = Backoff::new();
         while let Err(event_back) = self.tx.push(event) {
             if can_sleep {
@@ -162,10 +168,16 @@ impl ExistingRuntime for BackgroundThreadRuntime {
     fn send_event(&self, event: Event) {
         match self.finalized.get() {
             None => {
+                // # Async-signal-safety: `push_event` is safe if `can_sleep == false`
                 self.push_event(event, false);
             }
             Some(()) => {
-                // Silently drop the [`Event`] as the [`BackgroundThreadRuntime`] has already been [`BackgroundThreadRuntime::finalize`]d.
+                // Silently drop the [`Event`] as the [`BackgroundThreadRuntime`]
+                // has already been [`BackgroundThreadRuntime::finalize`]d.
+                //
+                // # Async-signal-safety: `skip_event(_, AfterMain)` is NOT SAFE;
+                // however, see the comment in `skip_event` for an explanation
+                // of why this will probably be okay in practice.
                 skip_event(event, SkipReason::AfterMain);
             }
         }

analysis/runtime/src/runtime/skip.rs

@@ -1,10 +1,8 @@
 use std::{
     fmt::{self, Display, Formatter},
-    sync::atomic::{AtomicU64, Ordering},
+    sync::atomic::{AtomicBool, AtomicU64, Ordering},
 };
 
-use once_cell::sync::OnceCell;
-
 use crate::events::Event;
 
 #[derive(Debug)]
@@ -24,8 +22,7 @@ impl Display for SkipReason {
 }
 
 static EVENTS_SKIPPED_BEFORE_MAIN: AtomicU64 = AtomicU64::new(0);
-
-static WARNED_AFTER_MAIN: OnceCell<()> = OnceCell::new();
+static WARNED_AFTER_MAIN: AtomicBool = AtomicBool::new(false);
 
 /// Notify the user if any [`Event`]s were skipped before `main`.
 ///
@@ -45,14 +42,22 @@ pub(super) fn skip_event(event: Event, reason: SkipReason) {
     use SkipReason::*;
     match reason {
         BeforeMain => {
+            // # Async-signal-safety: atomic increments are safe.
             EVENTS_SKIPPED_BEFORE_MAIN.fetch_add(1, Ordering::Relaxed);
         }
         AfterMain => {
-            // This is after `main`, so it's safe to use things like `eprintln!`,
-            // which uses `ReentrantMutex` internally, which may use `pthread` mutexes.
-            WARNED_AFTER_MAIN.get_or_init(|| {
+            // # Async-signal-safety: not really signal-safe, but if we
+            // get a signal after `main` ends, we're probably fine.
+            // The allocator should have enough free memory by now
+            // to not need to call `mmap`.
+            if !WARNED_AFTER_MAIN.swap(true, Ordering::Relaxed) {
+                // WARNED_AFTER_MAIN was previously `false` but we swapped it,
+                // which will happen exactly once per run so we can print now.
                 eprintln!("skipping {reason}");
-            });
+            }
+            // TODO: It would be nice to get rid of the two `eprintln`s here
+            // so we can guarantee signal safety, but then we would get no
+            // debugging output.
             eprintln!("skipped event after `main`: {:?}", event.kind);
         }
     };