CTFE: simplify Value type by not checking for alignment

Author: RalfJung

Diff for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: 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 for: src/test/ui/consts/const-eval/ub-ref.stderr

@@ -1,43 +1,35 @@
 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
    |
    = 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
    |
    = 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 .<deref>, but expected plain (non-pointer) bytes
    |
    = 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`.