From 41c36fabefbf20351a9c8263a17131e15b19d35a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 30 May 2019 13:05:05 +0200 Subject: [PATCH] light refactoring of global AllocMap * rename AllocKind -> GlobalAlloc. This stores the allocation itself, not just its kind. * rename the methods that allocate stuff to have consistent names. --- src/librustc/mir/interpret/mod.rs | 95 ++++++++++++---------- src/librustc/ty/context.rs | 2 +- src/librustc_codegen_llvm/common.rs | 8 +- src/librustc_codegen_ssa/mir/operand.rs | 2 +- src/librustc_mir/interpret/memory.rs | 26 +++--- src/librustc_mir/interpret/operand.rs | 2 +- src/librustc_mir/interpret/place.rs | 2 +- src/librustc_mir/interpret/validity.rs | 4 +- src/librustc_mir/monomorphize/collector.rs | 8 +- 9 files changed, 81 insertions(+), 68 deletions(-) diff --git a/src/librustc/mir/interpret/mod.rs b/src/librustc/mir/interpret/mod.rs index 7985af914ff..964d6c01d74 100644 --- a/src/librustc/mir/interpret/mod.rs +++ b/src/librustc/mir/interpret/mod.rs @@ -72,20 +72,20 @@ pub fn specialized_encode_alloc_id< tcx: TyCtxt<'a, 'tcx, 'tcx>, alloc_id: AllocId, ) -> Result<(), E::Error> { - let alloc_kind: AllocKind<'tcx> = + let alloc: GlobalAlloc<'tcx> = tcx.alloc_map.lock().get(alloc_id).expect("no value for AllocId"); - match alloc_kind { - AllocKind::Memory(alloc) => { + match alloc { + GlobalAlloc::Memory(alloc) => { trace!("encoding {:?} with {:#?}", alloc_id, alloc); AllocDiscriminant::Alloc.encode(encoder)?; alloc.encode(encoder)?; } - AllocKind::Function(fn_instance) => { + GlobalAlloc::Function(fn_instance) => { trace!("encoding {:?} with {:#?}", alloc_id, fn_instance); AllocDiscriminant::Fn.encode(encoder)?; fn_instance.encode(encoder)?; } - AllocKind::Static(did) => { + GlobalAlloc::Static(did) => { // referring to statics doesn't need to know about their allocations, // just about its DefId AllocDiscriminant::Static.encode(encoder)?; @@ -239,7 +239,7 @@ pub fn decode_alloc_id<'a, 'tcx, D>(&self, assert!(alloc_id.is_none()); trace!("creating extern static alloc id at"); let did = DefId::decode(decoder)?; - let alloc_id = decoder.tcx().alloc_map.lock().intern_static(did); + let alloc_id = decoder.tcx().alloc_map.lock().create_static_alloc(did); Ok(alloc_id) } } @@ -259,8 +259,10 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { } } +/// An allocation in the global (tcx-managed) memory can be either a function pointer, +/// a static, or a "real" allocation with some data in it. #[derive(Debug, Clone, Eq, PartialEq, Hash, RustcDecodable, RustcEncodable, HashStable)] -pub enum AllocKind<'tcx> { +pub enum GlobalAlloc<'tcx> { /// The alloc ID is used as a function pointer Function(Instance<'tcx>), /// The alloc ID points to a "lazy" static variable that did not get computed (yet). @@ -272,10 +274,12 @@ pub enum AllocKind<'tcx> { pub struct AllocMap<'tcx> { /// Lets you know what an `AllocId` refers to. - id_to_kind: FxHashMap>, + alloc_map: FxHashMap>, - /// Used to ensure that statics only get one associated `AllocId`. - type_interner: FxHashMap, AllocId>, + /// Used to ensure that statics and functions only get one associated `AllocId`. + /// Should never contain a `GlobalAlloc::Memory`! + /// FIXME: Should we just have two separate dedup maps for statics and functions each? + dedup: FxHashMap, AllocId>, /// The `AllocId` to assign to the next requested ID. /// Always incremented, never gets smaller. @@ -285,8 +289,8 @@ pub struct AllocMap<'tcx> { impl<'tcx> AllocMap<'tcx> { pub fn new() -> Self { AllocMap { - id_to_kind: Default::default(), - type_interner: Default::default(), + alloc_map: Default::default(), + dedup: Default::default(), next_id: AllocId(0), } } @@ -308,17 +312,32 @@ pub fn reserve( next } - fn intern(&mut self, alloc_kind: AllocKind<'tcx>) -> AllocId { - if let Some(&alloc_id) = self.type_interner.get(&alloc_kind) { + /// Reserve a new ID *if* this allocation has not been dedup-reserved before. + /// Should only be used for function pointers and statics, we don't want + /// to dedup IDs for "real" memory! + fn reserve_and_set_dedup(&mut self, alloc: GlobalAlloc<'tcx>) -> AllocId { + match alloc { + GlobalAlloc::Function(..) | GlobalAlloc::Static(..) => {}, + GlobalAlloc::Memory(..) => bug!("Trying to dedup-reserve memory with real data!"), + } + if let Some(&alloc_id) = self.dedup.get(&alloc) { return alloc_id; } let id = self.reserve(); - debug!("creating alloc_kind {:?} with id {}", alloc_kind, id); - self.id_to_kind.insert(id, alloc_kind.clone()); - self.type_interner.insert(alloc_kind, id); + debug!("creating alloc {:?} with id {}", alloc, id); + self.alloc_map.insert(id, alloc.clone()); + self.dedup.insert(alloc, id); id } + /// Generates an `AllocId` for a static or return a cached one in case this function has been + /// called on the same static before. + pub fn create_static_alloc(&mut self, static_id: DefId) -> AllocId { + self.reserve_and_set_dedup(GlobalAlloc::Static(static_id)) + } + + /// Generates an `AllocId` for a function. Depending on the function type, + /// this might get deduplicated or assigned a new ID each time. pub fn create_fn_alloc(&mut self, instance: Instance<'tcx>) -> AllocId { // Functions cannot be identified by pointers, as asm-equal functions can get deduplicated // by the linker (we set the "unnamed_addr" attribute for LLVM) and functions can be @@ -336,53 +355,47 @@ pub fn create_fn_alloc(&mut self, instance: Instance<'tcx>) -> AllocId { if is_generic { // Get a fresh ID let id = self.reserve(); - self.id_to_kind.insert(id, AllocKind::Function(instance)); + self.alloc_map.insert(id, GlobalAlloc::Function(instance)); id } else { // Deduplicate - self.intern(AllocKind::Function(instance)) + self.reserve_and_set_dedup(GlobalAlloc::Function(instance)) } } + /// Intern the `Allocation` and return a new `AllocId`, even if there's already an identical + /// `Allocation` with a different `AllocId`. + /// Statics with identical content will still point to the same `Allocation`, i.e., + /// their data will be deduplicated through `Allocation` interning -- but they + /// are different places in memory and as such need different IDs. + pub fn create_memory_alloc(&mut self, mem: &'tcx Allocation) -> AllocId { + let id = self.reserve(); + self.set_alloc_id_memory(id, mem); + id + } + /// Returns `None` in case the `AllocId` is dangling. An `InterpretCx` can still have a /// local `Allocation` for that `AllocId`, but having such an `AllocId` in a constant is /// illegal and will likely ICE. /// This function exists to allow const eval to detect the difference between evaluation- /// local dangling pointers and allocations in constants/statics. #[inline] - pub fn get(&self, id: AllocId) -> Option> { - self.id_to_kind.get(&id).cloned() + pub fn get(&self, id: AllocId) -> Option> { + self.alloc_map.get(&id).cloned() } /// Panics if the `AllocId` does not refer to an `Allocation` pub fn unwrap_memory(&self, id: AllocId) -> &'tcx Allocation { match self.get(id) { - Some(AllocKind::Memory(mem)) => mem, + Some(GlobalAlloc::Memory(mem)) => mem, _ => bug!("expected allocation id {} to point to memory", id), } } - /// Generates an `AllocId` for a static or return a cached one in case this function has been - /// called on the same static before. - pub fn intern_static(&mut self, static_id: DefId) -> AllocId { - self.intern(AllocKind::Static(static_id)) - } - - /// Intern the `Allocation` and return a new `AllocId`, even if there's already an identical - /// `Allocation` with a different `AllocId`. - // FIXME: is this really necessary? Can we ensure `FOO` and `BAR` being different after codegen - // in `static FOO: u32 = 42; static BAR: u32 = 42;` even if they reuse the same allocation - // inside rustc? - pub fn allocate(&mut self, mem: &'tcx Allocation) -> AllocId { - let id = self.reserve(); - self.set_alloc_id_memory(id, mem); - id - } - /// Freeze an `AllocId` created with `reserve` by pointing it at an `Allocation`. Trying to /// call this function twice, even with the same `Allocation` will ICE the compiler. pub fn set_alloc_id_memory(&mut self, id: AllocId, mem: &'tcx Allocation) { - if let Some(old) = self.id_to_kind.insert(id, AllocKind::Memory(mem)) { + if let Some(old) = self.alloc_map.insert(id, GlobalAlloc::Memory(mem)) { bug!("tried to set allocation id {}, but it was already existing as {:#?}", id, old); } } @@ -390,7 +403,7 @@ pub fn set_alloc_id_memory(&mut self, id: AllocId, mem: &'tcx Allocation) { /// Freeze an `AllocId` created with `reserve` by pointing it at an `Allocation`. May be called /// twice for the same `(AllocId, Allocation)` pair. fn set_alloc_id_same_memory(&mut self, id: AllocId, mem: &'tcx Allocation) { - self.id_to_kind.insert_same(id, AllocKind::Memory(mem)); + self.alloc_map.insert_same(id, GlobalAlloc::Memory(mem)); } } diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index ff218911ffb..9a7184dc194 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -1191,7 +1191,7 @@ pub fn allocate_bytes(self, bytes: &[u8]) -> interpret::AllocId { // create an allocation that just contains these bytes let alloc = interpret::Allocation::from_byte_aligned_bytes(bytes, ()); let alloc = self.intern_const_alloc(alloc); - self.alloc_map.lock().allocate(alloc) + self.alloc_map.lock().create_memory_alloc(alloc) } pub fn intern_stability(self, stab: attr::Stability) -> &'gcx attr::Stability { diff --git a/src/librustc_codegen_llvm/common.rs b/src/librustc_codegen_llvm/common.rs index c713362440d..0b23aac5224 100644 --- a/src/librustc_codegen_llvm/common.rs +++ b/src/librustc_codegen_llvm/common.rs @@ -12,7 +12,7 @@ use crate::consts::const_alloc_to_llvm; use rustc::ty::layout::{HasDataLayout, LayoutOf, self, TyLayout, Size}; -use rustc::mir::interpret::{Scalar, AllocKind, Allocation}; +use rustc::mir::interpret::{Scalar, GlobalAlloc, Allocation}; use rustc_codegen_ssa::mir::place::PlaceRef; use libc::{c_uint, c_char}; @@ -310,7 +310,7 @@ fn scalar_to_backend( Scalar::Ptr(ptr) => { let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); let base_addr = match alloc_kind { - Some(AllocKind::Memory(alloc)) => { + Some(GlobalAlloc::Memory(alloc)) => { let init = const_alloc_to_llvm(self, alloc); if alloc.mutability == Mutability::Mutable { self.static_addr_of_mut(init, alloc.align, None) @@ -318,10 +318,10 @@ fn scalar_to_backend( self.static_addr_of(init, alloc.align, None) } } - Some(AllocKind::Function(fn_instance)) => { + Some(GlobalAlloc::Function(fn_instance)) => { self.get_fn(fn_instance) } - Some(AllocKind::Static(def_id)) => { + Some(GlobalAlloc::Static(def_id)) => { assert!(self.tcx.is_static(def_id)); self.get_static(def_id) } diff --git a/src/librustc_codegen_ssa/mir/operand.rs b/src/librustc_codegen_ssa/mir/operand.rs index ec471a1323e..3f53f992a5b 100644 --- a/src/librustc_codegen_ssa/mir/operand.rs +++ b/src/librustc_codegen_ssa/mir/operand.rs @@ -98,7 +98,7 @@ pub fn from_const>( _ => bug!("from_const: invalid ScalarPair layout: {:#?}", layout) }; let a = Scalar::from(Pointer::new( - bx.tcx().alloc_map.lock().allocate(data), + bx.tcx().alloc_map.lock().create_memory_alloc(data), Size::from_bytes(start as u64), )).into(); let a_llval = bx.scalar_to_backend( diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 65a3b04c8b1..e089b9a148f 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -18,7 +18,7 @@ use super::{ Pointer, AllocId, Allocation, GlobalId, AllocationExtra, - EvalResult, Scalar, InterpError, AllocKind, PointerArithmetic, + EvalResult, Scalar, InterpError, GlobalAlloc, PointerArithmetic, Machine, AllocMap, MayLeak, ErrorHandled, CheckInAllocMsg, InboundsCheck, }; @@ -193,12 +193,12 @@ pub fn deallocate( None => { // Deallocating static memory -- always an error return match self.tcx.alloc_map.lock().get(ptr.alloc_id) { - Some(AllocKind::Function(..)) => err!(DeallocatedWrongMemoryKind( + Some(GlobalAlloc::Function(..)) => err!(DeallocatedWrongMemoryKind( "function".to_string(), format!("{:?}", kind), )), - Some(AllocKind::Static(..)) | - Some(AllocKind::Memory(..)) => err!(DeallocatedWrongMemoryKind( + Some(GlobalAlloc::Static(..)) | + Some(GlobalAlloc::Memory(..)) => err!(DeallocatedWrongMemoryKind( "static".to_string(), format!("{:?}", kind), )), @@ -313,15 +313,15 @@ fn get_static_alloc( ) -> EvalResult<'tcx, Cow<'tcx, Allocation>> { let alloc = tcx.alloc_map.lock().get(id); let def_id = match alloc { - Some(AllocKind::Memory(mem)) => { + Some(GlobalAlloc::Memory(mem)) => { // We got tcx memory. Let the machine figure out whether and how to // turn that into memory with the right pointer tag. return Ok(M::adjust_static_allocation(mem, memory_extra)) } - Some(AllocKind::Function(..)) => { + Some(GlobalAlloc::Function(..)) => { return err!(DerefFunctionPointer) } - Some(AllocKind::Static(did)) => { + Some(GlobalAlloc::Static(did)) => { did } None => @@ -429,8 +429,8 @@ pub fn get_size_and_align( } // Could also be a fn ptr or extern static match self.tcx.alloc_map.lock().get(id) { - Some(AllocKind::Function(..)) => Ok((Size::ZERO, Align::from_bytes(1).unwrap())), - Some(AllocKind::Static(did)) => { + Some(GlobalAlloc::Function(..)) => Ok((Size::ZERO, Align::from_bytes(1).unwrap())), + Some(GlobalAlloc::Static(did)) => { // The only way `get` couldn't have worked here is if this is an extern static assert!(self.tcx.is_foreign_item(did)); // Use size and align of the type @@ -456,7 +456,7 @@ pub fn get_fn(&self, ptr: Pointer) -> EvalResult<'tcx, Instance<' } trace!("reading fn ptr: {}", ptr.alloc_id); match self.tcx.alloc_map.lock().get(ptr.alloc_id) { - Some(AllocKind::Function(instance)) => Ok(instance), + Some(GlobalAlloc::Function(instance)) => Ok(instance), _ => Err(InterpError::ExecuteMemory.into()), } } @@ -554,16 +554,16 @@ pub fn dump_allocs(&self, mut allocs: Vec) { Err(()) => { // static alloc? match self.tcx.alloc_map.lock().get(id) { - Some(AllocKind::Memory(alloc)) => { + Some(GlobalAlloc::Memory(alloc)) => { self.dump_alloc_helper( &mut allocs_seen, &mut allocs_to_print, msg, alloc, " (immutable)".to_owned() ); } - Some(AllocKind::Function(func)) => { + Some(GlobalAlloc::Function(func)) => { trace!("{} {}", msg, func); } - Some(AllocKind::Static(did)) => { + Some(GlobalAlloc::Static(did)) => { trace!("{} {:?}", msg, did); } None => { diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 289379f34a9..764b5f38e49 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -550,7 +550,7 @@ pub(super) fn eval_operands( ConstValue::Slice { data, start, end } => Operand::Immediate(Immediate::ScalarPair( Scalar::from(Pointer::new( - self.tcx.alloc_map.lock().allocate(data), + self.tcx.alloc_map.lock().create_memory_alloc(data), Size::from_bytes(start as u64), )).with_default_tag().into(), Scalar::from_uint( diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 0ae4e90b7c2..faed52b74c8 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -597,7 +597,7 @@ pub(super) fn eval_static_to_mplace( // want! This way, computing statics works concistently between codegen // and miri: They use the same query to eventually obtain a `ty::Const` // and use that for further computation. - let alloc = self.tcx.alloc_map.lock().intern_static(cid.instance.def_id()); + let alloc = self.tcx.alloc_map.lock().create_static_alloc(cid.instance.def_id()); MPlaceTy::from_aligned_ptr(Pointer::from(alloc).with_default_tag(), layout) } }) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 072c6f4fd90..4afadaa4b19 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -8,7 +8,7 @@ use rustc::ty; use rustc_data_structures::fx::FxHashSet; use rustc::mir::interpret::{ - Scalar, AllocKind, EvalResult, InterpError, CheckInAllocMsg, + Scalar, GlobalAlloc, EvalResult, InterpError, CheckInAllocMsg, }; use super::{ @@ -403,7 +403,7 @@ fn visit_primitive(&mut self, value: OpTy<'tcx, M::PointerTag>) -> EvalResult<'t "integer pointer in non-ZST reference", self.path); // Skip validation entirely for some external statics let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id); - if let Some(AllocKind::Static(did)) = alloc_kind { + if let Some(GlobalAlloc::Static(did)) = alloc_kind { // `extern static` cannot be validated as they have no body. // FIXME: Statics from other crates are also skipped. // They might be checked at a different type, but for now we diff --git a/src/librustc_mir/monomorphize/collector.rs b/src/librustc_mir/monomorphize/collector.rs index c1131336f36..bd94e573c6f 100644 --- a/src/librustc_mir/monomorphize/collector.rs +++ b/src/librustc_mir/monomorphize/collector.rs @@ -187,7 +187,7 @@ use rustc::mir::{self, Location, Place, PlaceBase, Promoted, Static, StaticKind}; use rustc::mir::visit::Visitor as MirVisitor; use rustc::mir::mono::MonoItem; -use rustc::mir::interpret::{Scalar, GlobalId, AllocKind, ErrorHandled}; +use rustc::mir::interpret::{Scalar, GlobalId, GlobalAlloc, ErrorHandled}; use crate::monomorphize::{self, Instance}; use rustc::util::nodemap::{FxHashSet, FxHashMap, DefIdMap}; @@ -1183,20 +1183,20 @@ fn collect_miri<'a, 'tcx>( ) { let alloc_kind = tcx.alloc_map.lock().get(alloc_id); match alloc_kind { - Some(AllocKind::Static(did)) => { + Some(GlobalAlloc::Static(did)) => { let instance = Instance::mono(tcx, did); if should_monomorphize_locally(tcx, &instance) { trace!("collecting static {:?}", did); output.push(MonoItem::Static(did)); } } - Some(AllocKind::Memory(alloc)) => { + Some(GlobalAlloc::Memory(alloc)) => { trace!("collecting {:?} with {:#?}", alloc_id, alloc); for &((), inner) in alloc.relocations.values() { collect_miri(tcx, inner, output); } }, - Some(AllocKind::Function(fn_instance)) => { + Some(GlobalAlloc::Function(fn_instance)) => { if should_monomorphize_locally(tcx, &fn_instance) { trace!("collecting {:?} with {:#?}", alloc_id, fn_instance); output.push(create_fn_mono_item(fn_instance));