authorgravatar for Techcable@techcable.netTechcable <Techcable@techcable.net> 2022-09-04 17:16:26-07:00
committergravatar for Techcable@techcable.netTechcable <Techcable@techcable.net> 2022-10-01 15:22:10-07:00
log5b689d389fa50070c08520cdb6296dda3ad78a62
tree284c79f96a6ed88db8c68825bed2a4152412a0c4
parent34835bbbcfe81cc87e823d14dc9b25e698ad5edc
signaturelock-open Commit is signed but in an unrecognized format.

translate-c: packed struct implies align(1) on every field

Superceeds PR #12735 (now supporting all packed structs in GNU C) Fixes issue #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 #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.

6 files changed, 106 insertions(+), 50 deletions(-)

src/clang.zig+6
......@@ -470,6 +470,9 @@ pub const FieldDecl = opaque {
470470 pub const getAlignedAttribute = ZigClangFieldDecl_getAlignedAttribute;
471471 extern fn ZigClangFieldDecl_getAlignedAttribute(*const FieldDecl, *const ASTContext) c_uint;
472472
473 pub const getPackedAttribute = ZigClangFieldDecl_getPackedAttribute;
474 extern fn ZigClangFieldDecl_getPackedAttribute(*const FieldDecl) bool;
475
473476 pub const isAnonymousStructOrUnion = ZigClangFieldDecl_isAnonymousStructOrUnion;
474477 extern fn ZigClangFieldDecl_isAnonymousStructOrUnion(*const FieldDecl) bool;
475478
......@@ -1015,6 +1018,9 @@ pub const VarDecl = opaque {
10151018 pub const getAlignedAttribute = ZigClangVarDecl_getAlignedAttribute;
10161019 extern fn ZigClangVarDecl_getAlignedAttribute(*const VarDecl, *const ASTContext) c_uint;
10171020
1021 pub const getPackedAttribute = ZigClangVarDecl_getPackedAttribute;
1022 extern fn ZigClangVarDecl_getPackedAttribute(*const VarDecl) bool;
1023
10181024 pub const getCleanupAttribute = ZigClangVarDecl_getCleanupAttribute;
10191025 extern fn ZigClangVarDecl_getCleanupAttribute(*const VarDecl) ?*const FunctionDecl;
10201026
src/translate_c.zig+61-16
......@@ -878,7 +878,7 @@ fn visitVarDecl(c: *Context, var_decl: *const clang.VarDecl, mangled_name: ?[]co
878878 .is_export = is_export,
879879 .is_threadlocal = is_threadlocal,
880880 .linksection_string = linksection_string,
881 .alignment = zigAlignment(var_decl.getAlignedAttribute(c.clang_context)),
881 .alignment = ClangAlignment.forVar(c, var_decl).zigAlignment(),
882882 .name = var_name,
883883 .type = type_node,
884884 .init = init_node,
......@@ -1096,7 +1096,6 @@ fn transRecordDecl(c: *Context, scope: *Scope, record_decl: *const clang.RecordD
10961096 break :blk Tag.opaque_literal.init();
10971097 };
10981098
1099 const is_packed = record_decl.getPackedAttribute();
11001099 var fields = std.ArrayList(ast.Payload.Record.Field).init(c.gpa);
11011100 defer fields.deinit();
11021101
......@@ -1153,7 +1152,7 @@ fn transRecordDecl(c: *Context, scope: *Scope, record_decl: *const clang.RecordD
11531152 const alignment = if (has_flexible_array and field_decl.getFieldIndex() == 0)
11541153 @intCast(c_uint, record_alignment)
11551154 else
1156 zigAlignment(field_decl.getAlignedAttribute(c.clang_context));
1155 ClangAlignment.forField(c, field_decl, record_def).zigAlignment();
11571156
11581157 if (is_anon) {
11591158 try c.decl_table.putNoClobber(c.gpa, @ptrToInt(field_decl.getCanonicalDecl()), field_name);
......@@ -1166,15 +1165,11 @@ fn transRecordDecl(c: *Context, scope: *Scope, record_decl: *const clang.RecordD
11661165 });
11671166 }
11681167
1169 if (!c.zig_is_stage1 and is_packed) {
1170 return failDecl(c, record_loc, name, "cannot translate packed record union", .{});
1171 }
1172
11731168 const record_payload = try c.arena.create(ast.Payload.Record);
11741169 record_payload.* = .{
11751170 .base = .{ .tag = ([2]Tag{ .@"struct", .@"union" })[@boolToInt(is_union)] },
11761171 .data = .{
1177 .layout = if (is_packed) .@"packed" else .@"extern",
1172 .layout = .@"extern",
11781173 .fields = try c.arena.dupe(ast.Payload.Record.Field, fields.items),
11791174 .functions = try c.arena.dupe(Node, functions.items),
11801175 .variables = &.{},
......@@ -1851,12 +1846,62 @@ fn transCStyleCastExprClass(
18511846 return maybeSuppressResult(c, scope, result_used, cast_node);
18521847}
18531848
1854/// Clang reports the alignment in bits, we use bytes
1855/// Clang uses 0 for "no alignment specified", we use null
1856fn zigAlignment(bit_alignment: c_uint) ?c_uint {
1857 if (bit_alignment == 0) return null;
1858 return bit_alignment / 8;
1859}
1849/// The alignment of a variable or field
1850const ClangAlignment = struct {
1851 /// Clang reports the alignment in bits, we use bytes
1852 /// Clang uses 0 for "no alignment specified", we use null
1853 bit_alignment: c_uint,
1854 /// If the field or variable is marked as 'packed'
1855 ///
1856 /// According to the GCC variable attribute docs, this impacts alignment
1857 /// https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html
1858 ///
1859 /// > The packed attribute specifies that a structure member
1860 /// > should have the smallest possible alignment
1861 ///
1862 /// Note also that specifying the 'packed' attribute on a structure
1863 /// implicitly packs all its fields (making their alignment 1).
1864 ///
1865 /// This will be null if the AST node doesn't support packing (functions)
1866 is_packed: ?bool,
1867
1868 /// Get the alignment for a field, optionally taking into account the parent record
1869 pub fn forField(c: *const Context, field: *const clang.FieldDecl, parent: ?*const clang.RecordDecl) ClangAlignment {
1870 const parent_packed = if (parent) |record| record.getPackedAttribute() else false;
1871 // NOTE: According to GCC docs, parent attribute packed implies child attribute packed
1872 return ClangAlignment{
1873 .bit_alignment = field.getAlignedAttribute(c.clang_context),
1874 .is_packed = field.getPackedAttribute() or parent_packed,
1875 };
1876 }
1877
1878 pub fn forVar(c: *const Context, var_decl: *const clang.VarDecl) ClangAlignment {
1879 return ClangAlignment{
1880 .bit_alignment = var_decl.getAlignedAttribute(c.clang_context),
1881 .is_packed = var_decl.getPackedAttribute(),
1882 };
1883 }
1884
1885 pub fn forFunc(c: *const Context, fun: *const clang.FunctionDecl) ClangAlignment {
1886 return ClangAlignment{
1887 .bit_alignment = fun.getAlignedAttribute(c.clang_context),
1888 .is_packed = null, // not supported by GCC/clang (or meaningful),
1889 };
1890 }
1891
1892 /// Translate the clang alignment info into a zig alignment
1893 ///
1894 /// Returns null if there is no special alignment info
1895 pub fn zigAlignment(self: ClangAlignment) ?c_uint {
1896 if (self.bit_alignment != 0) {
1897 return self.bit_alignment / 8;
1898 } else if (self.is_packed orelse false) {
1899 return 1;
1900 } else {
1901 return null;
1902 }
1903 }
1904};
18601905
18611906fn transDeclStmtOne(
18621907 c: *Context,
......@@ -1910,7 +1955,7 @@ fn transDeclStmtOne(
19101955 .is_export = false,
19111956 .is_threadlocal = var_decl.getTLSKind() != .None,
19121957 .linksection_string = null,
1913 .alignment = zigAlignment(var_decl.getAlignedAttribute(c.clang_context)),
1958 .alignment = ClangAlignment.forVar(c, var_decl).zigAlignment(),
19141959 .name = var_name,
19151960 .type = type_node,
19161961 .init = init_node,
......@@ -5054,7 +5099,7 @@ fn finishTransFnProto(
50545099 break :blk null;
50555100 };
50565101
5057 const alignment = if (fn_decl) |decl| zigAlignment(decl.getAlignedAttribute(c.clang_context)) else null;
5102 const alignment = if (fn_decl) |decl| ClangAlignment.forFunc(c, decl).zigAlignment() else null;
50585103
50595104 const explicit_callconv = if ((is_inline or is_export or is_extern) and cc == .C) null else cc;
50605105
src/zig_clang.cpp+11-4
......@@ -1941,10 +1941,7 @@ const char* ZigClangVarDecl_getSectionAttribute(const struct ZigClangVarDecl *se
19411941
19421942bool ZigClangRecordDecl_getPackedAttribute(const ZigClangRecordDecl *zig_record_decl) {
19431943 const clang::RecordDecl *record_decl = reinterpret_cast<const clang::RecordDecl *>(zig_record_decl);
1944 if (record_decl->getAttr<clang::PackedAttr>()) {
1945 return true;
1946 }
1947 return false;
1944 return record_decl->hasAttr<clang::PackedAttr>();
19481945}
19491946
19501947unsigned ZigClangVarDecl_getAlignedAttribute(const struct ZigClangVarDecl *self, const ZigClangASTContext* ctx) {
......@@ -1985,6 +1982,16 @@ unsigned ZigClangFunctionDecl_getAlignedAttribute(const struct ZigClangFunctionD
19851982 return 0;
19861983}
19871984
1985bool ZigClangVarDecl_getPackedAttribute(const struct ZigClangVarDecl *self) {
1986 auto casted_self = reinterpret_cast<const clang::VarDecl *>(self);
1987 return casted_self->hasAttr<clang::PackedAttr>();
1988}
1989
1990bool ZigClangFieldDecl_getPackedAttribute(const struct ZigClangFieldDecl *self) {
1991 auto casted_self = reinterpret_cast<const clang::FieldDecl *>(self);
1992 return casted_self->hasAttr<clang::PackedAttr>();
1993}
1994
19881995ZigClangQualType ZigClangParmVarDecl_getOriginalType(const struct ZigClangParmVarDecl *self) {
19891996 return bitcast(reinterpret_cast<const clang::ParmVarDecl *>(self)->getOriginalType());
19901997}
src/zig_clang.h+2
......@@ -1101,6 +1101,8 @@ ZIG_EXTERN_C const struct ZigClangFunctionDecl *ZigClangVarDecl_getCleanupAttrib
11011101ZIG_EXTERN_C unsigned ZigClangVarDecl_getAlignedAttribute(const struct ZigClangVarDecl *self, const ZigClangASTContext* ctx);
11021102ZIG_EXTERN_C unsigned ZigClangFunctionDecl_getAlignedAttribute(const struct ZigClangFunctionDecl *self, const ZigClangASTContext* ctx);
11031103ZIG_EXTERN_C unsigned ZigClangFieldDecl_getAlignedAttribute(const struct ZigClangFieldDecl *self, const ZigClangASTContext* ctx);
1104ZIG_EXTERN_C bool ZigClangVarDecl_getPackedAttribute(const struct ZigClangVarDecl *self);
1105ZIG_EXTERN_C bool ZigClangFieldDecl_getPackedAttribute(const struct ZigClangFieldDecl *self);
11041106
11051107ZIG_EXTERN_C const struct ZigClangStringLiteral *ZigClangFileScopeAsmDecl_getAsmString(const struct ZigClangFileScopeAsmDecl *self);
11061108
test/run_translated_c.zig+12-14
......@@ -250,20 +250,18 @@ pub fn addCases(cases: *tests.RunTranslatedCContext) void {
250250 \\}
251251 , "");
252252
253 if (@import("builtin").zig_backend == .stage1) {
254 cases.add("struct initializer - packed",
255 \\#define _NO_CRT_STDIO_INLINE 1
256 \\#include <stdint.h>
257 \\#include <stdlib.h>
258 \\struct s {uint8_t x,y;
259 \\ uint32_t z;} __attribute__((packed)) s0 = {1, 2};
260 \\int main() {
261 \\ /* sizeof nor offsetof currently supported */
262 \\ if (((intptr_t)&s0.z - (intptr_t)&s0.x) != 2) abort();
263 \\ return 0;
264 \\}
265 , "");
266 }
253 cases.add("struct initializer - packed",
254 \\#define _NO_CRT_STDIO_INLINE 1
255 \\#include <stdint.h>
256 \\#include <stdlib.h>
257 \\struct s {uint8_t x,y;
258 \\ uint32_t z;} __attribute__((packed)) s0 = {1, 2};
259 \\int main() {
260 \\ /* sizeof nor offsetof currently supported */
261 \\ if (((intptr_t)&s0.z - (intptr_t)&s0.x) != 2) abort();
262 \\ return 0;
263 \\}
264 , "");
267265
268266 cases.add("cast signed array index to unsigned",
269267 \\#include <stdlib.h>
test/translate_c.zig+14-16
......@@ -728,22 +728,20 @@ pub fn addCases(cases: *tests.TranslateCContext) void {
728728 \\}
729729 });
730730
731 if (builtin.zig_backend == .stage1) {
732 cases.add("struct initializer - packed",
733 \\struct {int x,y,z;} __attribute__((packed)) s0 = {1, 2};
734 , &[_][]const u8{
735 \\const struct_unnamed_1 = packed struct {
736 \\ x: c_int,
737 \\ y: c_int,
738 \\ z: c_int,
739 \\};
740 \\pub export var s0: struct_unnamed_1 = struct_unnamed_1{
741 \\ .x = @as(c_int, 1),
742 \\ .y = @as(c_int, 2),
743 \\ .z = 0,
744 \\};
745 });
746 }
731 cases.add("struct initializer - packed",
732 \\struct {int x,y,z;} __attribute__((packed)) s0 = {1, 2};
733 , &[_][]const u8{
734 \\const struct_unnamed_1 = extern struct {
735 \\ x: c_int align(1),
736 \\ y: c_int align(1),
737 \\ z: c_int align(1),
738 \\};
739 \\pub export var s0: struct_unnamed_1 = struct_unnamed_1{
740 \\ .x = @as(c_int, 1),
741 \\ .y = @as(c_int, 2),
742 \\ .z = 0,
743 \\};
744 });
747745
748746 // Test case temporarily disabled:
749747 // https://github.com/ziglang/zig/issues/12055