From d17bd91964b318076e952ba6d2db91a5d601b56d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 11 Feb 2024 14:27:08 +0100 Subject: [PATCH] const_mut_refs: allow mutable refs to statics --- .../src/transform/check_consts/check.rs | 37 ++++++++++++++-- .../src/transform/check_consts/qualifs.rs | 24 +++++++++++ .../const-ref-to-static-linux-vtable.rs | 43 +++++++++++++++++++ .../ui/consts/issue-17718-const-bad-values.rs | 1 + .../issue-17718-const-bad-values.stderr | 12 +++++- tests/ui/consts/miri_unleashed/box.stderr | 2 +- .../mutable_references_err.32bit.stderr | 2 +- .../mutable_references_err.64bit.stderr | 2 +- tests/ui/consts/mut-ptr-to-static.rs | 40 +++++++++++++++++ tests/ui/error-codes/E0017.rs | 8 ++-- tests/ui/error-codes/E0017.stderr | 36 +++------------- tests/ui/error-codes/E0388.rs | 4 +- tests/ui/error-codes/E0388.stderr | 24 +++-------- 13 files changed, 173 insertions(+), 62 deletions(-) create mode 100644 tests/ui/consts/const-ref-to-static-linux-vtable.rs create mode 100644 tests/ui/consts/mut-ptr-to-static.rs diff --git a/compiler/rustc_const_eval/src/transform/check_consts/check.rs b/compiler/rustc_const_eval/src/transform/check_consts/check.rs index 28dc69859fd77..cc4a410cd704c 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/check.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/check.rs @@ -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 { @@ -946,6 +969,12 @@ fn place_as_reborrow<'tcx>( ) -> Option> { 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() { diff --git a/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs b/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs index 67fef20807910..7eb3c181d6958 100644 --- a/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs +++ b/compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs @@ -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`). @@ -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; } diff --git a/tests/ui/consts/const-ref-to-static-linux-vtable.rs b/tests/ui/consts/const-ref-to-static-linux-vtable.rs new file mode 100644 index 0000000000000..1e2c3bcfefae1 --- /dev/null +++ b/tests/ui/consts/const-ref-to-static-linux-vtable.rs @@ -0,0 +1,43 @@ +// check-pass +//! This is the reduced version of the "Linux kernel vtable" use-case. +#![feature(const_mut_refs, const_refs_to_static)] +use std::ptr::addr_of_mut; + +#[repr(C)] +struct ThisModule(i32); + +trait Module { + const THIS_MODULE_PTR: *mut ThisModule; +} + +struct MyModule; + +// Generated by a macro. +extern "C" { + static mut THIS_MODULE: ThisModule; +} + +// Generated by a macro. +impl Module for MyModule { + const THIS_MODULE_PTR: *mut ThisModule = unsafe { addr_of_mut!(THIS_MODULE) }; +} + +struct Vtable { + module: *mut ThisModule, + foo_fn: fn(*mut ()) -> i32, +} + +trait Foo { + type Mod: Module; + + fn foo(&mut self) -> i32; +} + +fn generate_vtable() -> &'static Vtable { + &Vtable { + module: T::Mod::THIS_MODULE_PTR, + foo_fn: |ptr| unsafe { &mut *ptr.cast::() }.foo(), + } +} + +fn main() {} diff --git a/tests/ui/consts/issue-17718-const-bad-values.rs b/tests/ui/consts/issue-17718-const-bad-values.rs index af50fed972df8..8fb1e8fa11a28 100644 --- a/tests/ui/consts/issue-17718-const-bad-values.rs +++ b/tests/ui/consts/issue-17718-const-bad-values.rs @@ -5,6 +5,7 @@ static mut S: usize = 3; const C2: &'static mut usize = unsafe { &mut S }; //~^ ERROR: referencing statics in constants //~| ERROR: referencing statics in constants +//~| ERROR: mutable references are not allowed //~| WARN mutable reference of mutable static is discouraged [static_mut_ref] fn main() {} diff --git a/tests/ui/consts/issue-17718-const-bad-values.stderr b/tests/ui/consts/issue-17718-const-bad-values.stderr index cda94490155c0..df053cfd885e6 100644 --- a/tests/ui/consts/issue-17718-const-bad-values.stderr +++ b/tests/ui/consts/issue-17718-const-bad-values.stderr @@ -44,7 +44,17 @@ LL | const C2: &'static mut usize = unsafe { &mut S }; = help: to fix this, the value can be extracted to a `const` and then used. = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` -error: aborting due to 3 previous errors; 1 warning emitted +error[E0658]: mutable references are not allowed in constants + --> $DIR/issue-17718-const-bad-values.rs:5:41 + | +LL | const C2: &'static mut usize = unsafe { &mut S }; + | ^^^^^^ + | + = note: see issue #57349 for more information + = help: add `#![feature(const_mut_refs)]` to the crate attributes to enable + = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date + +error: aborting due to 4 previous errors; 1 warning emitted Some errors have detailed explanations: E0658, E0764. For more information about an error, try `rustc --explain E0658`. diff --git a/tests/ui/consts/miri_unleashed/box.stderr b/tests/ui/consts/miri_unleashed/box.stderr index 5229f1e50cd44..a0518c99cda56 100644 --- a/tests/ui/consts/miri_unleashed/box.stderr +++ b/tests/ui/consts/miri_unleashed/box.stderr @@ -16,7 +16,7 @@ help: skipping check for `const_mut_refs` feature | LL | &mut *(Box::new(0)) | ^^^^^^^^^^^^^^^^^^^ -help: skipping check that does not even have a feature gate +help: skipping check for `const_mut_refs` feature --> $DIR/box.rs:8:5 | LL | &mut *(Box::new(0)) diff --git a/tests/ui/consts/miri_unleashed/mutable_references_err.32bit.stderr b/tests/ui/consts/miri_unleashed/mutable_references_err.32bit.stderr index b60f9a24f8c7a..02ad42e3caec9 100644 --- a/tests/ui/consts/miri_unleashed/mutable_references_err.32bit.stderr +++ b/tests/ui/consts/miri_unleashed/mutable_references_err.32bit.stderr @@ -119,7 +119,7 @@ help: skipping check for `const_refs_to_static` feature | LL | const SUBTLE: &mut i32 = unsafe { &mut FOO }; | ^^^ -help: skipping check that does not even have a feature gate +help: skipping check for `const_mut_refs` feature --> $DIR/mutable_references_err.rs:32:35 | LL | const SUBTLE: &mut i32 = unsafe { &mut FOO }; diff --git a/tests/ui/consts/miri_unleashed/mutable_references_err.64bit.stderr b/tests/ui/consts/miri_unleashed/mutable_references_err.64bit.stderr index 1e5d4bd890b99..a7de7fe137e77 100644 --- a/tests/ui/consts/miri_unleashed/mutable_references_err.64bit.stderr +++ b/tests/ui/consts/miri_unleashed/mutable_references_err.64bit.stderr @@ -119,7 +119,7 @@ help: skipping check for `const_refs_to_static` feature | LL | const SUBTLE: &mut i32 = unsafe { &mut FOO }; | ^^^ -help: skipping check that does not even have a feature gate +help: skipping check for `const_mut_refs` feature --> $DIR/mutable_references_err.rs:32:35 | LL | const SUBTLE: &mut i32 = unsafe { &mut FOO }; diff --git a/tests/ui/consts/mut-ptr-to-static.rs b/tests/ui/consts/mut-ptr-to-static.rs new file mode 100644 index 0000000000000..0efe9fa0ded50 --- /dev/null +++ b/tests/ui/consts/mut-ptr-to-static.rs @@ -0,0 +1,40 @@ +// run-pass +#![feature(const_mut_refs)] +#![feature(sync_unsafe_cell)] + +use std::cell::SyncUnsafeCell; +use std::ptr; + +#[repr(C)] +struct SyncPtr { + foo: *mut u32, +} +unsafe impl Sync for SyncPtr {} + +static mut STATIC: u32 = 42; + +static INTERIOR_MUTABLE_STATIC: SyncUnsafeCell = SyncUnsafeCell::new(42); + +// A static that mutably points to STATIC. +static PTR: SyncPtr = SyncPtr { + foo: unsafe { ptr::addr_of_mut!(STATIC) }, +}; +static INTERIOR_MUTABLE_PTR: SyncPtr = SyncPtr { + foo: ptr::addr_of!(INTERIOR_MUTABLE_STATIC) as *mut u32, +}; + +fn main() { + let ptr = PTR.foo; + unsafe { + assert_eq!(*ptr, 42); + *ptr = 0; + assert_eq!(*PTR.foo, 0); + } + + let ptr = INTERIOR_MUTABLE_PTR.foo; + unsafe { + assert_eq!(*ptr, 42); + *ptr = 0; + assert_eq!(*INTERIOR_MUTABLE_PTR.foo, 0); + } +} diff --git a/tests/ui/error-codes/E0017.rs b/tests/ui/error-codes/E0017.rs index 9d3433fa543fd..c128c2779e245 100644 --- a/tests/ui/error-codes/E0017.rs +++ b/tests/ui/error-codes/E0017.rs @@ -1,3 +1,5 @@ +#![feature(const_mut_refs)] + static X: i32 = 1; const C: i32 = 2; static mut M: i32 = 3; @@ -5,14 +7,12 @@ static mut M: i32 = 3; const CR: &'static mut i32 = &mut C; //~ ERROR mutable references are not allowed //~| WARN taking a mutable -static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR E0658 -//~| ERROR cannot borrow -//~| ERROR mutable references are not allowed +static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR cannot borrow immutable static item `X` as mutable static CONST_REF: &'static mut i32 = &mut C; //~ ERROR mutable references are not allowed //~| WARN taking a mutable -static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; //~ ERROR mutable references are not +static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; //~^ WARN mutable reference of mutable static is discouraged [static_mut_ref] fn main() {} diff --git a/tests/ui/error-codes/E0017.stderr b/tests/ui/error-codes/E0017.stderr index 2a70f2ee0ae82..eb626a7fe3a98 100644 --- a/tests/ui/error-codes/E0017.stderr +++ b/tests/ui/error-codes/E0017.stderr @@ -14,7 +14,7 @@ LL | static STATIC_MUT_REF: &'static mut i32 = unsafe { addr_of_mut!(M) }; | ~~~~~~~~~~~~~~~ warning: taking a mutable reference to a `const` item - --> $DIR/E0017.rs:5:30 + --> $DIR/E0017.rs:7:30 | LL | const CR: &'static mut i32 = &mut C; | ^^^^^^ @@ -22,36 +22,20 @@ LL | const CR: &'static mut i32 = &mut C; = note: each usage of a `const` item creates a new temporary = note: the mutable reference will refer to this temporary, not the original `const` item note: `const` item defined here - --> $DIR/E0017.rs:2:1 + --> $DIR/E0017.rs:4:1 | LL | const C: i32 = 2; | ^^^^^^^^^^^^ = note: `#[warn(const_item_mutation)]` on by default error[E0764]: mutable references are not allowed in the final value of constants - --> $DIR/E0017.rs:5:30 + --> $DIR/E0017.rs:7:30 | LL | const CR: &'static mut i32 = &mut C; | ^^^^^^ -error[E0658]: mutation through a reference is not allowed in statics - --> $DIR/E0017.rs:8:39 - | -LL | static STATIC_REF: &'static mut i32 = &mut X; - | ^^^^^^ - | - = note: see issue #57349 for more information - = help: add `#![feature(const_mut_refs)]` to the crate attributes to enable - = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date - -error[E0764]: mutable references are not allowed in the final value of statics - --> $DIR/E0017.rs:8:39 - | -LL | static STATIC_REF: &'static mut i32 = &mut X; - | ^^^^^^ - error[E0596]: cannot borrow immutable static item `X` as mutable - --> $DIR/E0017.rs:8:39 + --> $DIR/E0017.rs:10:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ cannot borrow as mutable @@ -65,7 +49,7 @@ LL | static CONST_REF: &'static mut i32 = &mut C; = note: each usage of a `const` item creates a new temporary = note: the mutable reference will refer to this temporary, not the original `const` item note: `const` item defined here - --> $DIR/E0017.rs:2:1 + --> $DIR/E0017.rs:4:1 | LL | const C: i32 = 2; | ^^^^^^^^^^^^ @@ -76,13 +60,7 @@ error[E0764]: mutable references are not allowed in the final value of statics LL | static CONST_REF: &'static mut i32 = &mut C; | ^^^^^^ -error[E0764]: mutable references are not allowed in the final value of statics - --> $DIR/E0017.rs:15:52 - | -LL | static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; - | ^^^^^^ - -error: aborting due to 6 previous errors; 3 warnings emitted +error: aborting due to 3 previous errors; 3 warnings emitted -Some errors have detailed explanations: E0596, E0658, E0764. +Some errors have detailed explanations: E0596, E0764. For more information about an error, try `rustc --explain E0596`. diff --git a/tests/ui/error-codes/E0388.rs b/tests/ui/error-codes/E0388.rs index 6049d95f0d277..bd371328e6bc9 100644 --- a/tests/ui/error-codes/E0388.rs +++ b/tests/ui/error-codes/E0388.rs @@ -3,9 +3,7 @@ const C: i32 = 2; const CR: &'static mut i32 = &mut C; //~ ERROR mutable references are not allowed //~| WARN taking a mutable -static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR cannot borrow - //~| ERROR E0658 - //~| ERROR mutable references are not allowed +static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR E0658 static CONST_REF: &'static mut i32 = &mut C; //~ ERROR mutable references are not allowed //~| WARN taking a mutable diff --git a/tests/ui/error-codes/E0388.stderr b/tests/ui/error-codes/E0388.stderr index 1f7b688899ed0..3e89e3f804b21 100644 --- a/tests/ui/error-codes/E0388.stderr +++ b/tests/ui/error-codes/E0388.stderr @@ -19,7 +19,7 @@ error[E0764]: mutable references are not allowed in the final value of constants LL | const CR: &'static mut i32 = &mut C; | ^^^^^^ -error[E0658]: mutation through a reference is not allowed in statics +error[E0658]: mutable references are not allowed in statics --> $DIR/E0388.rs:6:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; @@ -29,20 +29,8 @@ LL | static STATIC_REF: &'static mut i32 = &mut X; = help: add `#![feature(const_mut_refs)]` to the crate attributes to enable = note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date -error[E0764]: mutable references are not allowed in the final value of statics - --> $DIR/E0388.rs:6:39 - | -LL | static STATIC_REF: &'static mut i32 = &mut X; - | ^^^^^^ - -error[E0596]: cannot borrow immutable static item `X` as mutable - --> $DIR/E0388.rs:6:39 - | -LL | static STATIC_REF: &'static mut i32 = &mut X; - | ^^^^^^ cannot borrow as mutable - warning: taking a mutable reference to a `const` item - --> $DIR/E0388.rs:10:38 + --> $DIR/E0388.rs:8:38 | LL | static CONST_REF: &'static mut i32 = &mut C; | ^^^^^^ @@ -56,12 +44,12 @@ LL | const C: i32 = 2; | ^^^^^^^^^^^^ error[E0764]: mutable references are not allowed in the final value of statics - --> $DIR/E0388.rs:10:38 + --> $DIR/E0388.rs:8:38 | LL | static CONST_REF: &'static mut i32 = &mut C; | ^^^^^^ -error: aborting due to 5 previous errors; 2 warnings emitted +error: aborting due to 3 previous errors; 2 warnings emitted -Some errors have detailed explanations: E0596, E0658, E0764. -For more information about an error, try `rustc --explain E0596`. +Some errors have detailed explanations: E0658, E0764. +For more information about an error, try `rustc --explain E0658`.