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.

Sign up for GitHub

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

Jump to bottom

Retag as FnEntry on drop_in_place #103957

Merged
merged 4 commits into from
Dec 22, 2022
Merged

Retag as FnEntry on drop_in_place #103957

merged 4 commits into from
Dec 22, 2022

Conversation

JakobDegen
Copy link
Contributor

@JakobDegen JakobDegen commented Nov 4, 2022 •
edited
Loading

This commit changes the mir drop shim to always retag its argument as if it were a &mut.

cc rust-lang/unsafe-code-guidelines#373

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2022

r? @nagisa

(rustbot has picked a reviewer for you, use r? to override)

@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 Nov 4, 2022
@JakobDegen
Copy link
Contributor Author

Eh, sorry, forgot to r? ghost, feel free to unassign yourself

@rustbot
Copy link
Collaborator

rustbot commented Nov 4, 2022

Failed to set assignee to ghost: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@RalfJung
Copy link
Member

RalfJung commented Nov 5, 2022

Yeah assigning ghost isn't a thing any more with triagebot, I think...

@JakobDegen JakobDegen force-pushed the drop-retag branch 2 times, most recently from 9d269f6 to c4b5c4d Compare November 12, 2022 01:32
@JakobDegen
Copy link
Contributor Author

@saethlin had tested this and found no regressions in the ecosystem, so seems reasonable to have. Marking as ready:

r? @RalfJung

The MIR gen for this is covered by an existing test. I'd also like to add a test to ensure that we find the UB in Miri - should I be adding that to the miri subtree in this PR or submitting a separate PR to rust-lang/miri?

@JakobDegen JakobDegen marked this pull request as ready for review November 12, 2022 01:34
@rustbot
Copy link
Collaborator

rustbot commented Nov 12, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

RalfJung commented Nov 13, 2022 via email

RalfJung
RalfJung reviewed Nov 13, 2022
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

LGTM, apart from the missing test.

compiler/rustc_mir_transform/src/shim.rs Outdated Show resolved Hide resolved
@RalfJung RalfJung mentioned this pull request Nov 18, 2022
Closed
@RalfJung
Copy link
Member

In fact we probably want at least 2 tests -- one for the aliasing concerns and one for the validity (aligned and non-null).

@RalfJung
Copy link
Member

@rustbot author
@JakobDegen let me know if you need help adding the test. :)

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2022

The Miri subtree was changed

cc @rust-lang/miri

@JakobDegen
Copy link
Contributor Author

Sorry for the delay, was traveling. Tests added. @rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@JakobDegen
Copy link
Contributor Author

Rebased and attached a commit to the end. Drop terminators are now a complete nop if the type has no drop glue and are documented as such.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 21, 2022
RalfJung
RalfJung reviewed Dec 22, 2022
View reviewed changes
compiler/rustc_middle/src/mir/syntax.rs
Comment on lines +566 to +568
/// After drop elaboration: `Drop` terminators are a complete nop for types that have no drop
/// glue. For other types, `Drop` terminators behave exactly like a call to
/// `core::mem::drop_in_place` with a pointer to the given place.
Copy link
Member

Choose a reason for hiding this comment

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

This is the key change in the MIR spec here. It reflects what the codegen backends already do.

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 22, 2022

📌 Commit 0229281 has been approved by RalfJung

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 Dec 22, 2022
@RalfJung RalfJung mentioned this pull request Dec 22, 2022
Closed
@bors
Copy link
Contributor

bors commented Dec 22, 2022

⌛ Testing commit 0229281 with merge cca80b9...

@bors
Copy link
Contributor

bors commented Dec 22, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing cca80b9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2022
@bors bors merged commit cca80b9 into rust-lang:master Dec 22, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 22, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cca80b9): 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)

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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.4% [-3.4%, -3.4%] 1
All ❌✅ (primary) - - 0

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)		 2.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.0% [2.0%, 2.0%] 1

@RalfJung RalfJung mentioned this pull request Dec 23, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 23, 2022
@matthiaskrgr
Rollup merge of rust-lang#106084 - RalfJung:into-iter, r=thomcc
3b4acbc 
fix vec::IntoIter::drop on high-alignment ZST

This fixes a soundness bug: IntoIter would call `drop_in_place` on an insufficiently aligned pointer. So if a ZST with alignment greater 1 had drop glue, that would be called with an unaligned reference. Since rust-lang#103957, Miri checks alignment even if the type does not have drop glue, which is how this bug was found.

r? `@thomcc`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 23, 2022
@matthiaskrgr
Rollup merge of rust-lang#106084 - RalfJung:into-iter, r=thomcc
61fadf8 
fix vec::IntoIter::drop on high-alignment ZST

This fixes a soundness bug: IntoIter would call `drop_in_place` on an insufficiently aligned pointer. So if a ZST with alignment greater 1 had drop glue, that would be called with an unaligned reference. Since rust-lang#103957, Miri checks alignment even if the type does not have drop glue, which is how this bug was found.

r? ``@thomcc``
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 24, 2022
@matthiaskrgr
Rollup merge of #106084 - RalfJung:into-iter, r=thomcc
c765c7e 
fix vec::IntoIter::drop on high-alignment ZST

This fixes a soundness bug: IntoIter would call `drop_in_place` on an insufficiently aligned pointer. So if a ZST with alignment greater 1 had drop glue, that would be called with an unaligned reference. Since rust-lang/rust#103957, Miri checks alignment even if the type does not have drop glue, which is how this bug was found.

r? ``@thomcc``
@JakobDegen JakobDegen deleted the drop-retag branch January 1, 2023 21:27
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#103957 - JakobDegen:drop-retag, r=RalfJung
74843d3 
Retag as FnEntry on `drop_in_place`

This commit changes the mir drop shim to always retag its argument as if it were a `&mut`.

cc rust-lang/unsafe-code-guidelines#373
@JakobDegen JakobDegen mentioned this pull request Jan 27, 2023
Merged
@erikdesjardins erikdesjardins mentioned this pull request May 20, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2023
@bors
Auto merge of rust-lang#111807 - erikdesjardins:noalias, r=oli-obk
f3d597b 
[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes

This resurrects PR rust-lang#103614, which has sat idle for a while.

This could probably use a new perf run, since we're on a new LLVM version now.

r? `@oli-obk`
cc `@RalfJung`

---

LLVM can make use of the `noalias` parameter attribute on the parameter to `drop_in_place` in areas like argument promotion. Because the Rust compiler fully controls the code for `drop_in_place`, it can soundly deduce parameter attributes on it.

In rust-lang#103957, Miri was changed to retag `drop_in_place`'s argument as if it was `&mut`, matching this change.
@lukas-code lukas-code mentioned this pull request Oct 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

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
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.
9 participants
@JakobDegen @rustbot @RalfJung @rust-log-analyzer @bjorn3 @saethlin @bors @rust-timer @nagisa