-
Notifications
You must be signed in to change notification settings - Fork 13.8k
repr(C) enums: fix enum size computation #146504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ pub(crate) fn opts() -> TargetOptions { | |
emit_debug_gdb_scripts: false, | ||
archive_format: "coff".into(), | ||
|
||
// MSVC does not seem to ever automatically increase enums beyond their default size (see | ||
// <https://github.com/rust-lang/rust/issues/124403>, <https://godbolt.org/z/1Pdb3hP9E>). | ||
c_enum_min_bits: Some(32), | ||
c_enum_max_bits: Some(32), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also affects the UEFI targets. Is that intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, they use the MSVC ABI for things, yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the psABI document the enum size rules? I would be surprised if it did.^^ |
||
|
||
// Currently this is the only supported method of debuginfo on MSVC | ||
// where `*.pdb` files show up next to the final artifact. | ||
split_debuginfo: SplitDebuginfo::Packed, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
warning: literal out of range for `isize` | ||
--> $DIR/repr-c-size.rs:24:9 | ||
| | ||
LL | A = 9223372036854775807, // i64::MAX | ||
| ^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: the literal `9223372036854775807` does not fit into the type `isize` whose range is `-2147483648..=2147483647` | ||
note: the lint level is defined here | ||
--> $DIR/repr-c-size.rs:22:8 | ||
| | ||
LL | #[warn(overflowing_literals)] | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
warning: literal out of range for `isize` | ||
--> $DIR/repr-c-size.rs:43:9 | ||
| | ||
LL | A = 4294967294, // u32::MAX - 1 | ||
| ^^^^^^^^^^ | ||
| | ||
= note: the literal `4294967294` does not fit into the type `isize` whose range is `-2147483648..=2147483647` | ||
note: the lint level is defined here | ||
--> $DIR/repr-c-size.rs:41:8 | ||
| | ||
LL | #[warn(overflowing_literals)] | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
warning: 2 warnings emitted | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
warning: literal out of range for `i32` | ||
--> $DIR/repr-c-size.rs:24:9 | ||
| | ||
LL | A = 9223372036854775807, // i64::MAX | ||
| ^^^^^^^^^^^^^^^^^^^ | ||
| | ||
= note: the literal `9223372036854775807` does not fit into the type `i32` whose range is `-2147483648..=2147483647` | ||
= help: consider using the type `i64` instead | ||
note: the lint level is defined here | ||
--> $DIR/repr-c-size.rs:22:8 | ||
| | ||
LL | #[warn(overflowing_literals)] | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
warning: literal out of range for `i32` | ||
--> $DIR/repr-c-size.rs:43:9 | ||
| | ||
LL | A = 4294967294, // u32::MAX - 1 | ||
| ^^^^^^^^^^ | ||
| | ||
= note: the literal `4294967294` does not fit into the type `i32` whose range is `-2147483648..=2147483647` | ||
= help: consider using the type `u32` instead | ||
note: the lint level is defined here | ||
--> $DIR/repr-c-size.rs:41:8 | ||
| | ||
LL | #[warn(overflowing_literals)] | ||
| ^^^^^^^^^^^^^^^^^^^^ | ||
|
||
warning: 2 warnings emitted | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda feels like we should just have a Range here, but on the other hand ranges usually have an unwieldy API so I'm not sure that's actually a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how would the range look like in JSON?
Also, the top end of the range is optional so it can be pointer-sized (
Integer
only has fixed-sized variants). Though maybe that's not actually correct and the pointer size has no influence on the enum size in C.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does indeed seem to be an incorrect assumption. ( Apologies as I am probably going to reiterate things you have already read a bit, but there are a few comments I have to give along the way. )
C11
C11 says only that the enum's representation should fit an
int
:This would seem to disallow using something that only fits in a
long
, but then the following is specified:This is what suggests that you can just do whatever in terms of sizing the C enum. Some use this to shrink enums to
char
, thus our usage ofc_enum_min_size
to begin with, but the previous clause seems to ban usinglong int
and thus seems to mandate a maximum.Now, as we know, most C compilers except MSVC disregard this and allow
long
-fitted values in practice. This is partially because C++ allows larger enums, which also have specified underlying integer types (like Rust does).C23
So, they updated to allow the C++ form, and then updated the logic to make clearer that the real requirement is that there be a unifying underlying type:
An "extended... integer type" can include a "vendor type", as with
__int128
on some platforms (so, i128). By saying "standard or extended... integer type", they have opened the door to a compiler even using essentially any size of integer type. They then reiterate that there must be a unifying type for the enum (emphasis mine):Note however that the reality that the enumeration members, as syntactically interacted with by programmers, are really constants, remains in effect. That means that if there is no fixed underlying type named by the programmer, then they have their own types when named as constants, which may differ from the type of the enum. This becomes visible when you do a cast, as given by 6.7.7.3.20 (EXAMPLE 4):
The value returned is implementation-defined, based on whether the underlying type picked by the compiler for the enum is an
int
or anunsigned char
. If an underlying type has been specified using the equivalent to ourrepr(u8)
:Then we have a guaranteed value, because we have a guaranteed type for every one of the enum's constants.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...Mind, the enumeration constant detail is pure nonsense from Rust's POV, so it mostly serves to explain why C compilers have such a rough time figuring out how to handle this, since they didn't have the good sense to not extend the language with implementation-defined "sure, why not?"s. On some level they have to simultaneously pretend their enums are both
int
s and then some other type.Here, it does mean that "pointer-size" for a C enum would seem to be meaningless.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Thinking about it, I guess they can hypothetically pick
intptr_t
as the type they're using, if that's a type that is held as distinct from any other integer type instead of a typedef? Hmm. I kinda think that's not going to be the case, though, and the limits will be arbitrary instead.