bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Auto merge of #129313 - RalfJung:coroutine-niches, r=compiler-errors

Browse source 
Supress niches in coroutines to avoid aliasing violations

As mentioned [here](https://github.com/rust-lang/rust/issues/63818#issuecomment-2264915918), 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](https://github.com/rust-lang/miri/issues/3780).

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`](https://github.com/rust-lang/rust/issues/125735) 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?
This commit is contained in:
bors 2024-09-08 03:11:12 +00:00
commit 7f4b270aa4
6 changed files with 102 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

8
compiler/rustc_ty_utils/src/layout.rs
View file

@ -1001,7 +1001,13 @@ fn coroutine_layout<'tcx>(
},
fields: outer_fields,
abi,
largest_niche: prefix.largest_niche,
// Suppress niches inside coroutines. If the niche is inside a field that is aliased (due to
// self-referentiality), getting the discriminant can cause aliasing violations.
// `UnsafeCell` blocks niches for the same reason, but we don't yet have `UnsafePinned` that
// would do the same for us here.
// See <https://github.com/rust-lang/rust/issues/63818>, <https://github.com/rust-lang/miri/issues/3780>.
// FIXME: Remove when <https://github.com/rust-lang/rust/issues/125735> is implemented and aliased coroutine fields are wrapped in `UnsafePinned`.
largest_niche: None,
size,
align,
max_repr_align: None,