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

Conversation

lukas-code
Copy link
Member

Fixes #111502 by "turning off the sketchy optimization while we figure out if this is ok", like @JakobDegen said.

The first commit in this PR removes some suspicious looking logic from the same method, but should have no functional changes, since it doesn't modify the context outside of the method. Best reviewed commit by commit.

r? opsem

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 12, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

Opsem can help analyze whether an opt is correct, but at least I lack the experience to actually review their implementations.

r? mir-opt

@rustbot rustbot assigned wesleywiser and unassigned RalfJung May 13, 2023
@tmiasko
Copy link
Contributor

tmiasko commented May 13, 2023

Thanks.

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2023

📌 Commit 9c418e5 has been approved by tmiasko

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2023
@bors
Copy link
Contributor

bors commented May 14, 2023

⌛ Testing commit 9c418e5 with merge 3603a84...

@bors
Copy link
Contributor

bors commented May 14, 2023

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing 3603a84 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 14, 2023
@bors bors merged commit 3603a84 into rust-lang:master May 14, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 14, 2023
@lukas-code lukas-code deleted the addr-of-mutate branch May 14, 2023 13:40
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3603a84): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 660.495s -> 659.942s (-0.08%)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mutating through addr_of produces LLVM IR with UB
7 participants