From 10f439a0116df86f737d83cc620289b14c88660f Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 25 Dec 2019 13:58:02 +0100 Subject: [PATCH 1/8] Promoteds can contain raw pointers, but these must still only point to immutable allocations --- src/librustc_mir/const_eval.rs | 4 +- src/librustc_mir/const_eval/eval_queries.rs | 13 ++++-- src/librustc_mir/interpret/intern.rs | 46 ++++++++++++++++----- src/librustc_mir/interpret/mod.rs | 2 +- src/librustc_mir/transform/const_prop.rs | 8 ++-- src/test/ui/consts/raw_pointer_promoted.rs | 5 +++ 6 files changed, 56 insertions(+), 22 deletions(-) create mode 100644 src/test/ui/consts/raw_pointer_promoted.rs diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index aa7be3d80e1..aad0e162935 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -5,7 +5,7 @@ use rustc::ty::layout::VariantIdx; use rustc::ty::{self, TyCtxt}; use rustc_span::{source_map::DUMMY_SP, symbol::Symbol}; -use crate::interpret::{intern_const_alloc_recursive, ConstValue, InterpCx}; +use crate::interpret::{intern_const_alloc_recursive, ConstValue, InternKind, InterpCx}; mod error; mod eval_queries; @@ -52,7 +52,7 @@ pub(crate) fn const_caller_location<'tcx>( let loc_ty = tcx.caller_location_ty(); let loc_place = ecx.alloc_caller_location(file, line, col); - intern_const_alloc_recursive(&mut ecx, None, loc_place, false).unwrap(); + intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place, false).unwrap(); let loc_const = ty::Const { ty: loc_ty, val: ty::ConstKind::Value(ConstValue::Scalar(loc_place.ptr.into())), diff --git a/src/librustc_mir/const_eval/eval_queries.rs b/src/librustc_mir/const_eval/eval_queries.rs index d260a6808d1..442baf85f2b 100644 --- a/src/librustc_mir/const_eval/eval_queries.rs +++ b/src/librustc_mir/const_eval/eval_queries.rs @@ -1,9 +1,9 @@ use super::{error_to_const_error, CompileTimeEvalContext, CompileTimeInterpreter, MemoryExtra}; use crate::interpret::eval_nullary_intrinsic; use crate::interpret::{ - intern_const_alloc_recursive, Allocation, ConstValue, GlobalId, ImmTy, Immediate, InterpCx, - InterpResult, MPlaceTy, MemoryKind, OpTy, RawConst, RefTracking, Scalar, ScalarMaybeUndef, - StackPopCleanup, + intern_const_alloc_recursive, Allocation, ConstValue, GlobalId, ImmTy, Immediate, InternKind, + InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RawConst, RefTracking, Scalar, + ScalarMaybeUndef, StackPopCleanup, }; use rustc::mir; use rustc::mir::interpret::{ConstEvalErr, ErrorHandled}; @@ -56,9 +56,14 @@ fn eval_body_using_ecx<'mir, 'tcx>( ecx.run()?; // Intern the result + let intern_kind = match tcx.static_mutability(cid.instance.def_id()) { + Some(m) => InternKind::Static(m), + None if cid.promoted.is_some() => InternKind::Promoted, + _ => InternKind::Constant, + }; intern_const_alloc_recursive( ecx, - tcx.static_mutability(cid.instance.def_id()), + intern_kind, ret, body.ignore_interior_mut_in_const_validation, )?; diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 220761ce28d..8ff78ffb5ba 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -268,19 +268,27 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx } } +pub enum InternKind { + /// The `mutability` of the static, ignoring the type which may have interior mutability. + Static(hir::Mutability), + Constant, + Promoted, + ConstProp, +} + pub fn intern_const_alloc_recursive>( ecx: &mut InterpCx<'mir, 'tcx, M>, - // The `mutability` of the place, ignoring the type. - place_mut: Option, + intern_kind: InternKind, ret: MPlaceTy<'tcx>, ignore_interior_mut_in_const_validation: bool, ) -> InterpResult<'tcx> { let tcx = ecx.tcx; - let (base_mutability, base_intern_mode) = match place_mut { + let (base_mutability, base_intern_mode) = match intern_kind { // `static mut` doesn't care about interior mutability, it's mutable anyway - Some(mutbl) => (mutbl, InternMode::Static), - // consts, promoteds. FIXME: what about array lengths, array initializers? - None => (Mutability::Not, InternMode::ConstBase), + InternKind::Static(mutbl) => (mutbl, InternMode::Static), + // FIXME: what about array lengths, array initializers? + InternKind::Constant | InternKind::ConstProp => (Mutability::Not, InternMode::ConstBase), + InternKind::Promoted => (Mutability::Not, InternMode::ConstBase), }; // Type based interning. @@ -338,10 +346,23 @@ pub fn intern_const_alloc_recursive>( // We can't call the `intern_shallow` method here, as its logic is tailored to safe // references and a `leftover_allocations` set (where we only have a todo-list here). // So we hand-roll the interning logic here again. - match base_intern_mode { - InternMode::Static => {} - InternMode::Const | InternMode::ConstBase => { - // If it's not a static, it *must* be immutable. + match intern_kind { + // Mutable statics may contain mutable allocations even behind relocations + InternKind::Static(hir::Mutability::Mut) => {} + // Once we get heap allocations we need to revisit whether immutable statics can + // refer to mutable (e.g. via interior mutability) allocations. + InternKind::Static(hir::Mutability::Not) => { + alloc.mutability = Mutability::Not; + } + // Raw pointers in promoteds may only point to immutable things so we mark + // everything as immutable. Creating a promoted with interior mutability is UB, but + // there's no way we can check whether the user is using raw pointers correctly. + // So all we can do is mark this as immutable here. + InternKind::Promoted => { + alloc.mutability = Mutability::Not; + } + InternKind::Constant | InternKind::ConstProp => { + // If it's a constant, it *must* be immutable. // We cannot have mutable memory inside a constant. // We use `delay_span_bug` here, because this can be reached in the presence // of fancy transmutes. @@ -363,7 +384,10 @@ pub fn intern_const_alloc_recursive>( } else if ecx.memory.dead_alloc_map.contains_key(&alloc_id) { // dangling pointer throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into())) - } else if ecx.tcx.alloc_map.lock().get(alloc_id).is_none() { + } else if let Some(_) = ecx.tcx.alloc_map.lock().get(alloc_id) { + // FIXME: check if the allocation is ok as per the interning rules as if we interned + // it right here. + } else { span_bug!(ecx.tcx.span, "encountered unknown alloc id {:?}", alloc_id); } } diff --git a/src/librustc_mir/interpret/mod.rs b/src/librustc_mir/interpret/mod.rs index 2e8fbb95ca2..a519f38e712 100644 --- a/src/librustc_mir/interpret/mod.rs +++ b/src/librustc_mir/interpret/mod.rs @@ -32,6 +32,6 @@ pub use self::visitor::{MutValueVisitor, ValueVisitor}; pub use self::validity::RefTracking; -pub use self::intern::intern_const_alloc_recursive; +pub use self::intern::{intern_const_alloc_recursive, InternKind}; crate use self::intrinsics::eval_nullary_intrinsic; diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 1d5a643484a..7dd82e6a6f9 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -29,9 +29,9 @@ use syntax::ast::Mutability; use crate::const_eval::error_to_const_error; use crate::interpret::{ - self, intern_const_alloc_recursive, AllocId, Allocation, Frame, ImmTy, Immediate, InterpCx, - LocalState, LocalValue, Memory, MemoryKind, OpTy, Operand as InterpOperand, PlaceTy, Pointer, - ScalarMaybeUndef, StackPopCleanup, + self, intern_const_alloc_recursive, AllocId, Allocation, Frame, ImmTy, Immediate, InternKind, + InterpCx, LocalState, LocalValue, Memory, MemoryKind, OpTy, Operand as InterpOperand, PlaceTy, + Pointer, ScalarMaybeUndef, StackPopCleanup, }; use crate::transform::{MirPass, MirSource}; @@ -726,7 +726,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { )) => l.is_bits() && r.is_bits(), interpret::Operand::Indirect(_) if mir_opt_level >= 2 => { let mplace = op.assert_mem_place(&self.ecx); - intern_const_alloc_recursive(&mut self.ecx, None, mplace, false) + intern_const_alloc_recursive(&mut self.ecx, InternKind::ConstProp, mplace, false) .expect("failed to intern alloc"); true } diff --git a/src/test/ui/consts/raw_pointer_promoted.rs b/src/test/ui/consts/raw_pointer_promoted.rs new file mode 100644 index 00000000000..4c62ad444a5 --- /dev/null +++ b/src/test/ui/consts/raw_pointer_promoted.rs @@ -0,0 +1,5 @@ +// check-pass + +pub const FOO: &'static *const i32 = &(&0 as _); + +fn main() {} From 658c27b011b88dff18fd0137559123991a05f380 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 26 Dec 2019 00:30:02 +0100 Subject: [PATCH 2/8] Elaborate on comments --- src/librustc_mir/interpret/intern.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 8ff78ffb5ba..4c7dab338ab 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -351,6 +351,9 @@ pub fn intern_const_alloc_recursive>( InternKind::Static(hir::Mutability::Mut) => {} // Once we get heap allocations we need to revisit whether immutable statics can // refer to mutable (e.g. via interior mutability) allocations. + // Note: this is never the base value of the static, we can only get here for + // pointers encountered inside the base allocation, and then only for ones not at + // reference type, as that is checked by the type based main interner. InternKind::Static(hir::Mutability::Not) => { alloc.mutability = Mutability::Not; } @@ -385,6 +388,17 @@ pub fn intern_const_alloc_recursive>( // dangling pointer throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into())) } else if let Some(_) = ecx.tcx.alloc_map.lock().get(alloc_id) { + // If we encounter an `AllocId` that points to a mutable `Allocation`, + // (directly or via relocations in its `Allocation`), we should panic, + // the static rules should prevent this. + // We may hit an `AllocId` that belongs to an already interned static, + // and are thus not interning any further. + // But since we are also checking things during interning, + // we should probably continue doing those checks no matter what we encounter. + + // E.g. this should be unreachable for `InternKind::Promoted` except for allocations + // created for string and byte string literals. + // FIXME: check if the allocation is ok as per the interning rules as if we interned // it right here. } else { From 1ea44663c562ac40fbb81f59698e3861d622695b Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 8 Jan 2020 10:16:47 +0100 Subject: [PATCH 3/8] Elaborate on the details in some comments --- src/librustc_mir/interpret/intern.rs | 29 +++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 4c7dab338ab..6008834a573 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -361,6 +361,9 @@ pub fn intern_const_alloc_recursive>( // everything as immutable. Creating a promoted with interior mutability is UB, but // there's no way we can check whether the user is using raw pointers correctly. // So all we can do is mark this as immutable here. + // It is UB to mutate through a raw pointer obtained via an immutable reference. + // Since all references and pointers inside a promoted must by their very definition + // be created from an immutable reference, mutating though them would be UB. InternKind::Promoted => { alloc.mutability = Mutability::Not; } @@ -388,19 +391,27 @@ pub fn intern_const_alloc_recursive>( // dangling pointer throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into())) } else if let Some(_) = ecx.tcx.alloc_map.lock().get(alloc_id) { - // If we encounter an `AllocId` that points to a mutable `Allocation`, - // (directly or via relocations in its `Allocation`), we should panic, - // the static rules should prevent this. - // We may hit an `AllocId` that belongs to an already interned static, + // We have hit an `AllocId` that belongs to an already interned static, // and are thus not interning any further. - // But since we are also checking things during interning, - // we should probably continue doing those checks no matter what we encounter. // E.g. this should be unreachable for `InternKind::Promoted` except for allocations - // created for string and byte string literals. + // created for string and byte string literals, since these are interned immediately + // at creation time. - // FIXME: check if the allocation is ok as per the interning rules as if we interned - // it right here. + // FIXME(oli-obk): Since we are also checking things during interning, + // we should probably continue doing those checks no matter what we encounter. + // So we basically have to check if the allocation is ok as per the interning rules as + // if we interned it right here. + // This should be as simple as + /* + for &(_, ((), reloc)) in alloc.relocations().iter() { + if leftover_allocations.insert(reloc) { + todo.push(reloc); + } + } + */ + // But I (oli-obk) haven't thought about the ramnificatons yet. This also would cause + // compile-time regressions, so we should think about caching these. } else { span_bug!(ecx.tcx.span, "encountered unknown alloc id {:?}", alloc_id); } From eadfd63e3fc136aee68fa32e98ea91213ef2e075 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 9 Jan 2020 17:14:54 +0100 Subject: [PATCH 4/8] Clean up comment --- src/librustc_mir/interpret/intern.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 6008834a573..0cafd7cbbc0 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -358,12 +358,12 @@ pub fn intern_const_alloc_recursive>( alloc.mutability = Mutability::Not; } // Raw pointers in promoteds may only point to immutable things so we mark - // everything as immutable. Creating a promoted with interior mutability is UB, but - // there's no way we can check whether the user is using raw pointers correctly. - // So all we can do is mark this as immutable here. + // everything as immutable. // It is UB to mutate through a raw pointer obtained via an immutable reference. // Since all references and pointers inside a promoted must by their very definition // be created from an immutable reference, mutating though them would be UB. + // There's no way we can check whether the user is using raw pointers correctly, + // so all we can do is mark this as immutable here. InternKind::Promoted => { alloc.mutability = Mutability::Not; } @@ -394,7 +394,7 @@ pub fn intern_const_alloc_recursive>( // We have hit an `AllocId` that belongs to an already interned static, // and are thus not interning any further. - // E.g. this should be unreachable for `InternKind::Promoted` except for allocations + // For `InternKind::Promoted` this is only reachable for allocations // created for string and byte string literals, since these are interned immediately // at creation time. From b2c43dc1adcf1d264b981a506b1840ba42eeea7f Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Sat, 11 Jan 2020 14:45:00 +0100 Subject: [PATCH 5/8] Update src/librustc_mir/interpret/intern.rs Co-Authored-By: Ralf Jung --- src/librustc_mir/interpret/intern.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 0cafd7cbbc0..6fd15cc79a2 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -361,7 +361,8 @@ pub fn intern_const_alloc_recursive>( // everything as immutable. // It is UB to mutate through a raw pointer obtained via an immutable reference. // Since all references and pointers inside a promoted must by their very definition - // be created from an immutable reference, mutating though them would be UB. + // be created from an immutable reference (and promotion also excludes interior + // mutability), mutating though them would be UB. // There's no way we can check whether the user is using raw pointers correctly, // so all we can do is mark this as immutable here. InternKind::Promoted => { From a81784a09a084e63add1e4618dd82e8a7035c4f5 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Sat, 11 Jan 2020 15:01:58 +0100 Subject: [PATCH 6/8] Undo a change not neceesary for this bugfix --- src/librustc_mir/interpret/intern.rs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 6fd15cc79a2..68ac945ca89 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -347,16 +347,10 @@ pub fn intern_const_alloc_recursive>( // references and a `leftover_allocations` set (where we only have a todo-list here). // So we hand-roll the interning logic here again. match intern_kind { - // Mutable statics may contain mutable allocations even behind relocations - InternKind::Static(hir::Mutability::Mut) => {} - // Once we get heap allocations we need to revisit whether immutable statics can - // refer to mutable (e.g. via interior mutability) allocations. - // Note: this is never the base value of the static, we can only get here for - // pointers encountered inside the base allocation, and then only for ones not at - // reference type, as that is checked by the type based main interner. - InternKind::Static(hir::Mutability::Not) => { - alloc.mutability = Mutability::Not; - } + // Statics may contain mutable allocations even behind relocations. + // Even for immutable statics it would be ok to have mutable allocations behind + // raw pointers, e.g. for `static FOO: *const AtomicUsize = &AtomicUsize::new(42)`. + InternKind::Static(_) => {} // Raw pointers in promoteds may only point to immutable things so we mark // everything as immutable. // It is UB to mutate through a raw pointer obtained via an immutable reference. From 7bd01ed3c46183253dd0c28d9efa8678c426599e Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 15 Jan 2020 11:33:49 +0100 Subject: [PATCH 7/8] Typo --- src/librustc_mir/interpret/intern.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index 68ac945ca89..cb028d286fa 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -356,7 +356,7 @@ pub fn intern_const_alloc_recursive>( // It is UB to mutate through a raw pointer obtained via an immutable reference. // Since all references and pointers inside a promoted must by their very definition // be created from an immutable reference (and promotion also excludes interior - // mutability), mutating though them would be UB. + // mutability), mutating through them would be UB. // There's no way we can check whether the user is using raw pointers correctly, // so all we can do is mark this as immutable here. InternKind::Promoted => { From 69ffe7bb1314ccfdbde419d242cf636413dfd5f6 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 15 Jan 2020 11:36:06 +0100 Subject: [PATCH 8/8] Address review comments --- src/librustc_mir/interpret/intern.rs | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/src/librustc_mir/interpret/intern.rs b/src/librustc_mir/interpret/intern.rs index cb028d286fa..0c65b77a382 100644 --- a/src/librustc_mir/interpret/intern.rs +++ b/src/librustc_mir/interpret/intern.rs @@ -385,29 +385,9 @@ pub fn intern_const_alloc_recursive>( } else if ecx.memory.dead_alloc_map.contains_key(&alloc_id) { // dangling pointer throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into())) - } else if let Some(_) = ecx.tcx.alloc_map.lock().get(alloc_id) { - // We have hit an `AllocId` that belongs to an already interned static, - // and are thus not interning any further. - - // For `InternKind::Promoted` this is only reachable for allocations - // created for string and byte string literals, since these are interned immediately - // at creation time. - - // FIXME(oli-obk): Since we are also checking things during interning, - // we should probably continue doing those checks no matter what we encounter. - // So we basically have to check if the allocation is ok as per the interning rules as - // if we interned it right here. - // This should be as simple as - /* - for &(_, ((), reloc)) in alloc.relocations().iter() { - if leftover_allocations.insert(reloc) { - todo.push(reloc); - } - } - */ - // But I (oli-obk) haven't thought about the ramnificatons yet. This also would cause - // compile-time regressions, so we should think about caching these. - } else { + } else if ecx.tcx.alloc_map.lock().get(alloc_id).is_none() { + // We have hit an `AllocId` that is neither in local or global memory and isn't marked + // as dangling by local memory. span_bug!(ecx.tcx.span, "encountered unknown alloc id {:?}", alloc_id); } }