more review stuff

Author: nia-e

src/alloc/isolated_alloc.rs

@@ -303,14 +303,12 @@ impl IsolatedAlloc {
     }
 
     /// Returns a vector of page addresses managed by the allocator.
-    pub fn pages(&self) -> Vec<usize> {
+    pub fn pages(&self) -> impl Iterator<Item = usize> {
         let pages = self.page_ptrs.iter().map(|p| p.expose_provenance().get());
-        let pages = pages.chain(self.huge_ptrs.iter().flat_map(|(ptr, size)| {
+        pages.chain(self.huge_ptrs.iter().flat_map(|(ptr, size)| {
             (0..size / self.page_size)
                 .map(|i| ptr.expose_provenance().get().strict_add(i * self.page_size))
-        }));
-
-        pages.collect()
+        }))
     }
 
     /// Protects all owned memory as `PROT_NONE`, preventing accesses.

src/bin/miri.rs

@@ -354,6 +354,10 @@ fn run_compiler_and_exit(
     args: &[String],
     callbacks: &mut (dyn rustc_driver::Callbacks + Send),
 ) -> ! {
+    // Install the ctrlc handler that sets `rustc_const_eval::CTRL_C_RECEIVED`, even if
+    // MIRI_BE_RUSTC is set.
+    rustc_driver::install_ctrlc_handler();
+
     // Invoke compiler, catch any unwinding panics and handle return code.
     let exit_code =
         rustc_driver::catch_with_exit_code(move || rustc_driver::run_compiler(args, callbacks));
@@ -438,10 +442,6 @@ fn main() {
     let args = rustc_driver::catch_fatal_errors(|| rustc_driver::args::raw_args(&early_dcx))
         .unwrap_or_else(|_| std::process::exit(rustc_driver::EXIT_FAILURE));
 
-    // Install the ctrlc handler that sets `rustc_const_eval::CTRL_C_RECEIVED`, even if
-    // MIRI_BE_RUSTC is set.
-    rustc_driver::install_ctrlc_handler();
-
     // If the environment asks us to actually be rustc, then do that.
     if let Some(crate_kind) = env::var_os("MIRI_BE_RUSTC") {
         // Earliest rustc setup.
@@ -751,11 +751,7 @@ fn main() {
     debug!("crate arguments: {:?}", miri_config.args);
     if !miri_config.native_lib.is_empty() && miri_config.native_lib_enable_tracing {
         // FIXME(ptrace): This should display a diagnostic / warning on error
-        // SAFETY: If any other threads exist at this point (namely for the ctrlc
-        // handler), they will not interact with anything on the main rustc/Miri
-        // thread in an async-signal-unsafe way such as by accessing shared
-        // semaphores, etc.; the handler only calls `sleep()` and `exit()`, which
-        // are async-signal-safe, as is accessing atomics
+        // SAFETY: No other threads are running
         let _ = unsafe { miri::native_lib::init_sv() };
     }
     run_compiler_and_exit(

src/shims/native_lib/mod.rs

@@ -9,14 +9,6 @@ use rustc_middle::mir::interpret::Pointer;
 use rustc_middle::ty::{self as ty, IntTy, UintTy};
 use rustc_span::Symbol;
 
-#[cfg_attr(
-    all(
-        target_os = "linux",
-        target_env = "gnu",
-        any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64")
-    ),
-    path = "trace/mod.rs"
-)]
 #[cfg_attr(
     not(all(
         target_os = "linux",
@@ -41,10 +33,12 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
         libffi_args: Vec<libffi::high::Arg<'a>>,
     ) -> trace::CallResult<'tcx> {
         let this = self.eval_context_mut();
+        #[cfg(target_os = "linux")]
         let alloc = this.machine.allocator.as_ref().unwrap();
 
         // SAFETY: We don't touch the machine memory past this point.
-        let (guard, stack_ptr) = unsafe { Supervisor::start_ffi(alloc) };
+        #[cfg(target_os = "linux")]
+        let guard = unsafe { Supervisor::start_ffi(alloc) };
 
         // Call the function (`ptr`) with arguments `libffi_args`, and obtain the return value
         // as the specified primitive integer type
@@ -118,8 +112,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // SAFETY: We got the guard and stack pointer from start_ffi, and
         // the allocator is the same
-        let events = unsafe { Supervisor::end_ffi(alloc, guard, stack_ptr) };
-        //let events = None;
+        let events = unsafe { Supervisor::end_ffi(guard) };
 
         interp_ok((res?, events))
     }

src/shims/native_lib/trace/child.rs

@@ -10,6 +10,11 @@ use super::messages::{Confirmation, MemEvents, StartFfiInfo, TraceRequest};
 use super::parent::{ChildListener, sv_loop};
 use crate::alloc::isolated_alloc::IsolatedAlloc;
 
+/// A handle to the single, shared supervisor process across all `MiriMachine`s.
+/// Since it would be very difficult to trace multiple FFI calls in parallel, we
+/// need to ensure that either (a) only one `MiriMachine` is performing an FFI call
+/// at any given time, or (b) there are distinct supervisor and child processes for
+/// each machine. The former was chosen here.
 static SUPERVISOR: std::sync::Mutex<Option<Supervisor>> = std::sync::Mutex::new(None);
 
 /// The main means of communication between the child and parent process,
@@ -24,6 +29,12 @@ pub struct Supervisor {
     event_rx: ipc::IpcReceiver<MemEvents>,
 }
 
+pub struct SvFfiGuard<'a> {
+    alloc: &'a Rc<RefCell<IsolatedAlloc>>,
+    sv_guard: std::sync::MutexGuard<'static, Option<Supervisor>>,
+    cb_stack: Option<*mut [u8; CALLBACK_STACK_SIZE]>,
+}
+
 /// Marker representing that an error occurred during creation of the supervisor.
 #[derive(Debug)]
 pub struct SvInitError;
@@ -46,20 +57,24 @@ impl Supervisor {
     /// after the desired call has concluded.
     pub unsafe fn start_ffi(
         alloc: &Rc<RefCell<IsolatedAlloc>>,
-    ) -> (std::sync::MutexGuard<'static, Option<Supervisor>>, Option<*mut [u8; CALLBACK_STACK_SIZE]>)
+    ) -> SvFfiGuard<'_>
     {
         let mut sv_guard = SUPERVISOR.lock().unwrap();
         // If the supervisor is not initialised for whatever reason, fast-return.
         // This might be desired behaviour, as even on platforms where ptracing
         // is not implemented it enables us to enforce that only one FFI call
         // happens at a time.
         let Some(sv) = sv_guard.as_mut() else {
-            return (sv_guard, None);
+            return SvFfiGuard {
+                alloc,
+                sv_guard,
+                cb_stack: None,
+            };
         };
 
         // Get pointers to all the pages the supervisor must allow accesses in
         // and prepare the callback stack.
-        let page_ptrs = alloc.borrow().pages();
+        let page_ptrs = alloc.borrow().pages().collect();
         let raw_stack_ptr: *mut [u8; CALLBACK_STACK_SIZE] =
             Box::leak(Box::new([0u8; CALLBACK_STACK_SIZE])).as_mut_ptr().cast();
         let stack_ptr = raw_stack_ptr.expose_provenance();
@@ -86,7 +101,11 @@ impl Supervisor {
         // modifications to our memory - simply waiting on the recv() doesn't
         // count.
         signal::raise(signal::SIGSTOP).unwrap();
-        (sv_guard, Some(raw_stack_ptr))
+        SvFfiGuard {
+            alloc,
+            sv_guard,
+            cb_stack: Some(raw_stack_ptr),
+        }
     }
 
     /// Undoes FFI-related preparations, allowing Miri to continue as normal, then
@@ -98,10 +117,12 @@ impl Supervisor {
     /// received by a prior call to `start_ffi`, and the allocator must be the
     /// one passed to it also.
     pub unsafe fn end_ffi(
-        alloc: &Rc<RefCell<IsolatedAlloc>>,
-        mut sv_guard: std::sync::MutexGuard<'static, Option<Supervisor>>,
-        raw_stack_ptr: Option<*mut [u8; CALLBACK_STACK_SIZE]>,
+        guard: SvFfiGuard<'_>,
     ) -> Option<MemEvents> {
+        let alloc = guard.alloc;
+        let mut sv_guard = guard.sv_guard;
+        let cb_stack = guard.cb_stack;
+
         // We can't use IPC channels here to signal that FFI mode has ended,
         // since they might allocate memory which could get us stuck in a SIGTRAP
         // with no easy way out! While this could be worked around, it is much
@@ -121,7 +142,7 @@ impl Supervisor {
         // SAFETY: Caller upholds that this pointer was allocated as a box with
         // this type.
         unsafe {
-            drop(Box::from_raw(raw_stack_ptr.unwrap()));
+            drop(Box::from_raw(cb_stack.unwrap()));
         }
         // On the off-chance something really weird happens, don't block forever.
         sv.event_rx
@@ -130,7 +151,7 @@ impl Supervisor {
                 match e {
                     ipc::TryRecvError::IpcError(_) => (),
                     ipc::TryRecvError::Empty =>
-                        eprintln!("Waiting for accesses from supervisor timed out!"),
+                        panic!("Waiting for accesses from supervisor timed out!"),
                 }
             })
             .ok()
@@ -141,6 +162,10 @@ impl Supervisor {
 /// supervisor process could not be created successfully; else, the caller
 /// is now the child process and can communicate via `start_ffi`/`end_ffi`,
 /// receiving back events through `get_events`.
+/// 
+/// When forking to initialise the supervisor, the child raises a `SIGSTOP`; if
+/// the parent successfully ptraces the child, it will allow it to resume. Else,
+/// the child will be killed by the parent.
 ///
 /// # Safety
 /// The invariants for `fork()` must be upheld by the caller, namely either:
@@ -151,11 +176,6 @@ pub unsafe fn init_sv() -> Result<(), SvInitError> {
     // FIXME: Much of this could be reimplemented via the mitosis crate if we upstream the
     // relevant missing bits.
 
-    // Not on a properly supported architecture!
-    if cfg!(not(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64"))) {
-        return Err(SvInitError);
-    }
-
     // On Linux, this will check whether ptrace is fully disabled by the Yama module.
     // If Yama isn't running or we're not on Linux, we'll still error later, but
     // this saves a very expensive fork call.

src/shims/native_lib/trace/messages.rs

@@ -1,7 +1,8 @@
 //! Houses the types that are directly sent across the IPC channels.
+//! 
 //!
-//! The overall structure of a traced FFI call, from the child process's POV, is
-//! as follows:
+//! After initialisation is done, the overall structure of a traced FFI call from
+//! the child process's POV is as follows:
 //! ```
 //! message_tx.send(TraceRequest::StartFfi);
 //! confirm_rx.recv();

src/shims/native_lib/trace/mod.rs

@@ -5,6 +5,8 @@ mod parent;
 pub use self::child::{Supervisor, init_sv, register_retcode_sv};
 
 /// The size of the temporary stack we use for callbacks that the server executes in the client.
+/// This should be big enough that `mempr_on` and `mempr_off` can safely be jumped into with the
+/// stack pointer pointing to a "stack" of this size without overflowing it.
 const CALLBACK_STACK_SIZE: usize = 1024;
 
 pub(super) type CallResult<'tcx> = rustc_const_eval::interpret::InterpResult<

src/shims/native_lib/trace_stub.rs

@@ -10,12 +10,10 @@ impl Supervisor {
     }
 
     #[inline(always)]
-    pub unsafe fn start_ffi<T>(_: T) -> ((), ()) {
-        ((), ())
-    }
+    pub unsafe fn start_ffi<T>(_: T) {}
 
     #[inline(always)]
-    pub unsafe fn end_ffi<T, U, V>(_: T, _: U, _: V) -> Option<!> {
+    pub unsafe fn end_ffi<T>(_: T) -> Option<!> {
         None
     }
 }