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

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

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

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<PackedAttr>();
+  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<PackedAttr>())
+    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<AlignedAttr>())
       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<AlignedAttr>())
       FieldAlign = std::max(FieldAlign, AA->getAlignment());
     
@@ -470,7 +487,9 @@ ASTContext::getASTObjCInterfaceLayout(const ObjCInterfaceDecl *D) {
   }
   Entry = NewEntry;
 
-  bool IsPacked = D->getAttr<PackedAttr>();
+  unsigned StructPacking = 0;
+  if (const PackedAttr *PA = D->getAttr<PackedAttr>())
+    StructPacking = PA->getAlignment();
 
   if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
     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<PackedAttr>();
   bool IsUnion = D->isUnion();
 
+  unsigned StructPacking = 0;
+  if (const PackedAttr *PA = D->getAttr<PackedAttr>())
+    StructPacking = PA->getAlignment();
+
   if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
     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

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);

clang/lib/Sema/SemaDeclAttr.cpp

@@ -256,7 +256,7 @@ static void HandlePackedAttr(Decl *d, const AttributeList &Attr, Sema &S) {
   }
   
   if (TagDecl *TD = dyn_cast<TagDecl>(d))
-    TD->addAttr(new PackedAttr());
+    TD->addAttr(new PackedAttr(1));
   else if (FieldDecl *FD = dyn_cast<FieldDecl>(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());

clang/test/Sema/pragma-pack-2.c

@@ -0,0 +1,66 @@
+// RUN: clang -triple i686-apple-darwin9 %s -fsyntax-only -verify
+
+#include <stddef.h>
+
+#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];