Retag as FnEntry on `drop_in_place` by JakobDegen · Pull Request #103957 · rust-lang/rust

JakobDegen · 2022-11-04T07:07:53Z

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 · 2022-11-04T07:07:58Z

r? @nagisa

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

JakobDegen · 2022-11-04T07:08:19Z

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

rustbot · 2022-11-04T07:08:21Z

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.

compiler/rustc_mir_transform/src/shim.rs

RalfJung · 2022-11-05T09:44:08Z

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

JakobDegen · 2022-11-12T01:34:07Z

@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?

rustbot · 2022-11-12T01:34:13Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2022-11-13T15:53:06Z

Please add the miri test in the same PR. It's great that we can finally do this. :)

RalfJung

LGTM, apart from the missing test.

compiler/rustc_mir_transform/src/shim.rs

RalfJung · 2022-11-19T10:52:49Z

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

RalfJung · 2022-11-26T09:12:20Z

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

rustbot · 2022-11-27T00:11:32Z

The Miri subtree was changed

cc @rust-lang/miri

JakobDegen · 2022-11-27T00:11:53Z

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

JakobDegen · 2022-12-21T23:24:33Z

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

…onst eval

RalfJung · 2022-12-22T15:15:05Z

compiler/rustc_middle/src/mir/syntax.rs

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


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

RalfJung · 2022-12-22T15:15:30Z

@bors r+

bors · 2022-12-22T15:15:32Z

📌 Commit 0229281 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2022-12-22T17:48:46Z

⌛ Testing commit 0229281 with merge cca80b9...

bors · 2022-12-22T20:48:27Z

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

rust-timer · 2022-12-22T23:12:44Z

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

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`

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``

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``

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

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

rustbot assigned nagisa Nov 4, 2022

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

compiler-errors unassigned nagisa Nov 4, 2022

RalfJung reviewed Nov 5, 2022

View reviewed changes

compiler/rustc_mir_transform/src/shim.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 5, 2022

View reviewed changes

compiler/rustc_mir_transform/src/shim.rs Outdated Show resolved Hide resolved

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

rustbot assigned RalfJung Nov 12, 2022

JakobDegen marked this pull request as ready for review November 12, 2022 01:34

RalfJung reviewed Nov 13, 2022

View reviewed changes

compiler/rustc_mir_transform/src/shim.rs Outdated Show resolved Hide resolved

RalfJung mentioned this pull request Nov 18, 2022

[rustc_ty_utils] Add the LLVM noalias parameter attribute to drop_in_place in certain cases. #103614

Closed

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

JakobDegen force-pushed the drop-retag branch from c4b5c4d to 8c9d9df Compare November 27, 2022 00:11

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

This comment has been minimized.

Sign in to view

JakobDegen force-pushed the drop-retag branch from 8c9d9df to 143475e Compare November 27, 2022 03:04

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

JakobDegen force-pushed the drop-retag branch from 0fa3a32 to 8f3584f Compare December 21, 2022 23:28

Don't run Drop terminators on types that do not have drop glue in c…

0229281

…onst eval

JakobDegen force-pushed the drop-retag branch from 8f3584f to 0229281 Compare December 22, 2022 00:16

RalfJung reviewed Dec 22, 2022

View reviewed changes

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 mentioned this pull request Dec 22, 2022

Add test ensuring that drop terminator of no-drop type is a NOP rust-lang/miri#2737

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2022

bors merged commit cca80b9 into rust-lang:master Dec 22, 2022

rustbot added this to the 1.68.0 milestone Dec 22, 2022

RalfJung mentioned this pull request Dec 23, 2022

fix vec::IntoIter::drop on high-alignment ZST #106084

Merged

JakobDegen deleted the drop-retag branch January 1, 2023 21:27

JakobDegen mentioned this pull request Jan 27, 2023

Treat Drop as a rmw operation #107271

Merged

erikdesjardins mentioned this pull request May 20, 2023

[rustc_ty_utils] Treat drop_in_place's *mut argument like &mut when adding LLVM attributes #111807

Merged

lukas-code mentioned this pull request Oct 27, 2024

Rc/Arc: don't leak the allocation if drop panics #132231

Merged

Uh oh!

Conversation

JakobDegen commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Nov 4, 2022

Uh oh!

JakobDegen commented Nov 4, 2022

Uh oh!

rustbot commented Nov 4, 2022

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Nov 5, 2022

Uh oh!

JakobDegen commented Nov 12, 2022

Uh oh!

rustbot commented Nov 12, 2022

Uh oh!

RalfJung commented Nov 13, 2022 via email

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RalfJung commented Nov 19, 2022

Uh oh!

RalfJung commented Nov 26, 2022

Uh oh!

rustbot commented Nov 27, 2022

Uh oh!

JakobDegen commented Nov 27, 2022

Uh oh!

This comment has been minimized.

JakobDegen commented Dec 21, 2022

Uh oh!

RalfJung Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Dec 22, 2022

Uh oh!

bors commented Dec 22, 2022

Uh oh!

bors commented Dec 22, 2022

Uh oh!

bors commented Dec 22, 2022

Uh oh!

rust-timer commented Dec 22, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

JakobDegen commented Nov 4, 2022 •

edited

Loading