Merge pull request #4689 from RalfJung/retag-the-fields

remove -Zmiri-retag-fields

Author: RalfJung

README.md

@@ -464,11 +464,6 @@ to Miri failing to detect cases of undefined behavior in a program.
   errors and warnings.
 * `-Zmiri-recursive-validation` is a *highly experimental* flag that makes validity checking
   recurse below references.
-* `-Zmiri-retag-fields[=<all|none|scalar>]` controls when Stacked Borrows retagging recurses into
-  fields. `all` means it always recurses (the default, and equivalent to `-Zmiri-retag-fields`
-  without an explicit value), `none` means it never recurses, `scalar` means it only recurses for
-  types where we would also emit `noalias` annotations in the generated LLVM IR (types passed as
-  individual scalars or pairs of scalars). Setting this to `none` is **unsound**.
 * `-Zmiri-preemption-rate` configures the probability that at the end of a basic block, the active
   thread will be preempted. The default is `0.01` (i.e., 1%). Setting this to `0` disables
   preemption. Note that even without preemption, the schedule is still non-deterministic:

src/bin/miri.rs

@@ -31,7 +31,7 @@ use std::sync::atomic::{AtomicI32, AtomicU32, Ordering};
 
 use miri::{
     BacktraceStyle, BorrowTrackerMethod, GenmcConfig, GenmcCtx, MiriConfig, MiriEntryFnType,
-    ProvenanceMode, RetagFields, TreeBorrowsParams, ValidationMode, run_genmc_mode,
+    ProvenanceMode, TreeBorrowsParams, ValidationMode, run_genmc_mode,
 };
 use rustc_abi::ExternAbi;
 use rustc_data_structures::sync;
@@ -571,21 +571,17 @@ fn main() {
         } else if arg == "-Zmiri-mute-stdout-stderr" {
             miri_config.mute_stdout_stderr = true;
         } else if arg == "-Zmiri-retag-fields" {
-            miri_config.retag_fields = RetagFields::Yes;
+            eprintln!(
+                "warning: `-Zmiri-retag-fields` is a NOP and will be removed in a future version of Miri.\n\
+                Field retagging has been on-by-default for a long time."
+            );
         } else if arg == "-Zmiri-fixed-schedule" {
             miri_config.fixed_scheduling = true;
         } else if arg == "-Zmiri-deterministic-concurrency" {
             miri_config.fixed_scheduling = true;
             miri_config.address_reuse_cross_thread_rate = 0.0;
             miri_config.cmpxchg_weak_failure_rate = 0.0;
             miri_config.weak_memory_emulation = false;
-        } else if let Some(retag_fields) = arg.strip_prefix("-Zmiri-retag-fields=") {
-            miri_config.retag_fields = match retag_fields {
-                "all" => RetagFields::Yes,
-                "none" => RetagFields::No,
-                "scalar" => RetagFields::OnlyScalar,
-                _ => fatal_error!("`-Zmiri-retag-fields` can only be `all`, `none`, or `scalar`"),
-            };
         } else if let Some(param) = arg.strip_prefix("-Zmiri-seed=") {
             let seed = param.parse::<u64>().unwrap_or_else(|_| {
                 fatal_error!("-Zmiri-seed must be an integer that fits into u64")

src/borrow_tracker/mod.rs

@@ -116,8 +116,6 @@ pub struct GlobalStateInner {
     protected_tags: FxHashMap<BorTag, ProtectorKind>,
     /// The pointer ids to trace
     tracked_pointer_tags: FxHashSet<BorTag>,
-    /// Whether to recurse into datatypes when searching for pointers to retag.
-    retag_fields: RetagFields,
 }
 
 impl VisitProvenance for GlobalStateInner {
@@ -131,18 +129,6 @@ impl VisitProvenance for GlobalStateInner {
 /// We need interior mutable access to the global state.
 pub type GlobalState = RefCell<GlobalStateInner>;
 
-/// Policy on whether to recurse into fields to retag
-#[derive(Copy, Clone, Debug)]
-pub enum RetagFields {
-    /// Don't retag any fields.
-    No,
-    /// Retag all fields.
-    Yes,
-    /// Only retag fields of types with Scalar and ScalarPair layout,
-    /// to match the LLVM `noalias` we generate.
-    OnlyScalar,
-}
-
 /// The flavor of the protector.
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 pub enum ProtectorKind {
@@ -168,15 +154,13 @@ impl GlobalStateInner {
     pub fn new(
         borrow_tracker_method: BorrowTrackerMethod,
         tracked_pointer_tags: FxHashSet<BorTag>,
-        retag_fields: RetagFields,
     ) -> Self {
         GlobalStateInner {
             borrow_tracker_method,
             next_ptr_tag: BorTag::one(),
             root_ptr_tags: FxHashMap::default(),
             protected_tags: FxHashMap::default(),
             tracked_pointer_tags,
-            retag_fields,
         }
     }
 
@@ -244,11 +228,7 @@ pub struct TreeBorrowsParams {
 
 impl BorrowTrackerMethod {
     pub fn instantiate_global_state(self, config: &MiriConfig) -> GlobalState {
-        RefCell::new(GlobalStateInner::new(
-            self,
-            config.tracked_pointer_tags.clone(),
-            config.retag_fields,
-        ))
+        RefCell::new(GlobalStateInner::new(self, config.tracked_pointer_tags.clone()))
     }
 
     pub fn get_tree_borrows_params(self) -> TreeBorrowsParams {

src/borrow_tracker/stacked_borrows/mod.rs

@@ -9,7 +9,7 @@ use std::fmt::Write;
 use std::sync::atomic::AtomicBool;
 use std::{cmp, mem};
 
-use rustc_abi::{BackendRepr, Size};
+use rustc_abi::Size;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_middle::mir::{Mutability, RetagKind};
 use rustc_middle::ty::layout::HasTypingEnv;
@@ -887,22 +887,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         place: &PlaceTy<'tcx>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        let retag_fields = this.machine.borrow_tracker.as_mut().unwrap().get_mut().retag_fields;
         let retag_cause = match kind {
             RetagKind::TwoPhase => unreachable!(), // can only happen in `retag_ptr_value`
             RetagKind::FnEntry => RetagCause::FnEntry,
             RetagKind::Default | RetagKind::Raw => RetagCause::Normal,
         };
-        let mut visitor =
-            RetagVisitor { ecx: this, kind, retag_cause, retag_fields, in_field: false };
+        let mut visitor = RetagVisitor { ecx: this, kind, retag_cause, in_field: false };
         return visitor.visit_value(place);
 
         // The actual visitor.
         struct RetagVisitor<'ecx, 'tcx> {
             ecx: &'ecx mut MiriInterpCx<'tcx>,
             kind: RetagKind,
             retag_cause: RetagCause,
-            retag_fields: RetagFields,
             in_field: bool,
         }
         impl<'ecx, 'tcx> RetagVisitor<'ecx, 'tcx> {
@@ -967,24 +964,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                         self.walk_value(place)?;
                     }
                     _ => {
-                        // Not a reference/pointer/box. Only recurse if configured appropriately.
-                        let recurse = match self.retag_fields {
-                            RetagFields::No => false,
-                            RetagFields::Yes => true,
-                            RetagFields::OnlyScalar => {
-                                // Matching `ArgAbi::new` at the time of writing, only fields of
-                                // `Scalar` and `ScalarPair` ABI are considered.
-                                matches!(
-                                    place.layout.backend_repr,
-                                    BackendRepr::Scalar(..) | BackendRepr::ScalarPair(..)
-                                )
-                            }
-                        };
-                        if recurse {
-                            let in_field = mem::replace(&mut self.in_field, true); // remember and restore old value
-                            self.walk_value(place)?;
-                            self.in_field = in_field;
-                        }
+                        // Not a reference/pointer/box. Recurse.
+                        let in_field = mem::replace(&mut self.in_field, true); // remember and restore old value
+                        self.walk_value(place)?;
+                        self.in_field = in_field;
                     }
                 }
 

src/borrow_tracker/tree_borrows/mod.rs

@@ -1,4 +1,4 @@
-use rustc_abi::{BackendRepr, Size};
+use rustc_abi::Size;
 use rustc_middle::mir::{Mutability, RetagKind};
 use rustc_middle::ty::layout::HasTypingEnv;
 use rustc_middle::ty::{self, Ty};
@@ -468,16 +468,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         place: &PlaceTy<'tcx>,
     ) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
-        let options = this.machine.borrow_tracker.as_mut().unwrap().get_mut();
-        let retag_fields = options.retag_fields;
-        let mut visitor = RetagVisitor { ecx: this, kind, retag_fields };
+        let mut visitor = RetagVisitor { ecx: this, kind };
         return visitor.visit_value(place);
 
         // The actual visitor.
         struct RetagVisitor<'ecx, 'tcx> {
             ecx: &'ecx mut MiriInterpCx<'tcx>,
             kind: RetagKind,
-            retag_fields: RetagFields,
         }
         impl<'ecx, 'tcx> RetagVisitor<'ecx, 'tcx> {
             #[inline(always)] // yes this helps in our benchmarks
@@ -545,22 +542,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                         self.walk_value(place)?;
                     }
                     _ => {
-                        // Not a reference/pointer/box. Only recurse if configured appropriately.
-                        let recurse = match self.retag_fields {
-                            RetagFields::No => false,
-                            RetagFields::Yes => true,
-                            RetagFields::OnlyScalar => {
-                                // Matching `ArgAbi::new` at the time of writing, only fields of
-                                // `Scalar` and `ScalarPair` ABI are considered.
-                                matches!(
-                                    place.layout.backend_repr,
-                                    BackendRepr::Scalar(..) | BackendRepr::ScalarPair(..)
-                                )
-                            }
-                        };
-                        if recurse {
-                            self.walk_value(place)?;
-                        }
+                        // Not a reference/pointer/box. Recurse.
+                        self.walk_value(place)?;
                     }
                 }
                 interp_ok(())

src/eval.rs

@@ -88,8 +88,6 @@ pub struct MiriConfig {
     pub preemption_rate: f64,
     /// Report the current instruction being executed every N basic blocks.
     pub report_progress: Option<u32>,
-    /// Whether Stacked Borrows and Tree Borrows retagging should recurse into fields of datatypes.
-    pub retag_fields: RetagFields,
     /// The location of the shared object files to load when calling external functions
     pub native_lib: Vec<PathBuf>,
     /// Whether to enable the new native lib tracing system.
@@ -147,7 +145,6 @@ impl Default for MiriConfig {
             mute_stdout_stderr: false,
             preemption_rate: 0.01, // 1%
             report_progress: None,
-            retag_fields: RetagFields::Yes,
             native_lib: vec![],
             native_lib_enable_tracing: false,
             gc_interval: 10_000,

src/lib.rs

@@ -119,7 +119,7 @@ pub use crate::borrow_tracker::stacked_borrows::{
 };
 pub use crate::borrow_tracker::tree_borrows::{EvalContextExt as _, Tree};
 pub use crate::borrow_tracker::{
-    BorTag, BorrowTrackerMethod, EvalContextExt as _, RetagFields, TreeBorrowsParams,
+    BorTag, BorrowTrackerMethod, EvalContextExt as _, TreeBorrowsParams,
 };
 pub use crate::clock::{Instant, MonotonicClock};
 pub use crate::concurrency::cpu_affinity::MAX_CPUS;