Skip to content

Commit 5260893

Browse files
borsbors
committed
Auto merge of #122684 - oli-obk:delay_interning_errors_to_after_validaiton, r=RalfJung
Delay interning errors to after validation fixes #122398 fixes #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`
2 parents 9f432d7 + 126dcc6 commit 5260893

22 files changed

+405
-564
lines changed

‎compiler/rustc_const_eval/src/const_eval/eval_queries.rs

+28-3
Original file line numberDiff line numberDiff line change
@@ -10,20 +10,21 @@ use rustc_middle::traits::Reveal;
1010
use rustc_middle::ty::layout::LayoutOf;
1111
use rustc_middle::ty::print::with_no_trimmed_paths;
1212
use rustc_middle::ty::{self, Ty, TyCtxt};
13+
use rustc_session::lint;
1314
use rustc_span::def_id::LocalDefId;
1415
use rustc_span::Span;
1516
use rustc_target::abi::{self, Abi};
1617

1718
use super::{CanAccessMutGlobal, CompileTimeEvalContext, CompileTimeInterpreter};
1819
use crate::const_eval::CheckAlignment;
19-
use crate::errors;
2020
use crate::errors::ConstEvalError;
21-
use crate::interpret::eval_nullary_intrinsic;
21+
use crate::errors::{self, DanglingPtrInFinal};
2222
use crate::interpret::{
2323
create_static_alloc, intern_const_alloc_recursive, CtfeValidationMode, GlobalId, Immediate,
2424
InternKind, InterpCx, InterpError, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking,
2525
StackPopCleanup,
2626
};
27+
use crate::interpret::{eval_nullary_intrinsic, InternResult};
2728
use crate::CTRL_C_RECEIVED;
2829

2930
// Returns a pointer to where the result lives
@@ -89,11 +90,35 @@ fn eval_body_using_ecx<'mir, 'tcx, R: InterpretationResult<'tcx>>(
8990
}
9091

9192
// Intern the result
92-
intern_const_alloc_recursive(ecx, intern_kind, &ret)?;
93+
let intern_result = intern_const_alloc_recursive(ecx, intern_kind, &ret);
9394

9495
// Since evaluation had no errors, validate the resulting constant.
9596
const_validate_mplace(&ecx, &ret, cid)?;
9697

98+
// Only report this after validation, as validaiton produces much better diagnostics.
99+
// FIXME: ensure validation always reports this and stop making interning care about it.
100+
101+
match intern_result {
102+
Ok(()) => {}
103+
Err(InternResult::FoundDanglingPointer) => {
104+
return Err(ecx
105+
.tcx
106+
.dcx()
107+
.emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
108+
.into());
109+
}
110+
Err(InternResult::FoundBadMutablePointer) => {
111+
// only report mutable pointers if there were no dangling pointers
112+
let err_diag = errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
113+
ecx.tcx.emit_node_span_lint(
114+
lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
115+
ecx.best_lint_scope(),
116+
err_diag.span,
117+
err_diag,
118+
)
119+
}
120+
}
121+
97122
Ok(R::make_result(ret, ecx))
98123
}
99124

‎compiler/rustc_const_eval/src/interpret/intern.rs

+21-20
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,17 @@
1616
use hir::def::DefKind;
1717
use rustc_ast::Mutability;
1818
use rustc_data_structures::fx::{FxHashSet, FxIndexMap};
19-
use rustc_errors::ErrorGuaranteed;
2019
use rustc_hir as hir;
2120
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrs;
2221
use rustc_middle::mir::interpret::{ConstAllocation, CtfeProvenance, InterpResult};
2322
use rustc_middle::query::TyCtxtAt;
2423
use rustc_middle::ty::layout::TyAndLayout;
25-
use rustc_session::lint;
2624
use rustc_span::def_id::LocalDefId;
2725
use rustc_span::sym;
2826

2927
use super::{AllocId, Allocation, InterpCx, MPlaceTy, Machine, MemoryKind, PlaceTy};
3028
use crate::const_eval;
31-
use crate::errors::{DanglingPtrInFinal, MutablePtrInFinal, NestedStaticInThreadLocal};
29+
use crate::errors::NestedStaticInThreadLocal;
3230

3331
pub trait CompileTimeMachine<'mir, 'tcx: 'mir, T> = Machine<
3432
'mir,
@@ -134,6 +132,12 @@ pub enum InternKind {
134132
Promoted,
135133
}
136134

135+
#[derive(Debug)]
136+
pub enum InternResult {
137+
FoundBadMutablePointer,
138+
FoundDanglingPointer,
139+
}
140+
137141
/// Intern `ret` and everything it references.
138142
///
139143
/// This *cannot raise an interpreter error*. Doing so is left to validation, which
@@ -149,7 +153,7 @@ pub fn intern_const_alloc_recursive<
149153
ecx: &mut InterpCx<'mir, 'tcx, M>,
150154
intern_kind: InternKind,
151155
ret: &MPlaceTy<'tcx>,
152-
) -> Result<(), ErrorGuaranteed> {
156+
) -> Result<(), InternResult> {
153157
// We are interning recursively, and for mutability we are distinguishing the "root" allocation
154158
// that we are starting in, and all other allocations that we are encountering recursively.
155159
let (base_mutability, inner_mutability, is_static) = match intern_kind {
@@ -201,7 +205,7 @@ pub fn intern_const_alloc_recursive<
201205
// Whether we encountered a bad mutable pointer.
202206
// We want to first report "dangling" and then "mutable", so we need to delay reporting these
203207
// errors.
204-
let mut found_bad_mutable_pointer = false;
208+
let mut result = Ok(());
205209

206210
// Keep interning as long as there are things to intern.
207211
// We show errors if there are dangling pointers, or mutable pointers in immutable contexts
@@ -251,7 +255,10 @@ pub fn intern_const_alloc_recursive<
251255
// on the promotion analysis not screwing up to ensure that it is sound to intern
252256
// promoteds as immutable.
253257
trace!("found bad mutable pointer");
254-
found_bad_mutable_pointer = true;
258+
// Prefer dangling pointer errors over mutable pointer errors
259+
if result.is_ok() {
260+
result = Err(InternResult::FoundBadMutablePointer);
261+
}
255262
}
256263
if ecx.tcx.try_get_global_alloc(alloc_id).is_some() {
257264
// Already interned.
@@ -269,21 +276,15 @@ pub fn intern_const_alloc_recursive<
269276
// pointers before deciding which allocations can be made immutable; but for now we are
270277
// okay with losing some potential for immutability here. This can anyway only affect
271278
// `static mut`.
272-
todo.extend(intern_shallow(ecx, alloc_id, inner_mutability).map_err(|()| {
273-
ecx.tcx.dcx().emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
274-
})?);
275-
}
276-
if found_bad_mutable_pointer {
277-
let err_diag = MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
278-
ecx.tcx.emit_node_span_lint(
279-
lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
280-
ecx.best_lint_scope(),
281-
err_diag.span,
282-
err_diag,
283-
)
279+
match intern_shallow(ecx, alloc_id, inner_mutability) {
280+
Ok(nested) => todo.extend(nested),
281+
Err(()) => {
282+
ecx.tcx.dcx().delayed_bug("found dangling pointer during const interning");
283+
result = Err(InternResult::FoundDanglingPointer);
284+
}
285+
}
284286
}
285-
286-
Ok(())
287+
result
287288
}
288289

289290
/// Intern `ret`. This function assumes that `ret` references no other allocation.

‎compiler/rustc_const_eval/src/interpret/mod.rs

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pub use rustc_middle::mir::interpret::*; // have all the `interpret` symbols in
2323
pub use self::eval_context::{format_interp_error, Frame, FrameInfo, InterpCx, StackPopCleanup};
2424
pub use self::intern::{
2525
intern_const_alloc_for_constprop, intern_const_alloc_recursive, HasStaticRootDefId, InternKind,
26+
InternResult,
2627
};
2728
pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackPopJump};
2829
pub use self::memory::{AllocKind, AllocRef, AllocRefMut, FnVal, Memory, MemoryKind};

‎compiler/rustc_const_eval/src/interpret/validity.rs

+79-64
Original file line numberDiff line numberDiff line change
@@ -449,67 +449,41 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
449449
// `!` is a ZST and we want to validate it.
450450
if let Ok((alloc_id, _offset, _prov)) = self.ecx.ptr_try_get_alloc_id(place.ptr()) {
451451
let mut skip_recursive_check = false;
452-
// Let's see what kind of memory this points to.
453-
// `unwrap` since dangling pointers have already been handled.
454-
let alloc_kind = self.ecx.tcx.try_get_global_alloc(alloc_id).unwrap();
455-
let alloc_actual_mutbl = match alloc_kind {
456-
GlobalAlloc::Static(did) => {
457-
// Special handling for pointers to statics (irrespective of their type).
458-
assert!(!self.ecx.tcx.is_thread_local_static(did));
459-
assert!(self.ecx.tcx.is_static(did));
460-
// Mode-specific checks
461-
match self.ctfe_mode {
462-
Some(
463-
CtfeValidationMode::Static { .. }
464-
| CtfeValidationMode::Promoted { .. },
465-
) => {
466-
// We skip recursively checking other statics. These statics must be sound by
467-
// themselves, and the only way to get broken statics here is by using
468-
// unsafe code.
469-
// The reasons we don't check other statics is twofold. For one, in all
470-
// sound cases, the static was already validated on its own, and second, we
471-
// trigger cycle errors if we try to compute the value of the other static
472-
// and that static refers back to us (potentially through a promoted).
473-
// This could miss some UB, but that's fine.
474-
skip_recursive_check = true;
475-
}
476-
Some(CtfeValidationMode::Const { .. }) => {
477-
// We can't recursively validate `extern static`, so we better reject them.
478-
if self.ecx.tcx.is_foreign_item(did) {
479-
throw_validation_failure!(self.path, ConstRefToExtern);
480-
}
481-
}
482-
None => {}
452+
let alloc_actual_mutbl = mutability(self.ecx, alloc_id);
453+
if let GlobalAlloc::Static(did) = self.ecx.tcx.global_alloc(alloc_id) {
454+
let DefKind::Static { nested, .. } = self.ecx.tcx.def_kind(did) else { bug!() };
455+
// Special handling for pointers to statics (irrespective of their type).
456+
assert!(!self.ecx.tcx.is_thread_local_static(did));
457+
assert!(self.ecx.tcx.is_static(did));
458+
// Mode-specific checks
459+
match self.ctfe_mode {
460+
Some(
461+
CtfeValidationMode::Static { .. } | CtfeValidationMode::Promoted { .. },
462+
) => {
463+
// We skip recursively checking other statics. These statics must be sound by
464+
// themselves, and the only way to get broken statics here is by using
465+
// unsafe code.
466+
// The reasons we don't check other statics is twofold. For one, in all
467+
// sound cases, the static was already validated on its own, and second, we
468+
// trigger cycle errors if we try to compute the value of the other static
469+
// and that static refers back to us (potentially through a promoted).
470+
// This could miss some UB, but that's fine.
471+
// We still walk nested allocations, as they are fundamentally part of this validation run.
472+
// This means we will also recurse into nested statics of *other*
473+
// statics, even though we do not recurse into other statics directly.
474+
// That's somewhat inconsistent but harmless.
475+
skip_recursive_check = !nested;
483476
}
484-
// Return alloc mutability. For "root" statics we look at the type to account for interior
485-
// mutability; for nested statics we have no type and directly use the annotated mutability.
486-
let DefKind::Static { mutability, nested } = self.ecx.tcx.def_kind(did)
487-
else {
488-
bug!()
489-
};
490-
match (mutability, nested) {
491-
(Mutability::Mut, _) => Mutability::Mut,
492-
(Mutability::Not, true) => Mutability::Not,
493-
(Mutability::Not, false)
494-
if !self
495-
.ecx
496-
.tcx
497-
.type_of(did)
498-
.no_bound_vars()
499-
.expect("statics should not have generic parameters")
500-
.is_freeze(*self.ecx.tcx, ty::ParamEnv::reveal_all()) =>
501-
{
502-
Mutability::Mut
477+
Some(CtfeValidationMode::Const { .. }) => {
478+
// We can't recursively validate `extern static`, so we better reject them.
479+
if self.ecx.tcx.is_foreign_item(did) {
480+
throw_validation_failure!(self.path, ConstRefToExtern);
503481
}
504-
(Mutability::Not, false) => Mutability::Not,
505482
}
483+
None => {}
506484
}
507-
GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
508-
GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
509-
// These are immutable, we better don't allow mutable pointers here.
510-
Mutability::Not
511-
}
512-
};
485+
}
486+
513487
// Mutability check.
514488
// If this allocation has size zero, there is no actual mutability here.
515489
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, '
708682
fn in_mutable_memory(&self, op: &OpTy<'tcx, M::Provenance>) -> bool {
709683
if let Some(mplace) = op.as_mplace_or_imm().left() {
710684
if let Some(alloc_id) = mplace.ptr().provenance.and_then(|p| p.get_alloc_id()) {
711-
let mutability = match self.ecx.tcx.global_alloc(alloc_id) {
712-
GlobalAlloc::Static(_) => {
713-
self.ecx.memory.alloc_map.get(alloc_id).unwrap().1.mutability
685+
return mutability(self.ecx, alloc_id).is_mut();
686+
}
687+
}
688+
false
689+
}
690+
}
691+
692+
/// Returns whether the allocation is mutable, and whether it's actually a static.
693+
/// For "root" statics we look at the type to account for interior
694+
/// mutability; for nested statics we have no type and directly use the annotated mutability.
695+
fn mutability<'mir, 'tcx: 'mir>(
696+
ecx: &InterpCx<'mir, 'tcx, impl Machine<'mir, 'tcx>>,
697+
alloc_id: AllocId,
698+
) -> Mutability {
699+
// Let's see what kind of memory this points to.
700+
// We're not using `try_global_alloc` since dangling pointers have already been handled.
701+
match ecx.tcx.global_alloc(alloc_id) {
702+
GlobalAlloc::Static(did) => {
703+
let DefKind::Static { mutability, nested } = ecx.tcx.def_kind(did) else { bug!() };
704+
if nested {
705+
assert!(
706+
ecx.memory.alloc_map.get(alloc_id).is_none(),
707+
"allocations of nested statics are already interned: {alloc_id:?}, {did:?}"
708+
);
709+
// Nested statics in a `static` are never interior mutable,
710+
// so just use the declared mutability.
711+
mutability
712+
} else {
713+
let mutability = match mutability {
714+
Mutability::Not
715+
if !ecx
716+
.tcx
717+
.type_of(did)
718+
.no_bound_vars()
719+
.expect("statics should not have generic parameters")
720+
.is_freeze(*ecx.tcx, ty::ParamEnv::reveal_all()) =>
721+
{
722+
Mutability::Mut
714723
}
715-
GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
716-
_ => span_bug!(self.ecx.tcx.span, "not a memory allocation"),
724+
_ => mutability,
717725
};
718-
return mutability == Mutability::Mut;
726+
if let Some((_, alloc)) = ecx.memory.alloc_map.get(alloc_id) {
727+
assert_eq!(alloc.mutability, mutability);
728+
}
729+
mutability
719730
}
720731
}
721-
false
732+
GlobalAlloc::Memory(alloc) => alloc.inner().mutability,
733+
GlobalAlloc::Function(..) | GlobalAlloc::VTable(..) => {
734+
// These are immutable, we better don't allow mutable pointers here.
735+
Mutability::Not
736+
}
722737
}
723738
}
724739

‎tests/crashes/122548.rs

-17
This file was deleted.

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

+6-1
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,15 @@
22
#![feature(const_heap)]
33
#![feature(const_mut_refs)]
44

5+
// Strip out raw byte dumps to make comparison platform-independent:
6+
//@ normalize-stderr-test "(the raw bytes of the constant) \(size: [0-9]*, align: [0-9]*\)" -> "$1 (size: $$SIZE, align: $$ALIGN)"
7+
//@ normalize-stderr-test "([0-9a-f][0-9a-f] |╾─*A(LLOC)?[0-9]+(\+[a-z0-9]+)?(<imm>)?─*╼ )+ *│.*" -> "HEX_DUMP"
8+
//@ normalize-stderr-test "HEX_DUMP\s*\n\s*HEX_DUMP" -> "HEX_DUMP"
9+
510
use std::intrinsics;
611

712
const _X: &'static u8 = unsafe {
8-
//~^ error: dangling pointer in final value of constant
13+
//~^ error: it is undefined behavior to use this value
914
let ptr = intrinsics::const_allocate(4, 4);
1015
intrinsics::const_deallocate(ptr, 4, 4);
1116
&*ptr

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

+10-5
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,19 @@
1-
error: encountered dangling pointer in final value of constant
2-
--> $DIR/dealloc_intrinsic_dangling.rs:7:1
1+
error[E0080]: it is undefined behavior to use this value
2+
--> $DIR/dealloc_intrinsic_dangling.rs:12:1
33
|
44
LL | const _X: &'static u8 = unsafe {
5-
| ^^^^^^^^^^^^^^^^^^^^^
5+
| ^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered a dangling reference (use-after-free)
6+
|
7+
= 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.
8+
= note: the raw bytes of the constant (size: $SIZE, align: $ALIGN) {
9+
HEX_DUMP
10+
}
611

712
error[E0080]: evaluation of constant value failed
8-
--> $DIR/dealloc_intrinsic_dangling.rs:18:5
13+
--> $DIR/dealloc_intrinsic_dangling.rs:23:5
914
|
1015
LL | *reference
11-
| ^^^^^^^^^^ memory access failed: ALLOC0 has been freed, so this pointer is dangling
16+
| ^^^^^^^^^^ memory access failed: ALLOC1 has been freed, so this pointer is dangling
1217

1318
error: aborting due to 2 previous errors
1419

‎tests/ui/consts/const-mut-refs/mut_ref_in_final_dynamic_check.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ const fn helper_dangling() -> Option<&'static mut i32> { unsafe {
3333
// Undefined behaviour (dangling pointer), who doesn't love tests like this.
3434
Some(&mut *(&mut 42 as *mut i32))
3535
} }
36-
const DANGLING: Option<&mut i32> = helper_dangling(); //~ ERROR encountered dangling pointer
37-
static DANGLING_STATIC: Option<&mut i32> = helper_dangling(); //~ ERROR encountered dangling pointer
36+
const DANGLING: Option<&mut i32> = helper_dangling(); //~ ERROR it is undefined behavior to use this value
37+
static DANGLING_STATIC: Option<&mut i32> = helper_dangling(); //~ ERROR it is undefined behavior to use this value
3838

3939
// These are fine! Just statics pointing to mutable statics, nothing fundamentally wrong with this.
4040
static MUT_STATIC: Option<&mut i32> = helper();

0 commit comments

Comments
 (0)