make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias #106180

RalfJung · 2022-12-27T12:07:59Z

See rust-lang/unsafe-code-guidelines#381 and this LLVM discussion. The exact semantics of how noalias and dereferenceable interact are unclear, and @comex found a case of LLVM actually exploiting that ambiguity for optimizations. I think for now we should treat LLVM dereferenceable as implying a "fake read" to happen immediately at the top of the function (standing in for the spurious reads that LLVM might introduce), and that fake read is subject to all the usual noalias restrictions. This means we cannot put dereferenceable on &mut !Unpin references as those references can alias with other references that are being read and written inside the function (e.g. for self-referential generators), meaning the fake read introduces aliasing conflicts with those other accesses.

For & this is already not a problem due to #98017 which removed the dereferenceable attribute for other reasons.

Regular &mut Unpin references are unaffected, so I hope the impact of this is going to be tiny.

The first commit does some refactoring of the PointerKind enum since I found the old code very confusing each time I had to touch it. It doesn't change behavior.

Fixes rust-lang/miri#2714

EDIT: Turns out our Box<!Unpin> treatment was incorrect, too, so the PR also fixes that now (in codegen and Miri): we do not put noalias on these boxes any more.

rustbot · 2022-12-27T12:08:04Z

r? @jackh726

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

rustbot · 2022-12-27T12:08:07Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2022-12-27T12:08:26Z

Cc @nikic due to LLVM attribute involvement

RalfJung · 2022-12-27T12:10:29Z

@bors try @rust-timer queue

bors · 2022-12-27T12:10:37Z

⌛ Trying commit 3dbce6b7db10c346bec633288dcd314e23b80a41 with merge d0517ed036a21986873e247b24636d0f55aadebb...

nikic · 2022-12-27T12:22:44Z

No concerns from my side.

bors · 2022-12-27T14:51:00Z

☀️ Try build successful - checks-actions
Build commit: d0517ed036a21986873e247b24636d0f55aadebb (d0517ed036a21986873e247b24636d0f55aadebb)

rust-timer · 2022-12-27T16:09:55Z

Finished benchmarking commit (d0517ed036a21986873e247b24636d0f55aadebb): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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

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

RalfJung · 2022-12-27T22:13:06Z

src/tools/miri/tests/fail/stacked_borrows/deallocate_against_protector2.rs

-        let raw = x as *mut _;
-        drop(unsafe { Box::from_raw(raw) });
-    });
-}


FWIW we could keep this as UB (by still protecting &mut !Unpin references). But I don't see a good reason for doing that... and it would be asymmetric with &!Freeze references.

RalfJung · 2022-12-28T11:19:55Z

@rust-lang/lang not sure if this needs a full FCP, but I nominated this anyway to bring it to your attention -- I think putting dereferenceable on references where adding a read would cause aliasing violations in code we consider well-defined is a risk we don't currently want to take. It's not clear how and why such code would not be UB.

Put differently I think everywhere that we emit dereferenceable, we want Miri to do a "fake read" when the function starts to verify that indeed this is dereferenceable -- but for &mut !Unpin references we cannot do that, Miri would report UB on safe code (see rust-lang/unsafe-code-guidelines#381). Therefore we should not put dereferenceable on &mut !Unpin references, which is what this PR implements.

RalfJung · 2023-01-02T13:10:50Z

I have also added to this PR a commit which fixes a problem pointed out by @Darksonn: we are currently adding noalias to all Boxes, but we can only do that for Box<Unpin>, bot the same reason as with mutable references.

RalfJung · 2023-01-02T16:35:19Z

Cc @rust-lang/wg-mir-opt (AFAIK currently no MIR optimizations do anything aliasing-related but still)

bors · 2023-01-03T09:03:31Z

☔ The latest upstream changes (presumably #106371) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2023-02-06T10:34:54Z

All right, looks like I now need someone from t-compiler to review. @jackh726 you are currently assigned. Does that work or should I re-assign?

The code that consumes PointerKind (`adjust_for_rust_scalar` in rustc_ty_utils) ended up using PointerKind variants to talk about Rust reference types (& and &mut) anyway, making the old code structure quite confusing: one always had to keep in mind which PointerKind corresponds to which type. So this changes PointerKind to directly reflect the type. This does not change behavior.

See rust-lang/unsafe-code-guidelines#381 for discussion.

RalfJung · 2023-02-06T10:55:25Z

compiler/rustc_middle/src/ty/layout.rs

-                            }
-                        }
-                    };
+        let pointee_info = match *this.ty.kind() {


For some reason rustfmt decided to completely change how this match is formatted. Some recent other PR made the inverse change, and resolving those conflicts was quite painful (git is not great at handling whitespace differences during rebasing). @rust-lang/rustfmt any idea why this back-and-forth is happening? rustfmt seems to sometimes perefer

let var = match expr { pat => ... };

over

let val = match expr { pat => ... };

even though I would think the latter is always preferable?

This is where the inverse change recently happened.

jackh726 · 2023-02-06T15:29:16Z

I can review, but I'm not sure if I'm the best person for it (so will need to allocate some time to actually grok what's going on)

nbdd0121 · 2023-02-06T16:04:01Z

compiler/rustc_middle/src/ty/layout.rs

+                            frozen: optimize && ty.is_freeze(tcx, cx.param_env()),
+                        },
+                        hir::Mutability::Mut => PointerKind::MutableRef {
+                            unpin: optimize && ty.is_unpin(tcx, cx.param_env()),


This changed the behaviour of mutable ref in no-opt builds from SharedMutable to UniqueBorrowedPinned, as opposed to what is claimed in the commit message.

That doesn't actually change the emitted flags though, so the commit message is still correct. Or did I miss a case?

It changes the emitted flags if you only look at that commit, but of course the latter commits unify the behaviour of these two so the flags ended up being the same. I pointed this out because I'm reviewing per commit.

It changes the emitted flags if you only look at that commit,

I don't think it does... in no-opt builds we don't emit any flags either way.

If only this commit is applied then we will start emitting dereferenceable for mutable refs? Or am I missing something obvious?

When optimizations are disabled, we don't emit any of these attributes, no matter what happens here. That logic is... somewhere, I honestly don't know where exactly.

RalfJung · 2023-02-06T16:06:27Z

I can review, but I'm not sure if I'm the best person for it (so will need to allocate some time to actually grok what's going on)

Happy to assign someone else, if you prefer (and maybe have an idea about who would be a good reviewer).

RalfJung · 2023-02-06T16:14:15Z

r? @nbdd0121
since you seem on it already anyway. Thanks!

rustbot · 2023-02-06T16:14:18Z

Failed to set assignee to nbdd0121: invalid assignee

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

RalfJung · 2023-02-06T16:14:32Z

Oh, lol... bummer.^^

nbdd0121

The compiler change looks correct and consistent with the PR description. I am not familiar enough with MIRI to comment about the MIRI change.

nbdd0121 · 2023-02-06T16:18:56Z

Failed to set assignee to nbdd0121: invalid assignee

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

Yeah :) sorry. I work on the compiler a lot but isn't in the team.

jackh726 · 2023-02-06T23:58:11Z

@bors delegate=nbdd0121

bors · 2023-02-06T23:58:14Z

✌️ @nbdd0121 can now approve this pull request

nbdd0121 · 2023-02-07T02:11:53Z

@bors r+

bors · 2023-02-07T02:11:55Z

📌 Commit 1ef1687 has been approved by nbdd0121

It is now in the queue for this repository.

bors · 2023-02-07T03:35:13Z

⌛ Testing commit 1ef1687 with merge dffea43...

bors · 2023-02-07T06:43:03Z

☀️ Test successful - checks-actions
Approved by: nbdd0121
Pushing dffea43 to master...

rust-timer · 2023-02-07T08:02:48Z

Finished benchmarking commit (dffea43): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was 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)	-2.4%	[-3.8%, -1.1%]	2
All ❌✅ (primary)	-	-	0

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

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)	1.8%	[1.4%, 2.1%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

rustbot assigned jackh726 Dec 27, 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 Dec 27, 2022

RalfJung mentioned this pull request Dec 27, 2022

Resolve unsound interaction between noalias and self-referential data (incl. generators, async fn) #63818

Open

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 27, 2022

This comment has been minimized.

Sign in to view

RalfJung force-pushed the dereferenceable-generators branch from 3dbce6b to 28ed626 Compare December 27, 2022 15:02

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 27, 2022

RalfJung commented Dec 27, 2022

View reviewed changes

RalfJung force-pushed the dereferenceable-generators branch from 28ed626 to cea1604 Compare December 28, 2022 10:35

RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. labels Dec 28, 2022

RalfJung force-pushed the dereferenceable-generators branch from cea1604 to e68533e Compare January 2, 2023 13:09

RalfJung force-pushed the dereferenceable-generators branch from e68533e to f48d972 Compare January 2, 2023 13:16

RalfJung changed the title ~~make &mut !Unpin not dereferenceable~~ make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias Jan 2, 2023

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 31, 2023

RalfJung added 2 commits February 6, 2023 11:46

make &mut !Unpin not dereferenceable

ea541bc

See rust-lang/unsafe-code-guidelines#381 for discussion.

RalfJung force-pushed the dereferenceable-generators branch from 20a42c8 to 675dffb Compare February 6, 2023 10:52

RalfJung commented Feb 6, 2023

View reviewed changes

also do not add noalias on not-Unpin Box

1ef1687

RalfJung force-pushed the dereferenceable-generators branch from 675dffb to 1ef1687 Compare February 6, 2023 11:17

nbdd0121 reviewed Feb 6, 2023

View reviewed changes

nbdd0121 approved these changes Feb 6, 2023

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 Feb 7, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 7, 2023

bors merged commit dffea43 into rust-lang:master Feb 7, 2023

rustbot added this to the 1.69.0 milestone Feb 7, 2023

RalfJung deleted the dereferenceable-generators branch February 11, 2023 21:31

matthiaskrgr mentioned this pull request Jul 1, 2024

ICE: associated_item_def_ids: not impl or trait #127222

Closed

make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias #106180

make &mut !Unpin not dereferenceable, and Box<!Unpin> not noalias #106180

Conversation

RalfJung commented Dec 27, 2022 • edited Loading

rustbot commented Dec 27, 2022

rustbot commented Dec 27, 2022

RalfJung commented Dec 27, 2022

RalfJung commented Dec 27, 2022

This comment has been minimized.

bors commented Dec 27, 2022

This comment has been minimized.

nikic commented Dec 27, 2022

bors commented Dec 27, 2022

This comment has been minimized.

rust-timer commented Dec 27, 2022

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

RalfJung commented Dec 28, 2022

RalfJung commented Jan 2, 2023

RalfJung commented Jan 2, 2023

bors commented Jan 3, 2023

RalfJung commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 commented Feb 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Feb 6, 2023

RalfJung commented Feb 6, 2023

rustbot commented Feb 6, 2023

RalfJung commented Feb 6, 2023

nbdd0121 left a comment

Choose a reason for hiding this comment

nbdd0121 commented Feb 6, 2023

jackh726 commented Feb 6, 2023

bors commented Feb 6, 2023

nbdd0121 commented Feb 7, 2023

bors commented Feb 7, 2023

bors commented Feb 7, 2023

bors commented Feb 7, 2023

rust-timer commented Feb 7, 2023

Overall result: ✅ improvements - no action needed

RalfJung commented Dec 27, 2022 •

edited

Loading

RalfJung Feb 6, 2023 •

edited

Loading