From 6c3860ef321151f92ca07f906c3fe4337c7ed070 Mon Sep 17 00:00:00 2001 From: drpriver Date: Sat, 12 Apr 2025 15:55:56 -0700 Subject: [PATCH] ImportC: Fix interaction of aligned and packed structs (#21204) Previous iteration of this did not properly account for the interaction of aligned and packed and would even segfault on a null access in such a case. This version properly handles that interaction by aligning the struct itself instead of the first member and not forcing the struct alignment to 1 if it is packed. --- compiler/src/dmd/astbase.d | 4 +- compiler/src/dmd/cparse.d | 95 ++++++++++------------------ compiler/src/dmd/frontend.h | 1 + compiler/src/dmd/mtype.d | 4 +- compiler/test/compilable/test21150.c | 34 ++++++++++ 5 files changed, 76 insertions(+), 62 deletions(-) diff --git a/compiler/src/dmd/astbase.d b/compiler/src/dmd/astbase.d index 06eab18223..e430df654b 100644 --- a/compiler/src/dmd/astbase.d +++ b/compiler/src/dmd/astbase.d @@ -3775,13 +3775,14 @@ struct ASTBase TOK tok; Identifier id; structalign_t packalign; + Expressions* alignExps; Dsymbols* members; Type base; Type resolved; MOD mod; - extern (D) this(Loc loc, TOK tok, Identifier id, structalign_t packalign, Type base, Dsymbols* members) + extern (D) this(Loc loc, TOK tok, Identifier id, structalign_t packalign, Expressions* alignExps, Type base, Dsymbols* members) { //printf("TypeTag %p\n", this); super(Ttag); @@ -3789,6 +3790,7 @@ struct ASTBase this.tok = tok; this.id = id; this.packalign = packalign; + this.alignExps = alignExps; this.base = base; this.members = members; this.mod = 0; diff --git a/compiler/src/dmd/cparse.d b/compiler/src/dmd/cparse.d index e852dfd4c2..86f85e34ce 100644 --- a/compiler/src/dmd/cparse.d +++ b/compiler/src/dmd/cparse.d @@ -1775,7 +1775,7 @@ final class CParser(AST) : Parser!AST auto stag = (tt.tok == TOK.struct_) ? new AST.StructDeclaration(tt.loc, tt.id, false) : (tt.tok == TOK.union_) ? new AST.UnionDeclaration(tt.loc, tt.id) : new AST.EnumDeclaration(tt.loc, tt.id, tt.base); - if (!tt.packalign.isUnknown()) + if (!tt.alignExps && !tt.packalign.isUnknown()) { // saw `struct __declspec(align(N)) Tag ...` auto st = stag.isStructDeclaration(); @@ -1785,7 +1785,13 @@ final class CParser(AST) : Parser!AST tt.members = null; if (!symbols) symbols = new AST.Dsymbols(); - auto stags = applySpecifier(stag, specifier); + AST.Dsymbol stags = stag; + if (tt.alignExps) + { + auto decls = new AST.Dsymbols(1); + (*decls)[0] = stags; + stags = new AST.AlignDeclaration(stags.loc, tt.alignExps, decls); + } symbols.push(stags); return stags; } @@ -2452,25 +2458,7 @@ final class CParser(AST) : Parser!AST const sloc = token.loc; nextToken(); - Specifier tagSpecifier; - - /* GNU Extensions - * struct-or-union-specifier: - * struct-or-union gnu-attributes (opt) identifier (opt) { struct-declaration-list } gnu-attributes (opt) - * struct-or-union gnu-attribute (opt) identifier - */ - while (1) - { - if (token.value == TOK.__attribute__) - cparseGnuAttributes(tagSpecifier); - else if (token.value == TOK.__declspec) - cparseDeclspec(tagSpecifier); - else if (token.value == TOK.__pragma) - uupragmaDirective(sloc); - else - break; - } - t = cparseStruct(sloc, structOrUnion, tagSpecifier, symbols); + t = cparseStruct(sloc, structOrUnion, symbols); tkwx = TKW.xtag; break; } @@ -3940,7 +3928,7 @@ final class CParser(AST) : Parser!AST * redeclaration, or reference to existing declaration. * Defer to the semantic() pass with a TypeTag. */ - return new AST.TypeTag(loc, TOK.enum_, tag, structalign_t.init, base, members); + return new AST.TypeTag(loc, TOK.enum_, tag, structalign_t.init, null, base, members); } /************************************* @@ -3967,10 +3955,26 @@ final class CParser(AST) : Parser!AST * Returns: * type of the struct */ - private AST.Type cparseStruct(Loc loc, TOK structOrUnion, ref Specifier specifier, ref AST.Dsymbols* symbols) + private AST.Type cparseStruct(Loc loc, TOK structOrUnion, ref AST.Dsymbols* symbols) { + /* GNU Extensions + * struct-or-union-specifier: + * struct-or-union gnu-attributes (opt) identifier (opt) { struct-declaration-list } gnu-attributes (opt) + * struct-or-union gnu-attribute (opt) identifier + */ + Specifier tagSpecifier; + while (1) + { + if (token.value == TOK.__attribute__) + cparseGnuAttributes(tagSpecifier); + else if (token.value == TOK.__declspec) + cparseDeclspec(tagSpecifier); + else if (token.value == TOK.__pragma) + uupragmaDirective(loc); + else + break; + } Identifier tag; - auto packalign = specifier.packalign; if (token.value == TOK.identifier) { @@ -3985,7 +3989,7 @@ final class CParser(AST) : Parser!AST members = new AST.Dsymbols(); // so `members` will be non-null even with 0 members while (token.value != TOK.rightCurly) { - cparseStructDeclaration(members, packalign); + cparseStructDeclaration(members, tagSpecifier.packalign); if (token.value == TOK.endOfFile) break; @@ -4003,13 +4007,10 @@ final class CParser(AST) : Parser!AST /* GNU Extensions * Parse the postfix gnu-attributes (opt) */ - specifier.packalign = structalign_t(); if (token.value == TOK.__attribute__) - cparseGnuAttributes(specifier); - if (!specifier.packalign.isUnknown) + cparseGnuAttributes(tagSpecifier); + if (!tagSpecifier.packalign.isUnknown) { - packalign.set(specifier.packalign.get()); - packalign.setPack(specifier.packalign.isPack()); foreach (ref d; (*members)[]) { // skip possible static assert declarations @@ -4017,33 +4018,7 @@ final class CParser(AST) : Parser!AST auto decls = new AST.Dsymbols(1); (*decls)[0] = d; - d = new AST.AlignDeclaration(d.loc, specifier.packalign, decls); - } - } - if (specifier.alignExps && specifier.alignExps.length) - { - // Align the entire struct by aligning the first member. - if (members) - { - foreach (ref d; (*members)[]) - { - // skip possible static assert declarations - if (d.isStaticAssert()) continue; - - if (AST.AlignDeclaration ad = d.isAlignDeclaration()) - { - foreach (exp; *specifier.alignExps) - ad.exps.push(exp); - break; - } - else - { - auto decls = new AST.Dsymbols(1); - (*decls)[0] = d; - d = new AST.AlignDeclaration(d.loc, specifier.alignExps, decls); - break; - } - } + d = new AST.AlignDeclaration(d.loc, tagSpecifier.packalign, decls); } } } @@ -4051,14 +4026,14 @@ final class CParser(AST) : Parser!AST error("missing tag `identifier` after `%s`", Token.toChars(structOrUnion)); // many ways and places to declare alignment - if (packalign.isUnknown() && !this.packalign.isUnknown()) - packalign.set(this.packalign.get()); + if (tagSpecifier.packalign.isUnknown() && !this.packalign.isUnknown()) + tagSpecifier.packalign.set(this.packalign.get()); /* Need semantic information to determine if this is a declaration, * redeclaration, or reference to existing declaration. * Defer to the semantic() pass with a TypeTag. */ - return new AST.TypeTag(loc, structOrUnion, tag, packalign, null, members); + return new AST.TypeTag(loc, structOrUnion, tag, tagSpecifier.packalign, tagSpecifier.alignExps, null, members); } /************************************* diff --git a/compiler/src/dmd/frontend.h b/compiler/src/dmd/frontend.h index e072f2c91f..caff0d3310 100644 --- a/compiler/src/dmd/frontend.h +++ b/compiler/src/dmd/frontend.h @@ -4850,6 +4850,7 @@ public: Loc loc; TOK tok; structalign_t packalign; + Array* alignExps; Identifier* id; Type* base; Array* members; diff --git a/compiler/src/dmd/mtype.d b/compiler/src/dmd/mtype.d index e0af692bcc..9b46b26b4f 100644 --- a/compiler/src/dmd/mtype.d +++ b/compiler/src/dmd/mtype.d @@ -3761,6 +3761,7 @@ extern (C++) final class TypeTag : Type Loc loc; /// location of declaration TOK tok; /// TOK.struct_, TOK.union_, TOK.enum_ structalign_t packalign; /// alignment of struct/union fields + Expressions* alignExps; /// alignment of struct itself Identifier id; /// tag name identifier Type base; /// base type for enums otherwise null Dsymbols* members; /// members of struct, null if none @@ -3770,7 +3771,7 @@ extern (C++) final class TypeTag : Type /// struct S { int a; } s1, *s2; MOD mod; /// modifiers to apply after type is resolved (only MODFlags.const_ at the moment) - extern (D) this(Loc loc, TOK tok, Identifier id, structalign_t packalign, Type base, Dsymbols* members) @safe + extern (D) this(Loc loc, TOK tok, Identifier id, structalign_t packalign, Expressions* alignExps, Type base, Dsymbols* members) @safe { //printf("TypeTag ctor %s %p\n", id ? id.toChars() : "null".ptr, this); super(Ttag); @@ -3778,6 +3779,7 @@ extern (C++) final class TypeTag : Type this.tok = tok; this.id = id; this.packalign = packalign; + this.alignExps = alignExps; this.base = base; this.members = members; this.mod = 0; diff --git a/compiler/test/compilable/test21150.c b/compiler/test/compilable/test21150.c index 7f193b528f..c536d363e5 100644 --- a/compiler/test/compilable/test21150.c +++ b/compiler/test/compilable/test21150.c @@ -41,3 +41,37 @@ struct __attribute__((aligned(4))) D { __attribute__((aligned(8))); _Static_assert(_Alignof(struct D)==8, "D"); +// +// Interaction of aligned() and packed +// +#include + struct Spacked { + unsigned a; + unsigned long long b; + } __attribute__((aligned(4), packed)); +_Static_assert(_Alignof(struct Spacked) == 4, "Spacked"); +_Static_assert(_Alignof(struct Spacked) == 4, "Spacked"); +_Static_assert(offsetof(struct Spacked, a) == 0, "Spacked.a"); +_Static_assert(offsetof(struct Spacked, b) == sizeof(unsigned), "Spacked.b"); +_Static_assert(sizeof(struct Spacked) == sizeof(unsigned) + sizeof(unsigned long long), "sizeof(Spacked)"); + +struct __attribute__((aligned(4))) Spacked2 { + unsigned a; + unsigned long long b; +} __attribute__((packed)); +_Static_assert(_Alignof(struct Spacked2) == 4, "Spacked2"); +_Static_assert(offsetof(struct Spacked2, a) == 0, "Spacked2.a"); +_Static_assert(offsetof(struct Spacked2, b) == sizeof(unsigned), "Spacked2.b"); +_Static_assert(sizeof(struct Spacked2) == sizeof(unsigned) + sizeof(unsigned long long), "sizeof(Spacked2)"); + +#pragma pack(push) +#pragma pack(1) +struct __attribute__((aligned(4))) Spacked3 { + unsigned a; + unsigned long long b; +}; +#pragma pack(pop) +_Static_assert(_Alignof(struct Spacked3) == 4, "Spacked3"); +_Static_assert(offsetof(struct Spacked3, a) == 0, "Spacked3.a"); +_Static_assert(offsetof(struct Spacked3, b) == sizeof(unsigned), "Spacked3.b"); +_Static_assert(sizeof(struct Spacked3) == sizeof(unsigned) + sizeof(unsigned long long), "sizeof(Spacked3)");