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

Conversation

Projects
None yet
4 participants
@Hejsil
Member

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 added some commits May 1, 2018

@Hejsil Hejsil requested a review from andrewrk May 1, 2018

@alexnask

This comment has been minimized.

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

This comment has been minimized.

Member

Hejsil commented May 1, 2018

#MissClick

@Hejsil

This comment has been minimized.

Member

Hejsil commented May 2, 2018

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

@bnoordhuis

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

Member

Hejsil commented May 2, 2018

#It'sNeverThatSimple

@andrewrk

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

This comment has been minimized.

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

This comment has been minimized.

Member

andrewrk commented May 3, 2018

@andrewrk andrewrk merged commit aa2586d into master May 4, 2018

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@andrewrk andrewrk deleted the extern-enum-size-fix branch May 4, 2018

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