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

Techcable · 2022-09-04T08:16:48Z

This relaxes the restriction added in c7884af

It fixes issue #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 my other 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 with the appropriate error (in the sense it will emit @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.

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 4 more functions added to zig_clang.cpp used to
extract target info & query sizes.

Overall Effect on M1 Macs

As I mentioned in the issue, aarch64-macos.12 uses (ABI-sized) packed structs in stdlib.h. Even an empty file fails to compile without support. This makes resolving the underlying problem (#12733) crucial.

After fixing the bug (and applying #12730 ), I can get build test-run-translated-c to finally work!

Before, it just gave a nasty error:

/Users/nicholas/.cache/zig/o/d0183770046c7c4914f3676ac67443fd/source.zig:560:39: error: use of undeclared identifier 'struct__OSUnalignedU16'
    return _OSSwapInt16(@intToPtr([*c]struct__OSUnalignedU16, @intCast(usize, @ptrToInt(_base)) +% _offset).*.__val);
                                      ^~~~~~~~~~~~~~~~~~~~~~
/Users/nicholas/.cache/zig/o/d0183770046c7c4914f3676ac67443fd/source.zig:565:39: error: use of undeclared identifier 'struct__OSUnalignedU32'
    return _OSSwapInt32(@intToPtr([*c]struct__OSUnalignedU32, @intCast(usize, @ptrToInt(_base)) +% _offset).*.__val);
                                      ^~~~~~~~~~~~~~~~~~~~~~

Anyways, combining these two PRs seems to fix the most serious stage3 compiler and tranlsate-c bugs that I've been encountering on my M1 Mac.

In addition to fixing test-run-translated-c, it also fixes my own libraries. In particular the test suite of zig-mpack finally passes!

With stage3, most of the remaining issues in my main codebase seem to be bugs with my project that stage1 didn't catch. For example, I had two conflicting definitions of const debug = ...... It was defined twice in my root module.

Hopefully these fixes mean I can finally get back to doing real programming with stage3 and Zig as my backend. Big thanks of course to @Vexu and the whole Zig team 🎉

Remaining (possible) questions

Do I need to add test coverage for this new functionality?
- Usually this is required, however this is really more of a fix for existing (broken) tests than a new one
- I really don't understand why we only support certain packed struct in stage3. Ideally wouldn't we support all of them?
Is there a cleaner way to do this without querying clang so much?

Please note, I have homework due (that I've been dodging) so I may not be super responsive (maybe just amend the PR?)

Vexu

Packed structs are not ABI compatible with C structs but with ints. Ints and structs with only one pointer sized int seem to be always compatible so if you add a check for there only being one field then I think this is fine until we get a better solution.

ifreund · 2022-09-04T09:45:50Z

Packed structs are not ABI compatible with C structs but with ints. Ints and structs with only one pointer sized int seem to be always compatible so if you add a check for there only being one field then I think this is fine until we get a better solution.

I think even this specific case of packed structs with a single integer is incompatible as the alignment is different. It seems that the entire reason these structures use __attribute__(__packed__) in the header is to allow unaligned integers. However, zig's integer backed packed structs are no different from the backing integer on the ABI and use the natural alignment of the backing integer.

I believe the correct translation of this example would be an extern struct with align(1) on the field:

 struct _OSUnalignedU32 { 
 	uint32_t __val; 
 } __attribute__((__packed__));

pub const _OSUnalignedU32 = extern struct {
    val: u32 align(1),
};

I've left the volatile out of this example to keep things simpler as I forget how we translate that.

@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.

Techcable · 2022-09-04T23:02:59Z

Packed structs are not ABI compatible with C structs but with ints. Ints and structs with only one pointer sized int seem to be always compatible so if you add a check for there only being one field then I think this is fine until we get a better solution.

@Vexu

I have added in this restriction (and rebased onto master).

However we may want to look deeper into why the ABIs are incompatible. It may have something to do with @ifreund 's comment on alignment.

I think even this specific case of packed structs with a single integer is incompatible as the alignment is different. It seems that the entire reason these structures use __attribute__(__packed__) in the header is to allow unaligned integers. However, zig's integer backed packed structs are no different from the backing integer on the ABI and use the natural alignment of the backing integer.

@ifreund

Agreed. I think you are correct that align(1) is required. However, the way I am read the GCC documentation, this is actually implied by the packed modifier (maybe this is why we are having ABI troubles?).

First looking at the Common Type attributes page:

The final line of the documentation for __attribute__ ((aligned)) 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.
When used as part of a typedef, the aligned attribute can both increase and decrease alignment, and specifying the packed attribute generates a warning.

This implies the __packed__ attribute is actually used for alignment purposes. This is not immediately clear; there is a level of indirection in the docs.

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.

Then I went to go look at the documentation for Common Variable Attributes:

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.

Combining this means that align(1) is automatically implied for every element of a packed structure (from GCC perspective).

I think this is (yet another) independent bug. It is present even in stage1 translate-c.

We need to change translate-c to reflect that and add align(1) to every field. Maybe that will resolve some of our ABI problems and we can relax the "ABI-sized" restriction?

@ifreund

This is relevant to the fix in ziglang#12735 , but also fixes stage1. This comes from a careful reading of the GCC docs for type & variable attributes. 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], does not directly state this relationship with alignment. However, it *does* say that the 'packed' attribute is applied recursively to each member. > 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]: > 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. (NOTE: Somewhat confusingly, field attributes are documented under "variables") This means that a C structure with __attribute__((packed)) automatically implies align(1) on each of its member. Unfortunately, the current implementation of `translate-c` does not reflect this. 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)); ``` does not apply any alignment attributes to any fields except d. Sometimes __attribute__((packed)) is used only for alignment purposes. It is a semi-portable way to do unaligned stores. In particular, the aarch64-macos.12 headers do this in stdlib.h This was noted by @ifreund in the discussion of PR ziglang#12735 The structures in the MacOS headers were being used chiefly for their alignment properties, not for padding purposes (they only had one field). After applying this change, the translated structures have align(1) explicitly applied to all of their fields (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 = packed struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = packed struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` This is closely related with PR ziglang#12735 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 So now `translate-c` definitely needs to specify align(1) for every field (unless the spec is overriden).

@ifreund

This is relevant to the fix in ziglang#12735 , but also fixes stage1. This comes from a careful reading of the GCC docs for type & variable attributes. 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], does not directly state this relationship with alignment. However, it *does* say that the 'packed' attribute is applied recursively to each member. > 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]: > 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. (NOTE: Somewhat confusingly, field attributes are documented under "variables") This means that a C structure with __attribute__((packed)) automatically implies align(1) on each of its member. Unfortunately, the current implementation of `translate-c` does not reflect this. 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)); ``` does not apply any alignment attributes to any fields except d. Sometimes __attribute__((packed)) is used only for alignment purposes. It is a semi-portable way to do unaligned stores. In particular, the aarch64-macos.12 headers do this in stdlib.h This was noted by @ifreund in the discussion of PR ziglang#12735 The structures in the MacOS headers were being used chiefly for their alignment properties, not for padding purposes (they only had one field). After applying this change, the translated structures have align(1) explicitly applied to all of their fields (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 = packed struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = packed struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` This is closely related with PR ziglang#12735 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 So now `translate-c` definitely needs to specify align(1) for every field (unless the spec is overriden).

@ifreund

This is relevant to the fix in ziglang#12735 , but also fixes stage1. This comes from a careful reading of the GCC docs for type & variable attributes. 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], does not directly state this relationship with alignment. However, it *does* say that the 'packed' attribute is applied recursively to each member. > 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]: > 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. (NOTE: Somewhat confusingly, field attributes are documented under "variables") This means that a C structure with __attribute__((packed)) automatically implies align(1) on each of its member. Unfortunately, the current implementation of `translate-c` does not reflect this. 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)); ``` does not apply any alignment attributes to any fields except d. Sometimes __attribute__((packed)) is used only for alignment purposes. It is a semi-portable way to do unaligned stores. In particular, the aarch64-macos.12 headers do this in stdlib.h This was noted by @ifreund in the discussion of PR ziglang#12735 The structures in the MacOS headers were being used chiefly for their alignment properties, not for padding purposes (they only had one field). After applying this change, the translated structures have align(1) explicitly applied to all of their fields (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 = packed struct { a: u8 align(1), b: c_int align(1), }; pub const struct_bar = packed struct { a: u8 align(1), b: c_int align(1), c: c_short align(1), d: c_long align(8), }; ``` This is closely related with PR ziglang#12735 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 So now `translate-c` definitely needs to specify align(1) for every field (unless the spec is overriden).

Techcable · 2022-09-12T08:47:53Z

This PR technically works, but only by accident. In Zig , a packed struct is really just an integer. The correct translation is as an extern struct with align(1)

I plan to amend my other PR tomorrow (#12745) to fix this.

@Vexu has pointed this out a couple times, but it took a while to sink in 😉

Here is the code that (currently) lowers packed structs directly LLVM types:

zig/src/codegen/llvm.zig

Lines 3541 to 3572 in e323cf1

    
           if (struct_obj.layout == .Packed) { 
        
               const big_bits = struct_obj.backing_int_ty.bitSize(target); 
        
               const int_llvm_ty = dg.context.intType(@intCast(c_uint, big_bits)); 
        
               const fields = struct_obj.fields.values(); 
        
               comptime assert(Type.packed_struct_layout_version == 2); 
        
               var running_int: *const llvm.Value = int_llvm_ty.constNull(); 
        
               var running_bits: u16 = 0; 
        
               for (field_vals) |field_val, i| { 
        
                   const field = fields[i]; 
        
                   if (!field.ty.hasRuntimeBitsIgnoreComptime()) continue; 
        
                   const non_int_val = try dg.lowerValue(.{ 
        
                       .ty = field.ty, 
        
                       .val = field_val, 
        
                   }); 
        
                   const ty_bit_size = @intCast(u16, field.ty.bitSize(target)); 
        
                   const small_int_ty = dg.context.intType(ty_bit_size); 
        
                   const small_int_val = if (field.ty.isPtrAtRuntime()) 
        
                       non_int_val.constPtrToInt(small_int_ty) 
        
                   else 
        
                       non_int_val.constBitCast(small_int_ty); 
        
                   const shift_rhs = int_llvm_ty.constInt(running_bits, .False); 
        
                   // If the field is as large as the entire packed struct, this 
        
                   // zext would go from, e.g. i16 to i16. This is legal with 
        
                   // constZExtOrBitCast but not legal with constZExt. 
        
                   const extended_int_val = small_int_val.constZExtOrBitCast(int_llvm_ty); 
        
                   const shifted = extended_int_val.constShl(shift_rhs); 
        
                   running_int = running_int.constOr(shifted); 
        
                   running_bits += ty_bit_size; 
        
               } 
        
               return running_int; 
        
           }

@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 force-pushed the translate-c/support-abi-sized-packed-struct branch from bd59aa5 to eb1eea0 Compare September 4, 2022 08:24

Vexu requested changes Sep 4, 2022

View reviewed changes

Techcable added 2 commits September 4, 2022 15:23

A packed sturct must have only 1 field to be extern

dcac7a7

Techcable force-pushed the translate-c/support-abi-sized-packed-struct branch from eb1eea0 to dcac7a7 Compare September 4, 2022 22:52

Techcable requested a review from Vexu September 4, 2022 23:03

Techcable mentioned this pull request Sep 5, 2022

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

Merged

Techcable closed this Sep 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Techcable commented Sep 4, 2022 •

edited

Vexu left a comment

ifreund commented Sep 4, 2022

Techcable commented Sep 4, 2022

Techcable commented Sep 12, 2022

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

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

Conversation

Techcable commented Sep 4, 2022 • edited

Implementation details

Overall Effect on M1 Macs

Remaining (possible) questions

Vexu left a comment

Choose a reason for hiding this comment

ifreund commented Sep 4, 2022

Techcable commented Sep 4, 2022

Techcable commented Sep 12, 2022

Techcable commented Sep 4, 2022 •

edited