Skip to content

Commit f76f128

Browse files
RalfJungRalfJung
committed
const-eval interning: accpt interior mutable pointers in final value (but keep rejecting mutable references)
1 parent 304b7f8 commit f76f128

21 files changed

+177
-563
lines changed

Diff for: ‎compiler/rustc_const_eval/src/check_consts/check.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -538,8 +538,9 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
538538
// final value.
539539
// Note: This is only sound if every local that has a `StorageDead` has a
540540
// `StorageDead` in every control flow path leading to a `return` terminator.
541-
// The good news is that interning will detect if any unexpected mutable
542-
// pointer slips through.
541+
// If anything slips through, there's no safety net -- safe code can create
542+
// references to variants of `!Freeze` enums as long as that variant is `Freeze`,
543+
// so interning can't protect us here.
543544
if self.local_is_transient(place.local) {
544545
self.check_op(ops::TransientCellBorrow);
545546
} else {

Diff for: ‎compiler/rustc_const_eval/src/const_eval/eval_queries.rs

+13-13
Original file line numberDiff line numberDiff line change
@@ -10,21 +10,19 @@ 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;
1413
use rustc_span::def_id::LocalDefId;
1514
use rustc_span::{Span, DUMMY_SP};
1615
use rustc_target::abi::{self, Abi};
1716
use tracing::{debug, instrument, trace};
1817

1918
use super::{CanAccessMutGlobal, CompileTimeInterpCx, CompileTimeMachine};
2019
use crate::const_eval::CheckAlignment;
21-
use crate::errors::{self, ConstEvalError, DanglingPtrInFinal};
2220
use crate::interpret::{
2321
create_static_alloc, eval_nullary_intrinsic, intern_const_alloc_recursive, throw_exhaust,
2422
CtfeValidationMode, GlobalId, Immediate, InternKind, InternResult, InterpCx, InterpError,
2523
InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, StackPopCleanup,
2624
};
27-
use crate::CTRL_C_RECEIVED;
25+
use crate::{errors, CTRL_C_RECEIVED};
2826

2927
// Returns a pointer to where the result lives
3028
#[instrument(level = "trace", skip(ecx, body))]
@@ -105,18 +103,15 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
105103
return Err(ecx
106104
.tcx
107105
.dcx()
108-
.emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
106+
.emit_err(errors::DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
109107
.into());
110108
}
111109
Err(InternResult::FoundBadMutablePointer) => {
112-
// only report mutable pointers if there were no dangling pointers
113-
let err_diag = errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind };
114-
ecx.tcx.emit_node_span_lint(
115-
lint::builtin::CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
116-
ecx.machine.best_lint_scope(*ecx.tcx),
117-
err_diag.span,
118-
err_diag,
119-
)
110+
return Err(ecx
111+
.tcx
112+
.dcx()
113+
.emit_err(errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind })
114+
.into());
120115
}
121116
}
122117

@@ -448,7 +443,12 @@ fn report_eval_error<'tcx>(
448443
error,
449444
DUMMY_SP,
450445
|| super::get_span_and_frames(ecx.tcx, ecx.stack()),
451-
|span, frames| ConstEvalError { span, error_kind: kind, instance, frame_notes: frames },
446+
|span, frames| errors::ConstEvalError {
447+
span,
448+
error_kind: kind,
449+
instance,
450+
frame_notes: frames,
451+
},
452452
)
453453
}
454454

Diff for: ‎compiler/rustc_const_eval/src/const_eval/machine.rs

+18-5
Original file line numberDiff line numberDiff line change
@@ -718,16 +718,29 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
718718
_kind: mir::RetagKind,
719719
val: &ImmTy<'tcx, CtfeProvenance>,
720720
) -> InterpResult<'tcx, ImmTy<'tcx, CtfeProvenance>> {
721-
// If it's a frozen shared reference that's not already immutable, make it immutable.
721+
// If it's a frozen shared reference that's not already immutable, potentially make it immutable.
722722
// (Do nothing on `None` provenance, that cannot store immutability anyway.)
723723
if let ty::Ref(_, ty, mutbl) = val.layout.ty.kind()
724724
&& *mutbl == Mutability::Not
725-
&& val.to_scalar_and_meta().0.to_pointer(ecx)?.provenance.is_some_and(|p| !p.immutable())
726-
// That next check is expensive, that's why we have all the guards above.
727-
&& ty.is_freeze(*ecx.tcx, ecx.param_env)
725+
&& val
726+
.to_scalar_and_meta()
727+
.0
728+
.to_pointer(ecx)?
729+
.provenance
730+
.is_some_and(|p| !p.immutable())
728731
{
732+
// That next check is expensive, that's why we have all the guards above.
733+
let is_immutable = ty.is_freeze(*ecx.tcx, ecx.param_env);
729734
let place = ecx.ref_to_mplace(val)?;
730-
let new_place = place.map_provenance(CtfeProvenance::as_immutable);
735+
let new_place = if is_immutable {
736+
place.map_provenance(CtfeProvenance::as_immutable)
737+
} else {
738+
// Even if it is not immutable, remember that it is a shared reference.
739+
// This allows it to become part of the final value of the constant.
740+
// (See <https://github.com/rust-lang/rust/pull/128543> for why we allow this
741+
// even when there is interior mutability.)
742+
place.map_provenance(CtfeProvenance::as_shared_ref)
743+
};
731744
Ok(ImmTy::from_immediate(new_place.to_ref(ecx), val.layout))
732745
} else {
733746
Ok(val.clone())

Diff for: ‎compiler/rustc_const_eval/src/errors.rs

+2-5
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,10 @@ pub(crate) struct NestedStaticInThreadLocal {
3535
pub span: Span,
3636
}
3737

38-
#[derive(LintDiagnostic)]
38+
#[derive(Diagnostic)]
3939
#[diag(const_eval_mutable_ptr_in_final)]
4040
pub(crate) struct MutablePtrInFinal {
41-
// rust-lang/rust#122153: This was marked as `#[primary_span]` under
42-
// `derive(Diagnostic)`. Since we expect we may hard-error in future, we are
43-
// keeping the field (and skipping it under `derive(LintDiagnostic)`).
44-
#[skip_arg]
41+
#[primary_span]
4542
pub span: Span,
4643
pub kind: InternKind,
4744
}

Diff for: ‎compiler/rustc_const_eval/src/interpret/intern.rs

+11-7
Original file line numberDiff line numberDiff line change
@@ -223,16 +223,20 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
223223
continue;
224224
}
225225

226-
// Crucially, we check this *before* checking whether the `alloc_id`
227-
// has already been interned. The point of this check is to ensure that when
228-
// there are multiple pointers to the same allocation, they are *all* immutable.
229-
// Therefore it would be bad if we only checked the first pointer to any given
230-
// allocation.
226+
// Ensure that this is is derived from a shared reference. Crucially, we check this *before*
227+
// checking whether the `alloc_id` has already been interned. The point of this check is to
228+
// ensure that when there are multiple pointers to the same allocation, they are *all*
229+
// derived from a shared reference. Therefore it would be bad if we only checked the first
230+
// pointer to any given allocation.
231231
// (It is likely not possible to actually have multiple pointers to the same allocation,
232232
// so alternatively we could also check that and ICE if there are multiple such pointers.)
233+
// See <https://github.com/rust-lang/rust/pull/128543> for why we are checking for
234+
// "shared reference" and not "immutable", i.e., for why we are allowed interior-mutable
235+
// shared references: they can actually be created in safe code while pointing to apparently
236+
// "immutable" values, via promotion of `&None::<Cell<T>>`.
233237
if intern_kind != InternKind::Promoted
234238
&& inner_mutability == Mutability::Not
235-
&& !prov.immutable()
239+
&& !prov.shared_ref()
236240
{
237241
if ecx.tcx.try_get_global_alloc(alloc_id).is_some()
238242
&& !just_interned.contains(&alloc_id)
@@ -245,7 +249,7 @@ pub fn intern_const_alloc_recursive<'tcx, M: CompileTimeMachine<'tcx, const_eval
245249
// this to the todo list, since after all it is already interned.
246250
continue;
247251
}
248-
// Found a mutable pointer inside a const where inner allocations should be
252+
// Found a mutable reference inside a const where inner allocations should be
249253
// immutable. We exclude promoteds from this, since things like `&mut []` and
250254
// `&None::<Cell<i32>>` lead to promotion that can produce mutable pointers. We rely
251255
// on the promotion analysis not screwing up to ensure that it is sound to intern

Diff for: ‎compiler/rustc_lint/src/lib.rs

+4
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,10 @@ fn register_builtins(store: &mut LintStore) {
576576
<https://github.com/rust-lang/rust/issues/107457> for more information",
577577
);
578578
store.register_removed("writes_through_immutable_pointer", "converted into hard error");
579+
store.register_removed(
580+
"const_eval_mutable_ptr_in_final_value",
581+
"partially allowed now, otherwise turned into a hard error",
582+
);
579583
}
580584

581585
fn register_internals(store: &mut LintStore) {

Diff for: ‎compiler/rustc_lint_defs/src/builtin.rs

-46
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ declare_lint_pass! {
2929
CENUM_IMPL_DROP_CAST,
3030
COHERENCE_LEAK_CHECK,
3131
CONFLICTING_REPR_HINTS,
32-
CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
3332
CONST_EVALUATABLE_UNCHECKED,
3433
CONST_ITEM_MUTATION,
3534
DEAD_CODE,
@@ -2804,51 +2803,6 @@ declare_lint! {
28042803
@feature_gate = strict_provenance;
28052804
}
28062805

2807-
declare_lint! {
2808-
/// The `const_eval_mutable_ptr_in_final_value` lint detects if a mutable pointer
2809-
/// has leaked into the final value of a const expression.
2810-
///
2811-
/// ### Example
2812-
///
2813-
/// ```rust
2814-
/// pub enum JsValue {
2815-
/// Undefined,
2816-
/// Object(std::cell::Cell<bool>),
2817-
/// }
2818-
///
2819-
/// impl ::std::ops::Drop for JsValue {
2820-
/// fn drop(&mut self) {}
2821-
/// }
2822-
///
2823-
/// const UNDEFINED: &JsValue = &JsValue::Undefined;
2824-
///
2825-
/// fn main() {
2826-
/// }
2827-
/// ```
2828-
///
2829-
/// {{produces}}
2830-
///
2831-
/// ### Explanation
2832-
///
2833-
/// In the 1.77 release, the const evaluation machinery adopted some
2834-
/// stricter rules to reject expressions with values that could
2835-
/// end up holding mutable references to state stored in static memory
2836-
/// (which is inherently immutable).
2837-
///
2838-
/// This is a [future-incompatible] lint to ease the transition to an error.
2839-
/// See [issue #122153] for more details.
2840-
///
2841-
/// [issue #122153]: https://github.com/rust-lang/rust/issues/122153
2842-
/// [future-incompatible]: ../index.md#future-incompatible-lints
2843-
pub CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
2844-
Warn,
2845-
"detects a mutable pointer that has leaked into final value of a const expression",
2846-
@future_incompatible = FutureIncompatibleInfo {
2847-
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
2848-
reference: "issue #122153 <https://github.com/rust-lang/rust/issues/122153>",
2849-
};
2850-
}
2851-
28522806
declare_lint! {
28532807
/// The `const_evaluatable_unchecked` lint detects a generic constant used
28542808
/// in a type.

Diff for: ‎compiler/rustc_middle/src/mir/interpret/pointer.rs

+43-4
Original file line numberDiff line numberDiff line change
@@ -80,14 +80,23 @@ pub trait Provenance: Copy + fmt::Debug + 'static {
8080
}
8181

8282
/// The type of provenance in the compile-time interpreter.
83-
/// This is a packed representation of an `AllocId` and an `immutable: bool`.
83+
/// This is a packed representation of:
84+
/// - an `AllocId` (non-zero)
85+
/// - an `immutable: bool`
86+
/// - a `shared_ref: bool`
87+
///
88+
/// with the extra invariant that if `immutable` is `true`, then so
89+
/// is `shared_ref`.
8490
#[derive(Copy, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
8591
pub struct CtfeProvenance(NonZero<u64>);
8692

8793
impl From<AllocId> for CtfeProvenance {
8894
fn from(value: AllocId) -> Self {
8995
let prov = CtfeProvenance(value.0);
90-
assert!(!prov.immutable(), "`AllocId` with the highest bit set cannot be used in CTFE");
96+
assert!(
97+
prov.alloc_id() == value,
98+
"`AllocId` with the highest bits set cannot be used in CTFE"
99+
);
91100
prov
92101
}
93102
}
@@ -103,12 +112,14 @@ impl fmt::Debug for CtfeProvenance {
103112
}
104113

105114
const IMMUTABLE_MASK: u64 = 1 << 63; // the highest bit
115+
const SHARED_REF_MASK: u64 = 1 << 62;
116+
const ALLOC_ID_MASK: u64 = u64::MAX & !IMMUTABLE_MASK & !SHARED_REF_MASK;
106117

107118
impl CtfeProvenance {
108119
/// Returns the `AllocId` of this provenance.
109120
#[inline(always)]
110121
pub fn alloc_id(self) -> AllocId {
111-
AllocId(NonZero::new(self.0.get() & !IMMUTABLE_MASK).unwrap())
122+
AllocId(NonZero::new(self.0.get() & ALLOC_ID_MASK).unwrap())
112123
}
113124

114125
/// Returns whether this provenance is immutable.
@@ -117,10 +128,38 @@ impl CtfeProvenance {
117128
self.0.get() & IMMUTABLE_MASK != 0
118129
}
119130

131+
/// Returns whether this provenance is derived from a shared reference.
132+
#[inline]
133+
pub fn shared_ref(self) -> bool {
134+
self.0.get() & SHARED_REF_MASK != 0
135+
}
136+
137+
pub fn into_parts(self) -> (AllocId, bool, bool) {
138+
(self.alloc_id(), self.immutable(), self.shared_ref())
139+
}
140+
141+
pub fn from_parts((alloc_id, immutable, shared_ref): (AllocId, bool, bool)) -> Self {
142+
let prov = CtfeProvenance::from(alloc_id);
143+
if immutable {
144+
// This sets both flags, so we don't even have to check `shared_ref`.
145+
prov.as_immutable()
146+
} else if shared_ref {
147+
prov.as_shared_ref()
148+
} else {
149+
prov
150+
}
151+
}
152+
120153
/// Returns an immutable version of this provenance.
121154
#[inline]
122155
pub fn as_immutable(self) -> Self {
123-
CtfeProvenance(self.0 | IMMUTABLE_MASK)
156+
CtfeProvenance(self.0 | IMMUTABLE_MASK | SHARED_REF_MASK)
157+
}
158+
159+
/// Returns a "shared reference" (but not necessarily immutable!) version of this provenance.
160+
#[inline]
161+
pub fn as_shared_ref(self) -> Self {
162+
CtfeProvenance(self.0 | SHARED_REF_MASK)
124163
}
125164
}
126165

Diff for: ‎compiler/rustc_middle/src/ty/codec.rs

+3-6
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,7 @@ impl<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>> Encodable<E> for AllocId {
165165

166166
impl<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>> Encodable<E> for CtfeProvenance {
167167
fn encode(&self, e: &mut E) {
168-
self.alloc_id().encode(e);
169-
self.immutable().encode(e);
168+
self.into_parts().encode(e);
170169
}
171170
}
172171

@@ -295,10 +294,8 @@ impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for AllocId {
295294

296295
impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for CtfeProvenance {
297296
fn decode(decoder: &mut D) -> Self {
298-
let alloc_id: AllocId = Decodable::decode(decoder);
299-
let prov = CtfeProvenance::from(alloc_id);
300-
let immutable: bool = Decodable::decode(decoder);
301-
if immutable { prov.as_immutable() } else { prov }
297+
let parts = Decodable::decode(decoder);
298+
CtfeProvenance::from_parts(parts)
302299
}
303300
}
304301

Diff for: ‎compiler/rustc_middle/src/ty/impls_ty.rs

+1-3
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,9 @@ impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::AllocId {
7575
}
7676
}
7777

78-
// CtfeProvenance is an AllocId and a bool.
7978
impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::CtfeProvenance {
8079
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
81-
self.alloc_id().hash_stable(hcx, hasher);
82-
self.immutable().hash_stable(hcx, hasher);
80+
self.into_parts().hash_stable(hcx, hasher);
8381
}
8482
}
8583

Diff for: ‎tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.rs

-2
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
#![feature(core_intrinsics)]
22
#![feature(const_heap)]
33
#![feature(const_mut_refs)]
4-
#![deny(const_eval_mutable_ptr_in_final_value)]
54
use std::intrinsics;
65

76
const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 };
87
//~^ error: mutable pointer in final value of constant
9-
//~| WARNING this was previously accepted by the compiler
108

119
fn main() {}

Diff for: ‎tests/ui/consts/const-eval/heap/alloc_intrinsic_untyped.stderr

+1-24
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,8 @@
11
error: encountered mutable pointer in final value of constant
2-
--> $DIR/alloc_intrinsic_untyped.rs:7:1
2+
--> $DIR/alloc_intrinsic_untyped.rs:6:1
33
|
44
LL | const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 };
55
| ^^^^^^^^^^^^^^^^^^^
6-
|
7-
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
8-
= note: for more information, see issue #122153 <https://github.com/rust-lang/rust/issues/122153>
9-
note: the lint level is defined here
10-
--> $DIR/alloc_intrinsic_untyped.rs:4:9
11-
|
12-
LL | #![deny(const_eval_mutable_ptr_in_final_value)]
13-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
146

157
error: aborting due to 1 previous error
168

17-
Future incompatibility report: Future breakage diagnostic:
18-
error: encountered mutable pointer in final value of constant
19-
--> $DIR/alloc_intrinsic_untyped.rs:7:1
20-
|
21-
LL | const BAR: *mut i32 = unsafe { intrinsics::const_allocate(4, 4) as *mut i32 };
22-
| ^^^^^^^^^^^^^^^^^^^
23-
|
24-
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
25-
= note: for more information, see issue #122153 <https://github.com/rust-lang/rust/issues/122153>
26-
note: the lint level is defined here
27-
--> $DIR/alloc_intrinsic_untyped.rs:4:9
28-
|
29-
LL | #![deny(const_eval_mutable_ptr_in_final_value)]
30-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
31-

0 commit comments

Comments
 (0)