bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #120932 - RalfJung:mut-ptr-to-static, r=oli-obk

Browse source 
const_mut_refs: allow mutable pointers to statics

Fixes https://github.com/rust-lang/rust/issues/118447

Writing this PR became a bit messy, see [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Statics.20pointing.20to.20interior.20mutable.20statics) for some of my journey.^^ Turns out there was a long-standing bug in our qualif logic that led to us incorrectly classifying certain places as "no interior mut" when they actually had interior mut. Due to that the `const_refs_to_cell` feature gate was required far less often than it otherwise would, e.g. in [this code](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=9e0c042c451b3d11d64dd6263679a164). Fixing this however would be a massive breaking change all over libcore and likely the wider ecosystem. So I also changed the const-checking logic to just not require the feature gate for the affected cases. While doing so I discovered a bunch of logic that is not explained and that I could not explain. However I think stabilizing some const-eval feature will make most of that logic inconsequential so I just added some FIXMEs and left it at that.

r? `@oli-obk`
This commit is contained in:
Guillaume Boisseau 2024-02-17 11:23:03 +01:00 • committed by GitHub
commit f70f13a1d3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 173 additions and 62 deletions
Download patch file Download diff file Expand all files Collapse all files

37
compiler/rustc_const_eval/src/transform/check_consts/check.rs
View file

@ -344,7 +344,7 @@ impl<'mir, 'tcx> Checker<'mir, 'tcx> {
visitor.visit_ty(ty);
}
fn check_mut_borrow(&mut self, local: Local, kind: hir::BorrowKind) {
fn check_mut_borrow(&mut self, place: &Place<'_>, kind: hir::BorrowKind) {
match self.const_kind() {
// In a const fn all borrows are transient or point to the places given via
// references in the arguments (so we already checked them with
@ -355,10 +355,19 @@ impl<'mir, 'tcx> Checker<'mir, 'tcx> {
// to mutable memory.
hir::ConstContext::ConstFn => self.check_op(ops::TransientMutBorrow(kind)),
_ => {
// For indirect places, we are not creating a new permanent borrow, it's just as
// transient as the already existing one. For reborrowing references this is handled
// at the top of `visit_rvalue`, but for raw pointers we handle it here.
// Pointers/references to `static mut` and cases where the `*` is not the first
// projection also end up here.
// Locals with StorageDead do not live beyond the evaluation and can
// thus safely be borrowed without being able to be leaked to the final
// value of the constant.
if self.local_has_storage_dead(local) {
// 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 place.is_indirect() || self.local_has_storage_dead(place.local) {
self.check_op(ops::TransientMutBorrow(kind));
} else {
self.check_op(ops::MutBorrow(kind));
@ -390,6 +399,11 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
trace!("visit_rvalue: rvalue={:?} location={:?}", rvalue, location);
// Special-case reborrows to be more like a copy of a reference.
// FIXME: this does not actually handle all reborrows. It only detects cases where `*` is the outermost
// projection of the borrowed place, it skips deref'ing raw pointers and it skips `static`.
// All those cases are handled below with shared/mutable borrows.
// Once `const_mut_refs` is stable, we should be able to entirely remove this special case.
// (`const_refs_to_cell` is not needed, we already allow all borrows of indirect places anyway.)
match *rvalue {
Rvalue::Ref(_, kind, place) => {
if let Some(reborrowed_place_ref) = place_as_reborrow(self.tcx, self.body, place) {
@ -460,7 +474,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
if !is_allowed {
self.check_mut_borrow(
place.local,
place,
if matches!(rvalue, Rvalue::Ref(..)) {
hir::BorrowKind::Ref
} else {
@ -478,7 +492,14 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
place.as_ref(),
);
if borrowed_place_has_mut_interior {
// If the place is indirect, this is basically a reborrow. We have a reborrow
// special case above, but for raw pointers and pointers/references to `static` and
// when the `*` is not the first projection, `place_as_reborrow` does not recognize
// them as such, so we end up here. This should probably be considered a
// `TransientCellBorrow` (we consider the equivalent mutable case a
// `TransientMutBorrow`), but such reborrows got accidentally stabilized already and
// it is too much of a breaking change to take back.
if borrowed_place_has_mut_interior && !place.is_indirect() {
match self.const_kind() {
// In a const fn all borrows are transient or point to the places given via
// references in the arguments (so we already checked them with
@ -495,6 +516,8 @@ 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 self.local_has_storage_dead(place.local) {
self.check_op(ops::TransientCellBorrow);
} else {
@ -948,6 +971,12 @@ fn place_as_reborrow<'tcx>(
) -> Option<PlaceRef<'tcx>> {
match place.as_ref().last_projection() {
Some((place_base, ProjectionElem::Deref)) => {
// FIXME: why do statics and raw pointers get excluded here? This makes
// some code involving mutable pointers unstable, but it is unclear
// why that code is treated differently from mutable references.
// Once TransientMutBorrow and TransientCellBorrow are stable,
// this can probably be cleaned up without any behavioral changes.
// A borrow of a `static` also looks like `&(*_1)` in the MIR, but `_1` is a `const`
// that points to the allocation for the static. Don't treat these as reborrows.
if body.local_decls[place_base.local].is_ref_to_static() {

24
compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs
View file

@ -74,6 +74,13 @@ pub trait Qualif {
adt: AdtDef<'tcx>,
args: GenericArgsRef<'tcx>,
) -> bool;
/// Returns `true` if this `Qualif` behaves sructurally for pointers and references:
/// the pointer/reference qualifies if and only if the pointee qualifies.
///
/// (This is currently `false` for all our instances, but that may change in the future. Also,
/// by keeping it abstract, the handling of `Deref` in `in_place` becomes more clear.)
fn deref_structural<'tcx>(cx: &ConstCx<'_, 'tcx>) -> bool;
}
/// Constant containing interior mutability (`UnsafeCell<T>`).
@ -103,6 +110,10 @@ impl Qualif for HasMutInterior {
// It arises structurally for all other types.
adt.is_unsafe_cell()
}
fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
false
}
}
/// Constant containing an ADT that implements `Drop`.
@ -131,6 +142,10 @@ impl Qualif for NeedsDrop {
) -> bool {
adt.has_dtor(cx.tcx)
}
fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
false
}
}
/// Constant containing an ADT that implements non-const `Drop`.
@ -210,6 +225,10 @@ impl Qualif for NeedsNonConstDrop {
) -> bool {
adt.has_non_const_dtor(cx.tcx)
}
fn deref_structural<'tcx>(_cx: &ConstCx<'_, 'tcx>) -> bool {
false
}
}
// FIXME: Use `mir::visit::Visitor` for the `in_*` functions if/when it supports early return.
@ -303,6 +322,11 @@ where
return false;
}
if matches!(elem, ProjectionElem::Deref) && !Q::deref_structural(cx) {
// We have to assume that this qualifies.
return true;
}
place = place_base;
}