From 73dc95dad19e2fa513ea8ac2b76231474398c8ec Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 9 Dec 2024 08:52:23 +0100 Subject: [PATCH] interpret: clean up deduplicating allocation functions --- .../rustc_const_eval/src/interpret/place.rs | 44 +++++------- .../src/util/caller_location.rs | 9 +-- compiler/rustc_middle/src/ty/context.rs | 2 +- src/tools/miri/src/shims/backtrace.rs | 10 +-- src/tools/miri/src/shims/panic.rs | 5 +- .../tests/pass/backtrace/backtrace-api-v0.rs | 69 ------------------- .../pass/backtrace/backtrace-api-v0.stderr | 18 ----- .../pass/backtrace/backtrace-api-v0.stdout | 5 -- 8 files changed, 24 insertions(+), 138 deletions(-) delete mode 100644 src/tools/miri/tests/pass/backtrace/backtrace-api-v0.rs delete mode 100644 src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stderr delete mode 100644 src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stdout diff --git a/compiler/rustc_const_eval/src/interpret/place.rs b/compiler/rustc_const_eval/src/interpret/place.rs index f54a932e1b6f5..810e9356b26c4 100644 --- a/compiler/rustc_const_eval/src/interpret/place.rs +++ b/compiler/rustc_const_eval/src/interpret/place.rs @@ -5,8 +5,7 @@ use std::assert_matches::assert_matches; use either::{Either, Left, Right}; -use rustc_abi::{Align, BackendRepr, HasDataLayout, Size}; -use rustc_ast::Mutability; +use rustc_abi::{BackendRepr, HasDataLayout, Size}; use rustc_middle::ty::Ty; use rustc_middle::ty::layout::{LayoutOf, TyAndLayout}; use rustc_middle::{bug, mir, span_bug}; @@ -1018,40 +1017,31 @@ where self.allocate_dyn(layout, kind, MemPlaceMeta::None) } - /// Allocates a sequence of bytes in the interpreter's memory. - /// For immutable allocations, uses deduplication to reuse existing memory. - /// For mutable allocations, creates a new unique allocation. - pub fn allocate_bytes( + /// Allocates a sequence of bytes in the interpreter's memory with alignment 1. + /// This is allocated in immutable global memory and deduplicated. + pub fn allocate_bytes_dedup( &mut self, bytes: &[u8], - align: Align, - kind: MemoryKind, - mutbl: Mutability, ) -> InterpResult<'tcx, Pointer> { - // Use cache for immutable strings. - if mutbl.is_not() { - // Use dedup'd allocation function. - let salt = M::get_global_alloc_salt(self, None); - let id = self.tcx.allocate_bytes_dedup(bytes, salt); - - // Turn untagged "global" pointers (obtained via `tcx`) into the machine pointer to the allocation. - M::adjust_alloc_root_pointer(&self, Pointer::from(id), Some(kind)) - } else { - // Allocate new memory for mutable data. - self.allocate_bytes_ptr(bytes, align, kind, mutbl) - } + let salt = M::get_global_alloc_salt(self, None); + let id = self.tcx.allocate_bytes_dedup(bytes, salt); + + // Turn untagged "global" pointers (obtained via `tcx`) into the machine pointer to the allocation. + M::adjust_alloc_root_pointer( + &self, + Pointer::from(id), + M::GLOBAL_KIND.map(MemoryKind::Machine), + ) } - /// Allocates a string in the interpreter's memory with metadata for length. - /// Uses `allocate_bytes` internally but adds string-specific metadata handling. - pub fn allocate_str( + /// Allocates a string in the interpreter's memory, returning it as a (wide) place. + /// This is allocated in immutable global memory and deduplicated. + pub fn allocate_str_dedup( &mut self, str: &str, - kind: MemoryKind, - mutbl: Mutability, ) -> InterpResult<'tcx, MPlaceTy<'tcx, M::Provenance>> { let bytes = str.as_bytes(); - let ptr = self.allocate_bytes(bytes, Align::ONE, kind, mutbl)?; + let ptr = self.allocate_bytes_dedup(bytes)?; // Create length metadata for the string. let meta = Scalar::from_target_usize(u64::try_from(bytes.len()).unwrap(), self); diff --git a/compiler/rustc_const_eval/src/util/caller_location.rs b/compiler/rustc_const_eval/src/util/caller_location.rs index 9bf16d4fe1673..6593547cd2383 100644 --- a/compiler/rustc_const_eval/src/util/caller_location.rs +++ b/compiler/rustc_const_eval/src/util/caller_location.rs @@ -1,7 +1,7 @@ use rustc_hir::LangItem; use rustc_middle::query::TyCtxtAt; use rustc_middle::ty::layout::LayoutOf; -use rustc_middle::ty::{self, Mutability}; +use rustc_middle::ty::{self}; use rustc_middle::{bug, mir}; use rustc_span::symbol::Symbol; use tracing::trace; @@ -20,12 +20,9 @@ fn alloc_caller_location<'tcx>( // This can fail if rustc runs out of memory right here. Trying to emit an error would be // pointless, since that would require allocating more memory than these short strings. let file = if loc_details.file { - ecx.allocate_str(filename.as_str(), MemoryKind::CallerLocation, Mutability::Not).unwrap() + ecx.allocate_str_dedup(filename.as_str()).unwrap() } else { - // FIXME: This creates a new allocation each time. It might be preferable to - // perform this allocation only once, and re-use the `MPlaceTy`. - // See https://github.com/rust-lang/rust/pull/89920#discussion_r730012398 - ecx.allocate_str("", MemoryKind::CallerLocation, Mutability::Not).unwrap() + ecx.allocate_str_dedup("").unwrap() }; let file = file.map_provenance(CtfeProvenance::as_immutable); let line = if loc_details.line { Scalar::from_u32(line) } else { Scalar::from_u32(0) }; diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index d4835bb07f689..2841470d24878 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -1479,7 +1479,7 @@ impl<'tcx> TyCtxt<'tcx> { self.mk_adt_def_from_data(ty::AdtDefData::new(self, did, kind, variants, repr)) } - /// Allocates a read-only byte or string literal for `mir::interpret`. + /// Allocates a read-only byte or string literal for `mir::interpret` with alignment 1. /// Returns the same `AllocId` if called again with the same bytes. pub fn allocate_bytes_dedup(self, bytes: &[u8], salt: usize) -> interpret::AllocId { // Create an allocation that just contains these bytes. diff --git a/src/tools/miri/src/shims/backtrace.rs b/src/tools/miri/src/shims/backtrace.rs index 64bd546458d8a..c7b399228bfb4 100644 --- a/src/tools/miri/src/shims/backtrace.rs +++ b/src/tools/miri/src/shims/backtrace.rs @@ -1,5 +1,4 @@ use rustc_abi::{ExternAbi, Size}; -use rustc_ast::ast::Mutability; use rustc_middle::ty::layout::LayoutOf as _; use rustc_middle::ty::{self, Instance, Ty}; use rustc_span::{BytePos, Loc, Symbol, hygiene}; @@ -179,14 +178,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { match flags { 0 => { - // These are "mutable" allocations as we consider them to be owned by the callee. - let name_alloc = - this.allocate_str(&name, MiriMemoryKind::Rust.into(), Mutability::Mut)?; - let filename_alloc = - this.allocate_str(&filename, MiriMemoryKind::Rust.into(), Mutability::Mut)?; - - this.write_immediate(name_alloc.to_ref(this), &this.project_field(dest, 0)?)?; - this.write_immediate(filename_alloc.to_ref(this), &this.project_field(dest, 1)?)?; + throw_unsup_format!("miri_resolve_frame: v0 is not supported any more"); } 1 => { this.write_scalar( diff --git a/src/tools/miri/src/shims/panic.rs b/src/tools/miri/src/shims/panic.rs index 722c3a2f0c599..93479540009ea 100644 --- a/src/tools/miri/src/shims/panic.rs +++ b/src/tools/miri/src/shims/panic.rs @@ -12,7 +12,6 @@ //! metadata we remembered when pushing said frame. use rustc_abi::ExternAbi; -use rustc_ast::Mutability; use rustc_middle::{mir, ty}; use rustc_target::spec::PanicStrategy; @@ -161,7 +160,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); // First arg: message. - let msg = this.allocate_str(msg, MiriMemoryKind::Machine.into(), Mutability::Not)?; + let msg = this.allocate_str_dedup(msg)?; // Call the lang item. let panic = this.tcx.lang_items().panic_fn().unwrap(); @@ -180,7 +179,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); // First arg: message. - let msg = this.allocate_str(msg, MiriMemoryKind::Machine.into(), Mutability::Not)?; + let msg = this.allocate_str_dedup(msg)?; // Call the lang item. let panic = this.tcx.lang_items().panic_nounwind().unwrap(); diff --git a/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.rs b/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.rs deleted file mode 100644 index 3fff7921aff77..0000000000000 --- a/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.rs +++ /dev/null @@ -1,69 +0,0 @@ -//@normalize-stderr-test: "::<.*>" -> "" - -#[inline(never)] -fn func_a() -> Box<[*mut ()]> { - func_b::() -} -#[inline(never)] -fn func_b() -> Box<[*mut ()]> { - func_c() -} - -macro_rules! invoke_func_d { - () => { - func_d() - }; -} - -#[inline(never)] -fn func_c() -> Box<[*mut ()]> { - invoke_func_d!() -} -#[inline(never)] -fn func_d() -> Box<[*mut ()]> { - unsafe { miri_get_backtrace(0) } -} - -fn main() { - let mut seen_main = false; - let frames = func_a(); - for frame in frames.iter() { - let miri_frame = unsafe { miri_resolve_frame(*frame, 0) }; - let name = String::from_utf8(miri_frame.name.into()).unwrap(); - let filename = String::from_utf8(miri_frame.filename.into()).unwrap(); - - if name == "func_a" { - assert_eq!(func_a as *mut (), miri_frame.fn_ptr); - } - - // Print every frame to stderr. - let out = format!("{}:{}:{} ({})", filename, miri_frame.lineno, miri_frame.colno, name); - eprintln!("{}", out); - // Print the 'main' frame (and everything before it) to stdout, skipping - // the printing of internal (and possibly fragile) libstd frames. - // Stdout is less normalized so we see more, but it also means we can print less - // as platform differences would lead to test suite failures. - if !seen_main { - println!("{}", out); - seen_main = name == "main"; - } - } -} - -// This goes at the bottom of the file so that we can change it -// without disturbing line numbers of the functions in the backtrace. - -extern "Rust" { - fn miri_get_backtrace(flags: u64) -> Box<[*mut ()]>; - fn miri_resolve_frame(ptr: *mut (), flags: u64) -> MiriFrame; -} - -#[derive(Debug)] -#[repr(C)] -struct MiriFrame { - name: Box<[u8]>, - filename: Box<[u8]>, - lineno: u32, - colno: u32, - fn_ptr: *mut (), -} diff --git a/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stderr b/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stderr deleted file mode 100644 index 9849a1aa74ea0..0000000000000 --- a/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stderr +++ /dev/null @@ -1,18 +0,0 @@ -tests/pass/backtrace/backtrace-api-v0.rs:LL:CC (func_d) -tests/pass/backtrace/backtrace-api-v0.rs:LL:CC (func_c) -tests/pass/backtrace/backtrace-api-v0.rs:LL:CC (func_b) -tests/pass/backtrace/backtrace-api-v0.rs:LL:CC (func_a) -tests/pass/backtrace/backtrace-api-v0.rs:LL:CC (main) -RUSTLIB/core/src/ops/function.rs:LL:CC (>::call_once - shim(fn())) -RUSTLIB/std/src/sys/backtrace.rs:LL:CC (std::sys::backtrace::__rust_begin_short_backtrace) -RUSTLIB/std/src/rt.rs:LL:CC (std::rt::lang_start::{closure#0}) -RUSTLIB/core/src/ops/function.rs:LL:CC (std::ops::function::impls::call_once) -RUSTLIB/std/src/panicking.rs:LL:CC (std::panicking::r#try::do_call) -RUSTLIB/std/src/panicking.rs:LL:CC (std::panicking::r#try) -RUSTLIB/std/src/panic.rs:LL:CC (std::panic::catch_unwind) -RUSTLIB/std/src/rt.rs:LL:CC (std::rt::lang_start_internal::{closure#1}) -RUSTLIB/std/src/panicking.rs:LL:CC (std::panicking::r#try::do_call) -RUSTLIB/std/src/panicking.rs:LL:CC (std::panicking::r#try) -RUSTLIB/std/src/panic.rs:LL:CC (std::panic::catch_unwind) -RUSTLIB/std/src/rt.rs:LL:CC (std::rt::lang_start_internal) -RUSTLIB/std/src/rt.rs:LL:CC (std::rt::lang_start) diff --git a/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stdout b/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stdout deleted file mode 100644 index 8c1bc5c353e9f..0000000000000 --- a/src/tools/miri/tests/pass/backtrace/backtrace-api-v0.stdout +++ /dev/null @@ -1,5 +0,0 @@ -tests/pass/backtrace/backtrace-api-v0.rs:24:14 (func_d) -tests/pass/backtrace/backtrace-api-v0.rs:14:9 (func_c) -tests/pass/backtrace/backtrace-api-v0.rs:9:5 (func_b::) -tests/pass/backtrace/backtrace-api-v0.rs:5:5 (func_a) -tests/pass/backtrace/backtrace-api-v0.rs:29:18 (main)