Delay interning errors to after validation

Author: oli-obk

compiler/rustc_const_eval/messages.ftl

@@ -49,7 +49,6 @@ const_eval_dangling_int_pointer =
 const_eval_dangling_null_pointer =
     {$bad_pointer_message}: null pointer is a dangling pointer (it has no provenance)
 
-const_eval_dangling_ptr_in_final = encountered dangling pointer in final value of {const_eval_intern_kind}
 const_eval_dead_local =
     accessing a dead local variable
 const_eval_dealloc_immutable =

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -8,6 +8,7 @@ use rustc_middle::traits::Reveal;
 use rustc_middle::ty::layout::LayoutOf;
 use rustc_middle::ty::print::with_no_trimmed_paths;
 use rustc_middle::ty::{self, Ty, TyCtxt};
+use rustc_session::lint;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::Span;
 use rustc_target::abi::{self, Abi};
@@ -82,11 +83,23 @@ fn eval_body_using_ecx<'mir, 'tcx, R: InterpretationResult<'tcx>>(
     while ecx.step()? {}
 
     // Intern the result
-    intern_const_alloc_recursive(ecx, intern_kind, &ret)?;
+    let found_bad_mutable_pointer = intern_const_alloc_recursive(ecx, intern_kind, &ret);
 
     // Since evaluation had no errors, validate the resulting constant.
     const_validate_mplace(&ecx, &ret, cid)?;
 
+    // Only report this after validation, as validaiton produces much better diagnostics.
+    // FIXME: ensure validation always reports this and stop making interning care about it.
+    if found_bad_mutable_pointer {
+        let err_diag = errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
+        ecx.tcx.emit_node_span_lint(
+            lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
+            ecx.best_lint_scope(),
+            err_diag.span,
+            err_diag,
+        )
+    }
+
     Ok(R::make_result(ret, ecx))
 }
 
@@ -401,7 +414,7 @@ fn const_validate_mplace<'mir, 'tcx>(
                 CtfeValidationMode::Const { allow_immutable_unsafe_cell: !inner }
             }
         };
-        ecx.const_validate_operand(&mplace.into(), path, &mut ref_tracking, mode)
+        ecx.const_validate_operand(mplace, path, &mut ref_tracking, mode)
             // Instead of just reporting the `InterpError` via the usual machinery, we give a more targetted
             // error about the validation failure.
             .map_err(|error| report_validation_error(&ecx, error, alloc_id))?;

compiler/rustc_const_eval/src/const_eval/valtrees.rs

@@ -323,7 +323,7 @@ pub fn valtree_to_const_value<'tcx>(
 
             valtree_into_mplace(&mut ecx, &place, valtree);
             dump_place(&ecx, &place);
-            intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &place).unwrap();
+            assert!(!intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &place));
 
             op_to_const(&ecx, &place.into(), /* for diagnostics */ false)
         }
@@ -359,7 +359,7 @@ fn valtree_to_ref<'tcx>(
 
     valtree_into_mplace(ecx, &pointee_place, valtree);
     dump_place(ecx, &pointee_place);
-    intern_const_alloc_recursive(ecx, InternKind::Constant, &pointee_place).unwrap();
+    assert!(!intern_const_alloc_recursive(ecx, InternKind::Constant, &pointee_place));
 
     pointee_place.to_ref(&ecx.tcx)
 }

compiler/rustc_const_eval/src/errors.rs

@@ -17,14 +17,6 @@ use rustc_target::abi::{Size, WrappingRange};
 
 use crate::interpret::InternKind;
 
-#[derive(Diagnostic)]
-#[diag(const_eval_dangling_ptr_in_final)]
-pub(crate) struct DanglingPtrInFinal {
-    #[primary_span]
-    pub span: Span,
-    pub kind: InternKind,
-}
-
 #[derive(LintDiagnostic)]
 #[diag(const_eval_mutable_ptr_in_final)]
 pub(crate) struct MutablePtrInFinal {

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -16,18 +16,15 @@
 use hir::def::DefKind;
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
-use rustc_errors::ErrorGuaranteed;
 use rustc_hir as hir;
 use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
 use rustc_middle::query::TyCtxtAt;
 use rustc_middle::ty::layout::TyAndLayout;
-use rustc_session::lint;
 use rustc_span::def_id::LocalDefId;
 use rustc_span::sym;
 
 use super::{AllocId, Allocation, InterpCx, MPlaceTy, Machine, MemoryKind, PlaceTy};
 use crate::const_eval;
-use crate::errors::{DanglingPtrInFinal, MutablePtrInFinal};
 
 pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
         'mir,
@@ -138,7 +135,7 @@ pub fn intern_const_alloc_recursive<
     ecx: &mut InterpCx<'mir, 'tcx, M>,
     intern_kind: InternKind,
     ret: &MPlaceTy<'tcx>,
-) -> Result<(), ErrorGuaranteed> {
+) -> bool {
     // We are interning recursively, and for mutability we are distinguishing the "root" allocation
     // that we are starting in, and all other allocations that we are encountering recursively.
     let (base_mutability, inner_mutability, is_static) = match intern_kind {
@@ -258,21 +255,16 @@ pub fn intern_const_alloc_recursive<
         // pointers before deciding which allocations can be made immutable; but for now we are
         // okay with losing some potential for immutability here. This can anyway only affect
         // `static mut`.
-        todo.extend(intern_shallow(ecx, alloc_id, inner_mutability).map_err(|()| {
-            ecx.tcx.dcx().emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
-        })?);
-    }
-    if found_bad_mutable_pointer {
-        let err_diag = MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
-        ecx.tcx.emit_node_span_lint(
-            lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
-            ecx.best_lint_scope(),
-            err_diag.span,
-            err_diag,
-        )
+        match intern_shallow(ecx, alloc_id, inner_mutability) {
+            Ok(nested) => todo.extend(nested),
+            Err(()) => {
+                ecx.tcx
+                    .dcx()
+                    .span_delayed_bug(ecx.tcx.span, "validation did not reject dangling pointer");
+            }
+        }
     }
-
-    Ok(())
+    found_bad_mutable_pointer
 }
 
 /// Intern `ret`. This function assumes that `ret` references no other allocation.

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -10,6 +10,7 @@ use std::num::NonZero;
 use either::{Left, Right};
 
 use hir::def::DefKind;
+use hir::def_id::DefId;
 use rustc_ast::Mutability;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir as hir;
@@ -27,9 +28,9 @@ use rustc_target::abi::{
 use std::hash::Hash;
 
 use super::{
-    format_interp_error, machine::AllocMap, AllocId, CheckInAllocMsg, GlobalAlloc, ImmTy,
-    Immediate, InterpCx, InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy, Pointer, Projectable,
-    Scalar, ValueVisitor,
+    format_interp_error, AllocId, CheckInAllocMsg, GlobalAlloc, ImmTy, Immediate, InterpCx,
+    InterpResult, MPlaceTy, Machine, MemPlaceMeta, OpTy, Pointer, Projectable, Scalar,
+    ValueVisitor,
 };
 
 // for the validation errors
@@ -433,7 +434,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
             throw_validation_failure!(self.path, PtrToUninhabited { ptr_kind, ty })
         }
         // Recursive checking
-        if let Some(ref_tracking) = self.ref_tracking.as_deref_mut() {
+        if self.ref_tracking.is_some() {
             // Determine whether this pointer expects to be pointing to something mutable.
             let ptr_expected_mutbl = match ptr_kind {
                 PointerKind::Box => Mutability::Mut,
@@ -457,6 +458,12 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                         // Special handling for pointers to statics (irrespective of their type).
                         assert!(!self.ecx.tcx.is_thread_local_static(did));
                         assert!(self.ecx.tcx.is_static(did));
+                        // Return alloc mutability. For "root" statics we look at the type to account for interior
+                        // mutability; for nested statics we have no type and directly use the annotated mutability.
+                        let DefKind::Static { mutability, nested } = self.ecx.tcx.def_kind(did)
+                        else {
+                            bug!()
+                        };
                         // Mode-specific checks
                         match self.ctfe_mode {
                             Some(
@@ -471,7 +478,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                                 // trigger cycle errors if we try to compute the value of the other static
                                 // and that static refers back to us (potentially through a promoted).
                                 // This could miss some UB, but that's fine.
-                                skip_recursive_check = true;
+                                // We still walk nested allocations, as they are fundamentally part of this validation run.
+                                skip_recursive_check = !nested;
                             }
                             Some(CtfeValidationMode::Const { .. }) => {
                                 // We can't recursively validate `extern static`, so we better reject them.
@@ -481,28 +489,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                             }
                             None => {}
                         }
-                        // Return alloc mutability. For "root" statics we look at the type to account for interior
-                        // mutability; for nested statics we have no type and directly use the annotated mutability.
-                        let DefKind::Static { mutability, nested } = self.ecx.tcx.def_kind(did)
-                        else {
-                            bug!()
-                        };
-                        match (mutability, nested) {
-                            (Mutability::Mut, _) => Mutability::Mut,
-                            (Mutability::Not, true) => Mutability::Not,
-                            (Mutability::Not, false)
-                                if !self
-                                    .ecx
-                                    .tcx
-                                    .type_of(did)
-                                    .no_bound_vars()
-                                    .expect("statics should not have generic parameters")
-                                    .is_freeze(*self.ecx.tcx, ty::ParamEnv::reveal_all()) =>
-                            {
-                                Mutability::Mut
-                            }
-                            (Mutability::Not, false) => Mutability::Not,
-                        }
+                        self.static_mutability(mutability, nested, did)
                     }
                     GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
                     GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
@@ -535,7 +522,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 }
             }
             let path = &self.path;
-            ref_tracking.track(place, || {
+            self.ref_tracking.as_deref_mut().unwrap().track(place, || {
                 // We need to clone the path anyway, make sure it gets created
                 // with enough space for the additional `Deref`.
                 let mut new_path = Vec::with_capacity(path.len() + 1);
@@ -547,6 +534,25 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
         Ok(())
     }
 
+    fn static_mutability(&self, mutability: Mutability, nested: bool, did: DefId) -> Mutability {
+        match (mutability, nested) {
+            (Mutability::Mut, _) => Mutability::Mut,
+            (Mutability::Not, true) => Mutability::Not,
+            (Mutability::Not, false)
+                if !self
+                    .ecx
+                    .tcx
+                    .type_of(did)
+                    .no_bound_vars()
+                    .expect("statics should not have generic parameters")
+                    .is_freeze(*self.ecx.tcx, ty::ParamEnv::reveal_all()) =>
+            {
+                Mutability::Mut
+            }
+            (Mutability::Not, false) => Mutability::Not,
+        }
+    }
+
     /// Check if this is a value of primitive type, and if yes check the validity of the value
     /// at that type. Return `true` if the type is indeed primitive.
     ///
@@ -599,6 +605,15 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 if place.layout.is_unsized() {
                     self.check_wide_ptr_meta(place.meta(), place.layout)?;
                 }
+                if self.ref_tracking.is_some()
+                    && let Some(alloc_id) = place.ptr().provenance.and_then(|p| p.get_alloc_id())
+                    && self.ecx.tcx.try_get_global_alloc(alloc_id).is_none()
+                {
+                    throw_validation_failure!(
+                        self.path,
+                        DanglingPtrUseAfterFree { ptr_kind: PointerKind::Ref(Mutability::Not) }
+                    )
+                }
                 Ok(true)
             }
             ty::Ref(_, _ty, mutbl) => {
@@ -708,9 +723,12 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
         if let Some(mplace) = op.as_mplace_or_imm().left() {
             if let Some(alloc_id) = mplace.ptr().provenance.and_then(|p| p.get_alloc_id()) {
                 let mutability = match self.ecx.tcx.global_alloc(alloc_id) {
-                    GlobalAlloc::Static(_) => {
-                        self.ecx.memory.alloc_map.get(alloc_id).unwrap().1.mutability
-                    }
+                    GlobalAlloc::Static(id) => match self.ecx.tcx.def_kind(id) {
+                        DefKind::Static { nested, mutability } => {
+                            self.static_mutability(mutability, nested, id)
+                        }
+                        _ => bug!(),
+                    },
                     GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
                     _ => span_bug!(self.ecx.tcx.span, "not a memory allocation"),
                 };
@@ -975,15 +993,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         path: Vec<PathElem>,
         ref_tracking: Option<&mut RefTracking<MPlaceTy<'tcx, M::Provenance>, Vec<PathElem>>>,
         ctfe_mode: Option<CtfeValidationMode>,
-    ) -> InterpResult<'tcx> {
+    ) -> InterpResult<'tcx, Vec<PathElem>> {
         trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty);
 
         // Construct a visitor
         let mut visitor = ValidityVisitor { path, ref_tracking, ctfe_mode, ecx: self };
 
         // Run it.
         match self.run_for_validation(|| visitor.visit_value(op)) {
-            Ok(()) => Ok(()),
+            Ok(()) => Ok(visitor.path),
             // Pass through validation failures and "invalid program" issues.
             Err(err)
                 if matches!(
@@ -1003,7 +1021,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
         }
     }
+}
 
+impl<'mir, 'tcx> InterpCx<'mir, 'tcx, crate::const_eval::CompileTimeInterpreter<'mir, 'tcx>> {
     /// This function checks the data at `op` to be const-valid.
     /// `op` is assumed to cover valid memory if it is an indirect operand.
     /// It will error if the bits at the destination do not match the ones described by the layout.
@@ -1017,14 +1037,38 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     #[inline(always)]
     pub(crate) fn const_validate_operand(
         &self,
-        op: &OpTy<'tcx, M::Provenance>,
+        mplace: MPlaceTy<'tcx>,
         path: Vec<PathElem>,
-        ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::Provenance>, Vec<PathElem>>,
+        ref_tracking: &mut RefTracking<MPlaceTy<'tcx>, Vec<PathElem>>,
         ctfe_mode: CtfeValidationMode,
     ) -> InterpResult<'tcx> {
-        self.validate_operand_internal(op, path, Some(ref_tracking), Some(ctfe_mode))
+        let prov = mplace.ptr().provenance;
+        let path = self.validate_operand_internal(
+            &mplace.into(),
+            path,
+            Some(ref_tracking),
+            Some(ctfe_mode),
+        )?;
+
+        // There was no error, so let's check the rest of the relocations in the pointed-to allocation for
+        // dangling pointers.
+        if let Some(prov) = prov
+            && let Some(GlobalAlloc::Memory(alloc)) = self.tcx.try_get_global_alloc(prov.alloc_id())
+        {
+            for (_, prov) in alloc.0.provenance().ptrs().iter() {
+                if self.tcx.try_get_global_alloc(prov.alloc_id()).is_none() {
+                    throw_validation_failure!(
+                        path,
+                        DanglingPtrUseAfterFree { ptr_kind: PointerKind::Ref(Mutability::Not) }
+                    )
+                }
+            }
+        }
+        Ok(())
     }
+}
 
+impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// This function checks the data at `op` to be runtime-valid.
     /// `op` is assumed to cover valid memory if it is an indirect operand.
     /// It will error if the bits at the destination do not match the ones described by the layout.
@@ -1034,6 +1078,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         // still correct to not use `ctfe_mode`: that mode is for validation of the final constant
         // value, it rules out things like `UnsafeCell` in awkward places. It also can make checking
         // recurse through references which, for now, we don't want here, either.
-        self.validate_operand_internal(op, vec![], None, None)
+        self.validate_operand_internal(op, vec![], None, None).map(drop)
     }
 }

compiler/rustc_const_eval/src/util/caller_location.rs

@@ -64,8 +64,6 @@ pub(crate) fn const_caller_location_provider(
     );
 
     let loc_place = alloc_caller_location(&mut ecx, file, line, col);
-    if intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &loc_place).is_err() {
-        bug!("intern_const_alloc_recursive should not error in this case")
-    }
+    assert!(!intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &loc_place));
     mir::ConstValue::Scalar(Scalar::from_maybe_pointer(loc_place.ptr(), &tcx))
 }

tests/ui/consts/const-eval/heap/dealloc_intrinsic_dangling.rs

@@ -2,10 +2,15 @@
 #![feature(const_heap)]
 #![feature(const_mut_refs)]
 
+// Strip out raw byte dumps to make comparison platform-independent:
+//@ normalize-stderr-test "(the raw bytes of the constant) \(size: [0-9]*, align: [0-9]*\)" -> "$1 (size: $$SIZE, align: $$ALIGN)"
+//@ normalize-stderr-test "([0-9a-f][0-9a-f] |╾─*A(LLOC)?[0-9]+(\+[a-z0-9]+)?(<imm>)?─*╼ )+ *│.*" -> "HEX_DUMP"
+//@ normalize-stderr-test "HEX_DUMP\s*\n\s*HEX_DUMP" -> "HEX_DUMP"
+
 use std::intrinsics;
 
 const _X: &'static u8 = unsafe {
-    //~^ error: dangling pointer in final value of constant
+    //~^ error: it is undefined behavior to use this value
     let ptr = intrinsics::const_allocate(4, 4);
     intrinsics::const_deallocate(ptr, 4, 4);
     &*ptr