Auto merge of #111867 - RalfJung:miri, r=RalfJung

update Miri

Author: bors

Cargo.lock

@@ -29,6 +29,7 @@ smallvec = "1.7"
 # for more information.
 rustc-workspace-hack = "1.0.0"
 measureme = "10.0.0"
+ctrlc = "3.2.5"
 
 [target.'cfg(unix)'.dependencies]
 libc = "0.2"

Cargo.toml

@@ -40,7 +40,8 @@ impl<'s, I: Iterator<Item = Cow<'s, str>>> Iterator for ArgSplitFlagValue<'_, I>
         if arg == "--" {
             // Stop searching at `--`.
             self.args = None;
-            return None;
+            // But yield the `--` so that it does not get lost!
+            return Some(Err(Cow::Borrowed("--")));
         }
         // These branches cannot be merged if we want to avoid the allocation in the `Borrowed` branch.
         match &arg {
@@ -79,9 +80,8 @@ impl<'a, I: Iterator<Item = String> + 'a> ArgSplitFlagValue<'a, I> {
     ) -> impl Iterator<Item = Result<String, String>> + 'a {
         ArgSplitFlagValue::new(args.map(Cow::Owned), name).map(|x| {
             match x {
-                Ok(Cow::Owned(s)) => Ok(s),
-                Err(Cow::Owned(s)) => Err(s),
-                _ => panic!("iterator converted owned to borrowed"),
+                Ok(s) => Ok(s.into_owned()),
+                Err(s) => Err(s.into_owned()),
             }
         })
     }

cargo-miri/src/arg.rs

@@ -113,30 +113,17 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     };
     let metadata = get_cargo_metadata();
     let mut cmd = cargo();
-    cmd.arg(cargo_cmd);
-
-    // Forward all arguments before `--` other than `--target-dir` and its value to Cargo.
-    // (We want to *change* the target-dir value, so we must not forward it.)
-    let mut target_dir = None;
-    for arg in ArgSplitFlagValue::from_string_iter(&mut args, "--target-dir") {
-        match arg {
-            Ok(value) => {
-                if target_dir.is_some() {
-                    show_error!("`--target-dir` is provided more than once");
-                }
-                target_dir = Some(value.into());
-            }
-            Err(arg) => {
-                cmd.arg(arg);
-            }
-        }
+    cmd.arg(&cargo_cmd);
+    // In nextest we have to also forward the main `verb`.
+    if cargo_cmd == "nextest" {
+        cmd.arg(
+            args.next()
+                .unwrap_or_else(|| show_error!("`cargo miri nextest` expects a verb (e.g. `run`)")),
+        );
     }
-    // Detect the target directory if it's not specified via `--target-dir`.
-    // (`cargo metadata` does not support `--target-dir`, that's why we have to handle this ourselves.)
-    let target_dir = target_dir.get_or_insert_with(|| metadata.target_directory.clone());
-    // Set `--target-dir` to `miri` inside the original target directory.
-    target_dir.push("miri");
-    cmd.arg("--target-dir").arg(target_dir);
+    // We set the following flags *before* forwarding more arguments.
+    // This is needed to fix <https://github.com/rust-lang/miri/issues/2829>: cargo will stop
+    // interpreting things as flags when it sees the first positional argument.
 
     // Make sure the build target is explicitly set.
     // This is needed to make the `target.runner` settings do something,
@@ -154,8 +141,23 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
     cmd.arg("--config")
         .arg(format!("target.'cfg(all())'.runner=[{cargo_miri_path_for_toml}, 'runner']"));
 
-    // Forward all further arguments after `--` to cargo.
-    cmd.arg("--").args(args);
+    // Set `--target-dir` to `miri` inside the original target directory.
+    let mut target_dir = match get_arg_flag_value("--target-dir") {
+        Some(dir) => PathBuf::from(dir),
+        None => metadata.target_directory.clone().into_std_path_buf(),
+    };
+    target_dir.push("miri");
+    cmd.arg("--target-dir").arg(target_dir);
+
+    // *After* we set all the flags that need setting, forward everything else. Make sure to skip
+    // `--target-dir` (which would otherwise be set twice).
+    for arg in
+        ArgSplitFlagValue::from_string_iter(&mut args, "--target-dir").filter_map(Result::err)
+    {
+        cmd.arg(arg);
+    }
+    // Forward all further arguments (not consumed by `ArgSplitFlagValue`) to cargo.
+    cmd.args(args);
 
     // Set `RUSTC_WRAPPER` to ourselves.  Cargo will prepend that binary to its usual invocation,
     // i.e., the first argument is `rustc` -- which is what we use in `main` to distinguish

cargo-miri/src/phases.rs

@@ -1 +1 @@
-69fef92ab2f287f072b66fb7b4f62c8bb4acba43
+8b4b20836b832e91aa605a2faf5e2a55190202c8

rust-version

@@ -3,6 +3,7 @@
 use std::cell::RefCell;
 use std::collections::hash_map::Entry;
 use std::num::TryFromIntError;
+use std::sync::atomic::{AtomicBool, Ordering::Relaxed};
 use std::task::Poll;
 use std::time::{Duration, SystemTime};
 
@@ -1012,8 +1013,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     /// Run the core interpreter loop. Returns only when an interrupt occurs (an error or program
     /// termination).
     fn run_threads(&mut self) -> InterpResult<'tcx, !> {
+        static SIGNALED: AtomicBool = AtomicBool::new(false);
+        ctrlc::set_handler(move || {
+            // Indicate that we have ben signaled to stop. If we were already signaled, exit
+            // immediately. In our interpreter loop we try to consult this value often, but if for
+            // whatever reason we don't get to that check or the cleanup we do upon finding that
+            // this bool has become true takes a long time, the exit here will promptly exit the
+            // process on the second Ctrl-C.
+            if SIGNALED.swap(true, Relaxed) {
+                std::process::exit(1);
+            }
+        })
+        .unwrap();
         let this = self.eval_context_mut();
         loop {
+            if SIGNALED.load(Relaxed) {
+                this.machine.handle_abnormal_termination();
+                std::process::exit(1);
+            }
             match this.machine.threads.schedule(&this.machine.clock)? {
                 SchedulingAction::ExecuteStep => {
                     if !this.step()? {

src/concurrency/thread.rs

@@ -713,6 +713,15 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
         let def_id = frame.instance.def_id();
         def_id.is_local() || self.local_crates.contains(&def_id.krate)
     }
+
+    /// Called when the interpreter is going to shut down abnormally, such as due to a Ctrl-C.
+    pub(crate) fn handle_abnormal_termination(&mut self) {
+        // All strings in the profile data are stored in a single string table which is not
+        // written to disk until the profiler is dropped. If the interpreter exits without dropping
+        // the profiler, it is not possible to interpret the profile data and all measureme tools
+        // will panic when given the file.
+        drop(self.profiler.take());
+    }
 }
 
 impl VisitTags for MiriMachine<'_, '_> {

src/machine.rs

@@ -102,7 +102,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let ptr_layout = this.layout_of(ptr_ty)?;
 
                 for (i, ptr) in ptrs.into_iter().enumerate() {
-                    let offset = ptr_layout.size * i.try_into().unwrap();
+                    let offset = ptr_layout.size.checked_mul(i.try_into().unwrap(), this).unwrap();
 
                     let op_place = buf_place.offset(offset, ptr_layout, this)?;
 

src/shims/backtrace.rs

@@ -166,7 +166,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     dependency_format.1.iter().enumerate().filter_map(|(num, &linkage)| {
                         // We add 1 to the number because that's what rustc also does everywhere it
                         // calls `CrateNum::new`...
-                        #[allow(clippy::integer_arithmetic)]
+                        #[allow(clippy::arithmetic_side_effects)]
                         (linkage != Linkage::NotLinked).then_some(CrateNum::new(num + 1))
                     }),
                 ) {
@@ -707,7 +707,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     .position(|&c| c == val)
                 {
                     let idx = u64::try_from(idx).unwrap();
-                    #[allow(clippy::integer_arithmetic)] // idx < num, so this never wraps
+                    #[allow(clippy::arithmetic_side_effects)] // idx < num, so this never wraps
                     let new_ptr = ptr.offset(Size::from_bytes(num - idx - 1), this)?;
                     this.write_pointer(new_ptr, dest)?;
                 } else {
@@ -916,10 +916,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let a = this.read_scalar(a)?.to_u64()?;
                 let b = this.read_scalar(b)?.to_u64()?;
 
-                #[allow(clippy::integer_arithmetic)]
+                #[allow(clippy::arithmetic_side_effects)]
                 // adding two u64 and a u8 cannot wrap in a u128
                 let wide_sum = u128::from(c_in) + u128::from(a) + u128::from(b);
-                #[allow(clippy::integer_arithmetic)] // it's a u128, we can shift by 64
+                #[allow(clippy::arithmetic_side_effects)] // it's a u128, we can shift by 64
                 let (c_out, sum) = ((wide_sum >> 64).truncate::<u8>(), wide_sum.truncate::<u64>());
 
                 let c_out_field = this.place_field(dest, 0)?;

src/shims/foreign_items.rs

@@ -563,7 +563,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 let (op, op_len) = this.operand_to_simd(op)?;
                 let bitmask_len = op_len.max(8);
 
-                assert!(dest.layout.ty.is_integral());
+                // Returns either an unsigned integer or array of `u8`.
+                assert!(
+                    dest.layout.ty.is_integral()
+                        || matches!(dest.layout.ty.kind(), ty::Array(elemty, _) if elemty == &this.tcx.types.u8)
+                );
                 assert!(bitmask_len <= 64);
                 assert_eq!(bitmask_len, dest.layout.size.bits());
                 let op_len = u32::try_from(op_len).unwrap();
@@ -577,7 +581,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                             .unwrap();
                     }
                 }
-                this.write_int(res, dest)?;
+                // We have to force the place type to be an int so that we can write `res` into it.
+                let mut dest = this.force_allocation(dest)?;
+                dest.layout = this.machine.layouts.uint(dest.layout.size).unwrap();
+                this.write_int(res, &dest.into())?;
             }
 
             name => throw_unsup_format!("unimplemented intrinsic: `simd_{name}`"),
@@ -605,7 +612,7 @@ fn simd_bitmask_index(idx: u32, vec_len: u32, endianness: Endian) -> u32 {
     assert!(idx < vec_len);
     match endianness {
         Endian::Little => idx,
-        #[allow(clippy::integer_arithmetic)] // idx < vec_len
+        #[allow(clippy::arithmetic_side_effects)] // idx < vec_len
         Endian::Big => vec_len - 1 - idx, // reverse order of bits
     }
 }

src/shims/intrinsics/simd.rs

@@ -1,4 +1,4 @@
-#![warn(clippy::integer_arithmetic)]
+#![warn(clippy::arithmetic_side_effects)]
 
 mod backtrace;
 #[cfg(target_os = "linux")]

src/shims/mod.rs

@@ -108,7 +108,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         Ok(0)
     }
 
-    #[allow(non_snake_case, clippy::integer_arithmetic)]
+    #[allow(non_snake_case, clippy::arithmetic_side_effects)]
     fn GetSystemTimeAsFileTime(
         &mut self,
         LPFILETIME_op: &OpTy<'tcx, Provenance>,

src/shims/time.rs

@@ -56,7 +56,7 @@ impl<'tcx> Default for TlsData<'tcx> {
 impl<'tcx> TlsData<'tcx> {
     /// Generate a new TLS key with the given destructor.
     /// `max_size` determines the integer size the key has to fit in.
-    #[allow(clippy::integer_arithmetic)]
+    #[allow(clippy::arithmetic_side_effects)]
     pub fn create_tls_key(
         &mut self,
         dtor: Option<ty::Instance<'tcx>>,

src/shims/tls.rs

@@ -458,7 +458,7 @@ pub struct DirHandler {
 }
 
 impl DirHandler {
-    #[allow(clippy::integer_arithmetic)]
+    #[allow(clippy::arithmetic_side_effects)]
     fn insert_new(&mut self, read_dir: ReadDir) -> u64 {
         let id = self.next_id;
         self.next_id += 1;

src/shims/unix/fs.rs

@@ -247,7 +247,7 @@ pub fn futex<'tcx>(
             // before doing the syscall.
             this.atomic_fence(AtomicFenceOrd::SeqCst)?;
             let mut n = 0;
-            #[allow(clippy::integer_arithmetic)]
+            #[allow(clippy::arithmetic_side_effects)]
             for _ in 0..val {
                 if let Some(thread) = this.futex_wake(addr_usize, bitset) {
                     this.unblock_thread(thread);

src/shims/unix/linux/sync.rs

@@ -219,7 +219,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     .copied()
                     .scan(Size::ZERO, |a, x| {
                         let res = Some(*a);
-                        *a += x;
+                        *a = a.checked_add(x, this).unwrap();
                         res
                     })
                     .collect();

src/shims/windows/foreign_items.rs

@@ -62,7 +62,7 @@ impl Handle {
         let floor_log2 = variant_count.ilog2();
 
         // we need to add one for non powers of two to compensate for the difference
-        #[allow(clippy::integer_arithmetic)] // cannot overflow
+        #[allow(clippy::arithmetic_side_effects)] // cannot overflow
         if variant_count.is_power_of_two() { floor_log2 } else { floor_log2 + 1 }
     }
 
@@ -87,7 +87,7 @@ impl Handle {
 
         // packs the data into the lower `data_size` bits
         // and packs the discriminant right above the data
-        #[allow(clippy::integer_arithmetic)] // cannot overflow
+        #[allow(clippy::arithmetic_side_effects)] // cannot overflow
         return discriminant << data_size | data;
     }
 
@@ -106,11 +106,11 @@ impl Handle {
         let data_size = u32::BITS.checked_sub(disc_size).unwrap();
 
         // the lower `data_size` bits of this mask are 1
-        #[allow(clippy::integer_arithmetic)] // cannot overflow
+        #[allow(clippy::arithmetic_side_effects)] // cannot overflow
         let data_mask = 2u32.pow(data_size) - 1;
 
         // the discriminant is stored right above the lower `data_size` bits
-        #[allow(clippy::integer_arithmetic)] // cannot overflow
+        #[allow(clippy::arithmetic_side_effects)] // cannot overflow
         let discriminant = handle >> data_size;
 
         // the data is stored in the lower `data_size` bits

src/shims/windows/handle.rs

@@ -108,8 +108,9 @@ def test_cargo_miri_run():
         env={'MIRITESTVAR': "wrongval"}, # changing the env var causes a rebuild (re-runs build.rs),
                                          # so keep it set
     )
+    # This also covers passing arguments without `--`: Cargo will forward unused positional arguments to the program.
     test("`cargo miri run` (with arguments and target)",
-        cargo_miri("run") + ["--bin", "cargo-miri-test", "--", "hello world", '"hello world"', r'he\\llo\"world'],
+        cargo_miri("run") + ["--bin", "cargo-miri-test", "hello world", '"hello world"', r'he\\llo\"world'],
         "run.args.stdout.ref", "run.args.stderr.ref",
     )
     test("`cargo miri r` (subcrate, no isolation)",