make ptr_op finally reponsible for all ops involving pointers; make ValTy constructor private

RalfJung · RalfJung · commit 1d498d5a4311 · 2018-08-29T08:44:37.000+02:00
Also remove public OpTy constructors, but a pub(crate) constructor remains
diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs
@@ -288,21 +288,17 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeEvaluator {
         )
     }
 
-    fn try_ptr_op<'a>(
+    fn ptr_op<'a>(
         _ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
         _bin_op: mir::BinOp,
-        left: Scalar,
+        _left: Scalar,
         _left_layout: TyLayout<'tcx>,
-        right: Scalar,
+        _right: Scalar,
         _right_layout: TyLayout<'tcx>,
-    ) -> EvalResult<'tcx, Option<(Scalar, bool)>> {
-        if left.is_bits() && right.is_bits() {
-            Ok(None)
-        } else {
-            Err(
-                ConstEvalError::NeedsRfc("pointer arithmetic or comparison".to_string()).into(),
-            )
-        }
+    ) -> EvalResult<'tcx, (Scalar, bool)> {
+        Err(
+            ConstEvalError::NeedsRfc("pointer arithmetic or comparison".to_string()).into(),
+        )
     }
 
     fn find_foreign_static<'a>(
diff --git a/src/librustc_mir/interpret/cast.rs b/src/librustc_mir/interpret/cast.rs
@@ -48,13 +48,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
             Misc => {
                 let src = self.read_value(src)?;
                 if self.type_is_fat_ptr(src_layout.ty) {
-                    match (src.value, self.type_is_fat_ptr(dest.layout.ty)) {
+                    match (*src, self.type_is_fat_ptr(dest.layout.ty)) {
                         // pointers to extern types
                         (Value::Scalar(_),_) |
                         // slices and trait objects to other slices/trait objects
                         (Value::ScalarPair(..), true) => {
                             // No change to value
-                            self.write_value(src.value, dest)?;
+                            self.write_value(*src, dest)?;
                         }
                         // slices and trait objects to thin pointers (dropping the metadata)
                         (Value::ScalarPair(data, _), false) => {
diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs
@@ -69,20 +69,18 @@ pub trait Machine<'mir, 'tcx>: Clone + Eq + Hash {
         def_id: DefId,
     ) -> EvalResult<'tcx, &'tcx Allocation>;
 
-    /// Called for all binary operations except on float types.
-    ///
-    /// Returns `None` if the operation should be handled by the integer
-    /// op code in order to share more code between machines
+    /// Called for all binary operations on integer(-like) types when one operand is a pointer
+    /// value, and for the `Offset` operation that is inherently about pointers.
     ///
     /// Returns a (value, overflowed) pair if the operation succeeded
-    fn try_ptr_op<'a>(
+    fn ptr_op<'a>(
         ecx: &EvalContext<'a, 'mir, 'tcx, Self>,
         bin_op: mir::BinOp,
         left: Scalar,
         left_layout: TyLayout<'tcx>,
         right: Scalar,
         right_layout: TyLayout<'tcx>,
-    ) -> EvalResult<'tcx, Option<(Scalar, bool)>>;
+    ) -> EvalResult<'tcx, (Scalar, bool)>;
 
     /// Heap allocations via the `box` keyword
     ///
diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs
@@ -15,7 +15,7 @@ use std::hash::{Hash, Hasher};
 use std::convert::TryInto;
 
 use rustc::{mir, ty};
-use rustc::ty::layout::{self, Size, Align, LayoutOf, TyLayout, HasDataLayout, IntegerExt};
+use rustc::ty::layout::{self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt};
 use rustc_data_structures::indexed_vec::Idx;
 
 use rustc::mir::interpret::{
@@ -85,7 +85,7 @@ impl<'tcx> Value {
 // as input for binary and cast operations.
 #[derive(Copy, Clone, Debug)]
 pub struct ValTy<'tcx> {
-    pub value: Value,
+    value: Value,
     pub layout: TyLayout<'tcx>,
 }
 
@@ -107,16 +107,6 @@ pub enum Operand {
 }
 
 impl Operand {
-    #[inline]
-    pub fn from_ptr(ptr: Pointer, align: Align) -> Self {
-        Operand::Indirect(MemPlace::from_ptr(ptr, align))
-    }
-
-    #[inline]
-    pub fn from_scalar_value(val: Scalar) -> Self {
-        Operand::Immediate(Value::Scalar(val.into()))
-    }
-
     #[inline]
     pub fn to_mem_place(self) -> MemPlace {
         match self {
@@ -138,7 +128,7 @@ impl Operand {
 
 #[derive(Copy, Clone, Debug)]
 pub struct OpTy<'tcx> {
-    crate op: Operand, // ideally we'd make this private, but we are not there yet
+    crate op: Operand, // ideally we'd make this private, but const_prop needs this
     pub layout: TyLayout<'tcx>,
 }
 
@@ -184,23 +174,6 @@ impl<'tcx> PartialEq for OpTy<'tcx> {
 }
 impl<'tcx> Eq for OpTy<'tcx> {}
 
-impl<'tcx> OpTy<'tcx> {
-    #[inline]
-    pub fn from_ptr(ptr: Pointer, align: Align, layout: TyLayout<'tcx>) -> Self {
-        OpTy { op: Operand::from_ptr(ptr, align), layout }
-    }
-
-    #[inline]
-    pub fn from_aligned_ptr(ptr: Pointer, layout: TyLayout<'tcx>) -> Self {
-        OpTy { op: Operand::from_ptr(ptr, layout.align), layout }
-    }
-
-    #[inline]
-    pub fn from_scalar_value(val: Scalar, layout: TyLayout<'tcx>) -> Self {
-        OpTy { op: Operand::Immediate(Value::Scalar(val.into())), layout }
-    }
-}
-
 // Use the existing layout if given (but sanity check in debug mode),
 // or compute the layout.
 #[inline(always)]
@@ -507,7 +480,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
             ConstValue::ByRef(id, alloc, offset) => {
                 // We rely on mutability being set correctly in that allocation to prevent writes
                 // where none should happen -- and for `static mut`, we copy on demand anyway.
-                Ok(Operand::from_ptr(Pointer::new(id, offset), alloc.align))
+                Ok(Operand::Indirect(MemPlace::from_ptr(Pointer::new(id, offset), alloc.align)))
             },
             ConstValue::ScalarPair(a, b) =>
                 Ok(Operand::Immediate(Value::ScalarPair(a.into(), b))),
diff --git a/src/librustc_mir/interpret/operator.rs b/src/librustc_mir/interpret/operator.rs
@@ -28,7 +28,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         right: ValTy<'tcx>,
         dest: PlaceTy<'tcx>,
     ) -> EvalResult<'tcx> {
-        let (val, overflowed) = self.binary_op(op, left, right)?;
+        let (val, overflowed) = self.binary_op_val(op, left, right)?;
         let val = Value::ScalarPair(val.into(), Scalar::from_bool(overflowed).into());
         self.write_value(val, dest)
     }
@@ -42,7 +42,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         right: ValTy<'tcx>,
         dest: PlaceTy<'tcx>,
     ) -> EvalResult<'tcx> {
-        let (val, _overflowed) = self.binary_op(op, left, right)?;
+        let (val, _overflowed) = self.binary_op_val(op, left, right)?;
         self.write_scalar(val, dest)
     }
 }
@@ -282,16 +282,31 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
         Ok((val, false))
     }
 
+    /// Convenience wrapper that's useful when keeping the layout together with the
+    /// value.
+    #[inline]
+    pub fn binary_op_val(
+        &self,
+        bin_op: mir::BinOp,
+        left: ValTy<'tcx>,
+        right: ValTy<'tcx>,
+    ) -> EvalResult<'tcx, (Scalar, bool)> {
+        self.binary_op(
+            bin_op,
+            left.to_scalar()?, left.layout,
+            right.to_scalar()?, right.layout,
+        )
+    }
+
     /// Returns the result of the specified operation and whether it overflowed.
     pub fn binary_op(
         &self,
         bin_op: mir::BinOp,
-        ValTy { value: left, layout: left_layout }: ValTy<'tcx>,
-        ValTy { value: right, layout: right_layout }: ValTy<'tcx>,
+        left: Scalar,
+        left_layout: TyLayout<'tcx>,
+        right: Scalar,
+        right_layout: TyLayout<'tcx>,
     ) -> EvalResult<'tcx, (Scalar, bool)> {
-        let left = left.to_scalar()?;
-        let right = right.to_scalar()?;
-
         trace!("Running binary op {:?}: {:?} ({:?}), {:?} ({:?})",
             bin_op, left, left_layout.ty, right, right_layout.ty);
 
@@ -322,15 +337,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                     right_layout.ty.is_fn());
 
                 // Handle operations that support pointer values
-                if let Some(handled) =
-                    M::try_ptr_op(self, bin_op, left, left_layout, right, right_layout)?
-                {
-                    return Ok(handled);
+                if left.is_ptr() || right.is_ptr() || bin_op == mir::BinOp::Offset {
+                    return M::ptr_op(self, bin_op, left, left_layout, right, right_layout);
                 }
 
                 // Everything else only works with "proper" bits
-                let left = left.to_bits(left_layout.size)?;
-                let right = right.to_bits(right_layout.size)?;
+                let left = left.to_bits(left_layout.size).expect("we checked is_ptr");
+                let right = right.to_bits(right_layout.size).expect("we checked is_ptr");
                 self.binary_int_op(bin_op, left, left_layout, right, right_layout)
             }
         }
diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs
@@ -17,7 +17,7 @@ use rustc_target::spec::abi::Abi;
 
 use rustc::mir::interpret::{EvalResult, PointerArithmetic, EvalErrorKind, Scalar};
 use super::{
-    EvalContext, Machine, Value, OpTy, Place, PlaceTy, ValTy, Operand, StackPopCleanup
+    EvalContext, Machine, Value, OpTy, Place, PlaceTy, Operand, StackPopCleanup
 };
 
 impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
@@ -61,8 +61,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                     // Compare using binary_op, to also support pointer values
                     let const_int = Scalar::from_uint(const_int, discr.layout.size);
                     let (res, _) = self.binary_op(mir::BinOp::Eq,
-                        discr,
-                        ValTy { value: Value::Scalar(const_int.into()), layout: discr.layout }
+                        discr.to_scalar()?, discr.layout,
+                        const_int, discr.layout,
                     )?;
                     if res.to_bool()? {
                         target_block = targets[index];
diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs
@@ -22,7 +22,7 @@ use rustc::mir::interpret::{
 };
 use rustc::ty::{TyCtxt, self, Instance};
 use interpret::{EvalContext, CompileTimeEvaluator, eval_promoted, mk_borrowck_eval_cx};
-use interpret::{Value, OpTy, MemoryKind};
+use interpret::{self, Value, OpTy, MemoryKind};
 use transform::{MirPass, MirSource};
 use syntax::source_map::{Span, DUMMY_SP};
 use rustc::ty::subst::Substs;
@@ -358,13 +358,15 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> {
             Rvalue::Len(_) => None,
             Rvalue::NullaryOp(NullOp::SizeOf, ty) => {
                 type_size_of(self.tcx, self.param_env, ty).and_then(|n| Some((
-                    OpTy::from_scalar_value(
-                        Scalar::Bits {
-                            bits: n as u128,
-                            size: self.tcx.data_layout.pointer_size.bytes() as u8,
-                        },
-                        self.tcx.layout_of(self.param_env.and(self.tcx.types.usize)).ok()?,
-                    ),
+                    OpTy {
+                        op: interpret::Operand::Immediate(Value::Scalar(
+                            Scalar::Bits {
+                                bits: n as u128,
+                                size: self.tcx.data_layout.pointer_size.bytes() as u8,
+                            }.into()
+                        )),
+                        layout: self.tcx.layout_of(self.param_env.and(self.tcx.types.usize)).ok()?,
+                    },
                     span,
                 )))
             }
@@ -399,7 +401,11 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> {
                     // Now run the actual operation.
                     this.ecx.unary_op(op, prim, arg.layout)
                 })?;
-                Some((OpTy::from_scalar_value(val, place_layout), span))
+                let res = OpTy {
+                    op: interpret::Operand::Immediate(Value::Scalar(val.into())),
+                    layout: place_layout,
+                };
+                Some((res, span))
             }
             Rvalue::CheckedBinaryOp(op, ref left, ref right) |
             Rvalue::BinaryOp(op, ref left, ref right) => {
@@ -454,7 +460,7 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> {
                 })?;
                 trace!("const evaluating {:?} for {:?} and {:?}", op, left, right);
                 let (val, overflow) = self.use_ecx(source_info, |this| {
-                    this.ecx.binary_op(op, l, r)
+                    this.ecx.binary_op_val(op, l, r)
                 })?;
                 let val = if let Rvalue::CheckedBinaryOp(..) = *rvalue {
                     Value::ScalarPair(
@@ -470,7 +476,7 @@ impl<'b, 'a, 'tcx:'b> ConstPropagator<'b, 'a, 'tcx> {
                     Value::Scalar(val.into())
                 };
                 let res = OpTy {
-                    op: ::interpret::Operand::Immediate(val),
+                    op: interpret::Operand::Immediate(val),
                     layout: place_layout,
                 };
                 Some((res, span))
