From e590b849b83dd97fe98a39971cd91b692a0cf2a8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 28 Jul 2019 13:44:11 +0200 Subject: [PATCH 1/5] CTFE: simplify Value type by not checking for alignment --- src/librustc/mir/interpret/value.rs | 13 ++------- src/librustc/ty/structural_impls.rs | 4 +-- src/librustc_codegen_llvm/common.rs | 4 +-- src/librustc_codegen_llvm/consts.rs | 4 +-- src/librustc_codegen_ssa/mir/operand.rs | 4 +-- src/librustc_codegen_ssa/mir/place.rs | 4 +-- src/librustc_codegen_ssa/traits/consts.rs | 1 - src/librustc_mir/const_eval.rs | 11 +++++--- src/librustc_mir/hair/pattern/_match.rs | 4 +-- src/librustc_mir/interpret/machine.rs | 3 ++ src/librustc_mir/interpret/memory.rs | 31 ++++++++++++--------- src/librustc_mir/interpret/operand.rs | 4 +-- src/test/ui/consts/const-eval/ub-ref.rs | 4 +-- src/test/ui/consts/const-eval/ub-ref.stderr | 18 ++++-------- 14 files changed, 50 insertions(+), 59 deletions(-) diff --git a/src/librustc/mir/interpret/value.rs b/src/librustc/mir/interpret/value.rs index 607bcea7fe801..5381d46972440 100644 --- a/src/librustc/mir/interpret/value.rs +++ b/src/librustc/mir/interpret/value.rs @@ -2,7 +2,7 @@ use std::fmt; use rustc_macros::HashStable; use rustc_apfloat::{Float, ieee::{Double, Single}}; -use crate::ty::{Ty, InferConst, ParamConst, layout::{HasDataLayout, Size, Align}, subst::SubstsRef}; +use crate::ty::{Ty, InferConst, ParamConst, layout::{HasDataLayout, Size}, subst::SubstsRef}; use crate::ty::PlaceholderConst; use crate::hir::def_id::DefId; @@ -45,18 +45,11 @@ pub enum ConstValue<'tcx> { /// A value not represented/representable by `Scalar` or `Slice` ByRef { - /// The alignment exists to allow `const_field` to have `ByRef` access to nonprimitive - /// fields of `repr(packed)` structs. The alignment may be lower than the type of this - /// constant. This permits reads with lower alignment than what the type would normally - /// require. - /// FIXME(RalfJ,oli-obk): The alignment checks are part of miri, but const eval doesn't - /// really need them. Disabling them may be too hard though. - align: Align, - /// Offset into `alloc` - offset: Size, /// The backing memory of the value, may contain more memory than needed for just the value /// in order to share `Allocation`s between values alloc: &'tcx Allocation, + /// Offset into `alloc` + offset: Size, }, /// Used in the HIR by using `Unevaluated` everywhere and later normalizing to one of the other diff --git a/src/librustc/ty/structural_impls.rs b/src/librustc/ty/structural_impls.rs index 28b52dcea80f1..649a5244728ba 100644 --- a/src/librustc/ty/structural_impls.rs +++ b/src/librustc/ty/structural_impls.rs @@ -1367,8 +1367,8 @@ impl<'tcx> TypeFoldable<'tcx> for &'tcx ty::Const<'tcx> { impl<'tcx> TypeFoldable<'tcx> for ConstValue<'tcx> { fn super_fold_with>(&self, folder: &mut F) -> Self { match *self { - ConstValue::ByRef { offset, align, alloc } => - ConstValue::ByRef { offset, align, alloc }, + ConstValue::ByRef { alloc, offset } => + ConstValue::ByRef { alloc, offset }, ConstValue::Infer(ic) => ConstValue::Infer(ic.fold_with(folder)), ConstValue::Param(p) => ConstValue::Param(p.fold_with(folder)), ConstValue::Placeholder(p) => ConstValue::Placeholder(p), diff --git a/src/librustc_codegen_llvm/common.rs b/src/librustc_codegen_llvm/common.rs index f00624f3811f1..19b051a4f38e0 100644 --- a/src/librustc_codegen_llvm/common.rs +++ b/src/librustc_codegen_llvm/common.rs @@ -11,7 +11,7 @@ use crate::value::Value; use rustc_codegen_ssa::traits::*; use crate::consts::const_alloc_to_llvm; -use rustc::ty::layout::{HasDataLayout, LayoutOf, self, TyLayout, Size, Align}; +use rustc::ty::layout::{HasDataLayout, LayoutOf, self, TyLayout, Size}; use rustc::mir::interpret::{Scalar, GlobalAlloc, Allocation}; use rustc_codegen_ssa::mir::place::PlaceRef; @@ -329,10 +329,10 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> { fn from_const_alloc( &self, layout: TyLayout<'tcx>, - align: Align, alloc: &Allocation, offset: Size, ) -> PlaceRef<'tcx, &'ll Value> { + let align = alloc.align; // follow what CTFE did, not what the layout says let init = const_alloc_to_llvm(self, alloc); let base_addr = self.static_addr_of(init, align, None); diff --git a/src/librustc_codegen_llvm/consts.rs b/src/librustc_codegen_llvm/consts.rs index e02d14a151e62..0077df3cf5eea 100644 --- a/src/librustc_codegen_llvm/consts.rs +++ b/src/librustc_codegen_llvm/consts.rs @@ -72,8 +72,8 @@ pub fn codegen_static_initializer( let alloc = match static_.val { ConstValue::ByRef { - offset, align, alloc, - } if offset.bytes() == 0 && align == alloc.align => { + alloc, offset, + } if offset.bytes() == 0 => { alloc }, _ => bug!("static const eval returned {:#?}", static_), diff --git a/src/librustc_codegen_ssa/mir/operand.rs b/src/librustc_codegen_ssa/mir/operand.rs index 302dcfcc682a3..5e5804b72657b 100644 --- a/src/librustc_codegen_ssa/mir/operand.rs +++ b/src/librustc_codegen_ssa/mir/operand.rs @@ -109,8 +109,8 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> { let b_llval = bx.const_usize((end - start) as u64); OperandValue::Pair(a_llval, b_llval) }, - ConstValue::ByRef { offset, align, alloc } => { - return bx.load_operand(bx.from_const_alloc(layout, align, alloc, offset)); + ConstValue::ByRef { alloc, offset } => { + return bx.load_operand(bx.from_const_alloc(layout, alloc, offset)); }, }; diff --git a/src/librustc_codegen_ssa/mir/place.rs b/src/librustc_codegen_ssa/mir/place.rs index b38e58baaf6a4..a632838ba2442 100644 --- a/src/librustc_codegen_ssa/mir/place.rs +++ b/src/librustc_codegen_ssa/mir/place.rs @@ -466,8 +466,8 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { let layout = cx.layout_of(self.monomorphize(&ty)); match bx.tcx().const_eval(param_env.and(cid)) { Ok(val) => match val.val { - mir::interpret::ConstValue::ByRef { offset, align, alloc } => { - bx.cx().from_const_alloc(layout, align, alloc, offset) + mir::interpret::ConstValue::ByRef { alloc, offset } => { + bx.cx().from_const_alloc(layout, alloc, offset) } _ => bug!("promoteds should have an allocation: {:?}", val), }, diff --git a/src/librustc_codegen_ssa/traits/consts.rs b/src/librustc_codegen_ssa/traits/consts.rs index 248fadfaf0f27..e7ce03f183619 100644 --- a/src/librustc_codegen_ssa/traits/consts.rs +++ b/src/librustc_codegen_ssa/traits/consts.rs @@ -35,7 +35,6 @@ pub trait ConstMethods<'tcx>: BackendTypes { fn from_const_alloc( &self, layout: layout::TyLayout<'tcx>, - align: layout::Align, alloc: &Allocation, offset: layout::Size, ) -> PlaceRef<'tcx, Self::Value>; diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 1b92e4992ffb1..b94294578500f 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -98,7 +98,7 @@ fn op_to_const<'tcx>( Ok(mplace) => { let ptr = mplace.ptr.to_ptr().unwrap(); let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id); - ConstValue::ByRef { offset: ptr.offset, align: mplace.align, alloc } + ConstValue::ByRef { alloc, offset: ptr.offset } }, // see comment on `let try_as_immediate` above Err(ImmTy { imm: Immediate::Scalar(x), .. }) => match x { @@ -112,7 +112,7 @@ fn op_to_const<'tcx>( let mplace = op.assert_mem_place(); let ptr = mplace.ptr.to_ptr().unwrap(); let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id); - ConstValue::ByRef { offset: ptr.offset, align: mplace.align, alloc } + ConstValue::ByRef { alloc, offset: ptr.offset } }, }, Err(ImmTy { imm: Immediate::ScalarPair(a, b), .. }) => { @@ -326,6 +326,10 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir, const STATIC_KIND: Option = None; // no copying of statics allowed + // We do not check for alignment to avoid having to carry an `Align` + // in `ConstValue::ByRef`. + const CHECK_ALIGN: bool = false; + #[inline(always)] fn enforce_validity(_ecx: &InterpCx<'mir, 'tcx, Self>) -> bool { false // for now, we don't enforce validity @@ -552,9 +556,8 @@ fn validate_and_turn_into_const<'tcx>( let ptr = mplace.ptr.to_ptr()?; Ok(tcx.mk_const(ty::Const { val: ConstValue::ByRef { - offset: ptr.offset, - align: mplace.align, alloc: ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id), + offset: ptr.offset, }, ty: mplace.layout.ty, })) diff --git a/src/librustc_mir/hair/pattern/_match.rs b/src/librustc_mir/hair/pattern/_match.rs index 567bac777d265..b1a317ee65f6f 100644 --- a/src/librustc_mir/hair/pattern/_match.rs +++ b/src/librustc_mir/hair/pattern/_match.rs @@ -218,10 +218,8 @@ impl LiteralExpander<'tcx> { (ConstValue::Scalar(Scalar::Ptr(p)), x, y) if x == y => { let alloc = self.tcx.alloc_map.lock().unwrap_memory(p.alloc_id); ConstValue::ByRef { - offset: p.offset, - // FIXME(oli-obk): this should be the type's layout - align: alloc.align, alloc, + offset: p.offset, } }, // unsize array to slice if pattern is array but match value or other patterns are slice diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 78902b1016634..c6686fafd94ee 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -109,6 +109,9 @@ pub trait Machine<'mir, 'tcx>: Sized { /// that is added to the memory so that the work is not done twice. const STATIC_KIND: Option; + /// Whether memory accesses should be alignment-checked. + const CHECK_ALIGN: bool; + /// Whether to enforce the validity invariant fn enforce_validity(ecx: &InterpCx<'mir, 'tcx, Self>) -> bool; diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index a1574cd5935e9..fcefe1abd7063 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -338,11 +338,14 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { Ok(bits) => { let bits = bits as u64; // it's ptr-sized assert!(size.bytes() == 0); - // Must be non-NULL and aligned. + // Must be non-NULL. if bits == 0 { throw_unsup!(InvalidNullPointerUsage) } - check_offset_align(bits, align)?; + // Must be aligned. + if M::CHECK_ALIGN { + check_offset_align(bits, align)?; + } None } Err(ptr) => { @@ -355,18 +358,20 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { end_ptr.check_in_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?; // Test align. Check this last; if both bounds and alignment are violated // we want the error to be about the bounds. - if alloc_align.bytes() < align.bytes() { - // The allocation itself is not aligned enough. - // FIXME: Alignment check is too strict, depending on the base address that - // got picked we might be aligned even if this check fails. - // We instead have to fall back to converting to an integer and checking - // the "real" alignment. - throw_unsup!(AlignmentCheckFailed { - has: alloc_align, - required: align, - }) + if M::CHECK_ALIGN { + if alloc_align.bytes() < align.bytes() { + // The allocation itself is not aligned enough. + // FIXME: Alignment check is too strict, depending on the base address that + // got picked we might be aligned even if this check fails. + // We instead have to fall back to converting to an integer and checking + // the "real" alignment. + throw_unsup!(AlignmentCheckFailed { + has: alloc_align, + required: align, + }); + } + check_offset_align(ptr.offset.bytes(), align)?; } - check_offset_align(ptr.offset.bytes(), align)?; // We can still be zero-sized in this branch, in which case we have to // return `None`. diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index e64a474b4ca71..18a70ae260744 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -548,12 +548,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { self.layout_of(self.monomorphize(val.ty)?) })?; let op = match val.val { - ConstValue::ByRef { offset, align, alloc } => { + ConstValue::ByRef { alloc, offset } => { let id = self.tcx.alloc_map.lock().create_memory_alloc(alloc); // We rely on mutability being set correctly in that allocation to prevent writes // where none should happen. let ptr = self.tag_static_base_pointer(Pointer::new(id, offset)); - Operand::Indirect(MemPlace::from_ptr(ptr, align)) + Operand::Indirect(MemPlace::from_ptr(ptr, layout.align.abi)) }, ConstValue::Scalar(x) => Operand::Immediate(Immediate::Scalar(tag_scalar(x).into())), diff --git a/src/test/ui/consts/const-eval/ub-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs index 0d8f30159b316..accc520a64f0f 100644 --- a/src/test/ui/consts/const-eval/ub-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -4,9 +4,7 @@ use std::mem; -const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; -//~^ ERROR it is undefined behavior to use this value -//~^^ type validation failed: encountered unaligned reference (required 2 byte alignment but found 1) +const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; // Ok (CTFE does not check alignment) const NULL: &u16 = unsafe { mem::transmute(0usize) }; //~^ ERROR it is undefined behavior to use this value diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr index f1702955ed7b1..e6d45ca8c9de4 100644 --- a/src/test/ui/consts/const-eval/ub-ref.stderr +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -1,13 +1,5 @@ error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:7:1 - | -LL | const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered unaligned reference (required 2 byte alignment but found 1) - | - = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior - -error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:11:1 + --> $DIR/ub-ref.rs:9:1 | LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 @@ -15,7 +7,7 @@ LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:14:1 + --> $DIR/ub-ref.rs:12:1 | LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes @@ -23,7 +15,7 @@ LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:17:1 + --> $DIR/ub-ref.rs:15:1 | LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at ., but expected plain (non-pointer) bytes @@ -31,13 +23,13 @@ LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:20:1 + --> $DIR/ub-ref.rs:18:1 | LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (created from integer) | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors For more information about this error, try `rustc --explain E0080`. From a2152d257f75e9d46a6e292bcf0b87773c6c40a2 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 28 Jul 2019 14:01:27 +0200 Subject: [PATCH 2/5] tidy is reaching new levels every day... --- src/test/ui/consts/const-eval/ub-ref.rs | 1 - src/test/ui/consts/const-eval/ub-ref.stderr | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/ui/consts/const-eval/ub-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs index accc520a64f0f..7d2ac37f4e237 100644 --- a/src/test/ui/consts/const-eval/ub-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -1,4 +1,3 @@ -// ignore-tidy-linelength #![feature(const_transmute)] #![allow(const_err)] // make sure we cannot allow away the errors tested here diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr index e6d45ca8c9de4..d6a82517901fa 100644 --- a/src/test/ui/consts/const-eval/ub-ref.stderr +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -1,5 +1,5 @@ error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:9:1 + --> $DIR/ub-ref.rs:8:1 | LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 @@ -7,7 +7,7 @@ LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:12:1 + --> $DIR/ub-ref.rs:11:1 | LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes @@ -15,7 +15,7 @@ LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:15:1 + --> $DIR/ub-ref.rs:14:1 | LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at ., but expected plain (non-pointer) bytes @@ -23,7 +23,7 @@ LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:18:1 + --> $DIR/ub-ref.rs:17:1 | LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (created from integer) From 30c63aa2b8fbc870f32c737d494b6b1ef5f2b912 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 28 Jul 2019 18:17:58 +0200 Subject: [PATCH 3/5] assert consistency --- src/librustc_codegen_llvm/common.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_codegen_llvm/common.rs b/src/librustc_codegen_llvm/common.rs index 19b051a4f38e0..b0c94a139be08 100644 --- a/src/librustc_codegen_llvm/common.rs +++ b/src/librustc_codegen_llvm/common.rs @@ -332,9 +332,9 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> { alloc: &Allocation, offset: Size, ) -> PlaceRef<'tcx, &'ll Value> { - let align = alloc.align; // follow what CTFE did, not what the layout says + assert_eq!(alloc.align, layout.align.abi); let init = const_alloc_to_llvm(self, alloc); - let base_addr = self.static_addr_of(init, align, None); + let base_addr = self.static_addr_of(init, alloc.align, None); let llval = unsafe { llvm::LLVMConstInBoundsGEP( self.const_bitcast(base_addr, self.type_i8p()), @@ -342,7 +342,7 @@ impl ConstMethods<'tcx> for CodegenCx<'ll, 'tcx> { 1, )}; let llval = self.const_bitcast(llval, self.type_ptr_to(layout.llvm_type(self))); - PlaceRef::new_sized(llval, layout, align) + PlaceRef::new_sized(llval, layout, alloc.align) } fn const_ptrcast(&self, val: &'ll Value, ty: &'ll Type) -> &'ll Value { From 0de6873a319d86dc41b56d8339f75526473bac5f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 29 Jul 2019 15:27:01 +0200 Subject: [PATCH 4/5] miri: validity checks alignment even when machine otherwise does not --- src/librustc_mir/interpret/memory.rs | 17 +++++++++++++++-- src/librustc_mir/interpret/validity.rs | 4 +++- src/test/ui/consts/const-eval/ub-ref.rs | 3 ++- src/test/ui/consts/const-eval/ub-ref.stderr | 18 +++++++++++++----- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index fcefe1abd7063..eb98d07f901ba 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -306,11 +306,24 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { /// /// Most of the time you should use `check_mplace_access`, but when you just have a pointer, /// this method is still appropriate. + #[inline(always)] pub fn check_ptr_access( &self, sptr: Scalar, size: Size, align: Align, + ) -> InterpResult<'tcx, Option>> { + let align = if M::CHECK_ALIGN { Some(align) } else { None }; + self.check_ptr_access_align(sptr, size, align) + } + + /// Like `check_ptr_access`, but *definitely* checks alignment when `align` + /// is `Some` (overriding `M::CHECK_ALIGN`). + pub(super) fn check_ptr_access_align( + &self, + sptr: Scalar, + size: Size, + align: Option, ) -> InterpResult<'tcx, Option>> { fn check_offset_align(offset: u64, align: Align) -> InterpResult<'static> { if offset % align.bytes() == 0 { @@ -343,7 +356,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { throw_unsup!(InvalidNullPointerUsage) } // Must be aligned. - if M::CHECK_ALIGN { + if let Some(align) = align { check_offset_align(bits, align)?; } None @@ -358,7 +371,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> { end_ptr.check_in_alloc(allocation_size, CheckInAllocMsg::MemoryAccessTest)?; // Test align. Check this last; if both bounds and alignment are violated // we want the error to be about the bounds. - if M::CHECK_ALIGN { + if let Some(align) = align { if alloc_align.bytes() < align.bytes() { // The allocation itself is not aligned enough. // FIXME: Alignment check is too strict, depending on the base address that diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 072c9afc50ae0..82d6d7db01c8d 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -398,7 +398,9 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> // alignment and size determined by the layout (size will be 0, // alignment should take attributes into account). .unwrap_or_else(|| (layout.size, layout.align.abi)); - let ptr: Option<_> = match self.ecx.memory.check_ptr_access(ptr, size, align) { + let ptr: Option<_> = match + self.ecx.memory.check_ptr_access_align(ptr, size, Some(align)) + { Ok(ptr) => ptr, Err(err) => { info!( diff --git a/src/test/ui/consts/const-eval/ub-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs index 7d2ac37f4e237..3b19f3b07753e 100644 --- a/src/test/ui/consts/const-eval/ub-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -3,7 +3,8 @@ use std::mem; -const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; // Ok (CTFE does not check alignment) +const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; +//~^ ERROR it is undefined behavior to use this value const NULL: &u16 = unsafe { mem::transmute(0usize) }; //~^ ERROR it is undefined behavior to use this value diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr index d6a82517901fa..153c5381950f5 100644 --- a/src/test/ui/consts/const-eval/ub-ref.stderr +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -1,5 +1,13 @@ error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:8:1 + --> $DIR/ub-ref.rs:6:1 + | +LL | const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered unaligned reference (required 2 byte alignment but found 1) + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-ref.rs:9:1 | LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 @@ -7,7 +15,7 @@ LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:11:1 + --> $DIR/ub-ref.rs:12:1 | LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes @@ -15,7 +23,7 @@ LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:14:1 + --> $DIR/ub-ref.rs:15:1 | LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at ., but expected plain (non-pointer) bytes @@ -23,13 +31,13 @@ LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:17:1 + --> $DIR/ub-ref.rs:18:1 | LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (created from integer) | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors For more information about this error, try `rustc --explain E0080`. From 0cf43291d7007fa8edc31e0d7dda84acc453d86d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 29 Jul 2019 15:34:40 +0200 Subject: [PATCH 5/5] also test error messages --- src/test/ui/consts/const-eval/ub-ref.rs | 2 ++ src/test/ui/consts/const-eval/ub-ref.stderr | 10 +++++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/test/ui/consts/const-eval/ub-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs index 3b19f3b07753e..0d8f30159b316 100644 --- a/src/test/ui/consts/const-eval/ub-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -1,3 +1,4 @@ +// ignore-tidy-linelength #![feature(const_transmute)] #![allow(const_err)] // make sure we cannot allow away the errors tested here @@ -5,6 +6,7 @@ use std::mem; const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; //~^ ERROR it is undefined behavior to use this value +//~^^ type validation failed: encountered unaligned reference (required 2 byte alignment but found 1) const NULL: &u16 = unsafe { mem::transmute(0usize) }; //~^ ERROR it is undefined behavior to use this value diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr index 153c5381950f5..f1702955ed7b1 100644 --- a/src/test/ui/consts/const-eval/ub-ref.stderr +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -1,5 +1,5 @@ error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:6:1 + --> $DIR/ub-ref.rs:7:1 | LL | const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered unaligned reference (required 2 byte alignment but found 1) @@ -7,7 +7,7 @@ LL | const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:9:1 + --> $DIR/ub-ref.rs:11:1 | LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 @@ -15,7 +15,7 @@ LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:12:1 + --> $DIR/ub-ref.rs:14:1 | LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes @@ -23,7 +23,7 @@ LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:15:1 + --> $DIR/ub-ref.rs:17:1 | LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer at ., but expected plain (non-pointer) bytes @@ -31,7 +31,7 @@ LL | const REF_AS_USIZE_SLICE: &[usize] = &[unsafe { mem::transmute(&0) }]; = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: it is undefined behavior to use this value - --> $DIR/ub-ref.rs:18:1 + --> $DIR/ub-ref.rs:20:1 | LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (created from integer)