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

Proposal: add -fshort-enums support to translate-c #8662

Closed
ehaas opened this issue May 1, 2021 · 5 comments
Closed

Proposal: add -fshort-enums support to translate-c #8662

ehaas opened this issue May 1, 2021 · 5 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@ehaas
Copy link
Contributor

ehaas commented May 1, 2021

GCC & Clang have a command-line option -fshort-enums that can be used to control how many bytes are allocated for an enum type. It's equivalent to putting __attribute__((packed)) on all enums. From the GCC docs:

Normally, the type is unsigned int if there are no negative values in the enumeration, otherwise int. If -fshort-enums is specified, then if there are negative values it is the first of signed char, short and int that can represent all the values, otherwise it is the first of unsigned char, unsigned short and unsigned int that can represent all the values."

Super basic demonstration, compare output of the following C program when compiled with -fshort-enums vs not:

#include <stdio.h>
typedef enum { FOO } MyEnum;
int main(void) {
    printf("%d\n", (int)sizeof(MyEnum));
    return 0;
}

The relevance to translate-c is that enum sizes affect ABI compatibility. Currently translate-c always translates enums with an underlying type of c_int or c_uint, but this won't work if linking with code that was built with -fshort-enums. Short enums are fairly common in embedded systems where space is at a premium.

One way to solve this would be to add a special preprocessor symbol like __ZIG_SHORT_ENUMS (which can be used with zig translate-c via -D__ZIG_SHORT_ENUMS, and can be used with @cImport via @cDefine("__ZIG_SHORT_ENUMS", {});. translate-c could look for the existence of that symbol and use it to control whether enums are generated with the short enum algorithm described above. One downside to this approach is that it reifies the short enum decision into the generated code, so it wouldn't be possible to translate code once and have it automatically work across platforms.

@andrewrk andrewrk added accepted This proposal is planned. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels May 1, 2021
@andrewrk andrewrk added this to the 0.9.0 milestone May 1, 2021
@andrewrk
Copy link
Member

andrewrk commented May 1, 2021

Not only for translate-c but I think we should also add support for it in the main CLI options as well, but make it clear that it applies only to compiled C code. I think if you go to implement this it will become clear that these features are intertwined.

@andrewrk andrewrk removed accepted This proposal is planned. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. labels May 1, 2021
@ehaas
Copy link
Contributor Author

ehaas commented May 1, 2021

Are you suggesting that instead of doing this in translate-c; have a CLI option -fshort-enums that controls the size of extern enums?

@andrewrk
Copy link
Member

andrewrk commented May 1, 2021

Wait a minute, sorry, I just realized this should already be working, like this:

./zig translate-c -cflags -fshort-enums -- test.c -lc

I tried it just now and it did not affect the output, but that seems like a bug. Not sure why that is happening. I'm guessing the translate-c code is doing its own logic to determine the int tag of generated enums but it should be asking the Clang API for the integer type.

Semi-related: 557eb41

@ehaas
Copy link
Contributor Author

ehaas commented May 1, 2021

Ah ok, it might just be a bug in the cli parsing then. I just tested it out and -fshort-enums didn't make it into the clang args. But it's probably a much easier fix to do that than to do the preprocessor thing I suggested, I can take a look at it.

ehaas added a commit to ehaas/zig that referenced this issue May 1, 2021
ehaas added a commit to ehaas/zig that referenced this issue May 1, 2021
Additionally ensure that the Zig cache incorporates any extra cflags when
using translate-c.

Fixes the issue identified in ziglang#8662
@ehaas
Copy link
Contributor Author

ehaas commented May 3, 2021

Closed in favor of #8676

@ehaas ehaas closed this as completed May 3, 2021
andrewrk pushed a commit that referenced this issue May 12, 2021
Additionally ensure that the Zig cache incorporates any extra cflags when
using translate-c.

Fixes the issue identified in #8662
@andrewrk andrewrk modified the milestones: 0.9.0, 0.8.0 May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

2 participants