Disable dead variant removal for `#[repr(C)]` enums. #123043

GoldsteinE · 2024-03-25T18:08:48Z

This prevents removing dead branches from a #[repr(C)] enum (they now get discriminants allocated as if they were inhabited).

Implementation notes: ABI of something like

#[repr(C)]
enum Foo {
    Foo(!),
}

is still Uninhabited, but its layout is now computed as if all the branches were inhabited.
This seemed to me like a proper way to do it, especially given that ABI sanity check explicitly asserts that type-level uninhabitedness implies ABI uninhabitedness.

This probably needs some sort of FCP (given that it changes #[repr(C)] layout, which is a stable guarantee), but I’m not sure how to call for one or which team is the most relevant.

See rust-lang/unsafe-code-guidelines#500.

rustbot · 2024-03-25T18:08:55Z

r? @oli-obk

rustbot has assigned @oli-obk.
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

tests/ui/repr/repr-c-dead-variants.rs

GoldsteinE · 2024-04-23T13:38:54Z

@oli-obk Hi! Could you maybe express your opinion on the test issue (and just look at the PR in general?)

I could write a rustc_layout test for #[repr(C, int)] enums and a @run-pass test for #[repr(C)] enums, it looks like it would cover all the cases.

compiler/rustc_abi/src/layout.rs

traviscross · 2024-05-01T22:26:59Z

@rustbot labels -I-lang-nominated

We discussed this in the lang call today and we agreed that this is a bug fix. This is OK to move forward.

GoldsteinE · 2024-05-04T21:28:40Z

I pushed shiny new rustc_layout tests. There are two of them: repr-c-int-dead-variants tests #[repr(C, u8)] and is run for every platform and repr-c-dead-variants runs on four platforms: three major archs plus one ARM target that has interesting c-enum-min-bits (it’s done via --target, not only-, so it should be actually tested in CI).

I also added a line into abi/compatibility.rs that asserts the initial claim: ReprCEnum<Void> should be compatible with ReprCEnum<MaybeUninit<Void>>.

compiler/rustc_abi/src/lib.rs

GoldsteinE · 2024-05-04T22:30:53Z

Ok, apparently u64 has 4-byte align on some targets. I’ll make it less target-dependent.

tests/ui/abi/compatibility.rs

GoldsteinE · 2024-05-05T10:20:31Z

That seems spurious. I don’t suppose I have enough rights to retry the pipeline?

RalfJung · 2024-05-05T10:28:13Z

We're having some CI trouble currently, this may be related. So I suggest to wait for a few hours.

You can retrigger the pipeline by closing and re-opening the PR.

oli-obk · 2024-05-24T15:01:47Z

We discussed this in the lang call today and we agreed that this is a bug fix. This is OK to move forward.

still needs an FCP though, right? Or is that bugfixy enough to just land?

RalfJung · 2024-05-24T15:48:23Z

I understood this as saying that no FCP is needed.

WaffleLapkin · 2024-05-24T16:11:52Z

The meeting notes explicitly say no FCP is needed:

TC: What do we think?

NM: This seems like it's not changing the reference, but fixing a bug, in which case I'm in favor. The set of variants that are listed in the enum are about layout.

pnkfelix: I agree.

tmandry: It seems like a bug fix. Sounds good to me.

Consensus: We're OK with this and believe it to be a bug fix. Therefore, no FCP needed and it's OK to go ahead based on our meeting consensus.

(emph mine, ref: https://hackmd.io/@rust-lang-team/rkF1tAkzC#Disable-dead-variant-removal-for-reprC-enums-rust123043)

oli-obk · 2024-06-28T14:01:41Z

@rustbot author

See rust-lang/unsafe-code-guidelines#500.

Co-authored-by: Ralf Jung <[email protected]>

GoldsteinE · 2024-06-28T17:21:51Z

I removed the word “inhabited”.

@rustbot ready

oli-obk · 2024-07-04T10:51:38Z

@bors r+ rollup

bors · 2024-07-04T10:51:40Z

📌 Commit 8a3c07a has been approved by oli-obk

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#123043 (Disable dead variant removal for `#[repr(C)]` enums.) - rust-lang#126405 (Migrate some rustc_builtin_macros to SessionDiagnostic) - rust-lang#127037 (Remove some duplicated tests) - rust-lang#127283 (Reject SmartPointer constructions not serving the purpose) - rust-lang#127301 (Tweak some structured suggestions to be more verbose and accurate) - rust-lang#127307 (Allow to have different types for arguments of `Rustc::remap_path_prefix`) - rust-lang#127309 (jsondocck: add `$FILE` built-in variable) - rust-lang#127314 (Trivial update on tidy bless note) - rust-lang#127319 (Remove a use of `StructuredDiag`, which is incompatible with automatic error tainting and error translations) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123043 - GoldsteinE:fix/repr-c-dead-branches, r=oli-obk Disable dead variant removal for `#[repr(C)]` enums. This prevents removing dead branches from a `#[repr(C)]` enum (they now get discriminants allocated as if they were inhabited). Implementation notes: ABI of something like ```rust #[repr(C)] enum Foo { Foo(!), } ``` is still `Uninhabited`, but its layout is now computed as if all the branches were inhabited. This seemed to me like a proper way to do it, especially given that ABI sanity check explicitly asserts that type-level uninhabitedness implies ABI uninhabitedness. This probably needs some sort of FCP (given that it changes `#[repr(C)]` layout, which is a stable guarantee), but I’m not sure how to call for one or which team is the most relevant. See rust-lang/unsafe-code-guidelines#500.

GoldsteinE · 2024-07-04T19:00:19Z

Nice, thanks!

Should that be announced in some way? It’s kind of a rare case, but it’s also a subtle change in observable behaviour, so I’m not sure.

rustbot assigned oli-obk Mar 25, 2024

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 Mar 25, 2024

RalfJung reviewed Mar 25, 2024

View reviewed changes

tests/ui/repr/repr-c-dead-variants.rs Outdated Show resolved Hide resolved

oli-obk added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 23, 2024

oli-obk reviewed Apr 23, 2024

View reviewed changes

compiler/rustc_abi/src/layout.rs Show resolved Hide resolved

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label May 1, 2024

GoldsteinE force-pushed the fix/repr-c-dead-branches branch from eea0d6a to d34e10c Compare May 4, 2024 21:24

GoldsteinE commented May 4, 2024

View reviewed changes

compiler/rustc_abi/src/lib.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

GoldsteinE force-pushed the fix/repr-c-dead-branches branch from d34e10c to fa62263 Compare May 4, 2024 22:35

RalfJung reviewed May 5, 2024

View reviewed changes

tests/ui/abi/compatibility.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

GoldsteinE closed this May 5, 2024

GoldsteinE reopened this May 5, 2024

WaffleLapkin removed the needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. label May 24, 2024

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 Jun 28, 2024

GoldsteinE and others added 2 commits June 28, 2024 20:19

Disable dead variant removal for #[repr(C)] enums.

71dfbea

See rust-lang/unsafe-code-guidelines#500.

Clarify guarantees about ABI compatibility

8a3c07a

Co-authored-by: Ralf Jung <[email protected]>

GoldsteinE force-pushed the fix/repr-c-dead-branches branch from d529329 to 8a3c07a Compare June 28, 2024 17:20

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 Jun 28, 2024

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 Jul 4, 2024

matthiaskrgr mentioned this pull request Jul 4, 2024

Rollup of 9 pull requests #127326

Merged

bors merged commit f0a0f8f into rust-lang:master Jul 4, 2024

rustbot added this to the 1.81.0 milestone Jul 4, 2024

Disable dead variant removal for #[repr(C)] enums. #123043

Disable dead variant removal for #[repr(C)] enums. #123043

Uh oh!

Conversation

GoldsteinE commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 25, 2024

Uh oh!

Uh oh!

GoldsteinE commented Apr 23, 2024

Uh oh!

Uh oh!

traviscross commented May 1, 2024

Uh oh!

GoldsteinE commented May 4, 2024

Uh oh!

Uh oh!

This comment has been minimized.

GoldsteinE commented May 4, 2024

Uh oh!

Uh oh!

This comment has been minimized.

GoldsteinE commented May 5, 2024

Uh oh!

RalfJung commented May 5, 2024

Uh oh!

oli-obk commented May 24, 2024

Uh oh!

RalfJung commented May 24, 2024 via email

Uh oh!

WaffleLapkin commented May 24, 2024

Uh oh!

oli-obk commented Jun 28, 2024

Uh oh!

GoldsteinE commented Jun 28, 2024

Uh oh!

oli-obk commented Jul 4, 2024

Uh oh!

bors commented Jul 4, 2024

Uh oh!

GoldsteinE commented Jul 4, 2024

Uh oh!

Uh oh!

Disable dead variant removal for `#[repr(C)]` enums. #123043

Disable dead variant removal for `#[repr(C)]` enums. #123043

GoldsteinE commented Mar 25, 2024 •

edited

Loading