Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce an arena type which may be used to allocate a list of types with destructors #59536

Merged
merged 6 commits into from
Apr 12, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 206 additions & 0 deletions src/librustc/arena.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
use arena::{TypedArena, DroplessArena};
use std::mem;
use std::ptr;
use std::slice;
use std::cell::RefCell;
use std::marker::PhantomData;
use smallvec::SmallVec;

#[macro_export]
macro_rules! arena_types {
($macro:path, $args:tt, $tcx:lifetime) => (
$macro!($args, [
[] vtable_method: Option<(
rustc::hir::def_id::DefId,
rustc::ty::subst::SubstsRef<$tcx>
)>,
[few] mir_keys: rustc::util::nodemap::DefIdSet,
[decode] specialization_graph: rustc::traits::specialization_graph::Graph,
], $tcx);
Copy link
Contributor Author

@Zoxc Zoxc Mar 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list of types are declared here with a macro.

)
}

macro_rules! arena_for_type {
([][$ty:ty]) => {
TypedArena<$ty>
};
([few $(, $attrs:ident)*][$ty:ty]) => {
PhantomData<$ty>
};
([$ignore:ident $(, $attrs:ident)*]$args:tt) => {
arena_for_type!([$($attrs),*]$args)
};
}

macro_rules! declare_arena {
([], [$($a:tt $name:ident: $ty:ty,)*], $tcx:lifetime) => {
#[derive(Default)]
pub struct Arena<$tcx> {
dropless: DroplessArena,
drop: DropArena,
$($name: arena_for_type!($a[$ty]),)*
}
}
}

macro_rules! which_arena_for_type {
([][$arena:expr]) => {
Some($arena)
};
([few$(, $attrs:ident)*][$arena:expr]) => {
None
};
([$ignore:ident$(, $attrs:ident)*]$args:tt) => {
which_arena_for_type!([$($attrs),*]$args)
};
}

macro_rules! impl_arena_allocatable {
([], [$($a:tt $name:ident: $ty:ty,)*], $tcx:lifetime) => {
$(
impl ArenaAllocatable for $ty {}
unsafe impl<$tcx> ArenaField<$tcx> for $ty {
#[inline]
fn arena<'a>(_arena: &'a Arena<$tcx>) -> Option<&'a TypedArena<Self>> {
which_arena_for_type!($a[&_arena.$name])
}
}
)*
}
}

arena_types!(declare_arena, [], 'tcx);

arena_types!(impl_arena_allocatable, [], 'tcx);

pub trait ArenaAllocatable {}

impl<T: Copy> ArenaAllocatable for T {}

pub unsafe trait ArenaField<'tcx>: Sized {
/// Returns a specific arena to allocate from.
/// If None is returned, the DropArena will be used.
fn arena<'a>(arena: &'a Arena<'tcx>) -> Option<&'a TypedArena<Self>>;
}

unsafe impl<'tcx, T> ArenaField<'tcx> for T {
#[inline]
default fn arena<'a>(_: &'a Arena<'tcx>) -> Option<&'a TypedArena<Self>> {
panic!()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This impl can conflict with the types in the list :(

error[E0119]: conflicting implementations of trait `arena::ArenaAllocatable<'_>` for type `std::collections::HashSet<hir::item_local_id_inner::ItemLocalId, std::hash::BuildHasherDefault<rustc_data_structures::fx::FxHasher>>`:
  --> src\librustc\arena.rs:42:13
   |
4  |  / macro_rules! arena_types {
5  |  |     ($macro:path, $args:tt, $tcx:lifetime) => (
6  |  |         $macro!($args, [
   |  |_________-
7  | ||             [] vtable_method: Option<(
8  | ||                 rustc::hir::def_id::DefId,
9  | ||                 rustc::ty::subst::SubstsRef<$tcx>
...  ||
15 | ||             [] item_local_set: rustc::util::nodemap::ItemLocalSet,
16 | ||         ], $tcx);
   | ||_________________- in this macro invocation
17 |  |     )
18 |  | }
   |  |_- in this expansion of `arena_types!`
...
37 | /  macro_rules! impl_arena_allocatable {
38 | |      ([], [$($a:tt $name:ident: $ty:ty,)*], $tcx:lifetime) => {
39 | |          $(
40 | |              impl_specialized_decodable!($a $ty, $tcx);
41 | |
42 | |              impl<$tcx> ArenaAllocatable<$tcx> for $ty {
   | |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `std::collections::HashSet<hir::item_local_id_inner::ItemLocalId, std::hash::BuildHasherDefault<rustc_data_structures::fx::FxHasher>>`
...  |
49 | |      }
50 | |  }
   | |__- in this expansion of `impl_arena_allocatable!`
...
54 |    arena_types!(impl_arena_allocatable, [], 'tcx);
   |    ----------------------------------------------- in this macro invocation
...
62 |    impl<'tcx, T: Copy> ArenaAllocatable<'tcx> for T {
   |    ------------------------------------------------ first implementation here
   |
   = note: upstream crates may add new impl of trait `std::marker::Copy` for type `std::collections::HashSet<hir::item_local_id_inner::ItemLocalId, std::hash::BuildHasherDefault<rustc_data_structures::fx::FxHasher>>` in future versions

error: aborting due to previous error

Do we have some way to express an ArenaAllocatable or Copy bound?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, it would be nice to be able to rely on sets and maps never being Copy...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a way around this using a marker trait.

}

impl<'tcx> Arena<'tcx> {
#[inline]
pub fn alloc<T: ArenaAllocatable>(&self, value: T) -> &mut T {
if !mem::needs_drop::<T>() {
return self.dropless.alloc(value);
}
match <T as ArenaField<'tcx>>::arena(self) {
Some(arena) => arena.alloc(value),
None => unsafe { self.drop.alloc(value) },
}
}

pub fn alloc_from_iter<
T: ArenaAllocatable,
I: IntoIterator<Item = T>
>(
&'a self,
iter: I
) -> &'a mut [T] {
if !mem::needs_drop::<T>() {
return self.dropless.alloc_from_iter(iter);
}
match <T as ArenaField<'tcx>>::arena(self) {
Some(arena) => arena.alloc_from_iter(iter),
None => unsafe { self.drop.alloc_from_iter(iter) },
}
}
}

/// Calls the destructor for an object when dropped.
struct DropType {
drop_fn: unsafe fn(*mut u8),
obj: *mut u8,
}

unsafe fn drop_for_type<T>(to_drop: *mut u8) {
std::ptr::drop_in_place(to_drop as *mut T)
}

impl Drop for DropType {
fn drop(&mut self) {
unsafe {
(self.drop_fn)(self.obj)
}
}
}

/// An arena which can be used to allocate any type.
/// Allocating in this arena is unsafe since the type system
/// doesn't know which types it contains. In order to
/// allocate safetly, you must store a PhantomData<T>
Zoxc marked this conversation as resolved.
Show resolved Hide resolved
/// alongside this arena for each type T you allocate.
#[derive(Default)]
struct DropArena {
/// A list of destructors to run when the arena drops.
/// Ordered so `destructors` gets dropped before the arena
/// since its destructor can reference memory in the arena.
destructors: RefCell<Vec<DropType>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the impl, I think this will work for values which refer to other values in the same arena, as long as those types don't have interior mutability and can end up with references to values created later.

How are we ensuring this invariant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not an invariant that's upheld. We just let dropck ensure that types can't do anything bad in their destructors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about something like

struct A<'tcx> {
    x: Mutex<Option<&'tcx B>>,
}

impl<'tcx> Drop for A<'tcx> {
    fn drop(&mut self) {
        println!("{}", x.lock().unwrap().unwrap().s);
    }
}

struct B {
    s: String,
}

where

  1. intern a value of type A
  2. intern a value of type B
  3. change the value from 1. to point to 2.
  4. value from 2. is dropped
  5. value from 1. is dropped and accesses the value from 2. which has already been dropped

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can the same interning with (TypedArena<A<'tcx>>, TypedArena<B>).

arena: DroplessArena,
}

impl DropArena {
#[inline]
unsafe fn alloc<T>(&self, object: T) -> &mut T {
let mem = self.arena.alloc_raw(
mem::size_of::<T>(),
mem::align_of::<T>()
) as *mut _ as *mut T;
// Write into uninitialized memory.
ptr::write(mem, object);
let result = &mut *mem;
// Record the destructor after doing the allocation as that may panic
// and would cause `object`'s destuctor to run twice if it was recorded before
self.destructors.borrow_mut().push(DropType {
drop_fn: drop_for_type::<T>,
obj: result as *mut T as *mut u8,
});
result
}

#[inline]
unsafe fn alloc_from_iter<T, I: IntoIterator<Item = T>>(&self, iter: I) -> &mut [T] {
let mut vec: SmallVec<[_; 8]> = iter.into_iter().collect();
if vec.is_empty() {
return &mut [];
}
let len = vec.len();

let start_ptr = self.arena.alloc_raw(
len.checked_mul(mem::size_of::<T>()).unwrap(),
mem::align_of::<T>()
) as *mut _ as *mut T;

let mut destructors = self.destructors.borrow_mut();
// Reserve space for the destructors so we can't panic while adding them
destructors.reserve(len);

// Move the content to the arena by copying it and then forgetting
// the content of the SmallVec
vec.as_ptr().copy_to_nonoverlapping(start_ptr, len);
mem::forget(vec.drain());

// Record the destructors after doing the allocation as that may panic
// and would cause `object`'s destuctor to run twice if it was recorded before
for i in 0..len {
destructors.push(DropType {
drop_fn: drop_for_type::<T>,
obj: start_ptr.offset(i as isize) as *mut u8,
});
}

slice::from_raw_parts_mut(start_ptr, len)
}
}
3 changes: 3 additions & 0 deletions src/librustc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#![cfg_attr(windows, feature(libc))]
#![feature(never_type)]
#![feature(exhaustive_patterns)]
#![feature(overlapping_marker_traits)]
#![feature(extern_types)]
#![feature(nll)]
#![feature(non_exhaustive)]
Expand Down Expand Up @@ -103,6 +104,8 @@ pub mod diagnostics;
#[macro_use]
pub mod query;

#[macro_use]
pub mod arena;
pub mod cfg;
pub mod dep_graph;
pub mod hir;
Expand Down
7 changes: 3 additions & 4 deletions src/librustc/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ rustc_queries! {
/// Set of all the `DefId`s in this crate that have MIR associated with
/// them. This includes all the body owners, but also things like struct
/// constructors.
query mir_keys(_: CrateNum) -> Lrc<DefIdSet> {
query mir_keys(_: CrateNum) -> &'tcx DefIdSet {
desc { "getting a list of all mir_keys" }
}

Expand Down Expand Up @@ -516,7 +516,7 @@ rustc_queries! {

Other {
query vtable_methods(key: ty::PolyTraitRef<'tcx>)
-> Lrc<Vec<Option<(DefId, SubstsRef<'tcx>)>>> {
-> &'tcx [Option<(DefId, SubstsRef<'tcx>)>] {
no_force
desc { |tcx| "finding all methods for trait {}", tcx.def_path_str(key.def_id()) }
}
Expand All @@ -539,8 +539,7 @@ rustc_queries! {
query trait_impls_of(key: DefId) -> Lrc<ty::trait_def::TraitImpls> {
desc { |tcx| "trait impls of `{}`", tcx.def_path_str(key) }
}
query specialization_graph_of(_: DefId)
-> Lrc<specialization_graph::Graph> {}
query specialization_graph_of(_: DefId) -> &'tcx specialization_graph::Graph {}
query is_object_safe(key: DefId) -> bool {
desc { |tcx| "determine object safety of trait `{}`", tcx.def_path_str(key) }
}
Expand Down
7 changes: 3 additions & 4 deletions src/librustc/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use crate::infer::{InferCtxt, SuppressRegionErrors};
use crate::infer::outlives::env::OutlivesEnvironment;
use crate::middle::region;
use crate::mir::interpret::ErrorHandled;
use rustc_data_structures::sync::Lrc;
use rustc_macros::HashStable;
use syntax::ast;
use syntax_pos::{Span, DUMMY_SP};
Expand Down Expand Up @@ -984,11 +983,11 @@ fn substitute_normalize_and_test_predicates<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx
fn vtable_methods<'a, 'tcx>(
tcx: TyCtxt<'a, 'tcx, 'tcx>,
trait_ref: ty::PolyTraitRef<'tcx>)
-> Lrc<Vec<Option<(DefId, SubstsRef<'tcx>)>>>
-> &'tcx [Option<(DefId, SubstsRef<'tcx>)>]
{
debug!("vtable_methods({:?})", trait_ref);

Lrc::new(
tcx.arena.alloc_from_iter(
supertraits(tcx, trait_ref).flat_map(move |trait_ref| {
let trait_methods = tcx.associated_items(trait_ref.def_id())
.filter(|item| item.kind == ty::AssociatedKind::Method);
Expand Down Expand Up @@ -1039,7 +1038,7 @@ fn vtable_methods<'a, 'tcx>(

Some((def_id, substs))
})
}).collect()
})
)
}

Expand Down
5 changes: 2 additions & 3 deletions src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use crate::infer::{InferCtxt, InferOk};
use crate::lint;
use crate::traits::{self, coherence, FutureCompatOverlapErrorKind, ObligationCause, TraitEngine};
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::sync::Lrc;
use syntax_pos::DUMMY_SP;
use crate::traits::select::IntercrateAmbiguityCause;
use crate::ty::{self, TyCtxt, TypeFoldable};
Expand Down Expand Up @@ -289,7 +288,7 @@ fn fulfill_implication<'a, 'gcx, 'tcx>(infcx: &InferCtxt<'a, 'gcx, 'tcx>,
pub(super) fn specialization_graph_provider<'a, 'tcx>(
tcx: TyCtxt<'a, 'tcx, 'tcx>,
trait_id: DefId,
) -> Lrc<specialization_graph::Graph> {
) -> &'tcx specialization_graph::Graph {
let mut sg = specialization_graph::Graph::new();

let mut trait_impls = tcx.all_impls(trait_id);
Expand Down Expand Up @@ -383,7 +382,7 @@ pub(super) fn specialization_graph_provider<'a, 'tcx>(
}
}

Lrc::new(sg)
tcx.arena.alloc(sg)
}

/// Recovers the "impl X for Y" signature from `impl_def_id` and returns it as a
Expand Down
13 changes: 6 additions & 7 deletions src/librustc/traits/specialize/specialization_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher,
use crate::traits;
use crate::ty::{self, TyCtxt, TypeFoldable};
use crate::ty::fast_reject::{self, SimplifiedType};
use rustc_data_structures::sync::Lrc;
use syntax::ast::Ident;
use crate::util::captures::Captures;
use crate::util::nodemap::{DefIdMap, FxHashMap};
Expand Down Expand Up @@ -439,13 +438,13 @@ impl<'a, 'gcx, 'tcx> Node {
}
}

pub struct Ancestors {
pub struct Ancestors<'tcx> {
trait_def_id: DefId,
specialization_graph: Lrc<Graph>,
specialization_graph: &'tcx Graph,
current_source: Option<Node>,
}

impl Iterator for Ancestors {
impl Iterator for Ancestors<'_> {
type Item = Node;
fn next(&mut self) -> Option<Node> {
let cur = self.current_source.take();
Expand Down Expand Up @@ -476,7 +475,7 @@ impl<T> NodeItem<T> {
}
}

impl<'a, 'gcx, 'tcx> Ancestors {
impl<'a, 'gcx, 'tcx> Ancestors<'gcx> {
/// Search the items from the given ancestors, returning each definition
/// with the given name and the given kind.
// FIXME(#35870): avoid closures being unexported due to `impl Trait`.
Expand Down Expand Up @@ -509,10 +508,10 @@ impl<'a, 'gcx, 'tcx> Ancestors {

/// Walk up the specialization ancestors of a given impl, starting with that
/// impl itself.
pub fn ancestors(tcx: TyCtxt<'_, '_, '_>,
pub fn ancestors(tcx: TyCtxt<'_, 'tcx, '_>,
trait_def_id: DefId,
start_from_impl: DefId)
-> Ancestors {
-> Ancestors<'tcx> {
let specialization_graph = tcx.specialization_graph_of(trait_def_id);
Ancestors {
trait_def_id,
Expand Down
Loading