Skip to content

regression: translate-c output struct depends on itself (false positive) #17227

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

Closed
acgaudette opened this issue Sep 21, 2023 · 10 comments
Closed
Labels
bug Observed behavior contradicts documented or intended behavior translate-c C to Zig source translation feature (@cImport)
Milestone

Comments

@acgaudette
Copy link

Zig Version

0.12.0-dev.464+a63a1c5cb

Steps to Reproduce and Observed Behavior

a project depending on several C headers previously compiled, but now throws an error inside a file generated by translate-c.

the header in question is generated by cimgui (including as a keyword in case anyone else has this specific problem) and should probably be considered 'valid' as it has many dependents in the wild.

I've pared the error down to a minimal example:

const c = @cImport(@cInclude("include.h"));

pub fn main() void
{
        var b: c.B = undefined;
        _ = b;
}

'include.h'

struct A
{
        struct C* c;
};

struct B
{
        struct A* a;
};

struct C
{
        struct B b;
};
error: struct '.cimport.struct_B' depends on itself
pub const struct_B = extern struct {
                     ~~~~~~~^~~~~~

the error is pretty easy to reproduce with the actual cimgui header, so I can include that if necessary.

this occurs in 0.12.0-dev.464+a63a1c5cb but not 0.12.0-dev.415+5af5d87ad, which means this occurred somewhere inside the big batch of commits in the last couple days.

perhaps
f91ff9a @jayschwa
or changes by @andrewrk, @jacobly0

Expected Behavior

compilation succeeds

@acgaudette acgaudette added the bug Observed behavior contradicts documented or intended behavior label Sep 21, 2023
@acgaudette
Copy link
Author

to reproduce with the cimgui header:

include https://github.com/cimgui/cimgui/blob/docking_inter/cimgui.h

const c = @cImport({
        @cDefine("CIMGUI_DEFINE_ENUMS_AND_STRUCTS", {});
        @cInclude("include.h");
});

pub fn main() void
{
        var cmd: c.ImVector_ImDrawCmd = undefined;
        _ = cmd;
}
cimport.zig:552:46: error: struct '.cimport.struct_ImVector_ImDrawCmd' depends on itself
pub const struct_ImVector_ImDrawCmd = extern struct {
                                      ~~~~~~~^~~~~~

@jayschwa
Copy link
Contributor

There are several open issues for false dependency loops, which might be getting triggered here. I think what I'll do is make a new PR that temporarily disables zero values for structs. Once those false dependency loop issues have been fixed, zero values for structs can be re-enabled. Thanks for the report.

@acgaudette
Copy link
Author

👍 can you link me to any of those issues?

@nektro
Copy link
Contributor

nektro commented Sep 21, 2023

@jayschwa
Copy link
Contributor

👍 can you link me to any of those issues?

#12325 and #14147 seem like the relevant ones.

@acgaudette
Copy link
Author

ah I see. didn't expect anything so old / before 0.12.0-dev.415+5af5d87ad

@michal-z
Copy link
Contributor

michal-z commented Sep 22, 2023

This is a new bug (regression). cimgui header was working fine with cImport. zig-gamedev project is also affected by this issue.

The regression is in 0.12.0-dev.464 (0.12.0-dev.415 works fine).

@mlugg Could that be caused by your recent translate-c changes?

@mlugg
Copy link
Member

mlugg commented Sep 22, 2023

Nope, it's what @jayschwa said - default zero values causing false dependency loops. That PR wasn't me.

HOWEVER! This should actually be fixed on master as of the merging of #17172 (it works according to my local tests). The issue was related to struct alignment resolution forcing full layout resolution: when zeroes attempted to return a pointer, it at some point tried to resolve the pointer's alignment, which attempted to resolve the layout of the struct within, which triggered a dependency loop. This was fixed by the PR linked above, because struct alignment resolution is now distinct from general layout resolution.

For anyone who experienced this bug, try your code out now - it should be fixed!

@michal-z
Copy link
Contributor

I see, thanks for the info!

@jayschwa jayschwa added the translate-c C to Zig source translation feature (@cImport) label Sep 22, 2023
@jayschwa jayschwa added this to the 0.12.0 milestone Sep 22, 2023
@jayschwa
Copy link
Contributor

Confirmed that this is fixed in master after #17172 was merged. I tested with both the small reproducer and the cimgui example. It should be noted that the ziglang.org download page doesn't have a new build with the fix yet, but probably will within the next day or so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior translate-c C to Zig source translation feature (@cImport)
Projects
None yet
Development

No branches or pull requests

5 participants