From e86c2e0f6497a92aa544c176bfdf6040be6046d8 Mon Sep 17 00:00:00 2001 From: Techcable Date: Sun, 4 Sep 2022 17:16:26 -0700 Subject: [PATCH] translate-c: packed struct implies align(1) on every field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/clang.zig | 6 ++++ src/translate_c.zig | 76 ++++++++++++++++++++++++++++++++++++--------- src/zig_clang.cpp | 18 +++++++++++ src/zig_clang.h | 2 ++ 4 files changed, 87 insertions(+), 15 deletions(-) diff --git a/src/clang.zig b/src/clang.zig index 205a3cccc617..d3eacbfec449 100644 --- a/src/clang.zig +++ b/src/clang.zig @@ -470,6 +470,9 @@ pub const FieldDecl = opaque { pub const getAlignedAttribute = ZigClangFieldDecl_getAlignedAttribute; extern fn ZigClangFieldDecl_getAlignedAttribute(*const FieldDecl, *const ASTContext) c_uint; + pub const getPackedAttribute = ZigClangFieldDecl_getPackedAttribute; + extern fn ZigClangFieldDecl_getPackedAttribute(*const FieldDecl) bool; + pub const isAnonymousStructOrUnion = ZigClangFieldDecl_isAnonymousStructOrUnion; extern fn ZigClangFieldDecl_isAnonymousStructOrUnion(*const FieldDecl) bool; @@ -1015,6 +1018,9 @@ pub const VarDecl = opaque { pub const getAlignedAttribute = ZigClangVarDecl_getAlignedAttribute; extern fn ZigClangVarDecl_getAlignedAttribute(*const VarDecl, *const ASTContext) c_uint; + pub const getPackedAttribute = ZigClangVarDecl_getPackedAttribute; + extern fn ZigClangVarDecl_getPackedAttribute(*const VarDecl) bool; + pub const getCleanupAttribute = ZigClangVarDecl_getCleanupAttribute; extern fn ZigClangVarDecl_getCleanupAttribute(*const VarDecl) ?*const FunctionDecl; diff --git a/src/translate_c.zig b/src/translate_c.zig index f969bf1c8b94..2a70dbbbd79f 100644 --- a/src/translate_c.zig +++ b/src/translate_c.zig @@ -878,7 +878,7 @@ fn visitVarDecl(c: *Context, var_decl: *const clang.VarDecl, mangled_name: ?[]co .is_export = is_export, .is_threadlocal = is_threadlocal, .linksection_string = linksection_string, - .alignment = zigAlignment(var_decl.getAlignedAttribute(c.clang_context)), + .alignment = ClangAlignment.forVar(c, var_decl).zigAlignment(), .name = var_name, .type = type_node, .init = init_node, @@ -1153,7 +1153,7 @@ fn transRecordDecl(c: *Context, scope: *Scope, record_decl: *const clang.RecordD const alignment = if (has_flexible_array and field_decl.getFieldIndex() == 0) @intCast(c_uint, record_alignment) else - zigAlignment(field_decl.getAlignedAttribute(c.clang_context)); + ClangAlignment.forField(c, field_decl, record_def).zigAlignment(); if (is_anon) { try c.decl_table.putNoClobber(c.gpa, @ptrToInt(field_decl.getCanonicalDecl()), field_name); @@ -1166,15 +1166,11 @@ fn transRecordDecl(c: *Context, scope: *Scope, record_decl: *const clang.RecordD }); } - if (!c.zig_is_stage1 and is_packed) { - return failDecl(c, record_loc, name, "cannot translate packed record union", .{}); - } - const record_payload = try c.arena.create(ast.Payload.Record); record_payload.* = .{ .base = .{ .tag = ([2]Tag{ .@"struct", .@"union" })[@boolToInt(is_union)] }, .data = .{ - .layout = if (is_packed) .@"packed" else .@"extern", + .layout = if (is_packed and c.zig_is_stage1) .@"packed" else .@"extern", .fields = try c.arena.dupe(ast.Payload.Record.Field, fields.items), .functions = try c.arena.dupe(Node, functions.items), .variables = &.{}, @@ -1851,12 +1847,62 @@ fn transCStyleCastExprClass( return maybeSuppressResult(c, scope, result_used, cast_node); } -/// Clang reports the alignment in bits, we use bytes -/// Clang uses 0 for "no alignment specified", we use null -fn zigAlignment(bit_alignment: c_uint) ?c_uint { - if (bit_alignment == 0) return null; - return bit_alignment / 8; -} +/// The alignment of a variable or field +const ClangAlignment = struct { + /// Clang reports the alignment in bits, we use bytes + /// Clang uses 0 for "no alignment specified", we use null + bit_alignment: c_uint, + /// If the field or variable is marked as 'packed' + /// + /// According to the GCC variable attribute docs, this impacts alignment + /// https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html + /// + /// > The packed attribute specifies that a structure member + /// > should have the smallest possible alignment + /// + /// Note also that specifying the 'packed' attribute on a structure + /// implicitly packs all its fields (making their alignment 1). + /// + /// This will be null if the AST node doesn't support packing (functions) + is_packed: ?bool, + + /// Get the alignment for a field, optionally taking into account the parent record + pub fn forField(c: *const Context, field: *const clang.FieldDecl, parent: ?*const clang.RecordDecl) ClangAlignment { + const parent_packed = if (parent) |record| record.getPackedAttribute() else false; + // NOTE: According to GCC docs, parent attribute packed implies child attribute packed + return ClangAlignment{ + .bit_alignment = field.getAlignedAttribute(c.clang_context), + .is_packed = field.getPackedAttribute() or parent_packed, + }; + } + + pub fn forVar(c: *const Context, var_decl: *const clang.VarDecl) ClangAlignment { + return ClangAlignment{ + .bit_alignment = var_decl.getAlignedAttribute(c.clang_context), + .is_packed = var_decl.getPackedAttribute(), + }; + } + + pub fn forFunc(c: *const Context, fun: *const clang.FunctionDecl) ClangAlignment { + return ClangAlignment{ + .bit_alignment = fun.getAlignedAttribute(c.clang_context), + .is_packed = null, // not supported by GCC/clang (or meaningful), + }; + } + + /// Translate the clang alignment info into a zig alignment + /// + /// Returns null if there is no special alignment info + pub fn zigAlignment(self: ClangAlignment) ?c_uint { + if (self.bit_alignment != 0) { + return self.bit_alignment / 8; + } else if (self.is_packed orelse false) { + return 1; + } else { + return null; + } + } +}; fn transDeclStmtOne( c: *Context, @@ -1910,7 +1956,7 @@ fn transDeclStmtOne( .is_export = false, .is_threadlocal = var_decl.getTLSKind() != .None, .linksection_string = null, - .alignment = zigAlignment(var_decl.getAlignedAttribute(c.clang_context)), + .alignment = ClangAlignment.forVar(c, var_decl).zigAlignment(), .name = var_name, .type = type_node, .init = init_node, @@ -5054,7 +5100,7 @@ fn finishTransFnProto( break :blk null; }; - const alignment = if (fn_decl) |decl| zigAlignment(decl.getAlignedAttribute(c.clang_context)) else null; + const alignment = if (fn_decl) |decl| ClangAlignment.forFunc(c, decl).zigAlignment() else null; const explicit_callconv = if ((is_inline or is_export or is_extern) and cc == .C) null else cc; diff --git a/src/zig_clang.cpp b/src/zig_clang.cpp index 223dc29067be..1a82b58a460e 100644 --- a/src/zig_clang.cpp +++ b/src/zig_clang.cpp @@ -1985,6 +1985,24 @@ unsigned ZigClangFunctionDecl_getAlignedAttribute(const struct ZigClangFunctionD return 0; } +bool ZigClangVarDecl_getPackedAttribute(const struct ZigClangVarDecl *self) { + auto casted_self = reinterpret_cast(self); + if (const clang::PackedAttr *PA = casted_self->getAttr()) { + return true; + } else { + return false; + } +} + +bool ZigClangFieldDecl_getPackedAttribute(const struct ZigClangFieldDecl *self) { + auto casted_self = reinterpret_cast(self); + if (const clang::PackedAttr *PA = casted_self->getAttr()) { + return true; + } else { + return false; + } +} + ZigClangQualType ZigClangParmVarDecl_getOriginalType(const struct ZigClangParmVarDecl *self) { return bitcast(reinterpret_cast(self)->getOriginalType()); } diff --git a/src/zig_clang.h b/src/zig_clang.h index 5d556700e653..fab5b83b2d98 100644 --- a/src/zig_clang.h +++ b/src/zig_clang.h @@ -1101,6 +1101,8 @@ ZIG_EXTERN_C const struct ZigClangFunctionDecl *ZigClangVarDecl_getCleanupAttrib ZIG_EXTERN_C unsigned ZigClangVarDecl_getAlignedAttribute(const struct ZigClangVarDecl *self, const ZigClangASTContext* ctx); ZIG_EXTERN_C unsigned ZigClangFunctionDecl_getAlignedAttribute(const struct ZigClangFunctionDecl *self, const ZigClangASTContext* ctx); ZIG_EXTERN_C unsigned ZigClangFieldDecl_getAlignedAttribute(const struct ZigClangFieldDecl *self, const ZigClangASTContext* ctx); +ZIG_EXTERN_C bool ZigClangVarDecl_getPackedAttribute(const struct ZigClangVarDecl *self); +ZIG_EXTERN_C bool ZigClangFieldDecl_getPackedAttribute(const struct ZigClangFieldDecl *self); ZIG_EXTERN_C const struct ZigClangStringLiteral *ZigClangFileScopeAsmDecl_getAsmString(const struct ZigClangFileScopeAsmDecl *self);