Auto merge of rust-lang#3802 - RalfJung:no-more-call-id, r=RalfJung

Borrow tracking: remove the concept of a call ID

Turns out this is not needed any more ever since we started tracking the `protected_tags` list in the per-frame state.

Also thanks to `@JoJoDeveloping` for inspiring me to even consider this possibility. :)

Author: bors

src/tools/miri/README.md

@@ -414,10 +414,6 @@ to Miri failing to detect cases of undefined behavior in a program.
   being allocated or freed.  This helps in debugging memory leaks and
   use after free bugs. Specifying this argument multiple times does not overwrite the previous
   values, instead it appends its values to the list. Listing an id multiple times has no effect.
-* `-Zmiri-track-call-id=<id1>,<id2>,...` shows a backtrace when the given call ids are
-  assigned to a stack frame.  This helps in debugging UB related to Stacked
-  Borrows "protectors". Specifying this argument multiple times does not overwrite the previous
-  values, instead it appends its values to the list. Listing an id multiple times has no effect.
 * `-Zmiri-track-pointer-tag=<tag1>,<tag2>,...` shows a backtrace when a given pointer tag
   is created and when (if ever) it is popped from a borrow stack (which is where the tag becomes invalid
   and any future use of it will error).  This helps you in finding out why UB is

src/tools/miri/src/bin/miri.rs

@@ -581,17 +581,6 @@ fn main() {
                     show_error!("-Zmiri-track-pointer-tag requires nonzero arguments");
                 }
             }
-        } else if let Some(param) = arg.strip_prefix("-Zmiri-track-call-id=") {
-            let ids: Vec<u64> = parse_comma_list(param).unwrap_or_else(|err| {
-                show_error!("-Zmiri-track-call-id requires a comma separated list of valid `u64` arguments: {err}")
-            });
-            for id in ids.into_iter().map(miri::CallId::new) {
-                if let Some(id) = id {
-                    miri_config.tracked_call_ids.insert(id);
-                } else {
-                    show_error!("-Zmiri-track-call-id requires a nonzero argument");
-                }
-            }
         } else if let Some(param) = arg.strip_prefix("-Zmiri-track-alloc-id=") {
             let ids = parse_comma_list::<NonZero<u64>>(param).unwrap_or_else(|err| {
                 show_error!("-Zmiri-track-alloc-id requires a comma separated list of valid non-zero `u64` arguments: {err}")

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

@@ -12,8 +12,6 @@ use crate::*;
 pub mod stacked_borrows;
 pub mod tree_borrows;
 
-pub type CallId = NonZero<u64>;
-
 /// Tracking pointer provenance
 #[derive(Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
 pub struct BorTag(NonZero<u64>);
@@ -57,9 +55,6 @@ impl fmt::Debug for BorTag {
 /// Per-call-stack-frame data for borrow tracking
 #[derive(Debug)]
 pub struct FrameState {
-    /// The ID of the call this frame corresponds to.
-    call_id: CallId,
-
     /// If this frame is protecting any tags, they are listed here. We use this list to do
     /// incremental updates of the global list of protected tags stored in the
     /// `stacked_borrows::GlobalState` upon function return, and if we attempt to pop a protected
@@ -93,18 +88,13 @@ pub struct GlobalStateInner {
     /// The root tag is the one used for the initial pointer.
     /// We need this in a separate table to handle cyclic statics.
     root_ptr_tags: FxHashMap<AllocId, BorTag>,
-    /// Next unused call ID (for protectors).
-    next_call_id: CallId,
     /// All currently protected tags.
-    /// An item is protected if its tag is in this set, *and* it has the "protected" bit set.
     /// We add tags to this when they are created with a protector in `reborrow`, and
     /// we remove tags from this when the call which is protecting them returns, in
     /// `GlobalStateInner::end_call`. See `Stack::item_invalidated` for more details.
     protected_tags: FxHashMap<BorTag, ProtectorKind>,
     /// The pointer ids to trace
     tracked_pointer_tags: FxHashSet<BorTag>,
-    /// The call ids to trace
-    tracked_call_ids: FxHashSet<CallId>,
     /// Whether to recurse into datatypes when searching for pointers to retag.
     retag_fields: RetagFields,
     /// Whether `core::ptr::Unique` gets special (`Box`-like) handling.
@@ -168,18 +158,15 @@ impl GlobalStateInner {
     pub fn new(
         borrow_tracker_method: BorrowTrackerMethod,
         tracked_pointer_tags: FxHashSet<BorTag>,
-        tracked_call_ids: FxHashSet<CallId>,
         retag_fields: RetagFields,
         unique_is_unique: bool,
     ) -> Self {
         GlobalStateInner {
             borrow_tracker_method,
             next_ptr_tag: BorTag::one(),
             root_ptr_tags: FxHashMap::default(),
-            next_call_id: NonZero::new(1).unwrap(),
             protected_tags: FxHashMap::default(),
             tracked_pointer_tags,
-            tracked_call_ids,
             retag_fields,
             unique_is_unique,
         }
@@ -192,14 +179,8 @@ impl GlobalStateInner {
         id
     }
 
-    pub fn new_frame(&mut self, machine: &MiriMachine<'_>) -> FrameState {
-        let call_id = self.next_call_id;
-        trace!("new_frame: Assigning call ID {}", call_id);
-        if self.tracked_call_ids.contains(&call_id) {
-            machine.emit_diagnostic(NonHaltingDiagnostic::CreatedCallId(call_id));
-        }
-        self.next_call_id = NonZero::new(call_id.get() + 1).unwrap();
-        FrameState { call_id, protected_tags: SmallVec::new() }
+    pub fn new_frame(&mut self) -> FrameState {
+        FrameState { protected_tags: SmallVec::new() }
     }
 
     fn end_call(&mut self, frame: &machine::FrameExtra<'_>) {
@@ -252,7 +233,6 @@ impl BorrowTrackerMethod {
         RefCell::new(GlobalStateInner::new(
             self,
             config.tracked_pointer_tags.clone(),
-            config.tracked_call_ids.clone(),
             config.retag_fields,
             config.unique_is_unique,
         ))

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

@@ -429,30 +429,14 @@ impl<'history, 'ecx, 'tcx> DiagnosticCx<'history, 'ecx, 'tcx> {
             ProtectorKind::WeakProtector => "weakly protected",
             ProtectorKind::StrongProtector => "strongly protected",
         };
-        let item_tag = item.tag();
-        let call_id = self
-            .machine
-            .threads
-            .all_stacks()
-            .flat_map(|(_id, stack)| stack)
-            .map(|frame| {
-                frame.extra.borrow_tracker.as_ref().expect("we should have borrow tracking data")
-            })
-            .find(|frame| frame.protected_tags.iter().any(|(_, tag)| tag == &item_tag))
-            .map(|frame| frame.call_id)
-            .unwrap(); // FIXME: Surely we should find something, but a panic seems wrong here?
         match self.operation {
             Operation::Dealloc(_) =>
-                err_sb_ub(
-                    format!("deallocating while item {item:?} is {protected} by call {call_id:?}",),
-                    vec![],
-                    None,
-                ),
+                err_sb_ub(format!("deallocating while item {item:?} is {protected}",), vec![], None),
             Operation::Retag(RetagOp { orig_tag: tag, .. })
             | Operation::Access(AccessOp { tag, .. }) =>
                 err_sb_ub(
                     format!(
-                        "not granting access to tag {tag:?} because that would remove {item:?} which is {protected} because it is an argument of call {call_id:?}",
+                        "not granting access to tag {tag:?} because that would remove {item:?} which is {protected}",
                     ),
                     vec![],
                     tag.and_then(|tag| self.get_logs_relevant_to(tag, Some(item.tag()))),

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

@@ -7,9 +7,12 @@ use crate::borrow_tracker::BorTag;
 pub struct Item(u64);
 
 // An Item contains 3 bitfields:
-// * Bits 0-61 store a BorTag
-// * Bits 61-63 store a Permission
-// * Bit 64 stores a flag which indicates if we have a protector
+// * Bits 0-61 store a BorTag.
+// * Bits 61-63 store a Permission.
+// * Bit 64 stores a flag which indicates if we might have a protector.
+//   This is purely an optimization: if the bit is set, the tag *might* be
+//   in `protected_tags`, but if the bit is not set then the tag is definitely
+//   not in `protected_tags`.
 const TAG_MASK: u64 = u64::MAX >> 3;
 const PERM_MASK: u64 = 0x3 << 61;
 const PROTECTED_MASK: u64 = 0x1 << 63;

src/tools/miri/src/diagnostics.rs

@@ -116,7 +116,6 @@ pub enum NonHaltingDiagnostic {
     CreatedPointerTag(NonZero<u64>, Option<String>, Option<(AllocId, AllocRange, ProvenanceExtra)>),
     /// This `Item` was popped from the borrow stack. The string explains the reason.
     PoppedPointerTag(Item, String),
-    CreatedCallId(CallId),
     CreatedAlloc(AllocId, Size, Align, MemoryKind),
     FreedAlloc(AllocId),
     AccessedAlloc(AllocId, AccessKind),
@@ -607,7 +606,6 @@ impl<'tcx> MiriMachine<'tcx> {
                 ("reborrow of reference to `extern type`".to_string(), DiagLevel::Warning),
             CreatedPointerTag(..)
             | PoppedPointerTag(..)
-            | CreatedCallId(..)
             | CreatedAlloc(..)
             | AccessedAlloc(..)
             | FreedAlloc(..)
@@ -625,7 +623,6 @@ impl<'tcx> MiriMachine<'tcx> {
                     "created tag {tag:?} with {perm} at {alloc_id:?}{range:?} derived from {orig_tag:?}"
                 ),
             PoppedPointerTag(item, cause) => format!("popped tracked tag for item {item:?}{cause}"),
-            CreatedCallId(id) => format!("function call with id {id}"),
             CreatedAlloc(AllocId(id), size, align, kind) =>
                 format!(
                     "created {kind} allocation of {size} bytes (alignment {align} bytes) with id {id}",

src/tools/miri/src/eval.rs

@@ -118,8 +118,6 @@ pub struct MiriConfig {
     pub seed: Option<u64>,
     /// The stacked borrows pointer ids to report about
     pub tracked_pointer_tags: FxHashSet<BorTag>,
-    /// The stacked borrows call IDs to report about
-    pub tracked_call_ids: FxHashSet<CallId>,
     /// The allocation ids to report about.
     pub tracked_alloc_ids: FxHashSet<AllocId>,
     /// For the tracked alloc ids, also report read/write accesses.
@@ -183,7 +181,6 @@ impl Default for MiriConfig {
             args: vec![],
             seed: None,
             tracked_pointer_tags: FxHashSet::default(),
-            tracked_call_ids: FxHashSet::default(),
             tracked_alloc_ids: FxHashSet::default(),
             track_alloc_accesses: false,
             data_race_detector: true,

src/tools/miri/src/lib.rs

@@ -123,9 +123,7 @@ pub use crate::borrow_tracker::stacked_borrows::{
     EvalContextExt as _, Item, Permission, Stack, Stacks,
 };
 pub use crate::borrow_tracker::tree_borrows::{EvalContextExt as _, Tree};
-pub use crate::borrow_tracker::{
-    BorTag, BorrowTrackerMethod, CallId, EvalContextExt as _, RetagFields,
-};
+pub use crate::borrow_tracker::{BorTag, BorrowTrackerMethod, EvalContextExt as _, RetagFields};
 pub use crate::clock::{Clock, Instant};
 pub use crate::concurrency::{
     cpu_affinity::MAX_CPUS,

src/tools/miri/src/machine.rs

@@ -1368,7 +1368,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
         let borrow_tracker = ecx.machine.borrow_tracker.as_ref();
 
         let extra = FrameExtra {
-            borrow_tracker: borrow_tracker.map(|bt| bt.borrow_mut().new_frame(&ecx.machine)),
+            borrow_tracker: borrow_tracker.map(|bt| bt.borrow_mut().new_frame()),
             catch_unwind: None,
             timing,
             is_user_relevant: ecx.machine.is_user_relevant(&frame),

src/tools/miri/tests/fail/both_borrows/aliasing_mut1.stack.stderr

@@ -1,8 +1,8 @@
-error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
+error: Undefined Behavior: not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected
   --> $DIR/aliasing_mut1.rs:LL:CC
    |
 LL | pub fn safe(x: &mut i32, y: &mut i32) {
-   |                          ^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected because it is an argument of call ID
+   |                          ^ not granting access to tag <TAG> because that would remove [Unique for <TAG>] which is strongly protected
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information