diff --git a/Cargo.lock b/Cargo.lock index 4ae63df566..2a8d9e267f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -194,6 +194,8 @@ name = "c2rust-analysis-rt" version = "0.18.0" dependencies = [ "bincode", + "crossbeam-queue", + "crossbeam-utils", "enum_dispatch", "fs-err", "once_cell", @@ -559,6 +561,21 @@ dependencies = [ "libc", ] +[[package]] +name = "crossbeam-queue" +version = "0.3.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df0346b5d5e76ac2fe4e327c5fd1118d6be7c51dfb18f9b7922923f287471e35" +dependencies = [ + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-utils" +version = "0.8.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "248e3bacc7dc6baa3b21e405ee045c3047101a49145e7e9eca583ab4c2ca5345" + [[package]] name = "crypto-common" version = "0.1.6" diff --git a/analysis/runtime/Cargo.toml b/analysis/runtime/Cargo.toml index 68741ba80e..7f2b2bc538 100644 --- a/analysis/runtime/Cargo.toml +++ b/analysis/runtime/Cargo.toml @@ -17,3 +17,5 @@ bincode = "1.0.1" once_cell = "1" enum_dispatch = "0.3" fs-err = "2" +crossbeam-queue = "0.3" +crossbeam-utils = "0.8" diff --git a/analysis/runtime/src/handlers.rs b/analysis/runtime/src/handlers.rs index bbd155a2a1..03b2075f66 100644 --- a/analysis/runtime/src/handlers.rs +++ b/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`. diff --git a/analysis/runtime/src/runtime/backend.rs b/analysis/runtime/src/runtime/backend.rs index 4feca7d487..1093fae9c6 100644 --- a/analysis/runtime/src/runtime/backend.rs +++ b/analysis/runtime/src/runtime/backend.rs @@ -1,7 +1,10 @@ +use crossbeam_queue::ArrayQueue; +use crossbeam_utils::Backoff; use enum_dispatch::enum_dispatch; use fs_err::{File, OpenOptions}; use std::fmt::Debug; use std::io::{stderr, BufWriter, Write}; +use std::sync::Arc; use bincode; @@ -80,18 +83,33 @@ pub enum Backend { } impl Backend { - fn write_all(&mut self, events: impl IntoIterator) { - for event in events { + fn write_all(&mut self, events: Arc>) { + let backoff = Backoff::new(); + loop { + let event = match events.pop() { + Some(event) => event, + None => { + // 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; + } + }; + let done = matches!(event.kind, EventKind::Done); self.write(event); if done { break; } + + // Reset the backoff timer since we got an event + backoff.reset(); } self.flush(); } - pub fn run(&mut self, events: impl IntoIterator) { + pub fn run(&mut self, events: Arc>) { let (lock, cvar) = &*FINISHED; let mut finished = lock.lock().unwrap(); self.write_all(events); diff --git a/analysis/runtime/src/runtime/global_runtime.rs b/analysis/runtime/src/runtime/global_runtime.rs index 0f4b798935..e85a4e15ec 100644 --- a/analysis/runtime/src/runtime/global_runtime.rs +++ b/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) => { diff --git a/analysis/runtime/src/runtime/scoped_runtime.rs b/analysis/runtime/src/runtime/scoped_runtime.rs index 3a517283af..f2210d8d42 100644 --- a/analysis/runtime/src/runtime/scoped_runtime.rs +++ b/analysis/runtime/src/runtime/scoped_runtime.rs @@ -1,11 +1,10 @@ use std::{ - sync::{ - mpsc::{self, SyncSender}, - Mutex, - }, + sync::{Arc, Mutex}, thread, }; +use crossbeam_queue::ArrayQueue; +use crossbeam_utils::Backoff; use enum_dispatch::enum_dispatch; use once_cell::sync::OnceCell; @@ -104,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); } @@ -117,16 +118,36 @@ impl Runtime for MainThreadRuntime { } pub struct BackgroundThreadRuntime { - tx: SyncSender, + tx: Arc>, finalized: OnceCell<()>, } +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 { + backoff.snooze(); + } else { + // We have no choice but to spin here because + // we might be inside a signal handler + backoff.spin(); + } + event = event_back; + } + } +} + impl ExistingRuntime for BackgroundThreadRuntime { fn finalize(&self) { // Only run the finalizer once. self.finalized.get_or_init(|| { // Notify the backend that we're done. - self.tx.send(Event::done()).unwrap(); + self.push_event(Event::done(), true); // Wait for the backend thread to finish. let (lock, cvar) = &*FINISHED; @@ -147,12 +168,16 @@ impl ExistingRuntime for BackgroundThreadRuntime { fn send_event(&self, event: Event) { match self.finalized.get() { None => { - // `.unwrap()` as we're in no place to handle an error here, - // unless we should silently drop the [`Event`] instead. - self.tx.send(event).unwrap(); + // # 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); } } @@ -172,7 +197,8 @@ impl Runtime for BackgroundThreadRuntime { /// Initialize the [`BackgroundThreadRuntime`], which includes [`thread::spawn`]ing, /// so it must be run post-`main`. fn try_init(mut backend: Backend) -> Result { - let (tx, rx) = mpsc::sync_channel(1024); + let tx = Arc::new(ArrayQueue::new(1 << 20)); + let rx = Arc::clone(&tx); thread::spawn(move || backend.run(rx)); Ok(Self { tx, diff --git a/analysis/runtime/src/runtime/skip.rs b/analysis/runtime/src/runtime/skip.rs index 8fc05d9d37..be4b901b83 100644 --- a/analysis/runtime/src/runtime/skip.rs +++ b/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); } };