Visitar URL original
Separate `Primitive::Bool` and make `#[repr($int)]` enums always use the ABI of $int by asquared31415 · Pull Request #165481 · rust-lang/rust · GitHub
Skip to content

Separate Primitive::Bool and make #[repr($int)] enums always use the ABI of $int - #165481

Open
asquared31415 wants to merge 1 commit into
rust-lang:mainfrom
asquared31415:enum-int-repr
Open

asquared31415 wants to merge 1 commit into
rust-lang:mainfrom
asquared31415:enum-int-repr

Conversation

@asquared31415

@asquared31415 asquared31415 commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

View all comments

This properly gives bool its own ABI, shared only with bool-like enums that have no explicit repr. #[repr($int)] enums now always use the ABI of $int.

This PR makes no language guarantees, however it will help facilitate such guarantees, if they're decided on: rust-lang/unsafe-code-guidelines#628

Additionally, it seems to give LLVM slightly more information about the tags of certain enums, especially when it comes to variants with different sizes.

Previously, bools were identified for the purposes of ABI as being any u8 primitive that had a valid range of 0..=1. An enum with #[repr(u8)] and with discriminants 0 and 1 would satisfy that condition and be treated like a bool. This is somewhat sketchy, and while no miscompilation has been found, I would not be surprised if the i1 Extension Situation could cause problems on some targets.

fixes #165026

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 5, 2026
@rust-log-analyzer

This comment has been minimized.

@mu001999

mu001999 commented Nov 5, 2026

Copy link
Copy Markdown
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 5, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Nov 5, 2026
Separate `Primitive::Bool` and make `#[repr($int)]` enums always use the ABI of $int
@rust-log-analyzer

This comment has been minimized.

@asquared31415

This comment was marked as resolved.

@rust-bors

rust-bors Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 309247f (309247f2d061081730fed50fb52ae20610509cc8)
Base parent: 2d20b42 (2d20b42b883dcfc014f3c2c56081eeb6091c2179)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (309247f): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.2%, secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
3.2% [2.1%, 3.8%] 3
Regressions ❌
(secondary)
2.5% [2.1%, 3.2%] 5
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.7% [-5.3%, -2.5%] 3
All ❌✅ (primary) 3.2% [2.1%, 3.8%] 3

Cycles

This perf run didn't have relevant results for this metric.

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 480.462s -> 480.743s (0.06%)
Artifact size: 406.22 MiB -> 406.31 MiB (0.02%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 5, 2026
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Nov 6, 2026

@asquared31415 asquared31415 Nov 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is one of two meaningful test changes, the rest are debug layouts changing as expected.

This function and the corresponding function converting the other way are "regressed" in that now there will be an explicit trunc or zext. They were testing for the bad behavior (a repr(u8) that's actually a i1).

The trunc/ext should go away at the assembly level though.

View changes since the review

@asquared31415 asquared31415 Nov 6, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is the second meaningful test change.

The changes here are scary because I'm not quite sure of the requirements, but I asked on zulip for a second opinion.

I believe this is a correct change, with LLVM using more knowledge about the enum tag to generate a different instruction.

View changes since the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks fine to me. It makes sense that LLVM now exploits the known 0..=1 range of the discriminant.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

#[derive(Copy, Clone, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "nightly", derive(StableHash))]
pub enum Primitive {
Bool,

@workingjubilee workingjubilee Nov 7, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had been wondering when this would happen. Thanks.

View changes since the review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not so sure about it, between the number of things that were silently non-exhaustively matching on Primitive for the integer type and discarding the rest, and the fact that bool really is a Fancy Integer, I almost want to try and integrate it into the Integer variant.

But also, bool clearly acts differently in several places, so it sort of is special.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I have not scrutinized this PR closely. I think putting it in Integer would also make sense to me, but I'm going to wildly guess that it actually goes less well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It, at the very least, implies the existence of (bool, is_signed), which im sure explodes things if that accidentally leaks through. Integer also seems to be about being the byte-level "storage" type, so that probably isn't the best idea.

There probably exists a better version of Primitive, with a complete refactor, but this PR seems to at least disentangle bool Enough(tm) to get it to be its own ABI. We can delegate further things to "Future Work".

@asquared31415
asquared31415 marked this pull request as ready for review November 7, 2026 16:38
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2026
@rustbot

rustbot commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a, @makai410

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 7, 2026
@rustbot

rustbot commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

r? @JohnTitor

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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 79 candidates
  • Random selection from 22 candidates

Comment thread compiler/rustc_abi/src/lib.rs Resolved
@rust-bors

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-bors

This comment has been minimized.

@workingjubilee

Copy link
Copy Markdown
Member

I should probably try to make sure I look at this.

@asquared31415

Copy link
Copy Markdown
Contributor Author

Given the discussions in I think that the best thing for now is to add a "FIXME: this should work but doesn't" for test_abi_newtype::<bool>, and I don't think this makes anything worse in practice and is part of the path forward to improving the ABI Situations.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect / dubious LLVM-level ABI for two-variant #[repr(u8)] enums: lowered to i1

9 participants