Skip to content

Promoteds can contain raw pointers, but these must still only point to immutable allocations #67603

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

Merged
merged 8 commits into from
Jan 15, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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())),
Expand Down
13 changes: 9 additions & 4 deletions src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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,
)?;
Expand Down
44 changes: 34 additions & 10 deletions src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<M: CompileTimeMachine<'mir, 'tcx>>(
ecx: &mut InterpCx<'mir, 'tcx, M>,
// The `mutability` of the place, ignoring the type.
place_mut: Option<hir::Mutability>,
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.
Expand Down Expand Up @@ -338,10 +346,24 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
// 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 {
// 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.
// 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 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 => {
alloc.mutability = Mutability::Not;
}
InternKind::Constant | InternKind::ConstProp => {
Copy link
Member

Choose a reason for hiding this comment

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

Right, I was thinking about this branch here when asking about nested allocations in constants. Isn't this unreachable then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is reachable, but I'm entirely unclean on why it is reachable

const RAW_TRAIT_OBJ_CONTENT_INVALID: *const dyn Trait = &unsafe { BoolTransmute { val: 3 }.bl } as *const _;

hits the InternKind::Constant arm.

See the MIR on https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3bc6c8b165b614bbe6cf5566d30752de there's no StorageDead for the allocation containing the bool with bit pattern 3.

I'm not really sure what's going on there. Technically, since we're inside a constant, shouldn't this trigger promotion and promote the transmuted value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under unleash it's also reached in

const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;

but that triggers the delay_span_bug as expected.

Also under unleash, but without an ICE (just triggering the dynamic checks in interp):

const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;
//~^ WARN: skipping const checks

const MUTATING_BEHIND_RAW: () = {
    // Test that `MUTABLE_BEHIND_RAW` is actually immutable, by doing this at const time.
    unsafe {
        *MUTABLE_BEHIND_RAW = 99 //~ ERROR any use of this value will cause an error
    }
};

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure what's going on there. Technically, since we're inside a constant, shouldn't this trigger promotion and promote the transmuted value?

Ah, this must be the "not-promotion" also described in this document (Ctrl-F "looks like"). That makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

What I don't understand though is why RAW_TRAIT_OBJ_CONTENT_INVALID doesn't trigger the ICE. Why is the allocation already immutable?

Copy link
Member

Choose a reason for hiding this comment

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

(The PR doesn't change anything here so this doesn't block landing, but I'd really like to understand this and then see it documented.)

Copy link
Member

Choose a reason for hiding this comment

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

I would find documentation here immensely helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a holistic view of what happens, it would really help to write that down.

I don't, which is why my answers are so confusing. They are just a brain dump of me discovering what is going on, not me understanding it entirely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These issues are preexisting, so after this PR is merged I'll open a new one to write docs and actually figure out what is going on

Copy link
Member

Choose a reason for hiding this comment

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

I don't, which is why my answers are so confusing. They are just a brain dump of me discovering what is going on, not me understanding it entirely

Fair enough. :)

// 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.
Expand All @@ -364,6 +386,8 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
// dangling pointer
throw_unsup!(ValidationFailure("encountered dangling pointer in final constant".into()))
} 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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
8 changes: 4 additions & 4 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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
}
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/consts/raw_pointer_promoted.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// check-pass

pub const FOO: &'static *const i32 = &(&0 as _);

fn main() {}