From 4290d46bd4b1adf33d5891ef94c00ded0a78f56f Mon Sep 17 00:00:00 2001 From: Daniel Dunbar Date: Thu, 16 Oct 2008 02:34:03 +0000 Subject: [PATCH] Implement #pragma pack use in structure packing. The general approach is to encode the state of the #pragma pack stack as an attribute when the structure is declared. - Extend PackedAttr to take an alignment (in bits), and reuse for both __attribute__((packed)) (which takes no argument, instead packing tightly (to "minimize the memory required") and for #pragma pack (which allows specification of the maximum alignment in bytes). __attribute__((packed)) is just encoded as Alignment=1. This conflates two related but different mechanisms, but it didn't seem worth another attribute. - I have attempted to follow the MSVC semantics as opposed to the gcc ones, since if I understand correctly #pragma pack originated with MSVC. The semantics are generally equivalent except when the stack is altered during the definition of a structure; its not clear if anyone does this in practice. See testcase if curious. llvm-svn: 57623 --- clang/include/clang/AST/Attr.h | 12 +++-- clang/include/clang/AST/RecordLayout.h | 6 ++- clang/lib/AST/ASTContext.cpp | 42 ++++++++++++---- clang/lib/Sema/SemaDecl.cpp | 14 ++++++ clang/lib/Sema/SemaDeclAttr.cpp | 4 +- clang/test/Sema/pragma-pack-2.c | 66 ++++++++++++++++++++++++++ 6 files changed, 127 insertions(+), 17 deletions(-) create mode 100644 clang/test/Sema/pragma-pack-2.c diff --git a/clang/include/clang/AST/Attr.h b/clang/include/clang/AST/Attr.h index c6fb1d16c76a..8b0f79fbbce7 100644 --- a/clang/include/clang/AST/Attr.h +++ b/clang/include/clang/AST/Attr.h @@ -82,9 +82,14 @@ public: }; class PackedAttr : public Attr { + unsigned Alignment; + public: - PackedAttr() : Attr(Packed) {} - + PackedAttr(unsigned alignment) : Attr(Packed), Alignment(alignment) {} + + /// getAlignment - The specified alignment in bits. + unsigned getAlignment() const { return Alignment; } + // Implement isa/cast/dyncast/etc. static bool classof(const Attr *A) { return A->getKind() == Packed; @@ -96,7 +101,8 @@ class AlignedAttr : public Attr { unsigned Alignment; public: AlignedAttr(unsigned alignment) : Attr(Aligned), Alignment(alignment) {} - + + /// getAlignment - The specified alignment in bits. unsigned getAlignment() const { return Alignment; } // Implement isa/cast/dyncast/etc. diff --git a/clang/include/clang/AST/RecordLayout.h b/clang/include/clang/AST/RecordLayout.h index 1b72017cd3e0..a8b49fb2df00 100644 --- a/clang/include/clang/AST/RecordLayout.h +++ b/clang/include/clang/AST/RecordLayout.h @@ -60,9 +60,11 @@ class ASTRecordLayout { void SetAlignment(unsigned A) { Alignment = A; } - /// LayoutField - Field layout. + /// LayoutField - Field layout. StructPacking is the specified + /// packing alignment (maximum alignment) in bits to use for the + /// structure, or 0 if no packing alignment is specified. void LayoutField(const FieldDecl *FD, unsigned FieldNo, - bool IsUnion, bool StructIsPacked, + bool IsUnion, unsigned StructPacking, ASTContext &Context); ASTRecordLayout(const ASTRecordLayout&); // DO NOT IMPLEMENT diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index 572e003dd7cd..e8d7978bca41 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -371,12 +371,17 @@ ASTContext::getTypeInfo(QualType T) { /// LayoutField - Field layout. void ASTRecordLayout::LayoutField(const FieldDecl *FD, unsigned FieldNo, - bool IsUnion, bool StructIsPacked, + bool IsUnion, unsigned StructPacking, ASTContext &Context) { - bool FieldIsPacked = StructIsPacked || FD->getAttr(); + unsigned FieldPacking = StructPacking; uint64_t FieldOffset = IsUnion ? 0 : Size; uint64_t FieldSize; unsigned FieldAlign; + + // FIXME: Should this override struct packing? Probably we want to + // take the minimum? + if (const PackedAttr *PA = FD->getAttr()) + FieldPacking = PA->getAlignment(); if (const Expr *BitWidthExpr = FD->getBitWidth()) { // TODO: Need to check this algorithm on other targets! @@ -388,9 +393,14 @@ void ASTRecordLayout::LayoutField(const FieldDecl *FD, unsigned FieldNo, Context.getTypeInfo(FD->getType()); uint64_t TypeSize = FieldInfo.first; + // Determine the alignment of this bitfield. The packing + // attributes define a maximum and the alignment attribute defines + // a minimum. + // FIXME: What is the right behavior when the specified alignment + // is smaller than the specified packing? FieldAlign = FieldInfo.second; - if (FieldIsPacked) - FieldAlign = 1; + if (FieldPacking) + FieldAlign = std::min(FieldAlign, FieldPacking); if (const AlignedAttr *AA = FD->getAttr()) FieldAlign = std::max(FieldAlign, AA->getAlignment()); @@ -418,8 +428,15 @@ void ASTRecordLayout::LayoutField(const FieldDecl *FD, unsigned FieldNo, FieldAlign = FieldInfo.second; } - if (FieldIsPacked) - FieldAlign = 8; + // Determine the alignment of this bitfield. The packing + // attributes define a maximum and the alignment attribute defines + // a minimum. Additionally, the packing alignment must be at least + // a byte for non-bitfields. + // + // FIXME: What is the right behavior when the specified alignment + // is smaller than the specified packing? + if (FieldPacking) + FieldAlign = std::min(FieldAlign, std::max(8U, FieldPacking)); if (const AlignedAttr *AA = FD->getAttr()) FieldAlign = std::max(FieldAlign, AA->getAlignment()); @@ -470,7 +487,9 @@ ASTContext::getASTObjCInterfaceLayout(const ObjCInterfaceDecl *D) { } Entry = NewEntry; - bool IsPacked = D->getAttr(); + unsigned StructPacking = 0; + if (const PackedAttr *PA = D->getAttr()) + StructPacking = PA->getAlignment(); if (const AlignedAttr *AA = D->getAttr()) NewEntry->SetAlignment(std::max(NewEntry->getAlignment(), @@ -481,7 +500,7 @@ ASTContext::getASTObjCInterfaceLayout(const ObjCInterfaceDecl *D) { for (ObjCInterfaceDecl::ivar_iterator IVI = D->ivar_begin(), IVE = D->ivar_end(); IVI != IVE; ++IVI) { const ObjCIvarDecl* Ivar = (*IVI); - NewEntry->LayoutField(Ivar, i++, false, IsPacked, *this); + NewEntry->LayoutField(Ivar, i++, false, StructPacking, *this); } // Finally, round the size of the total struct up to the alignment of the @@ -507,9 +526,12 @@ const ASTRecordLayout &ASTContext::getASTRecordLayout(const RecordDecl *D) { Entry = NewEntry; NewEntry->InitializeLayout(D->getNumMembers()); - bool StructIsPacked = D->getAttr(); bool IsUnion = D->isUnion(); + unsigned StructPacking = 0; + if (const PackedAttr *PA = D->getAttr()) + StructPacking = PA->getAlignment(); + if (const AlignedAttr *AA = D->getAttr()) NewEntry->SetAlignment(std::max(NewEntry->getAlignment(), AA->getAlignment())); @@ -518,7 +540,7 @@ const ASTRecordLayout &ASTContext::getASTRecordLayout(const RecordDecl *D) { // the future, this will need to be tweakable by targets. for (unsigned i = 0, e = D->getNumMembers(); i != e; ++i) { const FieldDecl *FD = D->getMember(i); - NewEntry->LayoutField(FD, i, IsUnion, StructIsPacked, *this); + NewEntry->LayoutField(FD, i, IsUnion, StructPacking, *this); } // Finally, round the size of the total struct up to the alignment of the diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index fee732f2f56c..e32da244de9a 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -1939,6 +1939,20 @@ Sema::DeclTy *Sema::ActOnTagStruct(Scope *S, TagDecl::TagKind Kind, TagKind TK, // Add it to the decl chain. PushOnScopeChains(New, S); } + + // Handle #pragma pack: if the #pragma pack stack has non-default + // alignment, make up a packed attribute for this decl. These + // attributes are checked when the ASTContext lays out the + // structure. + // + // It is important for implementing the correct semantics that this + // happen here (in act on tag decl). The #pragma pack stack is + // maintained as a result of parser callbacks which can occur at + // many points during the parsing of a struct declaration (because + // the #pragma tokens are effectively skipped over during the + // parsing of the struct). + if (unsigned Alignment = PackContext.getAlignment()) + New->addAttr(new PackedAttr(Alignment * 8)); if (Attr) ProcessDeclAttributeList(New, Attr); diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 3ab0c91cb9c6..5c04bf0b9223 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -256,7 +256,7 @@ static void HandlePackedAttr(Decl *d, const AttributeList &Attr, Sema &S) { } if (TagDecl *TD = dyn_cast(d)) - TD->addAttr(new PackedAttr()); + TD->addAttr(new PackedAttr(1)); else if (FieldDecl *FD = dyn_cast(d)) { // If the alignment is less than or equal to 8 bits, the packed attribute // has no effect. @@ -266,7 +266,7 @@ static void HandlePackedAttr(Decl *d, const AttributeList &Attr, Sema &S) { diag::warn_attribute_ignored_for_field_of_type, Attr.getName()->getName(), FD->getType().getAsString()); else - FD->addAttr(new PackedAttr()); + FD->addAttr(new PackedAttr(1)); } else S.Diag(Attr.getLoc(), diag::warn_attribute_ignored, Attr.getName()->getName()); diff --git a/clang/test/Sema/pragma-pack-2.c b/clang/test/Sema/pragma-pack-2.c new file mode 100644 index 000000000000..e139be0be8fb --- /dev/null +++ b/clang/test/Sema/pragma-pack-2.c @@ -0,0 +1,66 @@ +// RUN: clang -triple i686-apple-darwin9 %s -fsyntax-only -verify + +#include + +#pragma pack(4) + +// Baseline +struct s0 { + char f0; + int f1; +}; +extern int a0[offsetof(struct s0, f1) == 4 ? 1 : -1]; + +#pragma pack(push, 2) +struct s1 { + char f0; + int f1; +}; +extern int a1[offsetof(struct s1, f1) == 2 ? 1 : -1]; +#pragma pack(pop) + +// Test scope of definition + +#pragma pack(push, 2) +struct s2_0 { +#pragma pack(pop) + char f0; + int f1; +}; +extern int a2_0[offsetof(struct s2_0, f1) == 2 ? 1 : -1]; + +struct s2_1 { + char f0; +#pragma pack(push, 2) + int f1; +#pragma pack(pop) +}; +extern int a2_1[offsetof(struct s2_1, f1) == 4 ? 1 : -1]; + +struct s2_2 { + char f0; + int f1; +#pragma pack(push, 2) +}; +#pragma pack(pop) +extern int a2_2[offsetof(struct s2_2, f1) == 4 ? 1 : -1]; + +struct s2_3 { + char f0; +#pragma pack(push, 2) + struct s2_3_0 { +#pragma pack(pop) + int f0; + } f1; +}; +extern int a2_3[offsetof(struct s2_3, f1) == 2 ? 1 : -1]; + +struct s2_4 { + char f0; + struct s2_4_0 { + int f0; +#pragma pack(push, 2) + } f1; +#pragma pack(pop) +}; +extern int a2_4[offsetof(struct s2_4, f1) == 4 ? 1 : -1];