From 24dca6aecaded209d47aba6e60e0b87eae2d49d6 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Mon, 10 Sep 2018 14:40:12 +0200 Subject: [PATCH] Simplify Scope/ScopeData to have less chance of introducing UB or size increases --- src/librustc/ich/impls_ty.rs | 15 +-- src/librustc/infer/error_reporting/mod.rs | 12 +-- src/librustc/middle/region.rs | 106 +++++++------------- src/librustc/util/ppaux.rs | 27 ++--- src/librustc_data_structures/indexed_vec.rs | 6 +- src/librustc_mir/build/cfg.rs | 2 +- src/librustc_mir/build/scope.rs | 2 +- src/librustc_mir/hair/cx/block.rs | 10 +- 8 files changed, 77 insertions(+), 103 deletions(-) diff --git a/src/librustc/ich/impls_ty.rs b/src/librustc/ich/impls_ty.rs index 16175e159dd2..2bf1c79c8a43 100644 --- a/src/librustc/ich/impls_ty.rs +++ b/src/librustc/ich/impls_ty.rs @@ -772,7 +772,15 @@ impl_stable_hash_for!(enum ty::cast::CastKind { FnPtrAddrCast }); -impl_stable_hash_for!(struct ::middle::region::Scope { id, code }); +impl_stable_hash_for!(struct ::middle::region::Scope { id, data }); + +impl_stable_hash_for!(enum ::middle::region::ScopeData { + Node, + CallSite, + Arguments, + Destruction, + Remainder(first_statement_index) +}); impl<'a> ToStableHashKey> for region::Scope { type KeyType = region::Scope; @@ -783,11 +791,6 @@ impl<'a> ToStableHashKey> for region::Scope { } } -impl_stable_hash_for!(struct ::middle::region::BlockRemainder { - block, - first_statement_index -}); - impl_stable_hash_for!(struct ty::adjustment::CoerceUnsizedInfo { custom_kind }); diff --git a/src/librustc/infer/error_reporting/mod.rs b/src/librustc/infer/error_reporting/mod.rs index eabcf1ce4136..a0c96554c91f 100644 --- a/src/librustc/infer/error_reporting/mod.rs +++ b/src/librustc/infer/error_reporting/mod.rs @@ -119,17 +119,17 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { } }; let scope_decorated_tag = match scope.data() { - region::ScopeData::Node(_) => tag, - region::ScopeData::CallSite(_) => "scope of call-site for function", - region::ScopeData::Arguments(_) => "scope of function body", - region::ScopeData::Destruction(_) => { + region::ScopeData::Node => tag, + region::ScopeData::CallSite => "scope of call-site for function", + region::ScopeData::Arguments => "scope of function body", + region::ScopeData::Destruction => { new_string = format!("destruction scope surrounding {}", tag); &new_string[..] } - region::ScopeData::Remainder(r) => { + region::ScopeData::Remainder(first_statement_index) => { new_string = format!( "block suffix following statement {}", - r.first_statement_index.index() + first_statement_index.index() ); &new_string[..] } diff --git a/src/librustc/middle/region.rs b/src/librustc/middle/region.rs index 0fbd74e250e7..6fa5d363ffa7 100644 --- a/src/librustc/middle/region.rs +++ b/src/librustc/middle/region.rs @@ -20,7 +20,6 @@ use ich::{StableHashingContext, NodeIdHashingMode}; use util::nodemap::{FxHashMap, FxHashSet}; use ty; -use std::fmt; use std::mem; use rustc_data_structures::sync::Lrc; use syntax::source_map; @@ -100,39 +99,29 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher, /// placate the same deriving in `ty::FreeRegion`, but we may want to /// actually attach a more meaningful ordering to scopes than the one /// generated via deriving here. -/// -/// Scope is a bit-packed to save space - if `code` is SCOPE_DATA_REMAINDER_MAX -/// or less, it is a `ScopeData::Remainder`, otherwise it is a type specified -/// by the bitpacking. -#[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Copy, RustcEncodable, RustcDecodable)] +#[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Debug, Copy, RustcEncodable, RustcDecodable)] pub struct Scope { pub(crate) id: hir::ItemLocalId, - pub(crate) code: u32 + pub(crate) data: ScopeData, } -const SCOPE_DATA_NODE: u32 = 0xFFFF_FFFF; -const SCOPE_DATA_CALLSITE: u32 = 0xFFFF_FFFE; -const SCOPE_DATA_ARGUMENTS: u32 = 0xFFFF_FFFD; -const SCOPE_DATA_DESTRUCTION: u32 = 0xFFFF_FFFC; -// be sure to add the MAX of FirstStatementIndex if you add more constants here - #[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Debug, Copy, RustcEncodable, RustcDecodable)] pub enum ScopeData { - Node(hir::ItemLocalId), + Node, // Scope of the call-site for a function or closure // (outlives the arguments as well as the body). - CallSite(hir::ItemLocalId), + CallSite, // Scope of arguments passed to a function or closure // (they outlive its body). - Arguments(hir::ItemLocalId), + Arguments, // Scope of destructors for temporaries of node-id. - Destruction(hir::ItemLocalId), + Destruction, // Scope following a `let id = expr;` binding in a block. - Remainder(BlockRemainder) + Remainder(FirstStatementIndex) } /// Represents a subscope of `block` for a binding that is introduced @@ -152,12 +141,6 @@ pub enum ScopeData { /// /// * the subscope with `first_statement_index == 1` is scope of `c`, /// and thus does not include EXPR_2, but covers the `...`. -#[derive(Clone, PartialEq, PartialOrd, Eq, Ord, Hash, RustcEncodable, - RustcDecodable, Debug, Copy)] -pub struct BlockRemainder { - pub block: hir::ItemLocalId, - pub first_statement_index: FirstStatementIndex, -} newtype_index! { pub struct FirstStatementIndex; @@ -165,68 +148,54 @@ newtype_index! { impl_stable_hash_for!(struct ::middle::region::FirstStatementIndex { private }); -impl From for Scope { - #[inline] - fn from(scope_data: ScopeData) -> Self { - let (id, code) = match scope_data { - ScopeData::Node(id) => (id, SCOPE_DATA_NODE), - ScopeData::CallSite(id) => (id, SCOPE_DATA_CALLSITE), - ScopeData::Arguments(id) => (id, SCOPE_DATA_ARGUMENTS), - ScopeData::Destruction(id) => (id, SCOPE_DATA_DESTRUCTION), - ScopeData::Remainder(r) => (r.block, r.first_statement_index.index() as u32) - }; - Self { id, code } - } -} - -impl fmt::Debug for Scope { - fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result { - fmt::Debug::fmt(&self.data(), formatter) - } -} +// compilation error if size of `ScopeData` is not the same as a `u32` +#[allow(dead_code)] +// only works on stage 1 when the rustc_layout_scalar_valid_range attribute actually exists +#[cfg(not(stage0))] +static ASSERT: () = [()][(mem::size_of::() != 4) as usize]; #[allow(non_snake_case)] impl Scope { #[inline] pub fn data(self) -> ScopeData { - match self.code { - SCOPE_DATA_NODE => ScopeData::Node(self.id), - SCOPE_DATA_CALLSITE => ScopeData::CallSite(self.id), - SCOPE_DATA_ARGUMENTS => ScopeData::Arguments(self.id), - SCOPE_DATA_DESTRUCTION => ScopeData::Destruction(self.id), - idx => ScopeData::Remainder(BlockRemainder { - block: self.id, - first_statement_index: FirstStatementIndex::new(idx as usize) - }) + self.data } + + #[inline] + pub fn new(id: hir::ItemLocalId, data: ScopeData) -> Self { + Scope { id, data } } #[inline] pub fn Node(id: hir::ItemLocalId) -> Self { - Self::from(ScopeData::Node(id)) + Self::new(id, ScopeData::Node) } #[inline] pub fn CallSite(id: hir::ItemLocalId) -> Self { - Self::from(ScopeData::CallSite(id)) + Self::new(id, ScopeData::CallSite) } #[inline] pub fn Arguments(id: hir::ItemLocalId) -> Self { - Self::from(ScopeData::Arguments(id)) + Self::new(id, ScopeData::Arguments) } #[inline] pub fn Destruction(id: hir::ItemLocalId) -> Self { - Self::from(ScopeData::Destruction(id)) + Self::new(id, ScopeData::Destruction) } #[inline] - pub fn Remainder(r: BlockRemainder) -> Self { - Self::from(ScopeData::Remainder(r)) + pub fn Remainder( + id: hir::ItemLocalId, + first: FirstStatementIndex, + ) -> Self { + Self::new(id, ScopeData::Remainder(first)) } } + impl Scope { /// Returns a item-local id associated with this scope. /// @@ -257,7 +226,7 @@ impl Scope { return DUMMY_SP; } let span = tcx.hir.span(node_id); - if let ScopeData::Remainder(r) = self.data() { + if let ScopeData::Remainder(first_statement_index) = self.data() { if let Node::Block(ref blk) = tcx.hir.get(node_id) { // Want span for scope starting after the // indexed statement and ending at end of @@ -267,7 +236,7 @@ impl Scope { // (This is the special case aluded to in the // doc-comment for this method) - let stmt_span = blk.stmts[r.first_statement_index.index()].span; + let stmt_span = blk.stmts[first_statement_index.index()].span; // To avoid issues with macro-generated spans, the span // of the statement must be nested in that of the block. @@ -511,8 +480,8 @@ impl<'tcx> ScopeTree { } // record the destruction scopes for later so we can query them - if let ScopeData::Destruction(n) = child.data() { - self.destruction_scopes.insert(n, child); + if let ScopeData::Destruction = child.data() { + self.destruction_scopes.insert(child.item_local_id(), child); } } @@ -595,7 +564,7 @@ impl<'tcx> ScopeTree { while let Some(&(p, _)) = self.parent_map.get(&id) { match p.data() { - ScopeData::Destruction(..) => { + ScopeData::Destruction => { debug!("temporary_scope({:?}) = {:?} [enclosing]", expr_id, id); return Some(id); @@ -650,8 +619,8 @@ impl<'tcx> ScopeTree { /// Returns the id of the innermost containing body pub fn containing_body(&self, mut scope: Scope)-> Option { loop { - if let ScopeData::CallSite(id) = scope.data() { - return Some(id); + if let ScopeData::CallSite = scope.data() { + return Some(scope.item_local_id()); } match self.opt_encl_scope(scope) { @@ -867,10 +836,7 @@ fn resolve_block<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, blk: // except for the first such subscope, which has the // block itself as a parent. visitor.enter_scope( - Scope::Remainder(BlockRemainder { - block: blk.hir_id.local_id, - first_statement_index: FirstStatementIndex::new(i) - }) + Scope::Remainder(blk.hir_id.local_id, FirstStatementIndex::new(i)) ); visitor.cx.var_parent = visitor.cx.parent; } @@ -1033,7 +999,7 @@ fn resolve_expr<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, expr: match visitor.scope_tree.parent_map.get(&scope) { // Don't cross from closure bodies to their parent. Some(&(superscope, _)) => match superscope.data() { - ScopeData::CallSite(_) => break, + ScopeData::CallSite => break, _ => scope = superscope }, None => break diff --git a/src/librustc/util/ppaux.rs b/src/librustc/util/ppaux.rs index ddcc0fa9c928..1206690c86e2 100644 --- a/src/librustc/util/ppaux.rs +++ b/src/librustc/util/ppaux.rs @@ -11,7 +11,7 @@ use hir::def_id::DefId; use hir::map::definitions::DefPathData; use mir::interpret::ConstValue; -use middle::region::{self, BlockRemainder}; +use middle::region; use ty::subst::{self, Subst}; use ty::{BrAnon, BrEnv, BrFresh, BrNamed}; use ty::{Bool, Char, Adt}; @@ -770,17 +770,20 @@ define_print! { } ty::ReScope(scope) if cx.identify_regions => { match scope.data() { - region::ScopeData::Node(id) => - write!(f, "'{}s", id.as_usize()), - region::ScopeData::CallSite(id) => - write!(f, "'{}cs", id.as_usize()), - region::ScopeData::Arguments(id) => - write!(f, "'{}as", id.as_usize()), - region::ScopeData::Destruction(id) => - write!(f, "'{}ds", id.as_usize()), - region::ScopeData::Remainder(BlockRemainder - { block, first_statement_index }) => - write!(f, "'{}_{}rs", block.as_usize(), first_statement_index.index()), + region::ScopeData::Node => + write!(f, "'{}s", scope.item_local_id().as_usize()), + region::ScopeData::CallSite => + write!(f, "'{}cs", scope.item_local_id().as_usize()), + region::ScopeData::Arguments => + write!(f, "'{}as", scope.item_local_id().as_usize()), + region::ScopeData::Destruction => + write!(f, "'{}ds", scope.item_local_id().as_usize()), + region::ScopeData::Remainder(first_statement_index) => write!( + f, + "'{}_{}rs", + scope.item_local_id().as_usize(), + first_statement_index.index() + ), } } ty::ReVar(region_vid) if cx.identify_regions => { diff --git a/src/librustc_data_structures/indexed_vec.rs b/src/librustc_data_structures/indexed_vec.rs index 0b5fb97f0e1f..2f11fea46d69 100644 --- a/src/librustc_data_structures/indexed_vec.rs +++ b/src/librustc_data_structures/indexed_vec.rs @@ -72,7 +72,8 @@ macro_rules! newtype_index { newtype_index!( // Leave out derives marker so we can use its absence to ensure it comes first @type [$name] - @max [0xFFFF_FFFE] + // shave off 256 indices at the end to allow space for packing these indices into enums + @max [0xFFFF_FF00] @vis [$v] @debug_format ["{}"]); ); @@ -82,7 +83,8 @@ macro_rules! newtype_index { newtype_index!( // Leave out derives marker so we can use its absence to ensure it comes first @type [$name] - @max [0xFFFF_FFFE] + // shave off 256 indices at the end to allow space for packing these indices into enums + @max [0xFFFF_FF00] @vis [$v] @debug_format ["{}"] $($tokens)+); diff --git a/src/librustc_mir/build/cfg.rs b/src/librustc_mir/build/cfg.rs index 1ed8289d4418..d46b0813ca70 100644 --- a/src/librustc_mir/build/cfg.rs +++ b/src/librustc_mir/build/cfg.rs @@ -51,7 +51,7 @@ impl<'tcx> CFG<'tcx> { source_info: SourceInfo, region_scope: region::Scope) { if tcx.emit_end_regions() { - if let region::ScopeData::CallSite(_) = region_scope.data() { + if let region::ScopeData::CallSite = region_scope.data() { // The CallSite scope (aka the root scope) is sort of weird, in that it is // supposed to "separate" the "interior" and "exterior" of a closure. Being // that, it is not really a part of the region hierarchy, but for some diff --git a/src/librustc_mir/build/scope.rs b/src/librustc_mir/build/scope.rs index 38e0854bcd61..1406183955bd 100644 --- a/src/librustc_mir/build/scope.rs +++ b/src/librustc_mir/build/scope.rs @@ -566,7 +566,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // We want `scopes[1]`, which is the `ParameterScope`. assert!(self.scopes.len() >= 2); assert!(match self.scopes[1].region_scope.data() { - region::ScopeData::Arguments(_) => true, + region::ScopeData::Arguments => true, _ => false, }); self.scopes[1].region_scope diff --git a/src/librustc_mir/hair/cx/block.rs b/src/librustc_mir/hair/cx/block.rs index 6c8b5d97b6ff..730603dff56f 100644 --- a/src/librustc_mir/hair/cx/block.rs +++ b/src/librustc_mir/hair/cx/block.rs @@ -11,7 +11,7 @@ use hair::*; use hair::cx::Cx; use hair::cx::to_ref::ToRef; -use rustc::middle::region::{self, BlockRemainder}; +use rustc::middle::region; use rustc::hir; use rustc_data_structures::indexed_vec::Idx; @@ -71,10 +71,10 @@ fn mirror_stmts<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, // ignore for purposes of the MIR } hir::DeclKind::Local(ref local) => { - let remainder_scope = region::Scope::Remainder(BlockRemainder { - block: block_id, - first_statement_index: region::FirstStatementIndex::new(index), - }); + let remainder_scope = region::Scope::Remainder( + block_id, + region::FirstStatementIndex::new(index), + ); let ty = local.ty.clone().map(|ty| ty.hir_id); let pattern = cx.pattern_from_hir(&local.pat);