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

translate-c: Ensure extra_cflags are passed to clang #8664

Merged
merged 1 commit into from May 12, 2021

Conversation

ehaas
Copy link
Sponsor Contributor

@ehaas ehaas commented May 1, 2021

Fixes the issue identified in #8662

@ehaas
Copy link
Sponsor Contributor Author

ehaas commented May 1, 2021

Is there a good way to write the test for this? translate-c behaves correctly now so it would just be a matter of comparing the output when the -fshort-enums flag is passed vs not, but I'm not sure where to put such a test.

I also noticed that the caching system does not notice that the cflag affects the output; is there more I need to do for that to work? For example if you translate a file without the flag, then translate it again with the flag, you get the same output. You have to delete the cache for it to translate correctly with a different flag.

@andrewrk
Copy link
Member

andrewrk commented May 1, 2021

Have a look at the function updateCObject and how it handles extra_flags. There is a block of code there that should probably be extracted into a function and called from cmdTranslateC as well:

    {
        // Hash the extra flags, with special care to call addFile for file parameters.
        // TODO this logic can likely be improved by utilizing clang_options_data.zig.
        const file_args = [_][]const u8{"-include"};
        var arg_i: usize = 0;
        while (arg_i < c_object.src.extra_flags.len) : (arg_i += 1) {
            const arg = c_object.src.extra_flags[arg_i];
            man.hash.addBytes(arg);
            for (file_args) |file_arg| {
                if (mem.eql(u8, file_arg, arg) and arg_i + 1 < c_object.src.extra_flags.len) {
                    arg_i += 1;
                    _ = try man.addFile(c_object.src.extra_flags[arg_i], null);
                }
            }
        }
    }

@ehaas ehaas marked this pull request as draft May 1, 2021 06:23
Additionally ensure that the Zig cache incorporates any extra cflags when
using translate-c.

Fixes the issue identified in ziglang#8662
@ehaas ehaas marked this pull request as ready for review May 1, 2021 20:13
@ehaas
Copy link
Sponsor Contributor Author

ehaas commented May 1, 2021

Ok, translate-c and the cache seem to be working.

Next question - is there a way to use -cflags with @cImport? Or would the recommended practice be to use zig translate-c with the correct flags and then import the translated file?

@andrewrk
Copy link
Member

andrewrk commented May 1, 2021

There is currently no way to do that, however I could see that being a reasonable enhancement to make to the @cImport feature, since you could have multiple @cImport in one compilation, and each could have different flags for their respective C headers. If you're interested in pursuing this, I suggest to open a new proposal for enhancing @cImport to support C compiler flags specific to that @cImport instance.

Or would the recommended practice be to use zig translate-c with the correct flags and then import the translated file?

This would certainly solve the problem, and often can be a generally more preferable approach than @cImport because it removes a dependency on the compiler having LLVM extensions enabled, and, assuming the ABI is stable, allows some manual cleanup to the translated types, especially with regards to pointer safety and coercion.

@andrewrk andrewrk merged commit 19aab53 into ziglang:master May 12, 2021
@andrewrk
Copy link
Member

Looks great, thank you!

@ehaas ehaas deleted the translate-c-cflags branch May 13, 2021 03:25
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

2 participants