fix review comments

Author: nia-e

no_alloc.json

@@ -484,7 +484,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
     /// called before the FFI (with `paranoid` set to false) then some of the writes may be
     /// lost!
     #[cfg(target_os = "linux")]
-    fn apply_events(&mut self, events: crate::shims::trace::MemEvents) -> InterpResult<'tcx> {
+    fn apply_events(&mut self, events: crate::shims::trace::messages::MemEvents) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
         let mut reads = vec![];

src/alloc_addresses/mod.rs

@@ -15,7 +15,7 @@ use crate::alloc::isolated_alloc::IsolatedAlloc;
 use crate::*;
 
 #[cfg(target_os = "linux")]
-type CallResult<'tcx> = InterpResult<'tcx, (ImmTy<'tcx>, Option<shims::trace::MemEvents>)>;
+type CallResult<'tcx> = InterpResult<'tcx, (ImmTy<'tcx>, Option<shims::trace::messages::MemEvents>)>;
 #[cfg(not(target_os = "linux"))]
 type CallResult<'tcx> = InterpResult<'tcx, (ImmTy<'tcx>, Option<!>)>;
 
@@ -261,7 +261,7 @@ unsafe fn do_native_call<T: libffi::high::CType>(
     ptr: CodePtr,
     args: &[ffi::Arg<'_>],
     alloc: Option<Rc<RefCell<IsolatedAlloc>>>,
-) -> (T, Option<shims::trace::MemEvents>) {
+) -> (T, Option<shims::trace::messages::MemEvents>) {
     use shims::trace::Supervisor;
 
     unsafe {

src/shims/native_lib.rs

@@ -6,8 +6,9 @@ use nix::sys::{ptrace, signal};
 use nix::unistd;
 
 use crate::alloc::isolated_alloc::IsolatedAlloc;
+use crate::shims::trace::messages::{Confirmation, MemEvents, TraceRequest};
 use crate::shims::trace::parent::{ChildListener, sv_loop};
-use crate::shims::trace::{FAKE_STACK_SIZE, MemEvents, StartFfiInfo, TraceRequest};
+use crate::shims::trace::{FAKE_STACK_SIZE, StartFfiInfo};
 
 static SUPERVISOR: std::sync::Mutex<Option<Supervisor>> = std::sync::Mutex::new(None);
 
@@ -18,7 +19,7 @@ pub struct Supervisor {
     message_tx: ipc::IpcSender<TraceRequest>,
     /// Used for synchronisation, allowing us to receive confirmation that the
     /// parent process has handled the request from `message_tx`.
-    confirm_rx: ipc::IpcReceiver<()>,
+    confirm_rx: ipc::IpcReceiver<Confirmation>,
     /// Receiver for memory acceses that ocurred during the FFI call.
     event_rx: ipc::IpcReceiver<MemEvents>,
 }

src/shims/trace/child.rs

@@ -0,0 +1,39 @@
+//! Houses the types that are directly sent across the IPC channels.
+
+/// An IPC request sent by the child process to the parent.
+///
+/// The sender for this channel should live on the child process.
+#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
+pub(super) enum TraceRequest {
+    /// Requests that tracing begins. Following this being sent, the child must
+    /// wait to receive a `Confirmation` on the respective channel and then
+    /// `raise(SIGSTOP)`.
+    /// 
+    /// To avoid possible issues while allocating memory for IPC channels, ending
+    /// the tracing is instead done via `raise(SIGUSR1)`.
+    StartFfi(super::StartFfiInfo),
+    /// Manually overrides the code that the supervisor will return upon exiting.
+    /// Once set, it is permanent. This can be called again to change the value.
+    OverrideRetcode(i32),
+}
+
+/// A marker type confirming that the supervisor has received the request to begin
+/// tracing and is now waiting for a `SIGSTOP`.
+/// 
+/// The sender for this channel should live on the parent process.
+#[derive(serde::Serialize, serde::Deserialize, Debug)]
+pub(super) struct Confirmation;
+
+/// The final results of an FFI trace, containing every relevant event detected
+/// by the tracer. Sent by the supervisor.
+/// 
+/// The sender for this channel should live on the parent process.
+#[derive(serde::Serialize, serde::Deserialize, Debug)]
+pub struct MemEvents {
+    /// An ordered list of memory accesses that occurred. These should be assumed
+    /// to be overcautious; that is, if the size of an access is uncertain it is
+    /// pessimistically rounded up, and if the type (read/write/both) is uncertain
+    /// it is reported as whatever would be safest to assume; i.e. a read + maybe-write
+    /// becomes a read + write, etc.
+    pub acc_events: Vec<super::AccessEvent>,
+}

src/shims/trace/messages.rs

@@ -1,5 +1,6 @@
 mod child;
 mod parent;
+pub mod messages;
 
 use std::ops::Range;
 
@@ -19,12 +20,6 @@ struct StartFfiInfo {
     stack_ptr: usize,
 }
 
-#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
-enum TraceRequest {
-    StartFfi(StartFfiInfo),
-    OverrideRetcode(i32),
-}
-
 /// A single memory access, conservatively overestimated
 /// in case of ambiguity.
 #[derive(serde::Serialize, serde::Deserialize, Debug)]
@@ -34,11 +29,3 @@ pub enum AccessEvent {
     /// A write may have occurred on no more than the specified address range.
     Write(Range<usize>),
 }
-
-/// The final results of an FFI trace, containing every relevant event detected
-/// by the tracer.
-#[derive(serde::Serialize, serde::Deserialize, Debug)]
-pub struct MemEvents {
-    /// An ordered list of memory accesses that occurred.
-    pub acc_events: Vec<AccessEvent>,
-}

src/shims/trace/mod.rs

@@ -4,7 +4,8 @@ use ipc_channel::ipc;
 use nix::sys::{ptrace, signal, wait};
 use nix::unistd;
 
-use crate::shims::trace::{AccessEvent, FAKE_STACK_SIZE, MemEvents, StartFfiInfo, TraceRequest};
+use crate::shims::trace::messages::{Confirmation, MemEvents, TraceRequest};
+use crate::shims::trace::{AccessEvent, FAKE_STACK_SIZE, StartFfiInfo};
 
 /// The flags to use when calling `waitid()`.
 /// Since bitwise or on the nix version of these flags is implemented as a trait,
@@ -242,7 +243,7 @@ enum ExecError {
 pub fn sv_loop(
     listener: ChildListener,
     event_tx: ipc::IpcSender<MemEvents>,
-    confirm_tx: ipc::IpcSender<()>,
+    confirm_tx: ipc::IpcSender<Confirmation>,
     page_size: usize,
 ) -> Result<!, Option<i32>> {
     // Things that we return to the child process
@@ -281,7 +282,7 @@ pub fn sv_loop(
                 // so we can let the child move on to the end of start_ffi where it will
                 // raise a SIGSTOP. We need it to be signal-stopped *and waited for* in
                 // order to do most ptrace operations!
-                confirm_tx.send(()).unwrap();
+                confirm_tx.send(Confirmation).unwrap();
                 wait_for_signal(main_pid, signal::SIGSTOP, false).unwrap();
 
                 ptrace::syscall(main_pid, None).unwrap();