From 97ed8083220f0924434f9c3863a78faa69035a7a Mon Sep 17 00:00:00 2001 From: Lukas Markeffsky <@> Date: Fri, 12 May 2023 23:13:30 +0200 Subject: [PATCH 1/2] remove no-op logic --- .../rustc_mir_transform/src/deduce_param_attrs.rs | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_mir_transform/src/deduce_param_attrs.rs b/compiler/rustc_mir_transform/src/deduce_param_attrs.rs index 8ee08c5be3419..cfd2655f35bfc 100644 --- a/compiler/rustc_mir_transform/src/deduce_param_attrs.rs +++ b/compiler/rustc_mir_transform/src/deduce_param_attrs.rs @@ -29,25 +29,14 @@ impl DeduceReadOnly { } impl<'tcx> Visitor<'tcx> for DeduceReadOnly { - fn visit_local(&mut self, local: Local, mut context: PlaceContext, _: Location) { + fn visit_local(&mut self, local: Local, context: PlaceContext, _location: Location) { // We're only interested in arguments. if local == RETURN_PLACE || local.index() > self.mutable_args.domain_size() { return; } - // Replace place contexts that are moves with copies. This is safe in all cases except - // function argument position, which we already handled in `visit_terminator()` by using the - // ArgumentChecker. See the comment in that method for more details. - // - // In the future, we might want to move this out into a separate pass, but for now let's - // just do it on the fly because that's faster. - if matches!(context, PlaceContext::NonMutatingUse(NonMutatingUseContext::Move)) { - context = PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy); - } - match context { - PlaceContext::MutatingUse(..) - | PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) => { + PlaceContext::MutatingUse(..) => { // This is a mutation, so mark it as such. self.mutable_args.insert(local.index() - 1); } From 9c418e5170a2fb125ae260af1fd94e538201888d Mon Sep 17 00:00:00 2001 From: Lukas Markeffsky <@> Date: Fri, 12 May 2023 19:31:37 +0200 Subject: [PATCH 2/2] allow mutating function args through `&raw const` --- .../src/deduce_param_attrs.rs | 21 +++++++++--- tests/codegen/addr-of-mutate.rs | 34 +++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) create mode 100644 tests/codegen/addr-of-mutate.rs diff --git a/compiler/rustc_mir_transform/src/deduce_param_attrs.rs b/compiler/rustc_mir_transform/src/deduce_param_attrs.rs index cfd2655f35bfc..a133c9d4782c8 100644 --- a/compiler/rustc_mir_transform/src/deduce_param_attrs.rs +++ b/compiler/rustc_mir_transform/src/deduce_param_attrs.rs @@ -8,7 +8,7 @@ use rustc_hir::def_id::LocalDefId; use rustc_index::bit_set::BitSet; use rustc_middle::mir::visit::{NonMutatingUseContext, PlaceContext, Visitor}; -use rustc_middle::mir::{Body, Local, Location, Operand, Terminator, TerminatorKind, RETURN_PLACE}; +use rustc_middle::mir::{Body, Location, Operand, Place, Terminator, TerminatorKind, RETURN_PLACE}; use rustc_middle::ty::{self, DeducedParamAttrs, Ty, TyCtxt}; use rustc_session::config::OptLevel; @@ -29,20 +29,31 @@ impl DeduceReadOnly { } impl<'tcx> Visitor<'tcx> for DeduceReadOnly { - fn visit_local(&mut self, local: Local, context: PlaceContext, _location: Location) { + fn visit_place(&mut self, place: &Place<'tcx>, context: PlaceContext, _location: Location) { // We're only interested in arguments. - if local == RETURN_PLACE || local.index() > self.mutable_args.domain_size() { + if place.local == RETURN_PLACE || place.local.index() > self.mutable_args.domain_size() { return; } - match context { + let mark_as_mutable = match context { PlaceContext::MutatingUse(..) => { // This is a mutation, so mark it as such. - self.mutable_args.insert(local.index() - 1); + true + } + PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf) => { + // Whether mutating though a `&raw const` is allowed is still undecided, so we + // disable any sketchy `readonly` optimizations for now. + // But we only need to do this if the pointer would point into the argument. + !place.is_indirect() } PlaceContext::NonMutatingUse(..) | PlaceContext::NonUse(..) => { // Not mutating, so it's fine. + false } + }; + + if mark_as_mutable { + self.mutable_args.insert(place.local.index() - 1); } } diff --git a/tests/codegen/addr-of-mutate.rs b/tests/codegen/addr-of-mutate.rs new file mode 100644 index 0000000000000..bea1aad235242 --- /dev/null +++ b/tests/codegen/addr-of-mutate.rs @@ -0,0 +1,34 @@ +// compile-flags: -C opt-level=3 -C no-prepopulate-passes +// min-llvm-version: 15.0 (for opaque pointers) + +#![crate_type = "lib"] + +// Test for the absence of `readonly` on the argument when it is mutated via `&raw const`. +// See . + +// CHECK: i8 @foo(ptr noalias nocapture noundef dereferenceable(128) %x) +#[no_mangle] +pub fn foo(x: [u8; 128]) -> u8 { + let ptr = core::ptr::addr_of!(x).cast_mut(); + unsafe { + (*ptr)[0] = 1; + } + x[0] +} + +// CHECK: i1 @second(ptr noalias nocapture noundef dereferenceable({{[0-9]+}}) %a_ptr_and_b) +#[no_mangle] +pub unsafe fn second(a_ptr_and_b: (*mut (i32, bool), (i64, bool))) -> bool { + let b_bool_ptr = core::ptr::addr_of!(a_ptr_and_b.1.1).cast_mut(); + (*b_bool_ptr) = true; + a_ptr_and_b.1.1 +} + +// If going through a deref (and there are no other mutating accesses), then `readonly` is fine. +// CHECK: i1 @third(ptr noalias nocapture noundef readonly dereferenceable({{[0-9]+}}) %a_ptr_and_b) +#[no_mangle] +pub unsafe fn third(a_ptr_and_b: (*mut (i32, bool), (i64, bool))) -> bool { + let b_bool_ptr = core::ptr::addr_of!((*a_ptr_and_b.0).1).cast_mut(); + (*b_bool_ptr) = true; + a_ptr_and_b.1.1 +}