Auto merge of #110168 - saethlin:miri, r=RalfJung

bors · bors · commit 8e1162f07e8b · 2023-04-11T08:11:46.000Z
update Miri

Most importantly, this should ensure that the Miri test suite passes in this repo, when the issue is fixed.

r? `@oli-obk`
diff --git a/src/tools/miri/.github/workflows/ci.yml b/src/tools/miri/.github/workflows/ci.yml
@@ -174,7 +174,12 @@ jobs:
           ~/.local/bin/zulip-send --stream miri --subject "Cron Job Failure (miri, $(date -u +%Y-%m))" \
             --message 'Dear @*T-miri*,
 
-          It would appear that the Miri cron job build failed. Would you mind investigating this issue?
+          It would appear that the [Miri cron job build]('"https://github.com/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID"') failed.
+          
+          This likely means that rustc changed the miri directory and
+          we now need to do a [`./miri rustc-pull`](https://github.com/rust-lang/miri/blob/master/CONTRIBUTING.md#importing-changes-from-the-rustc-repo).
+
+          Would you mind investigating this issue?
 
           Thanks in advance!
           Sincerely,
diff --git a/src/tools/miri/Cargo.lock b/src/tools/miri/Cargo.lock
@@ -282,19 +282,19 @@ checksum = "201de327520df007757c1f0adce6e827fe8562fbc28bfd9c15571c66ca1f5f79"
 
 [[package]]
 name = "libffi"
-version = "3.0.1"
+version = "3.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1e454b3efb16fba3b17810ae5e41df02b649e564ab3c5a34b3b93ed07ad287e6"
+checksum = "ce826c243048e3d5cec441799724de52e2d42f820468431fc3fceee2341871e2"
 dependencies = [
  "libc",
  "libffi-sys",
 ]
 
 [[package]]
 name = "libffi-sys"
-version = "2.0.1"
+version = "2.2.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "84e78d02e5a8eae9c24c38ce6e6026f80e16dff76adcdae4bc5c6c52c2de4a60"
+checksum = "dc65067b78c0fc069771e8b9a9e02df71e08858bec92c1f101377c67b9dca7c7"
 dependencies = [
  "cc",
 ]
diff --git a/src/tools/miri/README.md b/src/tools/miri/README.md
@@ -15,7 +15,7 @@ for example:
   or an invalid enum discriminant)
 * **Experimental**: Violations of the [Stacked Borrows] rules governing aliasing
   for reference types
-* **Experimental**: Violations of the Tree Borrows aliasing rules, as an optional
+* **Experimental**: Violations of the [Tree Borrows] aliasing rules, as an optional
   alternative to [Stacked Borrows]
 * **Experimental**: Data races
 
@@ -79,6 +79,7 @@ behavior** in your program, and cannot run all programs:
 [`unreachable_unchecked`]: https://doc.rust-lang.org/stable/std/hint/fn.unreachable_unchecked.html
 [`copy_nonoverlapping`]: https://doc.rust-lang.org/stable/std/ptr/fn.copy_nonoverlapping.html
 [Stacked Borrows]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md
+[Tree Borrows]: https://perso.crans.org/vanille/treebor/
 
 
 ## Using Miri
@@ -359,7 +360,7 @@ to Miri failing to detect cases of undefined behavior in a program.
 * `-Zmiri-disable-data-race-detector` disables checking for data races.  Using
   this flag is **unsound**. This implies `-Zmiri-disable-weak-memory-emulation`.
 * `-Zmiri-disable-stacked-borrows` disables checking the experimental
-  aliasing rules to track borrows ([Stacked Borrows] and Tree Borrows).
+  aliasing rules to track borrows ([Stacked Borrows] and [Tree Borrows]).
   This can make Miri run faster, but it also means no aliasing violations will
   be detected. Using this flag is **unsound** (but the affected soundness rules
   are experimental). Later flags take precedence: borrow tracking can be reactivated
@@ -425,7 +426,7 @@ to Miri failing to detect cases of undefined behavior in a program.
 * `-Zmiri-track-weak-memory-loads` shows a backtrace when weak memory emulation returns an outdated
   value from a load. This can help diagnose problems that disappear under
   `-Zmiri-disable-weak-memory-emulation`.
-* `-Zmiri-tree-borrows` replaces [Stacked Borrows] with the Tree Borrows rules.
+* `-Zmiri-tree-borrows` replaces [Stacked Borrows] with the [Tree Borrows] rules.
   The soundness rules are already experimental without this flag, but even more
   so with this flag.
 * `-Zmiri-force-page-size=<num>` overrides the default page size for an architecture, in multiples of 1k.
@@ -442,7 +443,7 @@ Some native rustc `-Z` flags are also very relevant for Miri:
   functions.  This is needed so that Miri can execute such functions, so Miri
   sets this flag per default.
 * `-Zmir-emit-retag` controls whether `Retag` statements are emitted. Miri
-  enables this per default because it is needed for [Stacked Borrows] and Tree Borrows.
+  enables this per default because it is needed for [Stacked Borrows] and [Tree Borrows].
 
 Moreover, Miri recognizes some environment variables:
 
diff --git a/src/tools/miri/ci.sh b/src/tools/miri/ci.sh
@@ -43,7 +43,9 @@ function run_tests {
     # optimizations up all the way, too).
     # Optimizations change diagnostics (mostly backtraces), so we don't check
     # them. Also error locations change so we don't run the failing tests.
-    MIRIFLAGS="${MIRIFLAGS:-} -O -Zmir-opt-level=4" MIRI_SKIP_UI_CHECKS=1 ./miri test -- tests/{pass,panic}
+    # We explicitly enable debug-assertions here, they are disabled by -O but we have tests
+    # which exist to check that we panic on debug assertion failures.
+    MIRIFLAGS="${MIRIFLAGS:-} -O -Zmir-opt-level=4 -Cdebug-assertions=yes" MIRI_SKIP_UI_CHECKS=1 ./miri test -- tests/{pass,panic}
 
     # Also run some many-seeds tests. 64 seeds means this takes around a minute per test.
     for FILE in tests/many-seeds/*.rs; do
diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version
@@ -1 +1 @@
-511364e7874dba9649a264100407e4bffe7b5425
+d4be8efc6296bace5b1e165f1b34d3c6da76aa8e
diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs
@@ -28,9 +28,11 @@ use rustc_middle::{
     middle::exported_symbols::{
         ExportedSymbol, SymbolExportInfo, SymbolExportKind, SymbolExportLevel,
     },
-    ty::{query::ExternProviders, TyCtxt},
     query::LocalCrate,
+    ty::{query::ExternProviders, TyCtxt},
 };
+use rustc_session::config::OptLevel;
+
 use rustc_session::{config::CrateType, search_paths::PathKind, CtfeBacktrace};
 
 use miri::{BacktraceStyle, BorrowTrackerMethod, ProvenanceMode, RetagFields};
@@ -83,6 +85,21 @@ impl rustc_driver::Callbacks for MiriCompilerCalls {
                 env::set_current_dir(cwd).unwrap();
             }
 
+            if tcx.sess.opts.optimize != OptLevel::No {
+                tcx.sess.warn("Miri does not support optimizations. If you have enabled optimizations \
+                    by selecting a Cargo profile (such as --release) which changes other profile settings \
+                    such as whether debug assertions and overflow checks are enabled, those settings are \
+                    still applied.");
+            }
+            if tcx.sess.mir_opt_level() > 0 {
+                tcx.sess.warn("You have explicitly enabled MIR optimizations, overriding Miri's default \
+                    which is to completely disable them. Any optimizations may hide UB that Miri would \
+                    otherwise detect, and it is not necessarily possible to predict what kind of UB will \
+                    be missed. If you are enabling optimizations to make Miri run faster, we advise using \
+                    cfg(miri) to shrink your workload instead. The performance benefit of enabling MIR \
+                    optimizations is usually marginal at best.");
+            }
+
             if let Some(return_code) = miri::eval_entry(tcx, entry_def_id, entry_type, config) {
                 std::process::exit(
                     i32::try_from(return_code).expect("Return value was too large!"),
diff --git a/src/tools/miri/src/clock.rs b/src/tools/miri/src/clock.rs
@@ -3,7 +3,10 @@ use std::time::{Duration, Instant as StdInstant};
 
 /// When using a virtual clock, this defines how many nanoseconds we pretend are passing for each
 /// basic block.
-const NANOSECONDS_PER_BASIC_BLOCK: u64 = 10;
+/// This number is pretty random, but it has been shown to approximately cause
+/// some sample programs to run within an order of magnitude of real time on desktop CPUs.
+/// (See `tests/pass/shims/time-with-isolation*.rs`.)
+const NANOSECONDS_PER_BASIC_BLOCK: u64 = 5000;
 
 #[derive(Debug)]
 pub struct Instant {
diff --git a/src/tools/miri/src/lib.rs b/src/tools/miri/src/lib.rs
@@ -124,11 +124,13 @@ pub use crate::tag_gc::{EvalContextExt as _, VisitTags};
 
 /// Insert rustc arguments at the beginning of the argument list that Miri wants to be
 /// set per default, for maximal validation power.
+/// Also disable the MIR pass that inserts an alignment check on every pointer dereference. Miri
+/// does that too, and with a better error message.
 pub const MIRI_DEFAULT_ARGS: &[&str] = &[
+    "--cfg=miri",
     "-Zalways-encode-mir",
+    "-Zextra-const-ub-checks",
     "-Zmir-emit-retag",
     "-Zmir-opt-level=0",
-    "--cfg=miri",
-    "-Cdebug-assertions=on",
-    "-Zextra-const-ub-checks",
+    "-Zmir-enable-passes=-CheckAlignment",
 ];
diff --git a/src/tools/miri/src/shims/panic.rs b/src/tools/miri/src/shims/panic.rs
@@ -157,7 +157,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 &[catch_unwind.data.into(), payload.into()],
                 None,
                 // Directly return to caller of `try`.
-                StackPopCleanup::Goto { ret: Some(catch_unwind.ret), unwind: mir::UnwindAction::Continue },
+                StackPopCleanup::Goto {
+                    ret: Some(catch_unwind.ret),
+                    unwind: mir::UnwindAction::Continue,
+                },
             )?;
 
             // We pushed a new stack frame, the engine should not do any jumping now!
@@ -211,10 +214,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     Abi::Rust,
                     &[index.into(), len.into()],
                     None,
-                    StackPopCleanup::Goto {
-                        ret: None,
-                        unwind,
-                    },
+                    StackPopCleanup::Goto { ret: None, unwind },
                 )?;
             }
             MisalignedPointerDereference { required, found } => {
@@ -235,19 +235,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     Abi::Rust,
                     &[required.into(), found.into()],
                     None,
-                    StackPopCleanup::Goto {
-                        ret: None,
-                        unwind,
-                    },
+                    StackPopCleanup::Goto { ret: None, unwind },
                 )?;
             }
 
             _ => {
                 // Forward everything else to `panic` lang item.
-                this.start_panic(
-                    msg.description(),
-                    unwind,
-                )?;
+                this.start_panic(msg.description(), unwind)?;
             }
         }
         Ok(())
diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs
@@ -628,13 +628,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let fd = this.read_scalar(&args[0])?.to_i32()?;
         let cmd = this.read_scalar(&args[1])?.to_i32()?;
 
-        // Reject if isolation is enabled.
-        if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
-            this.reject_in_isolation("`fcntl`", reject_with)?;
-            this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?;
-            return Ok(-1);
-        }
-
         // We only support getting the flags for a descriptor.
         if cmd == this.eval_libc_i32("F_GETFD") {
             // Currently this is the only flag that `F_GETFD` returns. It is OK to just return the
@@ -677,6 +670,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 None => this.handle_not_found(),
             }
         } else if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC") {
+            // Reject if isolation is enabled.
+            if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
+                this.reject_in_isolation("`fcntl`", reject_with)?;
+                this.set_last_error_from_io_error(ErrorKind::PermissionDenied)?;
+                return Ok(-1);
+            }
+
             if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) {
                 // FIXME: Support fullfsync for all FDs
                 let FileHandle { file, writable } =
diff --git a/src/tools/miri/tests/fail/terminate-terminator.stderr b/src/tools/miri/tests/fail/terminate-terminator.stderr
@@ -1,3 +1,5 @@
+warning: You have explicitly enabled MIR optimizations, overriding Miri's default which is to completely disable them. Any optimizations may hide UB that Miri would otherwise detect, and it is not necessarily possible to predict what kind of UB will be missed. If you are enabling optimizations to make Miri run faster, we advise using cfg(miri) to shrink your workload instead. The performance benefit of enabling MIR optimizations is usually marginal at best.
+
 thread 'main' panicked at 'explicit panic', $DIR/terminate-terminator.rs:LL:CC
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 error: abnormal termination: panic in a function that cannot unwind
@@ -23,5 +25,5 @@ LL |     panic_abort();
 
 note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
 
-error: aborting due to previous error
+error: aborting due to previous error; 1 warning emitted
 
diff --git a/src/tools/miri/tests/panic/alignment-assertion.rs b/src/tools/miri/tests/panic/alignment-assertion.rs
diff --git a/src/tools/miri/tests/panic/alignment-assertion.stderr b/src/tools/miri/tests/panic/alignment-assertion.stderr
diff --git a/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.rs b/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.rs
@@ -0,0 +1,12 @@
+//@only-target-apple: F_FULLFSYNC only on apple systems
+//@compile-flags: -Zmiri-isolation-error=warn-nobacktrace
+
+use std::io::Error;
+
+fn main() {
+    // test `fcntl(F_FULLFSYNC)`
+    unsafe {
+        assert_eq!(libc::fcntl(1, libc::F_FULLFSYNC, 0), -1);
+        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EPERM));
+    }
+}
diff --git a/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.stderr b/src/tools/miri/tests/pass-dep/shims/fcntl_f-fullfsync_apple.stderr
@@ -0,0 +1,2 @@
+warning: `fcntl` was made to return an error due to isolation
+
diff --git a/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs b/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.rs
@@ -7,10 +7,9 @@ use std::fs;
 use std::io::{Error, ErrorKind};
 
 fn main() {
-    // test `fcntl`
+    // test `fcntl(F_DUPFD): should work even with isolation.`
     unsafe {
-        assert_eq!(libc::fcntl(1, libc::F_DUPFD, 0), -1);
-        assert_eq!(Error::last_os_error().raw_os_error(), Some(libc::EPERM));
+        assert!(libc::fcntl(1, libc::F_DUPFD, 0) >= 0);
     }
 
     // test `readlink`
diff --git a/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr b/src/tools/miri/tests/pass-dep/shims/libc-fs-with-isolation.stderr
@@ -1,5 +1,3 @@
-warning: `fcntl` was made to return an error due to isolation
-
 warning: `readlink` was made to return an error due to isolation
 
 warning: `$STAT` was made to return an error due to isolation
diff --git a/src/tools/miri/tests/pass-dep/tokio/sleep.rs b/src/tools/miri/tests/pass-dep/tokio/sleep.rs
@@ -1,4 +1,4 @@
-//@compile-flags: -Zmiri-disable-isolation -Zmiri-permissive-provenance -Zmiri-backtrace=full
+//@compile-flags: -Zmiri-permissive-provenance -Zmiri-backtrace=full
 //@only-target-x86_64-unknown-linux: support for tokio only on linux and x86
 
 use tokio::time::{sleep, Duration, Instant};
@@ -7,8 +7,6 @@ use tokio::time::{sleep, Duration, Instant};
 async fn main() {
     let start = Instant::now();
     sleep(Duration::from_secs(1)).await;
-    // It takes 96 millisecond to sleep for 1 millisecond
-    // It takes 1025 millisecond to sleep for 1 second
     let time_elapsed = &start.elapsed().as_millis();
-    assert!(time_elapsed > &1000, "{}", time_elapsed);
+    assert!((1000..1100).contains(time_elapsed), "{}", time_elapsed);
 }
diff --git a/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs b/src/tools/miri/tests/pass-dep/tokio/tokio_mvp.rs
@@ -1,5 +1,5 @@
 // Need to disable preemption to stay on the supported MVP codepath in mio.
-//@compile-flags: -Zmiri-disable-isolation -Zmiri-permissive-provenance
+//@compile-flags: -Zmiri-permissive-provenance
 //@only-target-x86_64-unknown-linux: support for tokio exists only on linux and x86
 
 #[tokio::main]
diff --git a/src/tools/miri/tests/pass/concurrency/thread_park_isolated.rs b/src/tools/miri/tests/pass/concurrency/thread_park_isolated.rs
@@ -7,6 +7,6 @@ fn main() {
 
     thread::park_timeout(Duration::from_millis(200));
 
-    // Thanks to deterministic execution, this will wiat *exactly* 200ms (rounded to 1ms).
-    assert!((200..201).contains(&start.elapsed().as_millis()));
+    // Thanks to deterministic execution, this will wait *exactly* 200ms, plus the time for the surrounding code.
+    assert!((200..210).contains(&start.elapsed().as_millis()), "{}", start.elapsed().as_millis());
 }
diff --git a/src/tools/miri/tests/pass/shims/time-with-isolation.rs b/src/tools/miri/tests/pass/shims/time-with-isolation.rs
@@ -22,14 +22,23 @@ fn test_time_passes() {
     let diff = now2.duration_since(now1);
     assert_eq!(now1 + diff, now2);
     assert_eq!(now2 - diff, now1);
-    // The virtual clock is deterministic and I got 29us on a 64-bit Linux machine. However, this
+    // The virtual clock is deterministic and I got 15ms on a 64-bit Linux machine. However, this
     // changes according to the platform so we use an interval to be safe. This should be updated
     // if `NANOSECONDS_PER_BASIC_BLOCK` changes.
-    assert!(diff.as_micros() > 10);
-    assert!(diff.as_micros() < 40);
+    assert!(diff.as_millis() > 5);
+    assert!(diff.as_millis() < 20);
+}
+
+fn test_block_for_one_second() {
+    let end = Instant::now() + Duration::from_secs(1);
+    // This takes a long time, as we only increment when statements are executed.
+    // When we sleep on all threads, we fast forward to the sleep duration, which we can't
+    // do with busy waiting.
+    while Instant::now() < end {}
 }
 
 fn main() {
     test_time_passes();
+    test_block_for_one_second();
     test_sleep();
 }
diff --git a/src/tools/miri/tests/pass/shims/time-with-isolation2.rs b/src/tools/miri/tests/pass/shims/time-with-isolation2.rs
@@ -0,0 +1,8 @@
+use std::time::Instant;
+
+fn main() {
+    let begin = Instant::now();
+    for _ in 0..100_000 {}
+    let time = begin.elapsed();
+    println!("The loop took around {}s", time.as_secs());
+}
diff --git a/src/tools/miri/tests/pass/shims/time-with-isolation2.stdout b/src/tools/miri/tests/pass/shims/time-with-isolation2.stdout
@@ -0,0 +1 @@
+The loop took around 13s

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-511364e7874dba9649a264100407e4bffe7b5425`
	`1`	`+d4be8efc6296bace5b1e165f1b34d3c6da76aa8e`