translate-c refuses to translate packed structs the compiler would otherwise accept (needed for M1 macOS stdlib.h) #12733

Techcable · 2022-09-04T06:59:56Z

Zig Version

0.10.0-dev.3871+b7d5582de

Current ABI behavior of `packed struct` on stage2 (Zig side)

As of commit b83c037 , a packed struct is only allowed to be extern (on stage2) If it is "ABI sized".

According to @Vexu this is because the stage3 representation is incompatible with C.

Consider the following struct (full file here):

pub const Example = packed struct {
    a: i32 = 5,
    b: i32 = 6,
    c: i8 = 4,
};

This struct is not ABI-sized on my x86_64 Linux laptop, so I get the following error:

hello.zig:10:5: error: unable to export type 'hello.Example'
pub export var exported = Example { };
    ^~~~~~
hello.zig:10:5: note: only extern structs and ABI sized packed structs are extern compatible
hello.zig:4:28: note: struct declared here
pub const Example = packed struct {

This is the error I would expect (given the current rules).

If I delete the c: i8 field, then it successfully compiles and I get hello.Example{ .a = 5, .b = 6 } as output.

Failed `translate-c` error

Consider the following C header file:

struct example {
    int val; 
} __attribute__((__packed__));

If you ran zig translate-c on this, you would expect this to translate into packed struct { val: c_int }

However the current output is

pub const example = @compileError("cannot translate packed record union"); // hello.h:1:8

This is a false-positive! The restrictions translate-c puts on packed struct should be consistent with the regular compiler.

Cause

With a quick ripgrep, the source of translate_c.zig seems to have a blanket-ban on translating packed structs from C -> Zig.

zig/src/translate_c.zig

Lines 1169 to 1171 in b7d5582

    
           if (!c.zig_is_stage1 and is_packed) { 
        
               return failDecl(c, record_loc, bare_name, "cannot translate packed record union", .{}); 
        
           }

Unfortunately this blanket-ban on packed struct includes the "ABI-sized" structs that Zig would normally .

M1 Mac woes

While this bug is not platform-specific, it is really severe on aarch64-macos.12. The stdlib.h (indirectly) includes two of these structures for the purposes. They appear to be very important and I can't even compile an empty file without getting an error.

zig/lib/libc/include/aarch64-macos.12-none/libkern/arm/OSByteOrder.h

Lines 64 to 74 in b64a1d5

    
           struct _OSUnalignedU16 { 
        
           	volatile uint16_t __val; 
        
           } __attribute__((__packed__)); 
        
           struct _OSUnalignedU32 { 
        
           	volatile uint32_t __val; 
        
           } __attribute__((__packed__)); 
        
           struct _OSUnalignedU64 { 
        
           	volatile uint64_t __val; 
        
           } __attribute__((__packed__));

Notice that both of these packed structures "ABI-sized". Presumably, if translate-c were to accept them, the Zig compiler would handle them properly.

The text was updated successfully, but these errors were encountered:

@CompileError

This relaxes the restriction added in c7884af and fixes ziglang#12733 Right now, the compiler only considered packed structs to be C compatible (extern) if the structs are "ABI sized". See also b83c037 for details on the extern restrictions on `packed struct` This is an issue on all platforms, but is of particular importance on aarch64-macos (see issue for details) Previously a type like `` struct example { int val; } __attribute__((__packed__)); `` would fail to translate and turn into `` pub const example = @CompileError("cannot translate packed record union"); `` This fixes this issue for ABI-sized struct, making the translate-c rules consistent with the regular compiler. However, we still keep the restriction on structs that are not "ABI-sized" So `` struct example { long a, b, c } __attribute__((__packed__)); `` will still fail **Implementation details**: For the purposes of translate-c, a packed struct is considered "ABI-sized" when sizeof(target_struct) <= sizeof(void*) Unfortunately, right now the translate-c `*Context` structure (on the Zig side) doesn't directly have any information on the target we're translating into. However, clang has this target information, and keeps it in clang::ASTContext. I added a helper function to extract the `clang::TargetInfo` from this, then query the pointer size. This is potentially controversial, since it technically makes the output target-dependent. The decision of whether to accept/reject a packed struct varies depending on the target's pointer size. Howevee, the translator already asks for (integer) field offsets elsewhere, so I'm pretty sure target info already impacts translation. We also add getSize() and getDataSize() functions to *clang.RecordDecl. These return padded and unpadded sizes respectively. For a packed struct, they should be the same. The overall effect is ~3 more functions added to zig_clang.cpp used to extract target info. It alo makes the existing "cannot translate packed record" error condition on a new 'isAbiSizedPackedRecord' helper function (which is fairly small)

@CompileError

This relaxes the restriction added in c7884af and fixes ziglang#12733 Right now, the compiler only considers packed structs to be C compatible (extern) if the structs are "ABI sized". See also b83c037 for details on the extern restrictions on `packed struct` This is an issue on all platforms, but is of particular importance on aarch64-macos (seeissue for details) Before this commit, a type like `` struct example { int val; } __attribute__((__packed__)); `` would fail to translate and turn into `` pub const example = @CompileError("cannot translate packed record union"); `` This fixes this issue for ABI-sized structs, making the translate-c rules consistent with the regular compiler. However, we still reject packed structs that are not "ABI-sized", giving the same `@compileError` message So `` struct example { long a, b, c } __attribute__((__packed__)); `` will still fail (in the sense that it will emit a `@compileError`) **Implementation details**: For the purposes of translate-c, a packed struct is considered "ABI-sized" when sizeof(target_struct) <= sizeof(void*) Unfortunately, right now the translate-c `*Context` structure (on the Zig side) doesn't directly have any information on the target we're translating into. However, clang has this target information, and keeps it in clang::ASTContext. I added a helper function to extract the `clang::TargetInfo` from this, then query the pointer size. This is potentially controversial, since it technically makes the output target-dependent. The decision of whether to accept/reject a packed struct varies depending on the target's pointer size. However, the translator already asks for (integer) field offsets elsewhere, so I'm pretty sure target info already impacts translation. We also add getSize() and getDataSize() functions to *clang.RecordDecl. These return padded and unpadded sizes respectively. For a packed struct, they should be the same. The overall effect is 4 more functions added to zig_clang.cpp, in order to extract target info & query record sizes.

@CompileError

This relaxes the restriction added in c7884af and fixes ziglang#12733 Right now, the compiler only considers packed structs to be C compatible (extern) if the structs are "ABI sized". See also b83c037 for details on the extern restrictions on `packed struct` This is an issue on all platforms, but is of particular importance on aarch64-macos (seeissue for details) Before this commit, a type like `` struct example { int val; } __attribute__((__packed__)); `` would fail to translate and turn into `` pub const example = @CompileError("cannot translate packed record union"); `` This fixes this issue for ABI-sized structs, making the translate-c rules consistent with the regular compiler. However, we still reject packed structs that are not "ABI-sized", giving the same `@compileError` message So `` struct example { long a, b, c } __attribute__((__packed__)); `` will still fail (in the sense that it will emit a `@compileError`) **Implementation details**: For the purposes of translate-c, a packed struct is considered "ABI-sized" when sizeof(target_struct) <= sizeof(void*) Unfortunately, right now the translate-c `*Context` structure (on the Zig side) doesn't directly have any information on the target we're translating into. However, clang has this target information, and keeps it in clang::ASTContext. I added a helper function to extract the `clang::TargetInfo` from this, then query the pointer size. This is potentially controversial, since it technically makes the output target-dependent. The decision of whether to accept/reject a packed struct varies depending on the target's pointer size. However, the translator already asks for (integer) field offsets elsewhere, so I'm pretty sure target info already impacts translation. We also add getSize() and getDataSize() functions to *clang.RecordDecl. These return padded and unpadded sizes respectively. For a packed struct, they should be the same. The overall effect is 4 more functions added to zig_clang.cpp, in order to extract target info & query record sizes.

@Vexu

Superceeds PR ziglang#12735 (now supporting all packed structs in GNU C) Fixes issue ziglang#12733 This stops translating C packed struct as a Zig packed struct. Instead use a regular `extern struct` with `align(1)`. This is because (as @Vexu explained) Zig packed structs are really just integers (not structs). Alignment issue is more complicated. I think @ifreund was the first to notice it in his comment on PR ziglang#12735 Justification of my interpretion of the C(lang) behavior comes from a careful reading of the GCC docs for type & variable attributes: (clang emulates gnu's packed attribute here) The final line of the documentation for __attribute__ ((aligned)) [on types] says: > When used on a struct, or struct member, *the aligned attribute can only increase the alignment*; in order to decrease it, the packed attribute must be specified as well. This implies that GCC uses the `packed` attribute for alignment purposes in addition to eliminating padding. The documentation for __attribute__((packed)) [on types], states: > This attribute, attached to a struct, union, or C++ class type definition, specifies that each of its members (other than zero-width bit-fields) is placed to minimize the memory required. **This is equivalent to specifying the packed attribute on each of the members**. The key is resolving this indirection, and looking at the documentation for __attribute__((packed)) [on fields (wierdly under "variables" section)]: > The packed attribute specifies that a **structure member should have the smallest possible alignment** — one bit for a bit-field and one byte otherwise, unless a larger value is specified with the aligned attribute. The attribute does not apply to non-member objects. Furthermore, alignment is the only effect of the packed attribute mentioned in the GCC docs (for "common" architecture). Based on this, it seems safe to completely substitute C 'packed' with Zig 'align(1)'. Target-specific or undocumented behavior potentially changes this. Unfortunately, the current implementation of `translate-c` translates as `packed struct` without alignment info. Because Zig packed structs are really integers (as mentioned above), they are the wrong interpretation and we should be using 'extern struct'. Running `translate-c` on the following code: ```c struct foo { char a; int b; } __attribute__((packed)); struct bar { char a; int b; short c; __attribute__((aligned(8))) long d; } __attribute__((packed)); ``` Previously used a 'packed struct' (which was not FFI-safe on stage1). After applying this change, the translated structures have align(1) explicitly applied to all of their fields AS EXPECTED (unless explicitly overriden). This makes Zig behavior for `tranlsate-c` consistent with clang/GCC. Here is the newly produced (correct) output for the above example: ```zig pub const struct_foo = extern struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = extern struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` Also note for reference: Since the last stable release (0.9.1), there was a change in the language spec related to the alignment of packed structures. The docs for Zig 0.9.1 read: > Packed structs have 1-byte alignment. So the old behavior of translate-c (not specifying any alignment) was possibly correct back then. However the current docs read: > Packed structs have the same alignment as their backing integer Suggsestive both to the change to an integer-backed representation which is incompatible with C's notation.

@Vexu

Superceeds PR ziglang#12735 (now supporting all packed structs in GNU C) Fixes issue ziglang#12733 This stops translating C packed struct as a Zig packed struct. Instead use a regular `extern struct` with `align(1)`. This is because (as @Vexu explained) Zig packed structs are really just integers (not structs). Alignment issue is more complicated. I think @ifreund was the first to notice it in his comment on PR ziglang#12735 Justification of my interpretion of the C(lang) behavior comes from a careful reading of the GCC docs for type & variable attributes: (clang emulates gnu's packed attribute here) The final line of the documentation for __attribute__ ((aligned)) [on types] says: > When used on a struct, or struct member, *the aligned attribute can only increase the alignment*; in order to decrease it, the packed attribute must be specified as well. This implies that GCC uses the `packed` attribute for alignment purposes in addition to eliminating padding. The documentation for __attribute__((packed)) [on types], states: > This attribute, attached to a struct, union, or C++ class type definition, specifies that each of its members (other than zero-width bit-fields) is placed to minimize the memory required. **This is equivalent to specifying the packed attribute on each of the members**. The key is resolving this indirection, and looking at the documentation for __attribute__((packed)) [on fields (wierdly under "variables" section)]: > The packed attribute specifies that a **structure member should have the smallest possible alignment** — one bit for a bit-field and one byte otherwise, unless a larger value is specified with the aligned attribute. The attribute does not apply to non-member objects. Furthermore, alignment is the only effect of the packed attribute mentioned in the GCC docs (for "common" architecture). Based on this, it seems safe to completely substitute C 'packed' with Zig 'align(1)'. Target-specific or undocumented behavior potentially changes this. Unfortunately, the current implementation of `translate-c` translates as `packed struct` without alignment info. Because Zig packed structs are really integers (as mentioned above), they are the wrong interpretation and we should be using 'extern struct'. Running `translate-c` on the following code: ```c struct foo { char a; int b; } __attribute__((packed)); struct bar { char a; int b; short c; __attribute__((aligned(8))) long d; } __attribute__((packed)); ``` Previously used a 'packed struct' (which was not FFI-safe on stage1). After applying this change, the translated structures have align(1) explicitly applied to all of their fields AS EXPECTED (unless explicitly overriden). This makes Zig behavior for `tranlsate-c` consistent with clang/GCC. Here is the newly produced (correct) output for the above example: ```zig pub const struct_foo = extern struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = extern struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` Also note for reference: Since the last stable release (0.9.1), there was a change in the language spec related to the alignment of packed structures. The docs for Zig 0.9.1 read: > Packed structs have 1-byte alignment. So the old behavior of translate-c (not specifying any alignment) was possibly correct back then. However the current docs read: > Packed structs have the same alignment as their backing integer Suggsestive both to the change to an integer-backed representation which is incompatible with C's notation.

@Vexu

Superceeds PR ziglang#12735 (now supporting all packed structs in GNU C) Fixes issue ziglang#12733 This stops translating C packed struct as a Zig packed struct. Instead use a regular `extern struct` with `align(1)`. This is because (as @Vexu explained) Zig packed structs are really just integers (not structs). Alignment issue is more complicated. I think @ifreund was the first to notice it in his comment on PR ziglang#12735 Justification of my interpretion of the C(lang) behavior comes from a careful reading of the GCC docs for type & variable attributes: (clang emulates gnu's packed attribute here) The final line of the documentation for __attribute__ ((aligned)) [on types] says: > When used on a struct, or struct member, *the aligned attribute can only increase the alignment*; in order to decrease it, the packed attribute must be specified as well. This implies that GCC uses the `packed` attribute for alignment purposes in addition to eliminating padding. The documentation for __attribute__((packed)) [on types], states: > This attribute, attached to a struct, union, or C++ class type definition, specifies that each of its members (other than zero-width bit-fields) is placed to minimize the memory required. **This is equivalent to specifying the packed attribute on each of the members**. The key is resolving this indirection, and looking at the documentation for __attribute__((packed)) [on fields (wierdly under "variables" section)]: > The packed attribute specifies that a **structure member should have the smallest possible alignment** — one bit for a bit-field and one byte otherwise, unless a larger value is specified with the aligned attribute. The attribute does not apply to non-member objects. Furthermore, alignment is the only effect of the packed attribute mentioned in the GCC docs (for "common" architecture). Based on this, it seems safe to completely substitute C 'packed' with Zig 'align(1)'. Target-specific or undocumented behavior potentially changes this. Unfortunately, the current implementation of `translate-c` translates as `packed struct` without alignment info. Because Zig packed structs are really integers (as mentioned above), they are the wrong interpretation and we should be using 'extern struct'. Running `translate-c` on the following code: ```c struct foo { char a; int b; } __attribute__((packed)); struct bar { char a; int b; short c; __attribute__((aligned(8))) long d; } __attribute__((packed)); ``` Previously used a 'packed struct' (which was not FFI-safe on stage1). After applying this change, the translated structures have align(1) explicitly applied to all of their fields AS EXPECTED (unless explicitly overriden). This makes Zig behavior for `tranlsate-c` consistent with clang/GCC. Here is the newly produced (correct) output for the above example: ```zig pub const struct_foo = extern struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = extern struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` Also note for reference: Since the last stable release (0.9.1), there was a change in the language spec related to the alignment of packed structures. The docs for Zig 0.9.1 read: > Packed structs have 1-byte alignment. So the old behavior of translate-c (not specifying any alignment) was possibly correct back then. However the current docs read: > Packed structs have the same alignment as their backing integer Suggsestive both to the change to an integer-backed representation which is incompatible with C's notation.

@Vexu

Superceeds PR ziglang#12735 (now supporting all packed structs in GNU C) Fixes issue ziglang#12733 This stops translating C packed struct as a Zig packed struct. Instead use a regular `extern struct` with `align(1)`. This is because (as @Vexu explained) Zig packed structs are really just integers (not structs). Alignment issue is more complicated. I think @ifreund was the first to notice it in his comment on PR ziglang#12735 Justification of my interpretion of the C(lang) behavior comes from a careful reading of the GCC docs for type & variable attributes: (clang emulates gnu's packed attribute here) The final line of the documentation for __attribute__ ((aligned)) [on types] says: > When used on a struct, or struct member, *the aligned attribute can only increase the alignment*; in order to decrease it, the packed attribute must be specified as well. This implies that GCC uses the `packed` attribute for alignment purposes in addition to eliminating padding. The documentation for __attribute__((packed)) [on types], states: > This attribute, attached to a struct, union, or C++ class type definition, specifies that each of its members (other than zero-width bit-fields) is placed to minimize the memory required. **This is equivalent to specifying the packed attribute on each of the members**. The key is resolving this indirection, and looking at the documentation for __attribute__((packed)) [on fields (wierdly under "variables" section)]: > The packed attribute specifies that a **structure member should have the smallest possible alignment** — one bit for a bit-field and one byte otherwise, unless a larger value is specified with the aligned attribute. The attribute does not apply to non-member objects. Furthermore, alignment is the only effect of the packed attribute mentioned in the GCC docs (for "common" architecture). Based on this, it seems safe to completely substitute C 'packed' with Zig 'align(1)'. Target-specific or undocumented behavior potentially changes this. Unfortunately, the current implementation of `translate-c` translates as `packed struct` without alignment info. Because Zig packed structs are really integers (as mentioned above), they are the wrong interpretation and we should be using 'extern struct'. Running `translate-c` on the following code: ```c struct foo { char a; int b; } __attribute__((packed)); struct bar { char a; int b; short c; __attribute__((aligned(8))) long d; } __attribute__((packed)); ``` Previously used a 'packed struct' (which was not FFI-safe on stage1). After applying this change, the translated structures have align(1) explicitly applied to all of their fields AS EXPECTED (unless explicitly overriden). This makes Zig behavior for `tranlsate-c` consistent with clang/GCC. Here is the newly produced (correct) output for the above example: ```zig pub const struct_foo = extern struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = extern struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` Also note for reference: Since the last stable release (0.9.1), there was a change in the language spec related to the alignment of packed structures. The docs for Zig 0.9.1 read: > Packed structs have 1-byte alignment. So the old behavior of translate-c (not specifying any alignment) was possibly correct back then. However the current docs read: > Packed structs have the same alignment as their backing integer Suggsestive both to the change to an integer-backed representation which is incompatible with C's notation.

@Vexu

Superceeds PR ziglang#12735 (now supporting all packed structs in GNU C) Fixes issue ziglang#12733 This stops translating C packed struct as a Zig packed struct. Instead use a regular `extern struct` with `align(1)`. This is because (as @Vexu explained) Zig packed structs are really just integers (not structs). Alignment issue is more complicated. I think @ifreund was the first to notice it in his comment on PR ziglang#12735 Justification of my interpretion of the C(lang) behavior comes from a careful reading of the GCC docs for type & variable attributes: (clang emulates gnu's packed attribute here) The final line of the documentation for __attribute__ ((aligned)) [on types] says: > When used on a struct, or struct member, *the aligned attribute can only increase the alignment*; in order to decrease it, the packed attribute must be specified as well. This implies that GCC uses the `packed` attribute for alignment purposes in addition to eliminating padding. The documentation for __attribute__((packed)) [on types], states: > This attribute, attached to a struct, union, or C++ class type definition, specifies that each of its members (other than zero-width bit-fields) is placed to minimize the memory required. **This is equivalent to specifying the packed attribute on each of the members**. The key is resolving this indirection, and looking at the documentation for __attribute__((packed)) [on fields (wierdly under "variables" section)]: > The packed attribute specifies that a **structure member should have the smallest possible alignment** — one bit for a bit-field and one byte otherwise, unless a larger value is specified with the aligned attribute. The attribute does not apply to non-member objects. Furthermore, alignment is the only effect of the packed attribute mentioned in the GCC docs (for "common" architecture). Based on this, it seems safe to completely substitute C 'packed' with Zig 'align(1)'. Target-specific or undocumented behavior potentially changes this. Unfortunately, the current implementation of `translate-c` translates as `packed struct` without alignment info. Because Zig packed structs are really integers (as mentioned above), they are the wrong interpretation and we should be using 'extern struct'. Running `translate-c` on the following code: ```c struct foo { char a; int b; } __attribute__((packed)); struct bar { char a; int b; short c; __attribute__((aligned(8))) long d; } __attribute__((packed)); ``` Previously used a 'packed struct' (which was not FFI-safe on stage1). After applying this change, the translated structures have align(1) explicitly applied to all of their fields AS EXPECTED (unless explicitly overriden). This makes Zig behavior for `tranlsate-c` consistent with clang/GCC. Here is the newly produced (correct) output for the above example: ```zig pub const struct_foo = extern struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = extern struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` Also note for reference: Since the last stable release (0.9.1), there was a change in the language spec related to the alignment of packed structures. The docs for Zig 0.9.1 read: > Packed structs have 1-byte alignment. So the old behavior of translate-c (not specifying any alignment) was possibly correct back then. However the current docs read: > Packed structs have the same alignment as their backing integer Suggsestive both to the change to an integer-backed representation which is incompatible with C's notation.

Techcable added the bug Observed behavior contradicts documented or intended behavior label Sep 4, 2022

Techcable changed the title ~~translate-c refuses to translate any packed structs, even though "ABI-sized" ones are normally allowed~~ translate-c refuses to translate packed structs the compiler would otherwise accept (needed for M1 macOS stdlib.h) Sep 4, 2022

Techcable mentioned this issue Sep 4, 2022

translate-c: Allow translating packed C structs iff ABI-sized #12735

Closed

Vexu mentioned this issue Sep 4, 2022

Compilation error on stage2 with @cImport and SDL2 #12737

Closed

Techcable mentioned this issue Sep 5, 2022

translate-c: Translate clang packed struct C into Zig extern struct with align(1) #12745

Merged

Vexu added the translate-c C to Zig source translation feature (@cImport) label Sep 7, 2022

Vexu added this to the 0.10.0 milestone Sep 7, 2022

andrewrk modified the milestones: 0.10.0, 0.10.1 Oct 12, 2022

andrewrk modified the milestones: 0.10.1, 0.11.0 Jan 10, 2023

Vexu closed this as completed Apr 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

translate-c refuses to translate packed structs the compiler would otherwise accept (needed for M1 macOS stdlib.h) #12733

translate-c refuses to translate packed structs the compiler would otherwise accept (needed for M1 macOS stdlib.h) #12733

Techcable commented Sep 4, 2022 •

edited

Loading

translate-c refuses to translate packed structs the compiler would otherwise accept (needed for M1 macOS stdlib.h) #12733

translate-c refuses to translate packed structs the compiler would otherwise accept (needed for M1 macOS stdlib.h) #12733

Comments

Techcable commented Sep 4, 2022 • edited Loading

Zig Version

Current ABI behavior of packed struct on stage2 (Zig side)

Failed translate-c error

Cause

M1 Mac woes

Techcable commented Sep 4, 2022 •

edited

Loading

Current ABI behavior of `packed struct` on stage2 (Zig side)

Failed `translate-c` error