From 1ee055f0ecbadd9e6c51182a7179d8f8acfd4720 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 12 Oct 2023 08:25:22 +0200 Subject: [PATCH 1/5] add some comments and some cleanup around Miri intptrcast --- .../rustc_const_eval/src/interpret/memory.rs | 10 +++++ src/tools/miri/src/intptrcast.rs | 37 +++++++++---------- src/tools/miri/src/machine.rs | 18 ++++++--- 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/compiler/rustc_const_eval/src/interpret/memory.rs b/compiler/rustc_const_eval/src/interpret/memory.rs index ce666e6af3bdf..a646f09dee078 100644 --- a/compiler/rustc_const_eval/src/interpret/memory.rs +++ b/compiler/rustc_const_eval/src/interpret/memory.rs @@ -1249,6 +1249,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// Turning a "maybe pointer" into a proper pointer (and some information /// about where it points), or an absolute address. + /// + /// The result must be used immediately; it is not allowed to convert + /// the returned data back into a `Pointer` and store that in machine state. + /// (In fact that's not even possible since `M::ProvenanceExtra` is generic and + /// we don't have an operation to turn it back into `M::Provenance`.) pub fn ptr_try_get_alloc_id( &self, ptr: Pointer>, @@ -1267,6 +1272,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { } /// Turning a "maybe pointer" into a proper pointer (and some information about where it points). + /// + /// The result must be used immediately; it is not allowed to convert + /// the returned data back into a `Pointer` and store that in machine state. + /// (In fact that's not even possible since `M::ProvenanceExtra` is generic and + /// we don't have an operation to turn it back into `M::Provenance`.) #[inline(always)] pub fn ptr_get_alloc_id( &self, diff --git a/src/tools/miri/src/intptrcast.rs b/src/tools/miri/src/intptrcast.rs index 4fd0af35304ec..154d86375ca2a 100644 --- a/src/tools/miri/src/intptrcast.rs +++ b/src/tools/miri/src/intptrcast.rs @@ -119,24 +119,14 @@ impl<'mir, 'tcx> GlobalStateInner { Ok(()) } - pub fn ptr_from_addr_transmute( - _ecx: &MiriInterpCx<'mir, 'tcx>, - addr: u64, - ) -> Pointer> { - trace!("Transmuting {:#x} to a pointer", addr); - - // We consider transmuted pointers to be "invalid" (`None` provenance). - Pointer::new(None, Size::from_bytes(addr)) - } - pub fn ptr_from_addr_cast( ecx: &MiriInterpCx<'mir, 'tcx>, addr: u64, ) -> InterpResult<'tcx, Pointer>> { trace!("Casting {:#x} to a pointer", addr); + // Potentially emit a warning. let global_state = ecx.machine.intptrcast.borrow(); - match global_state.provenance_mode { ProvenanceMode::Default => { // The first time this happens at a particular location, print a warning. @@ -158,7 +148,12 @@ impl<'mir, 'tcx> GlobalStateInner { ProvenanceMode::Permissive => {} } - // This is how wildcard pointers are born. + // We do *not* look up the `AllocId` here! This is a `ptr as usize` cast, and it is + // completely legal to do a cast and then `wrapping_offset` to another allocation and only + // *then* do a memory access. So the allocation that the pointer happens to point to on a + // cast is fairly irrelevant. Instead we generate this as a "wildcard" pointer, such that + // *every time the pointer is used*, we do an `AllocId` lookup to find the (exposed) + // allocation it might be referencing. Ok(Pointer::new(Some(Provenance::Wildcard), Size::from_bytes(addr))) } @@ -219,22 +214,27 @@ impl<'mir, 'tcx> GlobalStateInner { }) } - /// Convert a relative (tcx) pointer to an absolute address. - pub fn rel_ptr_to_addr( + /// Convert a relative (tcx) pointer to a Miri pointer. + pub fn ptr_from_rel_ptr( ecx: &MiriInterpCx<'mir, 'tcx>, ptr: Pointer, - ) -> InterpResult<'tcx, u64> { + tag: BorTag, + ) -> InterpResult<'tcx, Pointer> { let (alloc_id, offset) = ptr.into_parts(); // offset is relative (AllocId provenance) let base_addr = GlobalStateInner::alloc_base_addr(ecx, alloc_id)?; // Add offset with the right kind of pointer-overflowing arithmetic. let dl = ecx.data_layout(); - Ok(dl.overflowing_offset(base_addr, offset.bytes()).0) + let absolute_addr = dl.overflowing_offset(base_addr, offset.bytes()).0; + Ok(Pointer::new( + Provenance::Concrete { alloc_id, tag }, + Size::from_bytes(absolute_addr), + )) } /// When a pointer is used for a memory access, this computes where in which allocation the /// access is going. - pub fn abs_ptr_to_rel( + pub fn ptr_get_alloc( ecx: &MiriInterpCx<'mir, 'tcx>, ptr: Pointer, ) -> Option<(AllocId, Size)> { @@ -252,12 +252,11 @@ impl<'mir, 'tcx> GlobalStateInner { let base_addr = GlobalStateInner::alloc_base_addr(ecx, alloc_id).unwrap(); // Wrapping "addr - base_addr" - let dl = ecx.data_layout(); #[allow(clippy::cast_possible_wrap)] // we want to wrap here let neg_base_addr = (base_addr as i64).wrapping_neg(); Some(( alloc_id, - Size::from_bytes(dl.overflowing_signed_offset(addr.bytes(), neg_base_addr).0), + Size::from_bytes(ecx.overflowing_signed_offset(addr.bytes(), neg_base_addr).0), )) } diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 3de27460860c9..96c734c550084 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -1136,19 +1136,16 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { _ => {} } } - let absolute_addr = intptrcast::GlobalStateInner::rel_ptr_to_addr(ecx, ptr)?; let tag = if let Some(borrow_tracker) = &ecx.machine.borrow_tracker { borrow_tracker.borrow_mut().base_ptr_tag(ptr.provenance, &ecx.machine) } else { // Value does not matter, SB is disabled BorTag::default() }; - Ok(Pointer::new( - Provenance::Concrete { alloc_id: ptr.provenance, tag }, - Size::from_bytes(absolute_addr), - )) + intptrcast::GlobalStateInner::ptr_from_rel_ptr(ecx, ptr, tag) } + /// Called on `usize as ptr` casts. #[inline(always)] fn ptr_from_addr_cast( ecx: &MiriInterpCx<'mir, 'tcx>, @@ -1157,6 +1154,9 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { intptrcast::GlobalStateInner::ptr_from_addr_cast(ecx, addr) } + /// Called on `ptr as usize` casts. + /// (Actually computing the resulting `usize` doesn't need machine help, + /// that's just `Scalar::try_to_int`.) fn expose_ptr( ecx: &mut InterpCx<'mir, 'tcx, Self>, ptr: Pointer, @@ -1174,11 +1174,17 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> { /// Convert a pointer with provenance into an allocation-offset pair, /// or a `None` with an absolute address if that conversion is not possible. + /// + /// This is called when a pointer is about to be used for memory access, + /// an in-bounds check, or anything else that requires knowing which allocation it points to. + /// The resulting `AllocId` will just be used for that one step and the forgotten again + /// (i.e., we'll never turn the data returned here back into a `Pointer` that might be + /// stored in machine state). fn ptr_get_alloc( ecx: &MiriInterpCx<'mir, 'tcx>, ptr: Pointer, ) -> Option<(AllocId, Size, Self::ProvenanceExtra)> { - let rel = intptrcast::GlobalStateInner::abs_ptr_to_rel(ecx, ptr); + let rel = intptrcast::GlobalStateInner::ptr_get_alloc(ecx, ptr); rel.map(|(alloc_id, size)| { let tag = match ptr.provenance { From e92c93466535d52aa569f6584606bb61e305acc6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 13 Oct 2023 07:41:10 +0200 Subject: [PATCH 2/5] doc comment for Provenance::Wildcard --- src/tools/miri/src/machine.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index 96c734c550084..f956015c80873 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -169,11 +169,29 @@ impl fmt::Display for MiriMemoryKind { /// Pointer provenance. #[derive(Clone, Copy)] pub enum Provenance { + /// For pointers with concrete provenance. we exactly know which allocation they are attached to + /// and what their borrow tag is. Concrete { alloc_id: AllocId, /// Borrow Tracker tag. tag: BorTag, }, + /// Pointers with wildcard provenance are created on int-to-ptr casts. According to the + /// specification, we should at that point angelically "guess" a provenance that will make all + /// future uses of this pointer work, if at all possible. Of course such a semantics cannot be + /// actually implemented in Miri. So instead, we approximate this, erroring on the side of + /// accepting too much code rather than rejecting correct code: a pointer with wildcard + /// provenance "acts like" any previously exposed pointer. Each time it is used, we check + /// whether *some* exposed pointer could have done what we want to do, and if the answer is yes + /// then we allow the access. This allows too much code in two ways: + /// - The same wildcard pointer can "take the role" of multiple different exposed pointers on + /// subsequenct memory accesses. + /// - In the aliasing model, we don't just have to know the borrow tag of the pointer used for + /// the access, we also have to update the aliasing state -- and that update can be very + /// different depending on which borrow tag we pick! Stacked Borrows has support for this by + /// switching to a stack that is only approximately known, i.e. we overapproximate the effect + /// of using *any* exposed pointer for this access, and only keep information about the borrow + /// stack that would be true with all possible choices. Wildcard, } From c1c5a1d69ace8bdfd34f449bdb00797f1fc723cc Mon Sep 17 00:00:00 2001 From: Camille GILLOT Date: Wed, 18 Oct 2023 16:34:04 +0000 Subject: [PATCH 3/5] Only check in a single place if a pass is enabled. --- compiler/rustc_mir_transform/src/lib.rs | 2 +- .../rustc_mir_transform/src/pass_manager.rs | 33 ++++++++++++------- tests/mir-opt/inline/unit_test.rs | 19 +++++++++++ 3 files changed, 41 insertions(+), 13 deletions(-) create mode 100644 tests/mir-opt/inline/unit_test.rs diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index 0113697473002..d579420ecb835 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -383,7 +383,7 @@ fn mir_drops_elaborated_and_const_checked(tcx: TyCtxt<'_>, def: LocalDefId) -> & let is_fn_like = tcx.def_kind(def).is_fn_like(); if is_fn_like { // Do not compute the mir call graph without said call graph actually being used. - if inline::Inline.is_enabled(&tcx.sess) { + if pm::should_run_pass(tcx, &inline::Inline) { tcx.ensure_with_value().mir_inliner_callees(ty::InstanceDef::Item(def.to_def_id())); } } diff --git a/compiler/rustc_mir_transform/src/pass_manager.rs b/compiler/rustc_mir_transform/src/pass_manager.rs index 5abb2f3d0412a..a8aba29adcd13 100644 --- a/compiler/rustc_mir_transform/src/pass_manager.rs +++ b/compiler/rustc_mir_transform/src/pass_manager.rs @@ -83,6 +83,25 @@ pub fn run_passes<'tcx>( run_passes_inner(tcx, body, passes, phase_change, true); } +pub fn should_run_pass<'tcx, P>(tcx: TyCtxt<'tcx>, pass: &P) -> bool +where + P: MirPass<'tcx> + ?Sized, +{ + let name = pass.name(); + + let overridden_passes = &tcx.sess.opts.unstable_opts.mir_enable_passes; + let overridden = + overridden_passes.iter().rev().find(|(s, _)| s == &*name).map(|(_name, polarity)| { + trace!( + pass = %name, + "{} as requested by flag", + if *polarity { "Running" } else { "Not running" }, + ); + *polarity + }); + overridden.unwrap_or_else(|| pass.is_enabled(&tcx.sess)) +} + fn run_passes_inner<'tcx>( tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>, @@ -100,19 +119,9 @@ fn run_passes_inner<'tcx>( for pass in passes { let name = pass.name(); - let overridden = overridden_passes.iter().rev().find(|(s, _)| s == &*name).map( - |(_name, polarity)| { - trace!( - pass = %name, - "{} as requested by flag", - if *polarity { "Running" } else { "Not running" }, - ); - *polarity - }, - ); - if !overridden.unwrap_or_else(|| pass.is_enabled(&tcx.sess)) { + if !should_run_pass(tcx, *pass) { continue; - } + }; let dump_enabled = pass.is_mir_dump_enabled(); diff --git a/tests/mir-opt/inline/unit_test.rs b/tests/mir-opt/inline/unit_test.rs new file mode 100644 index 0000000000000..0d877bb10b425 --- /dev/null +++ b/tests/mir-opt/inline/unit_test.rs @@ -0,0 +1,19 @@ +// Check that `-Zmir-enable-passes=+Inline` does not ICE because of stolen MIR. +// unit-test: Inline +// skip-filecheck +#![crate_type = "lib"] + +// Randomize `def_path_hash` by defining them under a module with different names +macro_rules! emit { + ($($m:ident)*) => {$( + pub mod $m { + pub fn main() { + let func = || 123u8; + func(); + } + } + )*}; +} + +// Increase the chance of triggering the bug +emit!(m00 m01 m02 m03 m04 m05 m06 m07 m08 m09 m10 m11 m12 m13 m14 m15 m16 m17 m18 m19); From 60c95448c378fd25ecd358454b9c5f763c294c19 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 18 Oct 2023 21:55:15 +0000 Subject: [PATCH 4/5] Use v0.0.0 in compiler crates --- Cargo.lock | 10 +++++----- compiler/rustc_fluent_macro/Cargo.toml | 2 +- compiler/rustc_hir_typeck/Cargo.toml | 2 +- compiler/rustc_lexer/Cargo.toml | 2 +- compiler/rustc_macros/Cargo.toml | 2 +- compiler/rustc_transmute/Cargo.toml | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c8e7665337fb5..aa2f9c4147e3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3839,7 +3839,7 @@ dependencies = [ [[package]] name = "rustc_fluent_macro" -version = "0.1.0" +version = "0.0.0" dependencies = [ "annotate-snippets", "fluent-bundle", @@ -3915,7 +3915,7 @@ dependencies = [ [[package]] name = "rustc_hir_typeck" -version = "0.1.0" +version = "0.0.0" dependencies = [ "rustc_ast", "rustc_attr", @@ -4043,7 +4043,7 @@ dependencies = [ [[package]] name = "rustc_lexer" -version = "0.1.0" +version = "0.0.0" dependencies = [ "expect-test", "unicode-properties", @@ -4112,7 +4112,7 @@ dependencies = [ [[package]] name = "rustc_macros" -version = "0.1.0" +version = "0.0.0" dependencies = [ "proc-macro2", "quote", @@ -4595,7 +4595,7 @@ dependencies = [ [[package]] name = "rustc_transmute" -version = "0.1.0" +version = "0.0.0" dependencies = [ "itertools", "rustc_data_structures", diff --git a/compiler/rustc_fluent_macro/Cargo.toml b/compiler/rustc_fluent_macro/Cargo.toml index f5a6585b5c68a..60b8e1e3786dc 100644 --- a/compiler/rustc_fluent_macro/Cargo.toml +++ b/compiler/rustc_fluent_macro/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustc_fluent_macro" -version = "0.1.0" +version = "0.0.0" edition = "2021" [lib] diff --git a/compiler/rustc_hir_typeck/Cargo.toml b/compiler/rustc_hir_typeck/Cargo.toml index ce91d023a0ae8..0666eeee4d38c 100644 --- a/compiler/rustc_hir_typeck/Cargo.toml +++ b/compiler/rustc_hir_typeck/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustc_hir_typeck" -version = "0.1.0" +version = "0.0.0" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/compiler/rustc_lexer/Cargo.toml b/compiler/rustc_lexer/Cargo.toml index 2211ac1c8a753..373a8970de83c 100644 --- a/compiler/rustc_lexer/Cargo.toml +++ b/compiler/rustc_lexer/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustc_lexer" -version = "0.1.0" +version = "0.0.0" license = "MIT OR Apache-2.0" edition = "2021" diff --git a/compiler/rustc_macros/Cargo.toml b/compiler/rustc_macros/Cargo.toml index 17651ce959879..6e7e19a24024f 100644 --- a/compiler/rustc_macros/Cargo.toml +++ b/compiler/rustc_macros/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustc_macros" -version = "0.1.0" +version = "0.0.0" edition = "2021" [lib] diff --git a/compiler/rustc_transmute/Cargo.toml b/compiler/rustc_transmute/Cargo.toml index aa6fe7d241968..c2b2730c328fd 100644 --- a/compiler/rustc_transmute/Cargo.toml +++ b/compiler/rustc_transmute/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "rustc_transmute" -version = "0.1.0" +version = "0.0.0" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html From 2d144d21150d98424f7dff8af028f58a9c9b6c82 Mon Sep 17 00:00:00 2001 From: Vitaliy Busko Date: Thu, 19 Oct 2023 08:52:46 +0700 Subject: [PATCH 5/5] fix(bootstrap) info message show correct path now --- src/bootstrap/src/core/build_steps/setup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/src/core/build_steps/setup.rs b/src/bootstrap/src/core/build_steps/setup.rs index ebe093674fc7c..435ebb6df90d0 100644 --- a/src/bootstrap/src/core/build_steps/setup.rs +++ b/src/bootstrap/src/core/build_steps/setup.rs @@ -183,7 +183,7 @@ pub fn setup(config: &Config, profile: Profile) { eprintln!(); eprintln!( "note: the `tools` profile sets up the `stage2` toolchain (use \ - `rustup toolchain link 'name' host/build/stage2` to use rustc)" + `rustup toolchain link 'name' build/host/stage2` to use rustc)" ) }