get rid of incorrect erase_for_fmt

RalfJung · RalfJung · commit 7c720ce6121d · 2021-07-16T10:09:56.000+02:00
diff --git a/compiler/rustc_middle/src/mir/interpret/allocation.rs b/compiler/rustc_middle/src/mir/interpret/allocation.rs
@@ -377,7 +377,9 @@ impl<Tag: Copy, Extra> Allocation<Tag, Extra> {
             }
         };
 
-        let (bytes, provenance) = match val.to_bits_or_ptr(range.size) {
+        // `to_bits_or_ptr_internal` is the right method because we just want to store this data
+        // as-is into memory.
+        let (bytes, provenance) = match val.to_bits_or_ptr_internal(range.size) {
             Err(val) => {
                 let (provenance, offset) = val.into_parts();
                 (u128::from(offset.bytes()), Some(provenance))
diff --git a/compiler/rustc_middle/src/mir/interpret/pointer.rs b/compiler/rustc_middle/src/mir/interpret/pointer.rs
@@ -89,16 +89,17 @@ impl<T: HasDataLayout> PointerArithmetic for T {}
 pub trait Provenance: Copy {
     /// Says whether the `offset` field of `Pointer`s with this provenance is the actual physical address.
     /// If `true, ptr-to-int casts work by simply discarding the provenance.
-    /// If `false`, ptr-to-int casts are not supported.
+    /// If `false`, ptr-to-int casts are not supported. The offset *must* be relative in that case.
     const OFFSET_IS_ADDR: bool;
 
     /// Determines how a pointer should be printed.
     fn fmt(ptr: &Pointer<Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result
     where
         Self: Sized;
 
-    /// "Erasing" a tag converts it to the default tag type if possible. Used only for formatting purposes!
-    fn erase_for_fmt(self) -> AllocId;
+    /// Provenance must always be able to identify the allocation this ptr points to.
+    /// (Identifying the offset in that allocation, however, is harder -- use `Memory::ptr_get_alloc` for that.)
+    fn get_alloc_id(self) -> AllocId;
 }
 
 impl Provenance for AllocId {
@@ -120,7 +121,7 @@ impl Provenance for AllocId {
         Ok(())
     }
 
-    fn erase_for_fmt(self) -> AllocId {
+    fn get_alloc_id(self) -> AllocId {
         self
     }
 }
@@ -177,14 +178,6 @@ impl<Tag> Pointer<Option<Tag>> {
             None => Err(self.offset),
         }
     }
-
-    #[inline(always)]
-    pub fn map_erase_for_fmt(self) -> Pointer<Option<AllocId>>
-    where
-        Tag: Provenance,
-    {
-        Pointer { offset: self.offset, provenance: self.provenance.map(Provenance::erase_for_fmt) }
-    }
 }
 
 impl<Tag> Pointer<Option<Tag>> {
@@ -208,15 +201,6 @@ impl<'tcx, Tag> Pointer<Tag> {
         (self.provenance, self.offset)
     }
 
-    #[inline(always)]
-    pub fn erase_for_fmt(self) -> Pointer
-    where
-        Tag: Provenance,
-    {
-        // FIXME: This is wrong! `self.offset` might be an absolute address.
-        Pointer { offset: self.offset, provenance: self.provenance.erase_for_fmt() }
-    }
-
     pub fn map_provenance(self, f: impl FnOnce(Tag) -> Tag) -> Self {
         Pointer { provenance: f(self.provenance), ..self }
     }
diff --git a/compiler/rustc_middle/src/mir/interpret/value.rs b/compiler/rustc_middle/src/mir/interpret/value.rs
@@ -289,9 +289,10 @@ impl<Tag> Scalar<Tag> {
     /// This is almost certainly not the method you want!  You should dispatch on the type
     /// and use `to_{u8,u16,...}`/`scalar_to_ptr` to perform ptr-to-int / int-to-ptr casts as needed.
     ///
-    /// This method only exists for the benefit of low-level memory operations.
+    /// This method only exists for the benefit of low-level operations that truly need to treat the
+    /// scalar in whatever form it is.
     #[inline]
-    pub fn to_bits_or_ptr(self, target_size: Size) -> Result<u128, Pointer<Tag>> {
+    pub fn to_bits_or_ptr_internal(self, target_size: Size) -> Result<u128, Pointer<Tag>> {
         assert_ne!(target_size.bytes(), 0, "you should never look at the bits of a ZST");
         match self {
             Scalar::Int(int) => Ok(int.assert_bits(target_size)),
@@ -304,32 +305,23 @@ impl<Tag> Scalar<Tag> {
 }
 
 impl<'tcx, Tag: Provenance> Scalar<Tag> {
-    /// Erase the tag from the scalar, if any.
-    ///
-    /// Used by error reporting code to avoid having the error type depend on `Tag`.
-    #[inline]
-    pub fn erase_for_fmt(self) -> Scalar {
-        match self {
-            Scalar::Ptr(ptr, sz) => Scalar::Ptr(ptr.erase_for_fmt(), sz),
-            Scalar::Int(int) => Scalar::Int(int),
-        }
-    }
-
     /// Fundamental scalar-to-int (cast) operation. Many convenience wrappers exist below, that you
     /// likely want to use instead.
     ///
     /// Will perform ptr-to-int casts if needed and possible.
+    /// If that fails, we know the offset is relative, so we return an "erased" Scalar
+    /// (which is useful for error messages but not much else).
     #[inline]
-    pub fn try_to_int(self) -> Option<ScalarInt> {
+    pub fn try_to_int(self) -> Result<ScalarInt, Scalar<AllocId>> {
         match self {
-            Scalar::Int(int) => Some(int),
+            Scalar::Int(int) => Ok(int),
             Scalar::Ptr(ptr, sz) => {
                 if Tag::OFFSET_IS_ADDR {
-                    Some(
-                        ScalarInt::try_from_uint(ptr.offset.bytes(), Size::from_bytes(sz)).unwrap(),
-                    )
+                    Ok(ScalarInt::try_from_uint(ptr.offset.bytes(), Size::from_bytes(sz)).unwrap())
                 } else {
-                    None
+                    // We know `offset` is relative, since `OFFSET_IS_ADDR == false`.
+                    let (tag, offset) = ptr.into_parts();
+                    Err(Scalar::Ptr(Pointer::new(tag.get_alloc_id(), offset), sz))
                 }
             }
         }
@@ -340,19 +332,20 @@ impl<'tcx, Tag: Provenance> Scalar<Tag> {
         self.try_to_int().unwrap()
     }
 
+    /// This throws UB (instead of ICEing) on a size mismatch since size mismatches can arise in
+    /// Miri when someone declares a function that we shim (such as `malloc`) with a wrong type.
     #[inline]
     pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> {
         assert_ne!(target_size.bytes(), 0, "you should never look at the bits of a ZST");
-        self.try_to_int()
-            .ok_or_else(|| err_unsup!(ReadPointerAsBytes))?
-            .to_bits(target_size)
-            .map_err(|size| {
+        self.try_to_int().map_err(|_| err_unsup!(ReadPointerAsBytes))?.to_bits(target_size).map_err(
+            |size| {
                 err_ub!(ScalarSizeMismatch {
                     target_size: target_size.bytes(),
                     data_size: size.bytes(),
                 })
                 .into()
-            })
+            },
+        )
     }
 
     #[inline(always)]
@@ -522,17 +515,6 @@ impl<Tag> ScalarMaybeUninit<Tag> {
 }
 
 impl<'tcx, Tag: Provenance> ScalarMaybeUninit<Tag> {
-    /// Erase the tag from the scalar, if any.
-    ///
-    /// Used by error reporting code to avoid having the error type depend on `Tag`.
-    #[inline]
-    pub fn erase_for_fmt(self) -> ScalarMaybeUninit {
-        match self {
-            ScalarMaybeUninit::Scalar(s) => ScalarMaybeUninit::Scalar(s.erase_for_fmt()),
-            ScalarMaybeUninit::Uninit => ScalarMaybeUninit::Uninit,
-        }
-    }
-
     #[inline(always)]
     pub fn to_bool(self) -> InterpResult<'tcx, bool> {
         self.check_init()?.to_bool()
diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs
@@ -1,5 +1,5 @@
 use crate::middle::cstore::{ExternCrate, ExternCrateSource};
-use crate::mir::interpret::{AllocRange, ConstValue, GlobalAlloc, Pointer, Scalar};
+use crate::mir::interpret::{AllocRange, ConstValue, GlobalAlloc, Pointer, Provenance, Scalar};
 use crate::ty::subst::{GenericArg, GenericArgKind, Subst};
 use crate::ty::{self, ConstInt, DefIdTree, ParamConst, ScalarInt, Ty, TyCtxt, TypeFoldable};
 use rustc_apfloat::ieee::{Double, Single};
@@ -1107,9 +1107,9 @@ pub trait PrettyPrinter<'tcx>:
 
     /// This is overridden for MIR printing because we only want to hide alloc ids from users, not
     /// from MIR where it is actually useful.
-    fn pretty_print_const_pointer(
+    fn pretty_print_const_pointer<Tag: Provenance>(
         mut self,
-        _: Pointer,
+        _: Pointer<Tag>,
         ty: Ty<'tcx>,
         print_ty: bool,
     ) -> Result<Self::Const, Self::Error> {
@@ -1680,9 +1680,9 @@ impl<F: fmt::Write> PrettyPrinter<'tcx> for FmtPrinter<'_, 'tcx, F> {
         }
     }
 
-    fn pretty_print_const_pointer(
+    fn pretty_print_const_pointer<Tag: Provenance>(
         self,
-        p: Pointer,
+        p: Pointer<Tag>,
         ty: Ty<'tcx>,
         print_ty: bool,
     ) -> Result<Self::Const, Self::Error> {
diff --git a/compiler/rustc_mir/src/interpret/eval_context.rs b/compiler/rustc_mir/src/interpret/eval_context.rs
@@ -897,11 +897,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     fn deallocate_local(&mut self, local: LocalValue<M::PointerTag>) -> InterpResult<'tcx> {
         if let LocalValue::Live(Operand::Indirect(MemPlace { ptr, .. })) = local {
             // All locals have a backing allocation, even if the allocation is empty
-            // due to the local having ZST type.
+            // due to the local having ZST type. Hence we can `unwrap`.
             trace!(
                 "deallocating local {:?}: {:?}",
                 local,
-                self.memory.dump_alloc(ptr.provenance.unwrap().erase_for_fmt())
+                self.memory.dump_alloc(ptr.provenance.unwrap().get_alloc_id())
             );
             self.memory.deallocate(ptr, None, MemoryKind::Stack)?;
         };
@@ -989,28 +989,28 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> std::fmt::Debug
                             },
                             mplace.ptr,
                         )?;
-                        allocs.extend(mplace.ptr.map_erase_for_fmt().provenance);
+                        allocs.extend(mplace.ptr.provenance.map(Provenance::get_alloc_id));
                     }
                     LocalValue::Live(Operand::Immediate(Immediate::Scalar(val))) => {
                         write!(fmt, " {:?}", val)?;
                         if let ScalarMaybeUninit::Scalar(Scalar::Ptr(ptr, _size)) = val {
-                            allocs.push(ptr.provenance.erase_for_fmt());
+                            allocs.push(ptr.provenance.get_alloc_id());
                         }
                     }
                     LocalValue::Live(Operand::Immediate(Immediate::ScalarPair(val1, val2))) => {
                         write!(fmt, " ({:?}, {:?})", val1, val2)?;
                         if let ScalarMaybeUninit::Scalar(Scalar::Ptr(ptr, _size)) = val1 {
-                            allocs.push(ptr.provenance.erase_for_fmt());
+                            allocs.push(ptr.provenance.get_alloc_id());
                         }
                         if let ScalarMaybeUninit::Scalar(Scalar::Ptr(ptr, _size)) = val2 {
-                            allocs.push(ptr.provenance.erase_for_fmt());
+                            allocs.push(ptr.provenance.get_alloc_id());
                         }
                     }
                 }
 
                 write!(fmt, ": {:?}", self.ecx.memory.dump_allocs(allocs))
             }
-            Place::Ptr(mplace) => match mplace.ptr.map_erase_for_fmt().provenance {
+            Place::Ptr(mplace) => match mplace.ptr.provenance.map(Provenance::get_alloc_id) {
                 Some(alloc_id) => write!(
                     fmt,
                     "by align({}) ref {:?}: {:?}",
diff --git a/compiler/rustc_mir/src/interpret/intrinsics.rs b/compiler/rustc_mir/src/interpret/intrinsics.rs
@@ -362,7 +362,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 //
                 // Control flow is weird because we cannot early-return (to reach the
                 // `go_to_block` at the end).
-                let done = if let (Some(a), Some(b)) = (a.try_to_int(), b.try_to_int()) {
+                let done = if let (Ok(a), Ok(b)) = (a.try_to_int(), b.try_to_int()) {
                     let a = a.try_to_machine_usize(*self.tcx).unwrap();
                     let b = b.try_to_machine_usize(*self.tcx).unwrap();
                     if a == b && a != 0 {
diff --git a/compiler/rustc_mir/src/interpret/memory.rs b/compiler/rustc_mir/src/interpret/memory.rs
@@ -757,12 +757,12 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         ptr: Pointer<Option<M::PointerTag>>,
     ) -> InterpResult<'tcx, FnVal<'tcx, M::ExtraFnVal>> {
         trace!("get_fn({:?})", ptr);
-        let (alloc_id, offset, ptr) = self.ptr_get_alloc(ptr)?;
+        let (alloc_id, offset, _ptr) = self.ptr_get_alloc(ptr)?;
         if offset.bytes() != 0 {
-            throw_ub!(InvalidFunctionPointer(ptr.erase_for_fmt()))
+            throw_ub!(InvalidFunctionPointer(Pointer::new(alloc_id, offset)))
         }
         self.get_fn_alloc(alloc_id)
-            .ok_or_else(|| err_ub!(InvalidFunctionPointer(ptr.erase_for_fmt())).into())
+            .ok_or_else(|| err_ub!(InvalidFunctionPointer(Pointer::new(alloc_id, offset))).into())
     }
 
     pub fn mark_immutable(&mut self, id: AllocId) -> InterpResult<'tcx> {
@@ -801,7 +801,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                 if reachable.insert(id) {
                     // This is a new allocation, add its relocations to `todo`.
                     if let Some((_, alloc)) = self.alloc_map.get(id) {
-                        todo.extend(alloc.relocations().values().map(|tag| tag.erase_for_fmt()));
+                        todo.extend(alloc.relocations().values().map(|tag| tag.get_alloc_id()));
                     }
                 }
             }
@@ -841,7 +841,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> std::fmt::Debug for DumpAllocs<'a,
             allocs_to_print: &mut VecDeque<AllocId>,
             alloc: &Allocation<Tag, Extra>,
         ) -> std::fmt::Result {
-            for alloc_id in alloc.relocations().values().map(|tag| tag.erase_for_fmt()) {
+            for alloc_id in alloc.relocations().values().map(|tag| tag.get_alloc_id()) {
                 allocs_to_print.push_back(alloc_id);
             }
             write!(fmt, "{}", pretty::display_allocation(tcx, alloc))
@@ -1129,7 +1129,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
 /// Machine pointer introspection.
 impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
     pub fn scalar_to_ptr(&self, scalar: Scalar<M::PointerTag>) -> Pointer<Option<M::PointerTag>> {
-        match scalar.to_bits_or_ptr(self.pointer_size()) {
+        // We use `to_bits_or_ptr_internal` since we are just implementing the method people need to
+        // call to force getting out a pointer.
+        match scalar.to_bits_or_ptr_internal(self.pointer_size()) {
             Err(ptr) => ptr.into(),
             Ok(bits) => {
                 let addr = u64::try_from(bits).unwrap();
diff --git a/compiler/rustc_mir/src/interpret/operand.rs b/compiler/rustc_mir/src/interpret/operand.rs
@@ -118,8 +118,14 @@ impl<Tag: Provenance> std::fmt::Display for ImmTy<'tcx, Tag> {
             ty: Ty<'tcx>,
         ) -> Result<FmtPrinter<'a, 'tcx, F>, std::fmt::Error> {
             match s {
-                ScalarMaybeUninit::Scalar(s) => {
-                    cx.pretty_print_const_scalar(s.erase_for_fmt(), ty, true)
+                ScalarMaybeUninit::Scalar(Scalar::Int(int)) => {
+                    cx.pretty_print_const_scalar_int(int, ty, true)
+                }
+                ScalarMaybeUninit::Scalar(Scalar::Ptr(ptr, _sz)) => {
+                    // Just print the ptr value. `pretty_print_const_scalar_ptr` would also try to
+                    // print what is points to, which would fail since it has no access to the local
+                    // memory.
+                    cx.pretty_print_const_pointer(ptr, ty, true)
                 }
                 ScalarMaybeUninit::Uninit => cx.typed_value(
                     |mut this| {
@@ -139,11 +145,11 @@ impl<Tag: Provenance> std::fmt::Display for ImmTy<'tcx, Tag> {
                         p(cx, s, ty)?;
                         return Ok(());
                     }
-                    write!(f, "{}: {}", s.erase_for_fmt(), self.layout.ty)
+                    write!(f, "{}: {}", s, self.layout.ty)
                 }
                 Immediate::ScalarPair(a, b) => {
                     // FIXME(oli-obk): at least print tuples and slices nicely
-                    write!(f, "({}, {}): {}", a.erase_for_fmt(), b.erase_for_fmt(), self.layout.ty,)
+                    write!(f, "({}, {}): {}", a, b, self.layout.ty,)
                 }
             }
         })
@@ -693,8 +699,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         Ok(match *tag_encoding {
             TagEncoding::Direct => {
                 let tag_bits = tag_val
-                    .to_bits(tag_layout.size)
-                    .map_err(|_| err_ub!(InvalidTag(tag_val.erase_for_fmt())))?;
+                    .try_to_int()
+                    .map_err(|dbg_val| err_ub!(InvalidTag(dbg_val)))?
+                    .assert_bits(tag_layout.size);
                 // Cast bits from tag layout to discriminant layout.
                 let discr_val = self.cast_from_scalar(tag_bits, tag_layout, discr_layout.ty);
                 let discr_bits = discr_val.assert_bits(discr_layout.size);
@@ -711,7 +718,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                     }
                     _ => span_bug!(self.cur_span(), "tagged layout for non-adt non-generator"),
                 }
-                .ok_or_else(|| err_ub!(InvalidTag(tag_val.erase_for_fmt())))?;
+                .ok_or_else(|| err_ub!(InvalidTag(Scalar::from_uint(tag_bits, tag_layout.size))))?;
                 // Return the cast value, and the index.
                 (discr_val, index.0)
             }
@@ -720,18 +727,23 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
                 // discriminant (encoded in niche/tag) and variant index are the same.
                 let variants_start = niche_variants.start().as_u32();
                 let variants_end = niche_variants.end().as_u32();
-                let variant = match tag_val.to_bits_or_ptr(tag_layout.size) {
-                    Err(ptr) => {
-                        // The niche must be just 0 (which an inbounds pointer value never is)
+                let variant = match tag_val.try_to_int() {
+                    Err(dbg_val) => {
+                        // So this is a pointer then, and casting to an int failed.
+                        // Can only happen during CTFE.
+                        let ptr = self.scalar_to_ptr(tag_val);
+                        // The niche must be just 0, and the ptr not null, then we know this is
+                        // okay. Everything else, we conservatively reject.
                         let ptr_valid = niche_start == 0
                             && variants_start == variants_end
-                            && !self.memory.ptr_may_be_null(ptr.into());
+                            && !self.memory.ptr_may_be_null(ptr);
                         if !ptr_valid {
-                            throw_ub!(InvalidTag(tag_val.erase_for_fmt()))
+                            throw_ub!(InvalidTag(dbg_val))
                         }
                         dataful_variant
                     }
                     Ok(tag_bits) => {
+                        let tag_bits = tag_bits.assert_bits(tag_layout.size);
                         // We need to use machine arithmetic to get the relative variant idx:
                         // variant_index_relative = tag_val - niche_start_val
                         let tag_val = ImmTy::from_uint(tag_bits, tag_layout);
diff --git a/compiler/rustc_mir/src/interpret/place.rs b/compiler/rustc_mir/src/interpret/place.rs
diff --git a/compiler/rustc_mir/src/interpret/validity.rs b/compiler/rustc_mir/src/interpret/validity.rs
diff --git a/compiler/rustc_mir/src/transform/const_prop.rs b/compiler/rustc_mir/src/transform/const_prop.rs
diff --git a/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs b/compiler/rustc_mir_build/src/thir/pattern/deconstruct_pat.rs
