Apply suggestions from code review

Co-authored-by: Ralf Jung <[email protected]>

Author: cjgillot

compiler/rustc_middle/src/mir/consts.rs

@@ -174,10 +174,13 @@ impl<'tcx> ConstValue<'tcx> {
     }
 
     /// Check if a constant may contain provenance information. This is used by MIR opts.
+    /// Can return `true` even if there is no provenance.
     pub fn may_have_provenance(&self, tcx: TyCtxt<'tcx>, size: Size) -> bool {
         match *self {
             ConstValue::ZeroSized | ConstValue::Scalar(Scalar::Int(_)) => return false,
             ConstValue::Scalar(Scalar::Ptr(..)) => return true,
+            // It's hard to find out the part of the allocation we point to;
+            // just conservatively check everything.
             ConstValue::Slice { data, meta: _ } => !data.inner().provenance().ptrs().is_empty(),
             ConstValue::Indirect { alloc_id, offset } => !tcx
                 .global_alloc(alloc_id)
@@ -504,10 +507,10 @@ impl<'tcx> Const<'tcx> {
     /// Return true if any evaluation of this constant always returns the same value,
     /// taking into account even pointer identity tests.
     pub fn is_deterministic(&self) -> bool {
-        // Some constants may contain pointers. We need to preserve the provenance of these
-        // pointers, but not all constants guarantee this:
-        // - valtrees purposefully do not;
-        // - ConstValue::Slice does not either.
+        // Some constants may generate fresh allocations for pointers they contain,
+        // so using the same constant twice can yield two different results:
+        // - valtrees purposefully generate new allocations
+        // - ConstValue::Slice also generate new allocations
         match self {
             Const::Ty(c) => match c.kind() {
                 ty::ConstKind::Param(..) => true,

compiler/rustc_mir_transform/src/gvn.rs

@@ -917,6 +917,7 @@ fn op_to_prop_const<'tcx>(
 
         // Do not try interning a value that contains provenance.
         // Due to https://github.com/rust-lang/rust/issues/79738, doing so could lead to bugs.
+        // FIXME: remove this hack once that issue is fixed.
         let alloc_ref = ecx.get_ptr_alloc(mplace.ptr(), size).ok()??;
         if alloc_ref.has_provenance() {
             return None;
@@ -928,6 +929,8 @@ fn op_to_prop_const<'tcx>(
         if matches!(ecx.tcx.global_alloc(alloc_id), GlobalAlloc::Memory(_)) {
             // `alloc_id` may point to a static. Codegen will choke on an `Indirect` with anything
             // by `GlobalAlloc::Memory`, so do fall through to copying if needed.
+            // FIXME: find a way to treat this more uniformly
+            // (probably by fixing codegen)
             return Some(ConstValue::Indirect { alloc_id, offset });
         }
     }
@@ -939,7 +942,7 @@ fn op_to_prop_const<'tcx>(
 
     // Check that we do not leak a pointer.
     // Those pointers may lose part of their identity in codegen.
-    // See https://github.com/rust-lang/rust/issues/79738.
+    // FIXME: remove this hack once https://github.com/rust-lang/rust/issues/79738 is fixed.
     if ecx.tcx.global_alloc(alloc_id).unwrap_memory().inner().provenance().ptrs().is_empty() {
         return Some(value);
     }
@@ -969,7 +972,7 @@ impl<'tcx> VnState<'_, 'tcx> {
 
         // Check that we do not leak a pointer.
         // Those pointers may lose part of their identity in codegen.
-        // See https://github.com/rust-lang/rust/issues/79738.
+        // FIXME: remove this hack once https://github.com/rust-lang/rust/issues/79738 is fixed.
         assert!(!value.may_have_provenance(self.tcx, op.layout.size));
 
         let const_ = Const::Val(value, op.layout.ty);