Auto merge of rust-lang#122684 - oli-obk:delay_interning_errors_to_after_validaiton, r=RalfJung

Delay interning errors to after validation

fixes rust-lang#122398
fixes rust-lang#122548

This improves diagnostics since validation errors are usually more helpful compared with interning errors that just make broad statements about the entire constant

r? `@RalfJung`

Author: bors

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -10,20 +10,21 @@ 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};
 
 use super::{CanAccessMutGlobal, CompileTimeEvalContext, CompileTimeInterpreter};
 use crate::const_eval::CheckAlignment;
-use crate::errors;
 use crate::errors::ConstEvalError;
-use crate::interpret::eval_nullary_intrinsic;
+use crate::errors::{self, DanglingPtrInFinal};
 use crate::interpret::{
     create_static_alloc, intern_const_alloc_recursive, CtfeValidationMode, GlobalId, Immediate,
     InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
     StackPopCleanup,
 };
+use crate::interpret::{eval_nullary_intrinsic, InternResult};
 use crate::CTRL_C_RECEIVED;
 
 // Returns a pointer to where the result lives
@@ -89,11 +90,35 @@ fn eval_body_using_ecx<'mir, 'tcx, R: InterpretationResult<'tcx>>(
     }
 
     // Intern the result
-    intern_const_alloc_recursive(ecx, intern_kind, &ret)?;
+    let intern_result = 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.
+
+    match intern_result {
+        Ok(()) => {}
+        Err(InternResult::FoundDanglingPointer) => {
+            return Err(ecx
+                .tcx
+                .dcx()
+                .emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
+                .into());
+        }
+        Err(InternResult::FoundBadMutablePointer) => {
+            // only report mutable pointers if there were no dangling pointers
+            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))
 }
 

compiler/rustc_const_eval/src/interpret/intern.rs

@@ -16,19 +16,17 @@
 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::middle::codegen_fn_attrs::CodegenFnAttrs;
 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, NestedStaticInThreadLocal};
+use crate::errors::NestedStaticInThreadLocal;
 
 pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
         'mir,
@@ -134,6 +132,12 @@ pub enum InternKind {
     Promoted,
 }
 
+#[derive(Debug)]
+pub enum InternResult {
+    FoundBadMutablePointer,
+    FoundDanglingPointer,
+}
+
 /// Intern `ret` and everything it references.
 ///
 /// This *cannot raise an interpreter error*. Doing so is left to validation, which
@@ -149,7 +153,7 @@ pub fn intern_const_alloc_recursive<
     ecx: &mut InterpCx<'mir, 'tcx, M>,
     intern_kind: InternKind,
     ret: &MPlaceTy<'tcx>,
-) -> Result<(), ErrorGuaranteed> {
+) -> Result<(), InternResult> {
     // 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 {
@@ -201,7 +205,7 @@ pub fn intern_const_alloc_recursive<
     // Whether we encountered a bad mutable pointer.
     // We want to first report "dangling" and then "mutable", so we need to delay reporting these
     // errors.
-    let mut found_bad_mutable_pointer = false;
+    let mut result = Ok(());
 
     // Keep interning as long as there are things to intern.
     // We show errors if there are dangling pointers, or mutable pointers in immutable contexts
@@ -251,7 +255,10 @@ pub fn intern_const_alloc_recursive<
             // on the promotion analysis not screwing up to ensure that it is sound to intern
             // promoteds as immutable.
             trace!("found bad mutable pointer");
-            found_bad_mutable_pointer = true;
+            // Prefer dangling pointer errors over mutable pointer errors
+            if result.is_ok() {
+                result = Err(InternResult::FoundBadMutablePointer);
+            }
         }
         if ecx.tcx.try_get_global_alloc(alloc_id).is_some() {
             // Already interned.
@@ -269,21 +276,15 @@ 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().delayed_bug("found dangling pointer during const interning");
+                result = Err(InternResult::FoundDanglingPointer);
+            }
+        }
     }
-
-    Ok(())
+    result
 }
 
 /// Intern `ret`. This function assumes that `ret` references no other allocation.

compiler/rustc_const_eval/src/interpret/mod.rs

@@ -23,6 +23,7 @@ pub use rustc_middle::mir::interpret::*; // have all the `interpret` symbols in
 pub use self::eval_context::{format_interp_error, Frame, FrameInfo, InterpCx, StackPopCleanup};
 pub use self::intern::{
     intern_const_alloc_for_constprop, intern_const_alloc_recursive, HasStaticRootDefId, InternKind,
+    InternResult,
 };
 pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackPopJump};
 pub use self::memory::{AllocKind, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind};

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -449,67 +449,41 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
             // `!` is a ZST and we want to validate it.
             if let Ok((alloc_id, _offset, _prov)) = self.ecx.ptr_try_get_alloc_id(place.ptr()) {
                 let mut skip_recursive_check = false;
-                // Let's see what kind of memory this points to.
-                // `unwrap` since dangling pointers have already been handled.
-                let alloc_kind = self.ecx.tcx.try_get_global_alloc(alloc_id).unwrap();
-                let alloc_actual_mutbl = match alloc_kind {
-                    GlobalAlloc::Static(did) => {
-                        // 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));
-                        // Mode-specific checks
-                        match self.ctfe_mode {
-                            Some(
-                                CtfeValidationMode::Static { .. }
-                                | CtfeValidationMode::Promoted { .. },
-                            ) => {
-                                // We skip recursively checking other statics. These statics must be sound by
-                                // themselves, and the only way to get broken statics here is by using
-                                // unsafe code.
-                                // The reasons we don't check other statics is twofold. For one, in all
-                                // sound cases, the static was already validated on its own, and second, we
-                                // 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;
-                            }
-                            Some(CtfeValidationMode::Const { .. }) => {
-                                // We can't recursively validate `extern static`, so we better reject them.
-                                if self.ecx.tcx.is_foreign_item(did) {
-                                    throw_validation_failure!(self.path, ConstRefToExtern);
-                                }
-                            }
-                            None => {}
+                let alloc_actual_mutbl = mutability(self.ecx, alloc_id);
+                if let GlobalAlloc::Static(did) = self.ecx.tcx.global_alloc(alloc_id) {
+                    let DefKind::Static { nested, .. } = self.ecx.tcx.def_kind(did) else { bug!() };
+                    // 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));
+                    // Mode-specific checks
+                    match self.ctfe_mode {
+                        Some(
+                            CtfeValidationMode::Static { .. } | CtfeValidationMode::Promoted { .. },
+                        ) => {
+                            // We skip recursively checking other statics. These statics must be sound by
+                            // themselves, and the only way to get broken statics here is by using
+                            // unsafe code.
+                            // The reasons we don't check other statics is twofold. For one, in all
+                            // sound cases, the static was already validated on its own, and second, we
+                            // 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.
+                            // We still walk nested allocations, as they are fundamentally part of this validation run.
+                            // This means we will also recurse into nested statics of *other*
+                            // statics, even though we do not recurse into other statics directly.
+                            // That's somewhat inconsistent but harmless.
+                            skip_recursive_check = !nested;
                         }
-                        // 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
+                        Some(CtfeValidationMode::Const { .. }) => {
+                            // We can't recursively validate `extern static`, so we better reject them.
+                            if self.ecx.tcx.is_foreign_item(did) {
+                                throw_validation_failure!(self.path, ConstRefToExtern);
                             }
-                            (Mutability::Not, false) => Mutability::Not,
                         }
+                        None => {}
                     }
-                    GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-                    GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
-                        // These are immutable, we better don't allow mutable pointers here.
-                        Mutability::Not
-                    }
-                };
+                }
+
                 // Mutability check.
                 // If this allocation has size zero, there is no actual mutability here.
                 let (size, _align, _alloc_kind) = self.ecx.get_alloc_info(alloc_id);
@@ -708,17 +682,58 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
     fn in_mutable_memory(&self, op: &OpTy<'tcx, M::Provenance>) -> bool {
         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
+                return mutability(self.ecx, alloc_id).is_mut();
+            }
+        }
+        false
+    }
+}
+
+/// Returns whether the allocation is mutable, and whether it's actually a static.
+/// 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.
+fn mutability<'mir, 'tcx: 'mir>(
+    ecx: &InterpCx<'mir, 'tcx, impl Machine<'mir, 'tcx>>,
+    alloc_id: AllocId,
+) -> Mutability {
+    // Let's see what kind of memory this points to.
+    // We're not using `try_global_alloc` since dangling pointers have already been handled.
+    match ecx.tcx.global_alloc(alloc_id) {
+        GlobalAlloc::Static(did) => {
+            let DefKind::Static { mutability, nested } = ecx.tcx.def_kind(did) else { bug!() };
+            if nested {
+                assert!(
+                    ecx.memory.alloc_map.get(alloc_id).is_none(),
+                    "allocations of nested statics are already interned: {alloc_id:?}, {did:?}"
+                );
+                // Nested statics in a `static` are never interior mutable,
+                // so just use the declared mutability.
+                mutability
+            } else {
+                let mutability = match mutability {
+                    Mutability::Not
+                        if !ecx
+                            .tcx
+                            .type_of(did)
+                            .no_bound_vars()
+                            .expect("statics should not have generic parameters")
+                            .is_freeze(*ecx.tcx, ty::ParamEnv::reveal_all()) =>
+                    {
+                        Mutability::Mut
                     }
-                    GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
-                    _ => span_bug!(self.ecx.tcx.span, "not a memory allocation"),
+                    _ => mutability,
                 };
-                return mutability == Mutability::Mut;
+                if let Some((_, alloc)) = ecx.memory.alloc_map.get(alloc_id) {
+                    assert_eq!(alloc.mutability, mutability);
+                }
+                mutability
             }
         }
-        false
+        GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
+        GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
+            // These are immutable, we better don't allow mutable pointers here.
+            Mutability::Not
+        }
     }
 }
 

tests/crashes/122548.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

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

@@ -1,14 +1,19 @@
-error: encountered dangling pointer in final value of constant
-  --> $DIR/dealloc_intrinsic_dangling.rs:7:1
+error[E0080]: it is undefined behavior to use this value
+  --> $DIR/dealloc_intrinsic_dangling.rs:12:1
    |
 LL | const _X: &'static u8 = unsafe {
-   | ^^^^^^^^^^^^^^^^^^^^^
+   | ^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered a dangling reference (use-after-free)
+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
+   = note: the raw bytes of the constant (size: $SIZE, align: $ALIGN) {
+               HEX_DUMP
+           }
 
 error[E0080]: evaluation of constant value failed
-  --> $DIR/dealloc_intrinsic_dangling.rs:18:5
+  --> $DIR/dealloc_intrinsic_dangling.rs:23:5
    |
 LL |     *reference
-   |     ^^^^^^^^^^ memory access failed: ALLOC0 has been freed, so this pointer is dangling
+   |     ^^^^^^^^^^ memory access failed: ALLOC1 has been freed, so this pointer is dangling
 
 error: aborting due to 2 previous errors
 

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

@@ -33,8 +33,8 @@ const fn helper_dangling() -> Option<&'static mut i32> { unsafe {
     // Undefined behaviour (dangling pointer), who doesn't love tests like this.
     Some(&mut *(&mut 42 as *mut i32))
 } }
-const DANGLING: Option<&mut i32> = helper_dangling(); //~ ERROR encountered dangling pointer
-static DANGLING_STATIC: Option<&mut i32> = helper_dangling(); //~ ERROR encountered dangling pointer
+const DANGLING: Option<&mut i32> = helper_dangling(); //~ ERROR it is undefined behavior to use this value
+static DANGLING_STATIC: Option<&mut i32> = helper_dangling(); //~ ERROR it is undefined behavior to use this value
 
 // These are fine! Just statics pointing to mutable statics, nothing fundamentally wrong with this.
 static MUT_STATIC: Option<&mut i32> = helper();