Supress niches in coroutines to avoid aliasing violations

[RalfJung]

As mentioned here, using niches in fields of coroutines that are referenced by other fields is unsound: the discriminant accesses violate the aliasing requirements of the reference pointing to the relevant field. This issue causes Miri errors in practice.

The "obvious" fix for this is to suppress niches in coroutines. That's what this PR does. However, we have several tests explicitly ensuring that we do use niches in coroutines. So I see two options:

• We guard this behavior behind a -Z flag (that Miri will set by default). There is no known case of these aliasing violations causing miscompilations. But absence of evidence is not evidence of absence...
• (What this PR does right now.) We temporarily adjust the coroutine layout logic and the associated tests until the proper fix lands. The "proper fix" here is to wrap fields that other fields can point to in UnsafePinned and make UnsafePinned suppress niches; that would then still permit using niches of other fields (those that never get borrowed). However, I know that coroutine sizes are already a problem, so I am not sure if this temporary size regression is acceptable.

@compiler-errors any opinion? Also who else should be Cc'd here?

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[compiler-errors]

I'm of the opinion that we do the latter (fix the tests, fix it the right way later when we can). But also it would be nice if we actually knew how bad this would affect layout sizes in practice... I have no idea how to gather that information, though.

[RalfJung]

I blessed the failing tests. async-drop for some reason seems quite badly affected. But that is an unstable feature.

[RalfJung]

Since we haven't heard from our reviewer, let's re-roll.
r? compiler

Also maybe it helps to get more people to take a look -- this is primarily about async fn, so Cc @rust-lang/wg-async

[compiler-errors]

I wanna think about this so

r? compiler-errors

please bother me if i dont review this in a few days!!

[compiler-errors]

Yeah, this seems necessary to do until we implement UnsafePinned correctly. I guess if people complain about this, we could gate this behind miri or something.

[compiler-errors]

@bors r+ rollup=never

[bors]

📌 Commit 12cda6e has been approved by compiler-errors

It is now in the queue for this repository.

[bors]

⌛ Testing commit 12cda6e with merge 15287f5...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] miri test:false 4.437
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Wed, Sep  4, 2024  2:53:57 AM
  network time: Wed, 04 Sep 2024 02:53:58 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[bors]

⌛ Testing commit 12cda6e with merge 7e24610...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] miri test:false 4.468
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Sat, Sep  7, 2024  1:35:54 AM
  network time: Sat, 07 Sep 2024 01:35:54 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[lqd]

@bors retry #127883

[bors]

⌛ Testing commit 12cda6e with merge b0cfc07...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] miri test:false 4.461
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Sat, Sep  7, 2024  5:48:03 AM
  network time: Sat, 07 Sep 2024 05:48:03 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[RalfJung]

@bors retry

[bors]

⌛ Testing commit 12cda6e with merge 7f4b270...

[bors]

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 7f4b270 to master...

[rust-timer]

Finished benchmarking commit (7f4b270): comparison URL.

Overall result: ❌ regressions - 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.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.9%)

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.6%	[2.6%, 2.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.4%	[-4.4%, -4.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-4.4%, 2.6%]	2

Cycles

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

Binary size

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

Bootstrap: 757.221s -> 754.658s (-0.34%)
Artifact size: 341.00 MiB -> 341.04 MiB (0.01%)