Auto merge of #63079 - RalfJung:ctfe-no-align, r=oli-obk

bors · bors · commit 4be067558962 · 2019-08-05T08:16:51.000Z
CTFE: simplify ConstValue by not checking for alignment I hope the test suite actually covers the problematic cases here? r? @oli-obk Fixes #61952
diff --git 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
@@ -1367,8 +1367,8 @@ impl<'tcx> TypeFoldable<'tcx> for &'tcx ty::Const<'tcx> {
 impl<'tcx> TypeFoldable<'tcx> for ConstValue<'tcx> {
     fn super_fold_with<F: TypeFolder<'tcx>>(&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
@@ -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,20 +329,20 @@ 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> {
+        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()),
             &self.const_usize(offset.bytes()),
             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 {
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -561,9 +565,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
@@ -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
@@ -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<Self::MemoryKinds>;
 
+    /// 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
@@ -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<M::PointerTag>,
         size: Size,
         align: Align,
+    ) -> InterpResult<'tcx, Option<Pointer<M::PointerTag>>> {
+        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<M::PointerTag>,
+        size: Size,
+        align: Option<Align>,
     ) -> InterpResult<'tcx, Option<Pointer<M::PointerTag>>> {
         fn check_offset_align(offset: u64, align: Align) -> InterpResult<'static> {
             if offset % align.bytes() == 0 {
@@ -338,11 +351,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 let Some(align) = align {
+                    check_offset_align(bits, align)?;
+                }
                 None
             }
             Err(ptr) => {
@@ -355,18 +371,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 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
+                        // 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
@@ -557,12 +557,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(tag_scalar(x).into()),
diff --git 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!(
