From 24be43356e31d6b0ee2ec8bf912c0a325f236a1d Mon Sep 17 00:00:00 2001 From: Camille Gillot Date: Fri, 27 Oct 2023 20:51:25 +0200 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Ralf Jung --- compiler/rustc_middle/src/mir/consts.rs | 11 +++++++---- compiler/rustc_mir_transform/src/gvn.rs | 7 +++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_middle/src/mir/consts.rs b/compiler/rustc_middle/src/mir/consts.rs index 9ddf3903d04ce..3cb2e349ce084 100644 --- a/compiler/rustc_middle/src/mir/consts.rs +++ b/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, diff --git a/compiler/rustc_mir_transform/src/gvn.rs b/compiler/rustc_mir_transform/src/gvn.rs index 39065962c6959..de0dc25808b30 100644 --- a/compiler/rustc_mir_transform/src/gvn.rs +++ b/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);