Merge pull request #4751 from RalfJung/genmc-warnings

genmc: suppress compare_exchange warnings from dependencies

Author: RalfJung

doc/genmc.md

@@ -1,7 +1,6 @@
 # **(WIP)** Documentation for Miri-GenMC
 
-**NOTE: GenMC mode is not yet fully implemented, and has [several correctness issues](https://github.com/rust-lang/miri/issues/4572). Using GenMC mode currently requires manually compiling Miri, see [Usage](#usage).**
-
+**NOTE: GenMC mode is not yet fully implemented, and has [several correctness issues](https://github.com/rust-lang/miri/issues/4572) and [other limitations](#limitations). Using GenMC mode currently requires manually compiling Miri, see [Usage](#usage).**
 
 [GenMC](https://github.com/MPI-SWS/genmc) is a stateless model checker for exploring concurrent executions of a program.
 Miri-GenMC integrates that model checker into Miri.
@@ -12,11 +11,14 @@ This includes all possible thread interleavings and all allowed return values fo
 It is hence still possible to have latent bugs in a test case even if they passed GenMC.)
 
 GenMC requires the input program to be bounded, i.e., have finitely many possible executions, otherwise it will not terminate.
-Any loops that may run infinitely must be replaced or bounded (see below).
+Any loops that may run infinitely must be replaced or bounded (see [below](#eliminating-unbounded-loops)).
 
 GenMC makes use of Dynamic Partial Order Reduction (DPOR) to reduce the number of executions that must be explored, but the runtime can still be super-exponential in the size of the input program (number of threads and amount of interaction between threads).
 Large programs may not be verifiable in a reasonable amount of time.
 
+GenMC currently only supports Linux hosts.
+Both the host and the target must be 64-bit little-endian.
+
 ## Usage
 
 For testing/developing Miri-GenMC:
@@ -50,16 +52,24 @@ Note that `cargo miri test` in GenMC mode is currently not supported.
     - `debug2`:   Print the execution graph after every memory access.
     - `debug3`:   Print reads-from values considered by GenMC.
 - `-Zmiri-genmc-verbose`: Show more information, such as estimated number of executions, and time taken for verification.
-
-#### Regular Miri parameters useful for GenMC mode
-
 - `-Zmiri-disable-weak-memory-emulation`: Disable any weak memory effects (effectively upgrading all atomic orderings in the program to `SeqCst`). This option may reduce the number of explored program executions, but any bugs related to weak memory effects will be missed. This option can help determine if an error is caused by weak memory effects (i.e., if it disappears with this option enabled).
 
 <!-- FIXME(genmc): explain Miri-GenMC specific functions. -->
 
-## Tips
+## Limitations
+
+There are several limitations which can make GenMC miss bugs:
+- GenMC does not support re-using freed memory for new allocations, so any bugs related to that will be missed.
+- GenMC does not support `compare_exchange_weak`, so the consequences of spurious failures are not explored.
+  A warning will be emitted if this affects code you wrote (but not if it happens inside your dependencies).
+- GenMC does not support the separate failure ordering of `compare_exchange`. Miri will take the maximum of the success and failure ordering and use that for the access; outcomes that rely on the real ordering being weaker will not be explored.
+  A warning will be emitted if this affects code you wrote (but not if it happens inside your dependencies).
+- GenMC is incompatible with borrow tracking (Stacked/Tree Borrows). You need to set `-Zmiri-disable-stacked-borrows` to use GenMC.
+- Like all C++ memory model verification tools, GenMC has to solve the [out-of-thin-air problem](https://www.cl.cam.ac.uk/~pes20/cpp/notes42.html).
+  It takes the [usual approach](https://plv.mpi-sws.org/scfix/paper.pdf) of requiring the union of "program-order" and "reads-from" to be acyclic.
+  This means it excludes certain behaviors allowed by the C++ memory model, some of which can occur on hardware that performs load buffering.
 
-<!-- FIXME(genmc): add tips for using Miri-GenMC more efficiently. -->
+## Tips
 
 ### Eliminating unbounded loops
 
@@ -121,24 +131,11 @@ fn count_until_true_genmc(flag: &AtomicBool) -> u64 {
 <!-- FIXME: update the code above once Miri supports a loop bounding features like GenMC's `--unroll=N`. -->
 <!-- FIXME: update this section once Miri-GenMC supports automatic program transformations (like spinloop-assume replacement). -->
 
-## Limitations
-
-Some or all of these limitations might get removed in the future:
-
-- Borrow tracking is currently incompatible (stacked/tree borrows).
-- Only Linux is supported for now.
-- No support for 32-bit or big-endian targets.
-- No cross-target interpretation.
-
-<!-- FIXME(genmc): document remaining limitations -->
-
 ## Development
 
 GenMC is written in C++, which complicates development a bit.
 The prerequisites for building Miri-GenMC are:
-- A compiler with C++23 support.
-- LLVM developments headers and clang.
-  <!-- FIXME(genmc,llvm): remove once LLVM dependency is no longer required. -->
+- A compiler with sufficient C++20 support (we are testing GCC 13).
 
 The actual code for GenMC is not contained in the Miri repo itself, but in a [separate GenMC repo](https://github.com/MPI-SWS/genmc) (with its own maintainers).
 These sources need to be available to build Miri-GenMC.
@@ -149,6 +146,8 @@ The process for obtaining them is as follows:
   If you place this directory inside the Miri folder, it is recommended to call it `genmc-src` as that tells `./miri fmt` to avoid
   formatting the Rust files inside that folder.
 
+<!-- FIXME(genmc): explain how submitting code to GenMC should be handled. -->
+
 ### Formatting the C++ code
 
 For formatting the C++ code we provide a `.clang-format` file in the `genmc-sys` directory.
@@ -157,7 +156,3 @@ With `clang-format` installed, run this command to format the c++ files (replace
 find ./genmc-sys/cpp/ -name "*.cpp" -o -name "*.hpp" | xargs clang-format --style=file:"./genmc-sys/.clang-format" -i
 ```
 NOTE: this is currently not done automatically on pull requests to Miri.
-
-<!-- FIXME(genmc): explain how submitting code to GenMC should be handled. -->
-
-<!-- FIXME(genmc): explain development. -->

src/concurrency/genmc/helper.rs

@@ -171,11 +171,8 @@ impl AtomicRwOrd {
             (AtomicReadOrd::Acquire, AtomicWriteOrd::Relaxed) => AtomicRwOrd::Acquire,
             (AtomicReadOrd::Relaxed, AtomicWriteOrd::Release) => AtomicRwOrd::Release,
             (AtomicReadOrd::Acquire, AtomicWriteOrd::Release) => AtomicRwOrd::AcqRel,
-            (AtomicReadOrd::SeqCst, AtomicWriteOrd::SeqCst) => AtomicRwOrd::SeqCst,
-            _ =>
-                panic!(
-                    "Unsupported memory ordering combination ({read_ordering:?}, {write_ordering:?})"
-                ),
+            (AtomicReadOrd::SeqCst, _) => AtomicRwOrd::SeqCst,
+            (_, AtomicWriteOrd::SeqCst) => AtomicRwOrd::SeqCst,
         }
     }
 

src/concurrency/genmc/mod.rs

@@ -402,8 +402,20 @@ impl GenmcCtx {
 
         // FIXME(genmc): remove once GenMC supports failure memory ordering in `compare_exchange`.
         let (effective_failure_ordering, _) = upgraded_success_ordering.split_memory_orderings();
-        // Return a warning if the actual orderings don't match the upgraded ones.
-        if success != upgraded_success_ordering || effective_failure_ordering != fail {
+
+        // Return a warning if we cannot explore all behaviors of this operation.
+        // Only emit this if the operation is "in user code": walk up across `#[track_caller]`
+        // frames, then check if the next frame is local.
+        let show_warning = || {
+            ecx.active_thread_stack()
+                .iter()
+                .rev()
+                .find(|f| !f.instance().def.requires_caller_location(*ecx.tcx))
+                .is_none_or(|f| ecx.machine.is_local(f.instance()))
+        };
+        if (success != upgraded_success_ordering || effective_failure_ordering != fail)
+            && show_warning()
+        {
             static DEDUP: SpanDedupDiagnostic = SpanDedupDiagnostic::new();
             ecx.dedup_diagnostic(&DEDUP, |_first| {
                 NonHaltingDiagnostic::GenmcCompareExchangeOrderingMismatch {
@@ -415,7 +427,7 @@ impl GenmcCtx {
             });
         }
         // FIXME(genmc): remove once GenMC implements spurious failures for `compare_exchange_weak`.
-        if can_fail_spuriously {
+        if can_fail_spuriously && show_warning() {
             static DEDUP: SpanDedupDiagnostic = SpanDedupDiagnostic::new();
             ecx.dedup_diagnostic(&DEDUP, |_first| NonHaltingDiagnostic::GenmcCompareExchangeWeak);
         }

tests/genmc/fail/shims/mutex_diff_thread_unlock.stderr

@@ -1,64 +1,4 @@
 Running GenMC Verification...
-warning: GenMC currently does not model the failure ordering for `compare_exchange`. Due to success ordering 'Acquire', the failure ordering 'Relaxed' is treated like 'Acquire'. Miri with GenMC might miss bugs related to this memory access.
-  --> RUSTLIB/std/src/sys/sync/PLATFORM/futex.rs:LL:CC
-   |
-LL |               || self
-   |  ________________^
-LL | |                 .state
-LL | |                 .compare_exchange_weak(state, state + READ_LOCKED, Acquire, Relaxed)
-   | |____________________________________________________________________________________^ GenMC might miss possible behaviors of this code
-   |
-   = note: BACKTRACE:
-   = note: inside `std::sys::sync::PLATFORM::futex::RwLock::read` at RUSTLIB/std/src/sys/sync/PLATFORM/futex.rs:LL:CC
-   = note: inside `std::sync::RwLock::<()>::read` at RUSTLIB/std/src/sync/poison/rwlock.rs:LL:CC
-   = note: inside `std::sys::env::PLATFORM::env_read_lock` at RUSTLIB/std/src/sys/env/PLATFORM.rs:LL:CC
-   = note: inside closure at RUSTLIB/std/src/sys/env/PLATFORM.rs:LL:CC
-   = note: inside `std::sys::pal::PLATFORM::small_c_string::run_with_cstr_stack::<std::option::Option<std::ffi::OsString>>` at RUSTLIB/std/src/sys/pal/PLATFORM/small_c_string.rs:LL:CC
-   = note: inside `std::sys::pal::PLATFORM::small_c_string::run_with_cstr::<std::option::Option<std::ffi::OsString>>` at RUSTLIB/std/src/sys/pal/PLATFORM/small_c_string.rs:LL:CC
-   = note: inside `std::sys::env::PLATFORM::getenv` at RUSTLIB/std/src/sys/env/PLATFORM.rs:LL:CC
-   = note: inside `std::env::_var_os` at RUSTLIB/std/src/env.rs:LL:CC
-   = note: inside `std::env::var_os::<&str>` at RUSTLIB/std/src/env.rs:LL:CC
-   = note: inside closure at RUSTLIB/std/src/thread/lifecycle.rs:LL:CC
-note: inside `miri_start`
-  --> tests/genmc/fail/shims/mutex_diff_thread_unlock.rs:LL:CC
-   |
-LL |       let handle = std::thread::spawn(move || {
-   |  __________________^
-LL | |         let guard = guard; // avoid field capturing
-LL | |         drop(guard);
-LL | |     });
-   | |______^
-
-warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
-  --> RUSTLIB/std/src/sys/sync/PLATFORM/futex.rs:LL:CC
-   |
-LL |               || self
-   |  ________________^
-LL | |                 .state
-LL | |                 .compare_exchange_weak(state, state + READ_LOCKED, Acquire, Relaxed)
-   | |____________________________________________________________________________________^ GenMC might miss possible behaviors of this code
-   |
-   = note: BACKTRACE:
-   = note: inside `std::sys::sync::PLATFORM::futex::RwLock::read` at RUSTLIB/std/src/sys/sync/PLATFORM/futex.rs:LL:CC
-   = note: inside `std::sync::RwLock::<()>::read` at RUSTLIB/std/src/sync/poison/rwlock.rs:LL:CC
-   = note: inside `std::sys::env::PLATFORM::env_read_lock` at RUSTLIB/std/src/sys/env/PLATFORM.rs:LL:CC
-   = note: inside closure at RUSTLIB/std/src/sys/env/PLATFORM.rs:LL:CC
-   = note: inside `std::sys::pal::PLATFORM::small_c_string::run_with_cstr_stack::<std::option::Option<std::ffi::OsString>>` at RUSTLIB/std/src/sys/pal/PLATFORM/small_c_string.rs:LL:CC
-   = note: inside `std::sys::pal::PLATFORM::small_c_string::run_with_cstr::<std::option::Option<std::ffi::OsString>>` at RUSTLIB/std/src/sys/pal/PLATFORM/small_c_string.rs:LL:CC
-   = note: inside `std::sys::env::PLATFORM::getenv` at RUSTLIB/std/src/sys/env/PLATFORM.rs:LL:CC
-   = note: inside `std::env::_var_os` at RUSTLIB/std/src/env.rs:LL:CC
-   = note: inside `std::env::var_os::<&str>` at RUSTLIB/std/src/env.rs:LL:CC
-   = note: inside closure at RUSTLIB/std/src/thread/lifecycle.rs:LL:CC
-note: inside `miri_start`
-  --> tests/genmc/fail/shims/mutex_diff_thread_unlock.rs:LL:CC
-   |
-LL |       let handle = std::thread::spawn(move || {
-   |  __________________^
-LL | |         let guard = guard; // avoid field capturing
-LL | |         drop(guard);
-LL | |     });
-   | |______^
-
 error: Undefined Behavior: Invalid unlock() operation
   --> RUSTLIB/std/src/sync/poison/mutex.rs:LL:CC
    |
@@ -82,5 +22,5 @@ note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a
 
 note: add `-Zmiri-genmc-print-genmc-output` to MIRIFLAGS to see the detailed GenMC error report
 
-error: aborting due to 1 previous error; 2 warnings emitted
+error: aborting due to 1 previous error
 

tests/genmc/pass/atomics/cas_simple.rs

@@ -30,5 +30,10 @@ fn miri_start(_argc: isize, _argv: *const *const u8) -> isize {
     if 2 != VALUE.load(SeqCst) {
         std::process::abort()
     }
+
+    // Check that we emit warnings for cases that are not fully supported.
+    let _ = VALUE.compare_exchange(99, 99, SeqCst, Relaxed);
+    let _ = VALUE.compare_exchange_weak(99, 99, Relaxed, SeqCst);
+
     0
 }

tests/genmc/pass/atomics/cas_simple.stderr

@@ -1,2 +1,20 @@
 Running GenMC Verification...
+warning: GenMC currently does not model the failure ordering for `compare_exchange`. Due to success ordering 'SeqCst', the failure ordering 'Relaxed' is treated like 'SeqCst'. Miri with GenMC might miss bugs related to this memory access.
+  --> tests/genmc/pass/atomics/cas_simple.rs:LL:CC
+   |
+LL |     let _ = VALUE.compare_exchange(99, 99, SeqCst, Relaxed);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
+
+warning: GenMC currently does not model the failure ordering for `compare_exchange`. Success ordering 'Relaxed' was upgraded to 'SeqCst' to match failure ordering 'SeqCst'. Miri with GenMC might miss bugs related to this memory access.
+  --> tests/genmc/pass/atomics/cas_simple.rs:LL:CC
+   |
+LL |     let _ = VALUE.compare_exchange_weak(99, 99, Relaxed, SeqCst);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
+
+warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
+  --> tests/genmc/pass/atomics/cas_simple.rs:LL:CC
+   |
+LL |     let _ = VALUE.compare_exchange_weak(99, 99, Relaxed, SeqCst);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
+
 Verification complete with 1 executions. No errors found.