Auto merge of #2024 - saethlin:better-local-check, r=RalfJung

Consider the cargo workspace when checking if a frame is local

`DefId::is_local` returns a result which is technically correct, but doesn't match the user's intuition when running integration tests or doctests. This incorporates the workspace crates mentioned in `cargo metadata` into the check for whether a frame is local to match user intuition.

For example, here is the backtrace you get from `MIRIFLAGS=-Zmiri-tag-raw-pointers cargo miri test` in `bytes` 1.1.0:
```
   --> /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:131:14
    |
131 |     unsafe { &mut *ptr::slice_from_raw_parts_mut(data, len) }
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for Unique at alloc67158, but parent tag <untagged> does not have an appropriate item in the borrow stack
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the 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

    = note: inside `std::slice::from_raw_parts_mut::<u8>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:131:14
    = note: inside `bytes::bytes::rebuild_boxed_slice` at /tmp/bytes-1.1.0/src/bytes.rs:938:19
    = note: inside closure at /tmp/bytes-1.1.0/src/bytes.rs:904:18
    = note: inside `<std::sync::atomic::AtomicPtr<()> as bytes::loom::sync::atomic::AtomicMut<()>>::with_mut::<[closure@bytes::bytes::promotable_even_drop::{closure#0}], ()>` at /tmp/bytes-1.1.0/src/loom.rs:17:17
    = note: inside `bytes::bytes::promotable_even_drop` at /tmp/bytes-1.1.0/src/bytes.rs:895:5
    = note: inside `<bytes::Bytes as std::ops::Drop>::drop` at /tmp/bytes-1.1.0/src/bytes.rs:515:18
    = note: inside `std::ptr::drop_in_place::<bytes::Bytes> - shim(Some(bytes::Bytes))` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:188:1
note: inside `copy_to_bytes_less` at tests/test_buf.rs:112:1
   --> tests/test_buf.rs:112:1
    |
112 | }
    | ^
note: inside closure at tests/test_buf.rs:106:1
   --> tests/test_buf.rs:106:1
    |
105 |   #[test]
    |   ------- in this procedural macro expansion
106 | / fn copy_to_bytes_less() {
107 | |     let mut buf = &b"hello world"[..];
108 | |
109 | |     let bytes = buf.copy_to_bytes(5);
110 | |     assert_eq!(bytes, &b"hello"[..]);
111 | |     assert_eq!(buf, &b" world"[..])
112 | | }
    | |_^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)
```
We get these because the integration tests are occurring in a crate called `test`, not the actual `bytes` crate. With this PR, we get this:
```
    = note: inside `std::slice::from_raw_parts_mut::<u8>` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/slice/raw.rs:131:14
note: inside `bytes::bytes::rebuild_boxed_slice` at /tmp/bytes-1.1.0/src/bytes.rs:938:19
   --> /tmp/bytes-1.1.0/src/bytes.rs:938:19
    |
938 |     Box::from_raw(slice::from_raw_parts_mut(buf, cap))
    |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside closure at /tmp/bytes-1.1.0/src/bytes.rs:904:18
   --> /tmp/bytes-1.1.0/src/bytes.rs:904:18
    |
904 |             drop(rebuild_boxed_slice(buf, ptr, len));
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `<std::sync::atomic::AtomicPtr<()> as bytes::loom::sync::atomic::AtomicMut<()>>::with_mut::<[closure@bytes::bytes::promotable_even_drop::{closure#0}], ()>` at /tmp/bytes-1.1.0/src/loom.rs:17:17
   --> /tmp/bytes-1.1.0/src/loom.rs:17:17
    |
17  |                 f(self.get_mut())
    |                 ^^^^^^^^^^^^^^^^^
note: inside `bytes::bytes::promotable_even_drop` at /tmp/bytes-1.1.0/src/bytes.rs:895:5
   --> /tmp/bytes-1.1.0/src/bytes.rs:895:5
    |
895 | /     data.with_mut(|shared| {
896 | |         let shared = *shared;
897 | |         let kind = shared as usize & KIND_MASK;
898 | |
...   |
905 | |         }
906 | |     });
    | |______^
note: inside `<bytes::Bytes as std::ops::Drop>::drop` at /tmp/bytes-1.1.0/src/bytes.rs:515:18
   --> /tmp/bytes-1.1.0/src/bytes.rs:515:18
    |
515 |         unsafe { (self.vtable.drop)(&mut self.data, self.ptr, self.len) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: inside `std::ptr::drop_in_place::<bytes::Bytes> - shim(Some(bytes::Bytes))` at /home/ben/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ptr/mod.rs:188:1
note: inside `copy_to_bytes_less` at tests/test_buf.rs:112:1
   --> tests/test_buf.rs:112:1
    |
112 | }
    | ^
note: inside closure at tests/test_buf.rs:106:1
   --> tests/test_buf.rs:106:1
    |
105 |   #[test]
    |   ------- in this procedural macro expansion
106 | / fn copy_to_bytes_less() {
107 | |     let mut buf = &b"hello world"[..];
108 | |
109 | |     let bytes = buf.copy_to_bytes(5);
110 | |     assert_eq!(bytes, &b"hello"[..]);
111 | |     assert_eq!(buf, &b" world"[..])
112 | | }
    | |_^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)
```

Note that this kind of inflation is rather rare to see. Most backtraces change not at all or only a tiny bit.

I originally implemented this to support another improvement to Miri diagnostics, but I think this is hairy enough to deserve its own PR, if somewhat poorly-motivated.

Author: bors

README.md

@@ -375,9 +375,12 @@ binaries, and as such worth documenting:
   directory after loading all the source files, but before commencing
   interpretation. This is useful if the interpreted program wants a different
   working directory at run-time than at build-time.
+* `MIRI_LOCAL_CRATES` is set by `cargo-miri` to tell the Miri driver which
+  crates should be given special treatment in diagnostics, in addition to the
+  crate currently being compiled.
 * `MIRI_VERBOSE` when set to any value tells the various `cargo-miri` phases to
   perform verbose logging.
-  
+
 [testing-miri]: CONTRIBUTING.md#testing-the-miri-driver
 
 ## Miri `extern` functions

cargo-miri/bin.rs

@@ -482,12 +482,13 @@ path = "lib.rs"
     }
 }
 
-/// Detect the target directory by calling `cargo metadata`.
-fn detect_target_dir() -> PathBuf {
-    #[derive(Deserialize)]
-    struct Metadata {
-        target_directory: PathBuf,
-    }
+#[derive(Deserialize)]
+struct Metadata {
+    target_directory: PathBuf,
+    workspace_members: Vec<String>,
+}
+
+fn get_cargo_metadata() -> Metadata {
     let mut cmd = cargo();
     // `-Zunstable-options` is required by `--config`.
     cmd.args(["metadata", "--no-deps", "--format-version=1", "-Zunstable-options"]);
@@ -514,9 +515,25 @@ fn detect_target_dir() -> PathBuf {
     if !status.success() {
         std::process::exit(status.code().unwrap_or(-1));
     }
-    metadata
-        .unwrap_or_else(|e| show_error(format!("invalid `cargo metadata` output: {}", e)))
-        .target_directory
+    metadata.unwrap_or_else(|e| show_error(format!("invalid `cargo metadata` output: {}", e)))
+}
+
+/// Pulls all the crates in this workspace from the cargo metadata.
+/// Workspace members are emitted like "miri 0.1.0 (path+file:///path/to/miri)"
+/// Additionally, somewhere between cargo metadata and TyCtxt, '-' gets replaced with '_' so we
+/// make that same transformation here.
+fn local_crates(metadata: &Metadata) -> String {
+    assert!(metadata.workspace_members.len() > 0);
+    let mut local_crates = String::new();
+    for member in &metadata.workspace_members {
+        let name = member.split(" ").nth(0).unwrap();
+        let name = name.replace("-", "_");
+        local_crates.push_str(&name);
+        local_crates.push(',');
+    }
+    local_crates.pop(); // Remove the trailing ','
+
+    local_crates
 }
 
 fn phase_cargo_miri(mut args: env::Args) {
@@ -595,8 +612,10 @@ fn phase_cargo_miri(mut args: env::Args) {
         }
     }
 
+    let metadata = get_cargo_metadata();
+
     // Detect the target directory if it's not specified via `--target-dir`.
-    let target_dir = target_dir.get_or_insert_with(detect_target_dir);
+    let target_dir = target_dir.get_or_insert_with(|| metadata.target_directory.clone());
 
     // Set `--target-dir` to `miri` inside the original target directory.
     target_dir.push("miri");
@@ -628,6 +647,8 @@ fn phase_cargo_miri(mut args: env::Args) {
     // Set rustdoc to us as well, so we can run doctests.
     cmd.env("RUSTDOC", &cargo_miri_path);
 
+    cmd.env("MIRI_LOCAL_CRATES", local_crates(&metadata));
+
     // Run cargo.
     if verbose {
         eprintln!("[cargo-miri miri] RUSTC_WRAPPER={:?}", cargo_miri_path);

src/diagnostics.rs

@@ -4,7 +4,7 @@ use std::num::NonZeroU64;
 
 use log::trace;
 
-use rustc_middle::ty::{self, TyCtxt};
+use rustc_middle::ty;
 use rustc_span::{source_map::DUMMY_SP, Span, SpanData, Symbol};
 
 use crate::stacked_borrows::{AccessKind, SbTag};
@@ -94,7 +94,7 @@ fn prune_stacktrace<'mir, 'tcx>(
             // Only prune frames if there is at least one local frame. This check ensures that if
             // we get a backtrace that never makes it to the user code because it has detected a
             // bug in the Rust runtime, we don't prune away every frame.
-            let has_local_frame = stacktrace.iter().any(|frame| frame.instance.def_id().is_local());
+            let has_local_frame = stacktrace.iter().any(|frame| ecx.machine.is_local(frame));
             if has_local_frame {
                 // This is part of the logic that `std` uses to select the relevant part of a
                 // backtrace. But here, we only look for __rust_begin_short_backtrace, not
@@ -115,7 +115,7 @@ fn prune_stacktrace<'mir, 'tcx>(
                 // This len check ensures that we don't somehow remove every frame, as doing so breaks
                 // the primary error message.
                 while stacktrace.len() > 1
-                    && stacktrace.last().map_or(false, |e| !e.instance.def_id().is_local())
+                    && stacktrace.last().map_or(false, |frame| !ecx.machine.is_local(frame))
                 {
                     stacktrace.pop();
                 }
@@ -218,7 +218,7 @@ pub fn report_error<'tcx, 'mir>(
     e.print_backtrace();
     msg.insert(0, e.to_string());
     report_msg(
-        *ecx.tcx,
+        ecx,
         DiagLevel::Error,
         &if let Some(title) = title { format!("{}: {}", title, msg[0]) } else { msg[0].clone() },
         msg,
@@ -264,19 +264,20 @@ pub fn report_error<'tcx, 'mir>(
 /// We want to present a multi-line span message for some errors. Diagnostics do not support this
 /// directly, so we pass the lines as a `Vec<String>` and display each line after the first with an
 /// additional `span_label` or `note` call.
-fn report_msg<'tcx>(
-    tcx: TyCtxt<'tcx>,
+fn report_msg<'mir, 'tcx>(
+    ecx: &InterpCx<'mir, 'tcx, Evaluator<'mir, 'tcx>>,
     diag_level: DiagLevel,
     title: &str,
     span_msg: Vec<String>,
     mut helps: Vec<(Option<SpanData>, String)>,
     stacktrace: &[FrameInfo<'tcx>],
 ) {
     let span = stacktrace.first().map_or(DUMMY_SP, |fi| fi.span);
+    let sess = ecx.tcx.sess;
     let mut err = match diag_level {
-        DiagLevel::Error => tcx.sess.struct_span_err(span, title).forget_guarantee(),
-        DiagLevel::Warning => tcx.sess.struct_span_warn(span, title),
-        DiagLevel::Note => tcx.sess.diagnostic().span_note_diag(span, title),
+        DiagLevel::Error => sess.struct_span_err(span, title).forget_guarantee(),
+        DiagLevel::Warning => sess.struct_span_warn(span, title),
+        DiagLevel::Note => sess.diagnostic().span_note_diag(span, title),
     };
 
     // Show main message.
@@ -306,7 +307,7 @@ fn report_msg<'tcx>(
     }
     // Add backtrace
     for (idx, frame_info) in stacktrace.iter().enumerate() {
-        let is_local = frame_info.instance.def_id().is_local();
+        let is_local = ecx.machine.is_local(frame_info);
         // No span for non-local frames and the first frame (which is the error site).
         if is_local && idx > 0 {
             err.span_note(frame_info.span, &frame_info.to_string());
@@ -426,7 +427,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     _ => ("tracking was triggered", DiagLevel::Note),
                 };
 
-                report_msg(*this.tcx, diag_level, title, vec![msg], vec![], &stacktrace);
+                report_msg(this, diag_level, title, vec![msg], vec![], &stacktrace);
             }
         });
     }

src/helpers.rs

@@ -12,7 +12,7 @@ use rustc_middle::ty::{
     layout::{LayoutOf, TyAndLayout},
     List, TyCtxt,
 };
-use rustc_span::Symbol;
+use rustc_span::{def_id::CrateNum, Symbol};
 use rustc_target::abi::{Align, FieldsShape, Size, Variants};
 use rustc_target::spec::abi::Abi;
 
@@ -775,3 +775,22 @@ pub fn isolation_abort_error(name: &str) -> InterpResult<'static> {
         name,
     )))
 }
+
+/// Retrieve the list of local crates that should have been passed by cargo-miri in
+/// MIRI_LOCAL_CRATES and turn them into `CrateNum`s.
+pub fn get_local_crates(tcx: &TyCtxt<'_>) -> Vec<CrateNum> {
+    // Convert the local crate names from the passed-in config into CrateNums so that they can
+    // be looked up quickly during execution
+    let local_crate_names = std::env::var("MIRI_LOCAL_CRATES")
+        .map(|crates| crates.split(",").map(|krate| krate.to_string()).collect::<Vec<_>>())
+        .unwrap_or_default();
+    let mut local_crates = Vec::new();
+    for &crate_num in tcx.crates(()) {
+        let name = tcx.crate_name(crate_num);
+        let name = name.as_str();
+        if local_crate_names.iter().any(|local_name| local_name == name) {
+            local_crates.push(crate_num);
+        }
+    }
+    local_crates
+}

src/machine.rs

@@ -19,7 +19,7 @@ use rustc_middle::{
         Instance, TyCtxt,
     },
 };
-use rustc_span::def_id::DefId;
+use rustc_span::def_id::{CrateNum, DefId};
 use rustc_span::symbol::{sym, Symbol};
 use rustc_target::abi::Size;
 use rustc_target::spec::abi::Abi;
@@ -349,10 +349,14 @@ pub struct Evaluator<'mir, 'tcx> {
 
     /// Equivalent setting as RUST_BACKTRACE on encountering an error.
     pub(crate) backtrace_style: BacktraceStyle,
+
+    /// Crates which are considered local for the purposes of error reporting.
+    pub(crate) local_crates: Vec<CrateNum>,
 }
 
 impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
     pub(crate) fn new(config: &MiriConfig, layout_cx: LayoutCx<'tcx, TyCtxt<'tcx>>) -> Self {
+        let local_crates = helpers::get_local_crates(&layout_cx.tcx);
         let layouts =
             PrimitiveLayouts::new(layout_cx).expect("Couldn't get layouts of primitive types");
         let profiler = config.measureme_out.as_ref().map(|out| {
@@ -381,12 +385,19 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
             exported_symbols_cache: FxHashMap::default(),
             panic_on_unsupported: config.panic_on_unsupported,
             backtrace_style: config.backtrace_style,
+            local_crates,
         }
     }
 
     pub(crate) fn communicate(&self) -> bool {
         self.isolated_op == IsolatedOp::Allow
     }
+
+    /// Check whether the stack frame that this `FrameInfo` refers to is part of a local crate.
+    pub(crate) fn is_local(&self, frame: &FrameInfo<'_>) -> bool {
+        let def_id = frame.instance.def_id();
+        def_id.is_local() || self.local_crates.contains(&def_id.krate)
+    }
 }
 
 /// A rustc InterpCx for Miri.