Skip to content

Commit

Permalink
Rollup merge of #71508 - oli-obk:alloc_map_unlock, r=RalfJung
Browse files Browse the repository at this point in the history
Simplify the `tcx.alloc_map` API

This PR changes all functions that require manually locking the `alloc_map` to functions on `TyCtxt` that lock the map internally. In the same step we make the `TyCtxt::alloc_map` field private.

r? @RalfJung
  • Loading branch information
RalfJung authored May 9, 2020
2 parents ce05553 + b07a44d commit 8c0310d
Show file tree
Hide file tree
Showing 19 changed files with 115 additions and 107 deletions.
10 changes: 4 additions & 6 deletions src/librustc_codegen_llvm/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,8 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
}
}
Scalar::Ptr(ptr) => {
let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id);
let base_addr = match alloc_kind {
Some(GlobalAlloc::Memory(alloc)) => {
let base_addr = match self.tcx.global_alloc(ptr.alloc_id) {
GlobalAlloc::Memory(alloc) => {
let init = const_alloc_to_llvm(self, alloc);
let value = match alloc.mutability {
Mutability::Mut => self.static_addr_of_mut(init, alloc.align, None),
Expand All @@ -257,12 +256,11 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> {
}
value
}
Some(GlobalAlloc::Function(fn_instance)) => self.get_fn_addr(fn_instance),
Some(GlobalAlloc::Static(def_id)) => {
GlobalAlloc::Function(fn_instance) => self.get_fn_addr(fn_instance),
GlobalAlloc::Static(def_id) => {
assert!(self.tcx.is_static(def_id));
self.get_static(def_id)
}
None => bug!("missing allocation {:?}", ptr.alloc_id),
};
let llval = unsafe {
llvm::LLVMConstInBoundsGEP(
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_ssa/mir/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
_ => bug!("from_const: invalid ScalarPair layout: {:#?}", layout),
};
let a = Scalar::from(Pointer::new(
bx.tcx().alloc_map.lock().create_memory_alloc(data),
bx.tcx().create_memory_alloc(data),
Size::from_bytes(start),
));
let a_llval = bx.scalar_to_backend(
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_middle/ich/impls_ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::AllocId {
ty::tls::with_opt(|tcx| {
trace!("hashing {:?}", *self);
let tcx = tcx.expect("can't hash AllocIds during hir lowering");
let alloc_kind = tcx.alloc_map.lock().get(*self);
alloc_kind.hash_stable(hcx, hasher);
tcx.get_global_alloc(*self).hash_stable(hcx, hasher);
});
}
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_middle/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#![feature(or_patterns)]
#![feature(range_is_empty)]
#![feature(specialization)]
#![feature(track_caller)]
#![feature(trusted_len)]
#![feature(vec_remove_item)]
#![feature(stmt_expr_attributes)]
Expand Down
116 changes: 70 additions & 46 deletions src/librustc_middle/mir/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,7 @@ pub fn specialized_encode_alloc_id<'tcx, E: Encoder>(
tcx: TyCtxt<'tcx>,
alloc_id: AllocId,
) -> Result<(), E::Error> {
let alloc: GlobalAlloc<'tcx> =
tcx.alloc_map.lock().get(alloc_id).expect("no value for given alloc ID");
match alloc {
match tcx.global_alloc(alloc_id) {
GlobalAlloc::Memory(alloc) => {
trace!("encoding {:?} with {:#?}", alloc_id, alloc);
AllocDiscriminant::Alloc.encode(encoder)?;
Expand Down Expand Up @@ -294,7 +292,7 @@ impl<'s> AllocDecodingSession<'s> {
AllocDiscriminant::Alloc => {
// If this is an allocation, we need to reserve an
// `AllocId` so we can decode cyclic graphs.
let alloc_id = decoder.tcx().alloc_map.lock().reserve();
let alloc_id = decoder.tcx().reserve_alloc_id();
*entry =
State::InProgress(TinyList::new_single(self.session_id), alloc_id);
Some(alloc_id)
Expand Down Expand Up @@ -338,23 +336,23 @@ impl<'s> AllocDecodingSession<'s> {
// We already have a reserved `AllocId`.
let alloc_id = alloc_id.unwrap();
trace!("decoded alloc {:?}: {:#?}", alloc_id, alloc);
decoder.tcx().alloc_map.lock().set_alloc_id_same_memory(alloc_id, alloc);
decoder.tcx().set_alloc_id_same_memory(alloc_id, alloc);
Ok(alloc_id)
}
AllocDiscriminant::Fn => {
assert!(alloc_id.is_none());
trace!("creating fn alloc ID");
let instance = ty::Instance::decode(decoder)?;
trace!("decoded fn alloc instance: {:?}", instance);
let alloc_id = decoder.tcx().alloc_map.lock().create_fn_alloc(instance);
let alloc_id = decoder.tcx().create_fn_alloc(instance);
Ok(alloc_id)
}
AllocDiscriminant::Static => {
assert!(alloc_id.is_none());
trace!("creating extern static alloc ID");
let did = DefId::decode(decoder)?;
trace!("decoded static def-ID: {:?}", did);
let alloc_id = decoder.tcx().alloc_map.lock().create_static_alloc(did);
let alloc_id = decoder.tcx().create_static_alloc(did);
Ok(alloc_id)
}
}
Expand All @@ -381,7 +379,29 @@ pub enum GlobalAlloc<'tcx> {
Memory(&'tcx Allocation),
}

pub struct AllocMap<'tcx> {
impl GlobalAlloc<'tcx> {
/// Panics if the `GlobalAlloc` does not refer to an `GlobalAlloc::Memory`
#[track_caller]
#[inline]
pub fn unwrap_memory(&self) -> &'tcx Allocation {
match *self {
GlobalAlloc::Memory(mem) => mem,
_ => bug!("expected memory, got {:?}", self),
}
}

/// Panics if the `GlobalAlloc` is not `GlobalAlloc::Function`
#[track_caller]
#[inline]
pub fn unwrap_fn(&self) -> Instance<'tcx> {
match *self {
GlobalAlloc::Function(instance) => instance,
_ => bug!("expected function, got {:?}", self),
}
}
}

crate struct AllocMap<'tcx> {
/// Maps `AllocId`s to their corresponding allocations.
alloc_map: FxHashMap<AllocId, GlobalAlloc<'tcx>>,

Expand All @@ -397,16 +417,10 @@ pub struct AllocMap<'tcx> {
}

impl<'tcx> AllocMap<'tcx> {
pub fn new() -> Self {
crate fn new() -> Self {
AllocMap { alloc_map: Default::default(), dedup: Default::default(), next_id: AllocId(0) }
}

/// Obtains a new allocation ID that can be referenced but does not
/// yet have an allocation backing it.
///
/// Make sure to call `set_alloc_id_memory` or `set_alloc_id_same_memory` before returning such
/// an `AllocId` from a query.
pub fn reserve(&mut self) -> AllocId {
fn reserve(&mut self) -> AllocId {
let next = self.next_id;
self.next_id.0 = self.next_id.0.checked_add(1).expect(
"You overflowed a u64 by incrementing by 1... \
Expand All @@ -415,34 +429,46 @@ impl<'tcx> AllocMap<'tcx> {
);
next
}
}

impl<'tcx> TyCtxt<'tcx> {
/// Obtains a new allocation ID that can be referenced but does not
/// yet have an allocation backing it.
///
/// Make sure to call `set_alloc_id_memory` or `set_alloc_id_same_memory` before returning such
/// an `AllocId` from a query.
pub fn reserve_alloc_id(&self) -> AllocId {
self.alloc_map.lock().reserve()
}

/// Reserves 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 {
fn reserve_and_set_dedup(&self, alloc: GlobalAlloc<'tcx>) -> AllocId {
let mut alloc_map = self.alloc_map.lock();
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) {
if let Some(&alloc_id) = alloc_map.dedup.get(&alloc) {
return alloc_id;
}
let id = self.reserve();
let id = alloc_map.reserve();
debug!("creating alloc {:?} with id {}", alloc, id);
self.alloc_map.insert(id, alloc.clone());
self.dedup.insert(alloc, id);
alloc_map.alloc_map.insert(id, alloc.clone());
alloc_map.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 {
pub fn create_static_alloc(&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 {
pub fn create_fn_alloc(&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
// duplicated across crates.
Expand All @@ -456,8 +482,9 @@ impl<'tcx> AllocMap<'tcx> {
});
if is_generic {
// Get a fresh ID.
let id = self.reserve();
self.alloc_map.insert(id, GlobalAlloc::Function(instance));
let mut alloc_map = self.alloc_map.lock();
let id = alloc_map.reserve();
alloc_map.alloc_map.insert(id, GlobalAlloc::Function(instance));
id
} else {
// Deduplicate.
Expand All @@ -470,8 +497,8 @@ impl<'tcx> AllocMap<'tcx> {
/// 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();
pub fn create_memory_alloc(&self, mem: &'tcx Allocation) -> AllocId {
let id = self.reserve_alloc_id();
self.set_alloc_id_memory(id, mem);
id
}
Expand All @@ -482,38 +509,35 @@ impl<'tcx> AllocMap<'tcx> {
/// 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<GlobalAlloc<'tcx>> {
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(GlobalAlloc::Memory(mem)) => mem,
_ => bug!("expected allocation ID {} to point to memory", id),
}
pub fn get_global_alloc(&self, id: AllocId) -> Option<GlobalAlloc<'tcx>> {
self.alloc_map.lock().alloc_map.get(&id).cloned()
}

/// Panics if the `AllocId` does not refer to a function
pub fn unwrap_fn(&self, id: AllocId) -> Instance<'tcx> {
match self.get(id) {
Some(GlobalAlloc::Function(instance)) => instance,
_ => bug!("expected allocation ID {} to point to a function", id),
#[inline]
#[track_caller]
/// Panics in case the `AllocId` is dangling. Since that is impossible for `AllocId`s in
/// constants (as all constants must pass interning and validation that check for dangling
/// ids), this function is frequently used throughout rustc, but should not be used within
/// the miri engine.
pub fn global_alloc(&self, id: AllocId) -> GlobalAlloc<'tcx> {
match self.get_global_alloc(id) {
Some(alloc) => alloc,
None => bug!("could not find allocation for {}", id),
}
}

/// Freezes 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.alloc_map.insert(id, GlobalAlloc::Memory(mem)) {
pub fn set_alloc_id_memory(&self, id: AllocId, mem: &'tcx Allocation) {
if let Some(old) = self.alloc_map.lock().alloc_map.insert(id, GlobalAlloc::Memory(mem)) {
bug!("tried to set allocation ID {}, but it was already existing as {:#?}", id, old);
}
}

/// Freezes 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.alloc_map.insert_same(id, GlobalAlloc::Memory(mem));
fn set_alloc_id_same_memory(&self, id: AllocId, mem: &'tcx Allocation) {
self.alloc_map.lock().alloc_map.insert_same(id, GlobalAlloc::Memory(mem));
}
}

Expand Down
10 changes: 3 additions & 7 deletions src/librustc_middle/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2404,13 +2404,9 @@ pub struct Constant<'tcx> {
impl Constant<'tcx> {
pub fn check_static_ptr(&self, tcx: TyCtxt<'_>) -> Option<DefId> {
match self.literal.val.try_to_scalar() {
Some(Scalar::Ptr(ptr)) => match tcx.alloc_map.lock().get(ptr.alloc_id) {
Some(GlobalAlloc::Static(def_id)) => Some(def_id),
Some(_) => None,
None => {
tcx.sess.delay_span_bug(DUMMY_SP, "MIR cannot contain dangling const pointers");
None
}
Some(Scalar::Ptr(ptr)) => match tcx.global_alloc(ptr.alloc_id) {
GlobalAlloc::Static(def_id) => Some(def_id),
_ => None,
},
_ => None,
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_middle/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,7 @@ pub struct GlobalCtxt<'tcx> {
allocation_interner: ShardedHashMap<&'tcx Allocation, ()>,

/// Stores memory for globals (statics/consts).
pub alloc_map: Lock<interpret::AllocMap<'tcx>>,
pub(crate) alloc_map: Lock<interpret::AllocMap<'tcx>>,

layout_interner: ShardedHashMap<&'tcx Layout, ()>,

Expand Down Expand Up @@ -1017,7 +1017,7 @@ impl<'tcx> TyCtxt<'tcx> {
// 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().create_memory_alloc(alloc)
self.create_memory_alloc(alloc)
}

pub fn intern_stability(self, stab: attr::Stability) -> &'tcx attr::Stability {
Expand Down
10 changes: 3 additions & 7 deletions src/librustc_middle/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -956,9 +956,8 @@ pub trait PrettyPrinter<'tcx>:
) => {
let byte_str = self
.tcx()
.alloc_map
.lock()
.unwrap_memory(ptr.alloc_id)
.global_alloc(ptr.alloc_id)
.unwrap_memory()
.get_bytes(&self.tcx(), ptr, Size::from_bytes(*data))
.unwrap();
p!(pretty_print_byte_str(byte_str));
Expand Down Expand Up @@ -1021,10 +1020,7 @@ pub trait PrettyPrinter<'tcx>:
)?;
}
(Scalar::Ptr(ptr), ty::FnPtr(_)) => {
let instance = {
let alloc_map = self.tcx().alloc_map.lock();
alloc_map.unwrap_fn(ptr.alloc_id)
};
let instance = self.tcx().global_alloc(ptr.alloc_id).unwrap_fn();
self = self.typed_value(
|this| this.print_value_path(instance.def_id(), instance.substs),
|this| this.print_type(ty),
Expand Down
5 changes: 2 additions & 3 deletions src/librustc_middle/ty/relate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -549,9 +549,8 @@ pub fn super_relate_consts<R: TypeRelation<'tcx>>(
if a_val == b_val {
Ok(ConstValue::Scalar(a_val))
} else if let ty::FnPtr(_) = a.ty.kind {
let alloc_map = tcx.alloc_map.lock();
let a_instance = alloc_map.unwrap_fn(a_val.assert_ptr().alloc_id);
let b_instance = alloc_map.unwrap_fn(b_val.assert_ptr().alloc_id);
let a_instance = tcx.global_alloc(a_val.assert_ptr().alloc_id).unwrap_fn();
let b_instance = tcx.global_alloc(b_val.assert_ptr().alloc_id).unwrap_fn();
if a_instance == b_instance {
Ok(ConstValue::Scalar(a_val))
} else {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ pub(super) fn op_to_const<'tcx>(

let to_const_value = |mplace: MPlaceTy<'_>| match mplace.ptr {
Scalar::Ptr(ptr) => {
let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id);
let alloc = ecx.tcx.global_alloc(ptr.alloc_id).unwrap_memory();
ConstValue::ByRef { alloc, offset: ptr.offset }
}
Scalar::Raw { data, .. } => {
Expand All @@ -155,7 +155,7 @@ pub(super) fn op_to_const<'tcx>(
Immediate::ScalarPair(a, b) => {
let (data, start) = match a.not_undef().unwrap() {
Scalar::Ptr(ptr) => {
(ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id), ptr.offset.bytes())
(ecx.tcx.global_alloc(ptr.alloc_id).unwrap_memory(), ptr.offset.bytes())
}
Scalar::Raw { .. } => (
ecx.tcx
Expand Down Expand Up @@ -203,7 +203,7 @@ fn validate_and_turn_into_const<'tcx>(
if is_static || cid.promoted.is_some() {
let ptr = mplace.ptr.assert_ptr();
Ok(ConstValue::ByRef {
alloc: ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id),
alloc: ecx.tcx.global_alloc(ptr.alloc_id).unwrap_memory(),
offset: ptr.offset,
})
} else {
Expand Down
Loading

0 comments on commit 8c0310d

Please sign in to comment.