Skip to content
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

Fixed extern enums having the wrong size #970

Merged
merged 3 commits into from May 4, 2018
Merged

Conversation

Hejsil
Copy link
Sponsor Contributor

@Hejsil Hejsil commented May 1, 2018

For extern enums, get_smallest_unsigned_int_type does not give us the tag type that GCC or Clang uses for enums. I've chosen to use c_int as a tag type for extern enum as it seems to be more in line with what they do, but I'm not sure if that is correct either.

C Spec:

Each enumerated type shall be compatible with char, a signed integer type, or an
unsigned integer type. The choice of type is implementation-defined,110) but shall be
capable of representing the values of all the members of the enumeration. The
enumerated type is incomplete until after the } that terminates the list of enumerator
declarations.

It seems that C compilers are allowed to use the smallest size, so how do we handle that when mixing Zig with some niche C compiler that does this?

Also, should specifying the tag type on extern enums be a compiler error?

@Hejsil Hejsil requested a review from andrewrk May 1, 2018 12:39
@alexnask
Copy link
Contributor

alexnask commented May 1, 2018

Imo explicit extern enum tag types should be disallowed and extern enum should mean that the tag type is 'c_int'.
If the user needs to interface with an exotic C compiler (or GCC with -fshort-enums) they should specify the tag type themselves.
Is zig allowed to change the sizeof an enum type with an explicit tag type?

(GCC, Clang and MSVC all seem to use 'int' or 'unsigned int')

@Hejsil Hejsil closed this May 1, 2018
@Hejsil Hejsil reopened this May 1, 2018
@Hejsil
Copy link
Sponsor Contributor Author

Hejsil commented May 1, 2018

#MissClick

@Hejsil
Copy link
Sponsor Contributor Author

Hejsil commented May 2, 2018

Test 330/472 compiler-rt-windows-x86_64-ReleaseFast-c std.atomic.queue...
🤔🤔🤔🤔🤔

@bnoordhuis
Copy link
Contributor

bnoordhuis commented May 2, 2018

It seems that C compilers are allowed to use the smallest size, so how do we handle that when mixing Zig with some niche C compiler that does this?

I had a discussion about this in a different context a while ago and the conclusion was that it's most likely a theoretical concern. There appear to be no production compilers that would willingly pick a size < sizeof(int).

It's kind of pointless because most ABIs dictate promotion to word size anyway when passing it around in registers or embedding it in a struct. gcc and clang have -fshort-enums but that emits ABI-incompatible code and I've never seen it used.

@andrewrk
Copy link
Member

andrewrk commented May 2, 2018

I'll have a look at this PR tonight.

As for the windows threading issue, I may have found a clue:

A thread in an executable that calls the C run-time library (CRT) should use the _beginthreadex and _endthreadex functions for thread management rather than CreateThread and ExitThread; this requires the use of the multithreaded version of the CRT.

@Hejsil
Copy link
Sponsor Contributor Author

Hejsil commented May 2, 2018

Since both @bnoordhuis and @alexnask have given good reasons for just using c_int, I only really need your opinion on compiler error for explicit tag type on extern enums :)

@andrewrk
Copy link
Member

andrewrk commented May 2, 2018

There's one detail I want to double check -

The Clang API has the concept of specifying int type of enums. I believe right now in translate-c we support this. So that makes me think there is some GNU attribute that lets you do this, although I can not find it.

See also https://stackoverflow.com/questions/9524342/how-to-specify-enum-size-in-gcc#9525276
It appears that enum types can sometimes not be a c_int

Is zig allowed to change the sizeof an enum type with an explicit tag type?

Only if the target C environment would let you do so.

Zig gives the promise of "C ABI compatibility" in various contexts, one of which being extern enum. How can we have C ABI compatibility when the C ABI is left to be implementation defined? The answer is that the user provides the target C environment (gnu, msvc, cygwin, etc) - this is the --target-environ option. And then we simply match the ABI of that particular C environment.

So for extern enum we need to match whatever the target C environment would do.

I'll look more into this tonight

@Hejsil
Copy link
Sponsor Contributor Author

Hejsil commented May 2, 2018

#It'sNeverThatSimple

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

OK, so this is definitely an improvement. Feel free to merge.

I'll make an issue to track C ABI compatibility for enums and we can revisit making it a compile error at that time. Until then we can leave the ability to override the int type even for extern enums, as an available workaround for zig potentially not supporting e.g. enums upgrading to c_longlong if you use such a value in it and are targeting GNU.

#977

@andrewrk
Copy link
Member

andrewrk commented May 3, 2018

@alexnask

If the user needs to interface with an exotic C compiler (or GCC with -fshort-enums) they should specify the tag type themselves.

I agree with this in spirit, but if they were going to specify the tag type themselves, they would still have to use extern because zig reserves the right for @sizeOf(AnEnumType) to be arbitrarily large, unless you use extern or packed. It's also planned to have an equivalent of @setRuntimeSafety(false); for aggregate types so that you can optimize for size on specific types without having to use packed. I'll type up an issue for it.

@andrewrk
Copy link
Member

andrewrk commented May 3, 2018

#978

@andrewrk andrewrk merged commit aa2586d into master May 4, 2018
@andrewrk andrewrk deleted the extern-enum-size-fix branch May 4, 2018 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants