Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow mutating function args through &raw const #111517

Merged
merged 2 commits into from
May 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions compiler/rustc_mir_transform/src/deduce_param_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -29,31 +29,31 @@ impl DeduceReadOnly {
}

impl<'tcx> Visitor<'tcx> for DeduceReadOnly {
fn visit_local(&mut self, local: Local, mut context: PlaceContext, _: 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;
}

// 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) => {
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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it okay to consider indirect places immutable? I have no idea what kind of code this affects, but it seems very suspicious to me that whether or not the place is indirect would make any difference.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have

fn foo(arg: *const u8) { /* ... */ }

then addr_of!(*arg) can't change the value of arg, only addr_of!(arg) can.

In practice, this doesn't actually make a difference and we could always return true here, because currently

  1. single pointers are never passed indirectly
  2. when accessing a pointer in a compound type, e.g. addr_of!(*(arg.ptr)) this operation is split in two mir statements let tmp = deref_copy arg.ptr and &raw const *tmp and so this &raw const already doesn't affect whether arg is readonly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then addr_of!(*arg) can't change the value of arg, only addr_of!(arg) can.

This should apply uniformly to &[raw] [mut|const] (including regular non-raw borrows) then, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to be correct. In that case I'd probably just remove the special case here (and just return true) rather than introduce more special casing, because it doesn't actually matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

}
PlaceContext::NonMutatingUse(..) | PlaceContext::NonUse(..) => {
// Not mutating, so it's fine.
false
}
};

if mark_as_mutable {
self.mutable_args.insert(place.local.index() - 1);
}
}

Expand Down
34 changes: 34 additions & 0 deletions tests/codegen/addr-of-mutate.rs
Original file line number Diff line number Diff line change
@@ -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 <https://github.com/rust-lang/rust/issues/111502>.

// 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
}