Merge pull request rust-lang#4617 from RalfJung/warn-dedup

deduplicate warnings globally

Author: RalfJung

src/tools/miri/src/alloc_addresses/mod.rs

@@ -13,6 +13,7 @@ use rustc_middle::ty::TyCtxt;
 pub use self::address_generator::AddressGenerator;
 use self::reuse_pool::ReusePool;
 use crate::concurrency::VClock;
+use crate::diagnostics::SpanDedupDiagnostic;
 use crate::*;
 
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
@@ -341,12 +342,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         match global_state.provenance_mode {
             ProvenanceMode::Default => {
                 // The first time this happens at a particular location, print a warning.
-                let mut int2ptr_warned = this.machine.int2ptr_warned.borrow_mut();
-                let first = int2ptr_warned.is_empty();
-                if int2ptr_warned.insert(this.cur_span()) {
-                    // Newly inserted, so first time we see this span.
-                    this.emit_diagnostic(NonHaltingDiagnostic::Int2Ptr { details: first });
-                }
+                static DEDUP: SpanDedupDiagnostic = SpanDedupDiagnostic::new();
+                this.dedup_diagnostic(&DEDUP, |first| {
+                    NonHaltingDiagnostic::Int2Ptr { details: first }
+                });
             }
             ProvenanceMode::Strict => {
                 throw_machine_stop!(TerminationInfo::Int2PtrWithStrictProvenance);

src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs

@@ -6,6 +6,7 @@ mod item;
 mod stack;
 
 use std::fmt::Write;
+use std::sync::atomic::AtomicBool;
 use std::{cmp, mem};
 
 use rustc_abi::{BackendRepr, Size};
@@ -822,7 +823,8 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> {
         let size = match size {
             Some(size) => size,
             None => {
-                if !this.machine.sb_extern_type_warned.replace(true) {
+                static DEDUP: AtomicBool = AtomicBool::new(false);
+                if !DEDUP.swap(true, std::sync::atomic::Ordering::Relaxed) {
                     this.emit_diagnostic(NonHaltingDiagnostic::ExternTypeReborrow);
                 }
                 return interp_ok(place.clone());

src/tools/miri/src/concurrency/genmc/helper.rs

@@ -1,58 +1,19 @@
-use std::sync::RwLock;
-
 use genmc_sys::{MemOrdering, RMWBinOp};
 use rustc_abi::Size;
 use rustc_const_eval::interpret::{InterpResult, interp_ok};
-use rustc_data_structures::fx::FxHashSet;
 use rustc_middle::mir;
 use rustc_middle::mir::interpret;
 use rustc_middle::ty::ScalarInt;
-use rustc_span::Span;
 use tracing::debug;
 
 use super::GenmcScalar;
 use crate::alloc_addresses::EvalContextExt as _;
-use crate::diagnostics::EvalContextExt as _;
 use crate::intrinsics::AtomicRmwOp;
-use crate::{
-    AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, BorTag, GenmcCtx, InterpCx,
-    MiriInterpCx, MiriMachine, NonHaltingDiagnostic, Scalar, machine, throw_unsup_format,
-};
+use crate::*;
 
 /// Maximum size memory access in bytes that GenMC supports.
 pub(super) const MAX_ACCESS_SIZE: u64 = 8;
 
-/// Type for storing spans for already emitted warnings.
-pub(super) type WarningCache = RwLock<FxHashSet<Span>>;
-
-#[derive(Default)]
-pub(super) struct Warnings {
-    pub(super) compare_exchange_failure_ordering: WarningCache,
-    pub(super) compare_exchange_weak: WarningCache,
-}
-
-/// Emit a warning if it hasn't already been reported for current span.
-pub(super) fn emit_warning<'tcx>(
-    ecx: &InterpCx<'tcx, MiriMachine<'tcx>>,
-    cache: &WarningCache,
-    diagnostic: impl FnOnce() -> NonHaltingDiagnostic,
-) {
-    // FIXME: This is not the right span to use (it's always inside the local crates so if the same
-    // operation is invoked from multiple places it will warn multiple times). `cur_span` is not
-    // right either though (we should honor `#[track_caller]`). Ultimately what we want is "the
-    // primary span the warning would point at".
-    let span = ecx.machine.current_user_relevant_span();
-    if cache.read().unwrap().contains(&span) {
-        return;
-    }
-    // This span has not yet been reported, so we insert it into the cache and report it.
-    let mut cache = cache.write().unwrap();
-    if cache.insert(span) {
-        // Some other thread may have added this span while we didn't hold the lock, so we only emit it if the insertions succeeded.
-        ecx.emit_diagnostic(diagnostic());
-    }
-}
-
 /// This function is used to split up a large memory access into aligned, non-overlapping chunks of a limited size.
 /// Returns an iterator over the chunks, yielding `(base address, size)` of each chunk, ordered by address.
 pub fn split_access(address: Size, size: Size) -> impl Iterator<Item = (u64, u64)> {

src/tools/miri/src/concurrency/genmc/mod.rs

@@ -14,18 +14,15 @@ use tracing::{debug, info};
 
 use self::global_allocations::{EvalContextExt as _, GlobalAllocationHandler};
 use self::helper::{
-    MAX_ACCESS_SIZE, Warnings, emit_warning, genmc_scalar_to_scalar,
-    maybe_upgrade_compare_exchange_success_orderings, scalar_to_genmc_scalar, to_genmc_rmw_op,
+    MAX_ACCESS_SIZE, genmc_scalar_to_scalar, maybe_upgrade_compare_exchange_success_orderings,
+    scalar_to_genmc_scalar, to_genmc_rmw_op,
 };
 use self::run::GenmcMode;
 use self::thread_id_map::ThreadIdMap;
 use crate::concurrency::genmc::helper::split_access;
+use crate::diagnostics::SpanDedupDiagnostic;
 use crate::intrinsics::AtomicRmwOp;
-use crate::{
-    AtomicFenceOrd, AtomicReadOrd, AtomicRwOrd, AtomicWriteOrd, MemoryKind, MiriConfig,
-    MiriMachine, MiriMemoryKind, NonHaltingDiagnostic, Scalar, TerminationInfo, ThreadId,
-    ThreadManager, VisitProvenance, VisitWith,
-};
+use crate::*;
 
 mod config;
 mod global_allocations;
@@ -104,18 +101,11 @@ struct GlobalState {
     /// Keep track of global allocations, to ensure they keep the same address across different executions, even if the order of allocations changes.
     /// The `AllocId` for globals is stable across executions, so we can use it as an identifier.
     global_allocations: GlobalAllocationHandler,
-
-    /// Cache for which warnings have already been shown to the user.
-    /// `None` if warnings are disabled.
-    warning_cache: Option<Warnings>,
 }
 
 impl GlobalState {
-    fn new(target_usize_max: u64, print_warnings: bool) -> Self {
-        Self {
-            global_allocations: GlobalAllocationHandler::new(target_usize_max),
-            warning_cache: print_warnings.then(Default::default),
-        }
+    fn new(target_usize_max: u64) -> Self {
+        Self { global_allocations: GlobalAllocationHandler::new(target_usize_max) }
     }
 }
 
@@ -412,27 +402,24 @@ impl GenmcCtx {
         let upgraded_success_ordering =
             maybe_upgrade_compare_exchange_success_orderings(success, fail);
 
-        if let Some(warning_cache) = &self.global_state.warning_cache {
-            // 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 {
-                emit_warning(ecx, &warning_cache.compare_exchange_failure_ordering, || {
-                    NonHaltingDiagnostic::GenmcCompareExchangeOrderingMismatch {
-                        success_ordering: success,
-                        upgraded_success_ordering,
-                        failure_ordering: fail,
-                        effective_failure_ordering,
-                    }
-                });
-            }
-            // FIXME(genmc): remove once GenMC implements spurious failures for `compare_exchange_weak`.
-            if can_fail_spuriously {
-                emit_warning(ecx, &warning_cache.compare_exchange_weak, || {
-                    NonHaltingDiagnostic::GenmcCompareExchangeWeak
-                });
-            }
+        // 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 {
+            static DEDUP: SpanDedupDiagnostic = SpanDedupDiagnostic::new();
+            ecx.dedup_diagnostic(&DEDUP, |_first| {
+                NonHaltingDiagnostic::GenmcCompareExchangeOrderingMismatch {
+                    success_ordering: success,
+                    upgraded_success_ordering,
+                    failure_ordering: fail,
+                    effective_failure_ordering,
+                }
+            });
+        }
+        // FIXME(genmc): remove once GenMC implements spurious failures for `compare_exchange_weak`.
+        if can_fail_spuriously {
+            static DEDUP: SpanDedupDiagnostic = SpanDedupDiagnostic::new();
+            ecx.dedup_diagnostic(&DEDUP, |_first| NonHaltingDiagnostic::GenmcCompareExchangeWeak);
         }
 
         debug!(

src/tools/miri/src/concurrency/genmc/run.rs

@@ -16,13 +16,6 @@ pub(super) enum GenmcMode {
     Verification,
 }
 
-impl GenmcMode {
-    /// Return whether warnings on unsupported features should be printed in this mode.
-    fn print_unsupported_warnings(self) -> bool {
-        self == GenmcMode::Verification
-    }
-}
-
 /// Do a complete run of the program in GenMC mode.
 /// This will call `eval_entry` multiple times, until either:
 /// - An error is detected (indicated by a `None` return value)
@@ -57,8 +50,7 @@ fn run_genmc_mode_impl<'tcx>(
     // There exists only one `global_state` per full run in GenMC mode.
     // It is shared by all `GenmcCtx` in this run.
     // FIXME(genmc): implement multithreading once GenMC supports it.
-    let global_state =
-        Arc::new(GlobalState::new(tcx.target_usize_max(), mode.print_unsupported_warnings()));
+    let global_state = Arc::new(GlobalState::new(tcx.target_usize_max()));
     let genmc_ctx = Rc::new(GenmcCtx::new(config, global_state, mode));
 
     // `rep` is used to report the progress, Miri will panic on wrap-around.

src/tools/miri/src/diagnostics.rs

@@ -1,9 +1,11 @@
 use std::fmt::{self, Write};
 use std::num::NonZero;
+use std::sync::Mutex;
 
 use rustc_abi::{Align, Size};
 use rustc_errors::{Diag, DiagMessage, Level};
-use rustc_span::{DUMMY_SP, SpanData, Symbol};
+use rustc_hash::FxHashSet;
+use rustc_span::{DUMMY_SP, Span, SpanData, Symbol};
 
 use crate::borrow_tracker::stacked_borrows::diagnostics::TagHistory;
 use crate::borrow_tracker::tree_borrows::diagnostics as tree_diagnostics;
@@ -835,4 +837,45 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             &this.machine,
         );
     }
+
+    /// Call `f` only if this is the first time we are seeing this span.
+    /// The `first` parameter indicates whether this is the first time *ever* that this diagnostic
+    /// is emitted.
+    fn dedup_diagnostic(
+        &self,
+        dedup: &SpanDedupDiagnostic,
+        f: impl FnOnce(/*first*/ bool) -> NonHaltingDiagnostic,
+    ) {
+        let this = self.eval_context_ref();
+        // We want to deduplicate both based on where the error seems to be located "from the user
+        // perspective", and the location of the actual operation (to avoid warning about the same
+        // operation called from different places in the local code).
+        let span1 = this.machine.current_user_relevant_span();
+        // For the "location of the operation", we still skip `track_caller` frames, to match the
+        // span that the diagnostic will point at.
+        let span2 = this
+            .active_thread_stack()
+            .iter()
+            .rev()
+            .find(|frame| !frame.instance().def.requires_caller_location(*this.tcx))
+            .map(|frame| frame.current_span())
+            .unwrap_or(span1);
+
+        let mut lock = dedup.0.lock().unwrap();
+        let first = lock.is_empty();
+        // Avoid mutating the hashset unless both spans are new.
+        if !lock.contains(&span2) && lock.insert(span1) && (span1 == span2 || lock.insert(span2)) {
+            // Both of the two spans were newly inserted.
+            this.emit_diagnostic(f(first));
+        }
+    }
+}
+
+/// Helps deduplicate a diagnostic to ensure it is only shown once per span.
+pub struct SpanDedupDiagnostic(Mutex<FxHashSet<Span>>);
+
+impl SpanDedupDiagnostic {
+    pub const fn new() -> Self {
+        Self(Mutex::new(FxHashSet::with_hasher(rustc_hash::FxBuildHasher)))
+    }
 }

src/tools/miri/src/helpers.rs

@@ -1,11 +1,13 @@
 use std::num::NonZero;
+use std::sync::Mutex;
 use std::time::Duration;
 use std::{cmp, iter};
 
 use rand::RngCore;
 use rustc_abi::{Align, ExternAbi, FieldIdx, FieldsShape, Size, Variants};
 use rustc_apfloat::Float;
 use rustc_apfloat::ieee::{Double, Half, Quad, Single};
+use rustc_hash::FxHashSet;
 use rustc_hir::Safety;
 use rustc_hir::def::{DefKind, Namespace};
 use rustc_hir::def_id::{CRATE_DEF_INDEX, CrateNum, DefId, LOCAL_CRATE};
@@ -650,7 +652,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         match reject_with {
             RejectOpWith::Abort => isolation_abort_error(op_name),
             RejectOpWith::WarningWithoutBacktrace => {
-                let mut emitted_warnings = this.machine.reject_in_isolation_warned.borrow_mut();
+                // Deduplicate these warnings *by shim* (not by span)
+                static DEDUP: Mutex<FxHashSet<String>> =
+                    Mutex::new(FxHashSet::with_hasher(rustc_hash::FxBuildHasher));
+                let mut emitted_warnings = DEDUP.lock().unwrap();
                 if !emitted_warnings.contains(op_name) {
                     // First time we are seeing this.
                     emitted_warnings.insert(op_name.to_owned());

src/tools/miri/src/lib.rs

@@ -60,6 +60,7 @@ extern crate rustc_ast;
 extern crate rustc_const_eval;
 extern crate rustc_data_structures;
 extern crate rustc_errors;
+extern crate rustc_hash;
 extern crate rustc_hir;
 extern crate rustc_index;
 extern crate rustc_middle;

src/tools/miri/src/machine.rs

@@ -649,16 +649,6 @@ pub struct MiriMachine<'tcx> {
     pub(crate) pthread_rwlock_sanity: Cell<bool>,
     pub(crate) pthread_condvar_sanity: Cell<bool>,
 
-    /// Remembers whether we already warned about an extern type with Stacked Borrows.
-    pub(crate) sb_extern_type_warned: Cell<bool>,
-    /// Remember whether we already warned about sharing memory with a native call.
-    #[allow(unused)]
-    pub(crate) native_call_mem_warned: Cell<bool>,
-    /// Remembers which shims have already shown the warning about erroring in isolation.
-    pub(crate) reject_in_isolation_warned: RefCell<FxHashSet<String>>,
-    /// Remembers which int2ptr casts we have already warned about.
-    pub(crate) int2ptr_warned: RefCell<FxHashSet<Span>>,
-
     /// Cache for `mangle_internal_symbol`.
     pub(crate) mangle_internal_symbol_cache: FxHashMap<&'static str, String>,
 
@@ -826,10 +816,6 @@ impl<'tcx> MiriMachine<'tcx> {
             pthread_mutex_sanity: Cell::new(false),
             pthread_rwlock_sanity: Cell::new(false),
             pthread_condvar_sanity: Cell::new(false),
-            sb_extern_type_warned: Cell::new(false),
-            native_call_mem_warned: Cell::new(false),
-            reject_in_isolation_warned: Default::default(),
-            int2ptr_warned: Default::default(),
             mangle_internal_symbol_cache: Default::default(),
             force_intrinsic_fallback: config.force_intrinsic_fallback,
             float_nondet: config.float_nondet,
@@ -1003,10 +989,6 @@ impl VisitProvenance for MiriMachine<'_> {
             pthread_mutex_sanity: _,
             pthread_rwlock_sanity: _,
             pthread_condvar_sanity: _,
-            sb_extern_type_warned: _,
-            native_call_mem_warned: _,
-            reject_in_isolation_warned: _,
-            int2ptr_warned: _,
             mangle_internal_symbol_cache: _,
             force_intrinsic_fallback: _,
             float_nondet: _,

src/tools/miri/src/shims/native_lib/mod.rs

@@ -1,6 +1,7 @@
 //! Implements calling functions from a native library.
 
 use std::ops::Deref;
+use std::sync::atomic::AtomicBool;
 
 use libffi::low::CodePtr;
 use libffi::middle::Type as FfiType;
@@ -279,8 +280,8 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         // Helper to print a warning when a pointer is shared with the native code.
         let expose = |prov: Provenance| -> InterpResult<'tcx> {
-            // The first time this happens, print a warning.
-            if !this.machine.native_call_mem_warned.replace(true) {
+            static DEDUP: AtomicBool = AtomicBool::new(false);
+            if !DEDUP.swap(true, std::sync::atomic::Ordering::Relaxed) {
                 // Newly set, so first time we get here.
                 this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing });
             }