Extend check for UnsafeCell in consts to cover unions #90383
Conversation
|
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
tmiasko
force-pushed
the
union-validity
branch
from
2d15e53
to
ce852a4
Compare
| // Note that this test case relies on undefined behaviour to construct a | ||
| // constant with interior mutability that is "invisible" to the static checks. | ||
| // If for some reason this approach no longer works, it is should be fine to | ||
| // remove the test case. |
There was a problem hiding this comment.
Wow, what a test. :D
That said, which part of this test causes UB? With a repr, I think changing the tag of an enum via raw ptr manipulation is totally fine. It seems promotion analysis is making some IMO unfounded assumptions if it thinks the tag cannot change here.
There was a problem hiding this comment.
Oh wait, you are writing through a shared ref. That is more tricky... though the exact scope of UnsafeCell inside enums is not decided yet, so this is not as clear-cut as one might think at first sight. For example, for an Option<Cell<NonZeroI32>>, mutating the discriminant through a shared ref is almost certainly going to be allowed.
Cc rust-lang/unsafe-code-guidelines#204 rust-lang/unsafe-code-guidelines#236
There was a problem hiding this comment.
For example, for an
Option<Cell<NonZeroI32>>, mutating the discriminant through a shared ref is almost certainly going to be allowed.
was this a typo? I thought this specifically wasnt allowed, we made UnsafeCell inhibit niches, i.e. size_of::<Option<UnsafeCell<NonZeroU32>>>() == 8
edit: oh no I just checked and it doesnt apply when the unsafecell is newtyped..? That seems bad
There was a problem hiding this comment.
Test case creates a reference to S::x field which has a u32 type, so no interior mutability here. The const qualification assumes that resulting pointer cannot be used to access the other parts of a local. The assumption is violated here by offsetting the pointer and overwriting an adjacent tag.
edit: oh no I just checked and it doesnt apply when the unsafecell is newtyped..? That seems bad
Good catch. I think we already have an open issue for niche hiding not working exactly as intended.
There was a problem hiding this comment.
Ah right UnsafeCell masks niches these days (except when newtpyed but that is a bug: #87341).
Test case creates a reference to S::x field which has a u32 type, so no interior mutability here. The const qualification assumes that resulting pointer cannot be used to access the other parts of a local. The assumption is violated here by offsetting the pointer and overwriting an adjacent tag.
Ah I see. So this makes certain assumptions about the aliasing model, but this particular case is probably fine -- however, I am still a bit worried that promotion analysis might make more assumptions than we can actually uphold. In this case we are creating an &T where T: Freeze. But if we ever do anything value-based here (e.g., &None::<T> where T might be !Freeze), that would be sketchy.
|
r? @RalfJung |
tmiasko
force-pushed
the
union-validity
branch
from
ce852a4
to
02ca6c0
Compare
tmiasko
force-pushed
the
union-validity
branch
from
02ca6c0
to
c750d7e
Compare
apiraino
added
the
T-compiler
JohnCSimon
added
S-waiting-on-review
|
Sorry, this fell through the cracks because GH does not always send email notifications for force-pushes. This change makes sense to me. @bors r+ |
|
📌 Commit c750d7e8620ba05a3c07583c1630523e50bfa8b7 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit c750d7e8620ba05a3c07583c1630523e50bfa8b7 with merge eb69c9f6df723a8cceb0aeefcf93a9ffbabe38a4... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
tmiasko
force-pushed
the
union-validity
branch
from
c750d7e
to
63afad3
Compare
|
Rebased and updated the test output. |
This comment has been minimized.
This comment has been minimized.
A validity companion to changes from rust-lang#90373.
tmiasko
force-pushed
the
union-validity
branch
from
63afad3
to
2eb637a
Compare
|
@bors r+ |
|
📌 Commit 2eb637a has been approved by |
bors
added
S-waiting-on-bors
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#90383 (Extend check for UnsafeCell in consts to cover unions) - rust-lang#91375 (config.rs: Add support for a per-target default_linker option.) - rust-lang#91480 (rustdoc: use smaller number of colors to distinguish items) - rust-lang#92338 (Add try_reserve and try_reserve_exact for OsString) - rust-lang#92405 (Add a couple needs-asm-support headers to tests) - rust-lang#92435 (Sync rustc_codegen_cranelift) - rust-lang#92440 (Fix mobile toggles position) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
A validity companion to changes from #90373.
@rust-lang/wg-const-eval