rust-lang/rust
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

const-eval interning: accpt interior mutable pointers in final value (but keep rejecting mutable references)

Browse Source
This commit is contained in:
Ralf Jung
2024-08-02 15:34:59 +02:00
parent 304b7f801b
commit f76f128dc9
21 changed files with 177 additions and 563 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
compiler/rustc_const_eval/src/check_consts/check.rs
View File

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

26
compiler/rustc_const_eval/src/const_eval/eval_queries.rs
View File

@@ -10,7 +10,6 @@ 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, DUMMY_SP};
use rustc_target::abi::{self, Abi};
@@ -18,13 +17,12 @@ use tracing::{debug, instrument, trace};
use super::{CanAccessMutGlobal, CompileTimeInterpCx, CompileTimeMachine};
use crate::const_eval::CheckAlignment;
use crate::errors::{self, ConstEvalError, DanglingPtrInFinal};
use crate::interpret::{
create_static_alloc, eval_nullary_intrinsic, intern_const_alloc_recursive, throw_exhaust,
CtfeValidationMode, GlobalId, Immediate, InternKind, InternResult, InterpCx, InterpError,
InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, StackPopCleanup,
};
use crate::CTRL_C_RECEIVED;
use crate::{errors, CTRL_C_RECEIVED};
// Returns a pointer to where the result lives
#[instrument(level = "trace", skip(ecx, body))]
@@ -105,18 +103,15 @@ fn eval_body_using_ecx<'tcx, R: InterpretationResult<'tcx>>(
return Err(ecx
.tcx
.dcx()
.emit_err(DanglingPtrInFinal { span: ecx.tcx.span, kind: intern_kind })
.emit_err(errors::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.machine.best_lint_scope(*ecx.tcx),
err_diag.span,
err_diag,
)
return Err(ecx
.tcx
.dcx()
.emit_err(errors::MutablePtrInFinal { span: ecx.tcx.span, kind: intern_kind })
.into());
}
}
@@ -448,7 +443,12 @@ fn report_eval_error<'tcx>(
error,
DUMMY_SP,
|| super::get_span_and_frames(ecx.tcx, ecx.stack()),
|span, frames| ConstEvalError { span, error_kind: kind, instance, frame_notes: frames },
|span, frames| errors::ConstEvalError {
span,
error_kind: kind,
instance,
frame_notes: frames,
},
)
}

23
compiler/rustc_const_eval/src/const_eval/machine.rs
View File

@@ -718,16 +718,29 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
_kind: mir::RetagKind,
val: &ImmTy<'tcx, CtfeProvenance>,
) -> InterpResult<'tcx, ImmTy<'tcx, CtfeProvenance>> {
// If it's a frozen shared reference that's not already immutable, make it immutable.
// If it's a frozen shared reference that's not already immutable, potentially make it immutable.
// (Do nothing on `None` provenance, that cannot store immutability anyway.)
if let ty::Ref(_, ty, mutbl) = val.layout.ty.kind()
&& *mutbl == Mutability::Not
&& val.to_scalar_and_meta().0.to_pointer(ecx)?.provenance.is_some_and(|p| !p.immutable())
// That next check is expensive, that's why we have all the guards above.
&& ty.is_freeze(*ecx.tcx, ecx.param_env)
&& val
.to_scalar_and_meta()
.0
.to_pointer(ecx)?
.provenance
.is_some_and(|p| !p.immutable())
{
// That next check is expensive, that's why we have all the guards above.
let is_immutable = ty.is_freeze(*ecx.tcx, ecx.param_env);
let place = ecx.ref_to_mplace(val)?;
let new_place = place.map_provenance(CtfeProvenance::as_immutable);
let new_place = if is_immutable {
place.map_provenance(CtfeProvenance::as_immutable)
} else {
// Even if it is not immutable, remember that it is a shared reference.
// This allows it to become part of the final value of the constant.
// (See <https://github.com/rust-lang/rust/pull/128543> for why we allow this
// even when there is interior mutability.)
place.map_provenance(CtfeProvenance::as_shared_ref)
};
Ok(ImmTy::from_immediate(new_place.to_ref(ecx), val.layout))
} else {
Ok(val.clone())

7
compiler/rustc_const_eval/src/errors.rs
View File

@@ -35,13 +35,10 @@ pub(crate) struct NestedStaticInThreadLocal {
pub span: Span,
}
#[derive(LintDiagnostic)]
#[derive(Diagnostic)]
#[diag(const_eval_mutable_ptr_in_final)]
pub(crate) struct MutablePtrInFinal {
// rust-lang/rust#122153: This was marked as `#[primary_span]` under
// `derive(Diagnostic)`. Since we expect we may hard-error in future, we are
// keeping the field (and skipping it under `derive(LintDiagnostic)`).
#[skip_arg]
#[primary_span]
pub span: Span,
pub kind: InternKind,
}

18
compiler/rustc_const_eval/src/interpret/intern.rs
View File

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

4
compiler/rustc_lint/src/lib.rs
View File

@@ -576,6 +576,10 @@ fn register_builtins(store: &mut LintStore) {
<https://github.com/rust-lang/rust/issues/107457> for more information",
);
store.register_removed("writes_through_immutable_pointer", "converted into hard error");
store.register_removed(
"const_eval_mutable_ptr_in_final_value",
"partially allowed now, otherwise turned into a hard error",
);
}
fn register_internals(store: &mut LintStore) {

46
compiler/rustc_lint_defs/src/builtin.rs
View File

@@ -29,7 +29,6 @@ declare_lint_pass! {
CENUM_IMPL_DROP_CAST,
COHERENCE_LEAK_CHECK,
CONFLICTING_REPR_HINTS,
CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
CONST_EVALUATABLE_UNCHECKED,
CONST_ITEM_MUTATION,
DEAD_CODE,
@@ -2804,51 +2803,6 @@ declare_lint! {
@feature_gate = strict_provenance;
}
declare_lint! {
/// The `const_eval_mutable_ptr_in_final_value` lint detects if a mutable pointer
/// has leaked into the final value of a const expression.
///
/// ### Example
///
/// ```rust
/// pub enum JsValue {
/// Undefined,
/// Object(std::cell::Cell<bool>),
/// }
///
/// impl ::std::ops::Drop for JsValue {
/// fn drop(&mut self) {}
/// }
///
/// const UNDEFINED: &JsValue = &JsValue::Undefined;
///
/// fn main() {
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// In the 1.77 release, the const evaluation machinery adopted some
/// stricter rules to reject expressions with values that could
/// end up holding mutable references to state stored in static memory
/// (which is inherently immutable).
///
/// This is a [future-incompatible] lint to ease the transition to an error.
/// See [issue #122153] for more details.
///
/// [issue #122153]: https://github.com/rust-lang/rust/issues/122153
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE,
Warn,
"detects a mutable pointer that has leaked into final value of a const expression",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
reference: "issue #122153 <https://github.com/rust-lang/rust/issues/122153>",
};
}
declare_lint! {
/// The `const_evaluatable_unchecked` lint detects a generic constant used
/// in a type.

47
compiler/rustc_middle/src/mir/interpret/pointer.rs
View File

@@ -80,14 +80,23 @@ pub trait Provenance: Copy + fmt::Debug + 'static {
}
/// The type of provenance in the compile-time interpreter.
/// This is a packed representation of an `AllocId` and an `immutable: bool`.
/// This is a packed representation of:
/// - an `AllocId` (non-zero)
/// - an `immutable: bool`
/// - a `shared_ref: bool`
///
/// with the extra invariant that if `immutable` is `true`, then so
/// is `shared_ref`.
#[derive(Copy, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub struct CtfeProvenance(NonZero<u64>);
impl From<AllocId> for CtfeProvenance {
fn from(value: AllocId) -> Self {
let prov = CtfeProvenance(value.0);
assert!(!prov.immutable(), "`AllocId` with the highest bit set cannot be used in CTFE");
assert!(
prov.alloc_id() == value,
"`AllocId` with the highest bits set cannot be used in CTFE"
);
prov
}
}
@@ -103,12 +112,14 @@ impl fmt::Debug for CtfeProvenance {
}
const IMMUTABLE_MASK: u64 = 1 << 63; // the highest bit
const SHARED_REF_MASK: u64 = 1 << 62;
const ALLOC_ID_MASK: u64 = u64::MAX & !IMMUTABLE_MASK & !SHARED_REF_MASK;
impl CtfeProvenance {
/// Returns the `AllocId` of this provenance.
#[inline(always)]
pub fn alloc_id(self) -> AllocId {
AllocId(NonZero::new(self.0.get() & !IMMUTABLE_MASK).unwrap())
AllocId(NonZero::new(self.0.get() & ALLOC_ID_MASK).unwrap())
}
/// Returns whether this provenance is immutable.
@@ -117,10 +128,38 @@ impl CtfeProvenance {
self.0.get() & IMMUTABLE_MASK != 0
}
/// Returns whether this provenance is derived from a shared reference.
#[inline]
pub fn shared_ref(self) -> bool {
self.0.get() & SHARED_REF_MASK != 0
}
pub fn into_parts(self) -> (AllocId, bool, bool) {
(self.alloc_id(), self.immutable(), self.shared_ref())
}
pub fn from_parts((alloc_id, immutable, shared_ref): (AllocId, bool, bool)) -> Self {
let prov = CtfeProvenance::from(alloc_id);
if immutable {
// This sets both flags, so we don't even have to check `shared_ref`.
prov.as_immutable()
} else if shared_ref {
prov.as_shared_ref()
} else {
prov
}
}
/// Returns an immutable version of this provenance.
#[inline]
pub fn as_immutable(self) -> Self {
CtfeProvenance(self.0 | IMMUTABLE_MASK)
CtfeProvenance(self.0 | IMMUTABLE_MASK | SHARED_REF_MASK)
}
/// Returns a "shared reference" (but not necessarily immutable!) version of this provenance.
#[inline]
pub fn as_shared_ref(self) -> Self {
CtfeProvenance(self.0 | SHARED_REF_MASK)
}
}

9
compiler/rustc_middle/src/ty/codec.rs
View File

@@ -165,8 +165,7 @@ impl<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>> Encodable<E> for AllocId {
impl<'tcx, E: TyEncoder<I = TyCtxt<'tcx>>> Encodable<E> for CtfeProvenance {
fn encode(&self, e: &mut E) {
self.alloc_id().encode(e);
self.immutable().encode(e);
self.into_parts().encode(e);
}
}
@@ -295,10 +294,8 @@ impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for AllocId {
impl<'tcx, D: TyDecoder<I = TyCtxt<'tcx>>> Decodable<D> for CtfeProvenance {
fn decode(decoder: &mut D) -> Self {
let alloc_id: AllocId = Decodable::decode(decoder);
let prov = CtfeProvenance::from(alloc_id);
let immutable: bool = Decodable::decode(decoder);
if immutable { prov.as_immutable() } else { prov }
let parts = Decodable::decode(decoder);
CtfeProvenance::from_parts(parts)
}
}

4
compiler/rustc_middle/src/ty/impls_ty.rs
View File

@@ -75,11 +75,9 @@ impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::AllocId {
}
}
// CtfeProvenance is an AllocId and a bool.
impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::CtfeProvenance {
fn hash_stable(&self, hcx: &mut StableHashingContext<'a>, hasher: &mut StableHasher) {
self.alloc_id().hash_stable(hcx, hasher);
self.immutable().hash_stable(hcx, hasher);
self.into_parts().hash_stable(hcx, hasher);
}
}