Cleanup/refactoring of Sema struct layout. This patch unifies the struct

and union codepaths and fixes some minor bugs.

I'm reasonably confident this is accurate, at least for X86.  I'll 
correct any bugs as I find them; I haven't found any for a while, 
though.

llvm-svn: 51762

3 files changed, 131 insertions, 98 deletions

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 6344ce570cf..5a3ca44a6b0 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -210,8 +210,8 @@ ASTContext::getTypeInfo(QualType T) {
     std::pair<uint64_t, unsigned> EltInfo = 
       getTypeInfo(cast<VectorType>(T)->getElementType());
     Width = EltInfo.first*cast<VectorType>(T)->getNumElements();
-    // FIXME: Vector alignment is not the alignment of its elements.
-    Align = EltInfo.second;
+    // FIXME: This isn't right for unusual vectors
+    Align = Width;
     break;
   }
 
@@ -316,121 +316,102 @@ ASTContext::getTypeInfo(QualType T) {
 /// position information.
 const ASTRecordLayout &ASTContext::getASTRecordLayout(const RecordDecl *D) {
   assert(D->isDefinition() && "Cannot get layout of forward declarations!");
-  
+
   // Look up this layout, if already laid out, return what we have.
   const ASTRecordLayout *&Entry = ASTRecordLayouts[D];
   if (Entry) return *Entry;
-  
+
   // Allocate and assign into ASTRecordLayouts here.  The "Entry" reference can
   // be invalidated (dangle) if the ASTRecordLayouts hashtable is inserted into.
   ASTRecordLayout *NewEntry = new ASTRecordLayout();
   Entry = NewEntry;
-  
+
   uint64_t *FieldOffsets = new uint64_t[D->getNumMembers()];
   uint64_t RecordSize = 0;
   unsigned RecordAlign = 8;  // Default alignment = 1 byte = 8 bits.
+  bool StructIsPacked = D->getAttr<PackedAttr>();
+  bool IsUnion = (D->getKind() == Decl::Union);
+
+  if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
+    RecordAlign = std::max(RecordAlign, AA->getAlignment());
+
+  // Layout each field, for now, just sequentially, respecting alignment.  In
+  // 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);
+    bool FieldIsPacked = StructIsPacked || FD->getAttr<PackedAttr>();
+    uint64_t FieldOffset = IsUnion ? 0 : RecordSize;
+    uint64_t FieldSize;
+    unsigned FieldAlign;
+
+    if (const Expr *BitWidthExpr = FD->getBitWidth()) {
+      // TODO: Need to check this algorithm on other targets!
+      //       (tested on Linux-X86)
+      llvm::APSInt I(32);
+      bool BitWidthIsICE = 
+        BitWidthExpr->isIntegerConstantExpr(I, *this);
+      assert (BitWidthIsICE  && "Invalid BitField size expression");
+      FieldSize = I.getZExtValue();
 
-  if (D->getKind() != Decl::Union) {
-    if (const AlignedAttr *AA = D->getAttr<AlignedAttr>())
-      RecordAlign = std::max(RecordAlign, AA->getAlignment());
-        
-    bool StructIsPacked = D->getAttr<PackedAttr>();
-    
-    // Layout each field, for now, just sequentially, respecting alignment.  In
-    // 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);
-      bool FieldIsPacked = StructIsPacked || FD->getAttr<PackedAttr>();
-      uint64_t FieldSize;
-      unsigned FieldAlign;
-      
-      if (const Expr *BitWidthExpr = FD->getBitWidth()) {
-        llvm::APSInt I(32);
-        bool BitWidthIsICE = 
-          BitWidthExpr->isIntegerConstantExpr(I, *this);
-        assert (BitWidthIsICE  && "Invalid BitField size expression");
-        FieldSize = I.getZExtValue();
-
-        std::pair<uint64_t, unsigned> TypeInfo = getTypeInfo(FD->getType());
-        uint64_t TypeSize = TypeInfo.first;
-        
-        if (const AlignedAttr *AA = FD->getAttr<AlignedAttr>())
-          FieldAlign = AA->getAlignment();
-        else if (FieldIsPacked)
-          FieldAlign = 8;
-        else {
-          FieldAlign = TypeInfo.second;
-        }
-
-        // Check if we need to add padding to give the field the correct
-        // alignment.
-        if (RecordSize % FieldAlign + FieldSize > TypeSize)
-          RecordSize = (RecordSize+FieldAlign-1) & ~(FieldAlign-1);
-
+      std::pair<uint64_t, unsigned> FieldInfo = getTypeInfo(FD->getType());
+      uint64_t TypeSize = FieldInfo.first;
+
+      FieldAlign = FieldInfo.second;
+      if (FieldIsPacked)
+        FieldAlign = 1;
+      if (const AlignedAttr *AA = FD->getAttr<AlignedAttr>())
+        FieldAlign = std::max(FieldAlign, AA->getAlignment());
+
+      // Check if we need to add padding to give the field the correct
+      // alignment.
+      if (FieldSize == 0 || (FieldOffset & (FieldAlign-1)) + FieldSize > TypeSize)
+        FieldOffset = (FieldOffset + (FieldAlign-1)) & ~(FieldAlign-1);
+
+      // Padding members don't affect overall alignment
+      if (!FD->getIdentifier())
+        FieldAlign = 1;
+    } else {
+      if (FD->getType()->isIncompleteType()) {
+        // This must be a flexible array member; we can't directly
+        // query getTypeInfo about these, so we figure it out here.
+        // Flexible array members don't have any size, but they
+        // have to be aligned appropriately for their element type.
+        FieldSize = 0;
+        const ArrayType* ATy = FD->getType()->getAsArrayType();
+        FieldAlign = getTypeAlign(ATy->getElementType());
       } else {
-        if (FD->getType()->isIncompleteType()) {
-          // This must be a flexible array member; we can't directly
-          // query getTypeInfo about these, so we figure it out here.
-          // Flexible array members don't have any size, but they
-          // have to be aligned appropriately for their element type.
-        
-          if (const AlignedAttr *AA = FD->getAttr<AlignedAttr>())
-            FieldAlign = AA->getAlignment();
-          else if (FieldIsPacked)
-            FieldAlign = 8;
-          else {
-            const ArrayType* ATy = FD->getType()->getAsArrayType();
-            FieldAlign = getTypeAlign(ATy->getElementType());
-          }
-          FieldSize = 0;
-        } else {
-          std::pair<uint64_t, unsigned> FieldInfo = getTypeInfo(FD->getType());
-          FieldSize = FieldInfo.first;
-        
-          if (const AlignedAttr *AA = FD->getAttr<AlignedAttr>())
-            FieldAlign = AA->getAlignment();
-          else if (FieldIsPacked)
-            FieldAlign = 8;
-          else
-            FieldAlign = FieldInfo.second;
-        }
-
-        // Round up the current record size to the field's alignment boundary.
-        RecordSize = (RecordSize+FieldAlign-1) & ~(FieldAlign-1);
+        std::pair<uint64_t, unsigned> FieldInfo = getTypeInfo(FD->getType());
+        FieldSize = FieldInfo.first;
+        FieldAlign = FieldInfo.second;
       }
-      
-      // Place this field at the current location.
-      FieldOffsets[i] = RecordSize;
-      
-      // Reserve space for this field.
-      RecordSize += FieldSize;
-      
-      // Remember max struct/class alignment.
-      RecordAlign = std::max(RecordAlign, FieldAlign);
-    }
-    
-    // Finally, round the size of the total struct up to the alignment of the
-    // struct itself.
-    RecordSize = (RecordSize+RecordAlign-1) & ~(RecordAlign-1);
-  } else {
-    // Union layout just puts each member at the start of the record.
-    for (unsigned i = 0, e = D->getNumMembers(); i != e; ++i) {
-      const FieldDecl *FD = D->getMember(i);
-      std::pair<uint64_t, unsigned> FieldInfo = getTypeInfo(FD->getType());
-      uint64_t FieldSize = FieldInfo.first;
-      unsigned FieldAlign = FieldInfo.second;
+
+      if (FieldIsPacked)
+        FieldAlign = 8;
+      if (const AlignedAttr *AA = FD->getAttr<AlignedAttr>())
+        FieldAlign = std::max(FieldAlign, AA->getAlignment());
 
       // Round up the current record size to the field's alignment boundary.
-      RecordSize = std::max(RecordSize, FieldSize);
+      FieldOffset = (FieldOffset + (FieldAlign-1)) & ~(FieldAlign-1);
+    }
 
-      // Place this field at the start of the record.
-      FieldOffsets[i] = 0;
+    // Place this field at the current location.
+    FieldOffsets[i] = FieldOffset;
 
-      // Remember max struct/class alignment.
-      RecordAlign = std::max(RecordAlign, FieldAlign);
+    // Reserve space for this field.
+    if (IsUnion) {
+      RecordSize = std::max(RecordSize, FieldSize);
+    } else {
+      RecordSize = FieldOffset + FieldSize;
     }
+
+    // Remember max struct/class alignment.
+    RecordAlign = std::max(RecordAlign, FieldAlign);
   }
-  
+
+  // Finally, round the size of the total struct up to the alignment of the
+  // struct itself.
+  RecordSize = (RecordSize + (RecordAlign-1)) & ~(RecordAlign-1);
+
   NewEntry->SetLayout(RecordSize, RecordAlign, FieldOffsets);
   return *NewEntry;
 }
diff --git a/clang/test/Sema/bitfield-layout.c b/clang/test/Sema/bitfield-layout.c
new file mode 100644
index 00000000000..0ac5dd260a8
--- /dev/null
+++ b/clang/test/Sema/bitfield-layout.c
@@ -0,0 +1,32 @@
+// RUN: clang %s -fsyntax-only -verify -triple=i686-apple-darwin9
+
+#define CHECK_SIZE(kind, name, size) extern int name##1[sizeof(kind name) == size ? 1 : -1];
+#define CHECK_ALIGN(kind, name, size) extern int name##2[__alignof(kind name) == size ? 1 : -1];
+
+// Zero-width bit-fields
+struct a {char x; int : 0; char y;};
+CHECK_SIZE(struct, a, 5)
+CHECK_ALIGN(struct, a, 1)
+
+union b {char x; int : 0; char y;};
+CHECK_SIZE(union, b, 1)
+CHECK_ALIGN(union, b, 1)
+
+// Unnamed bit-field align
+struct c {char x; int : 20;};
+CHECK_SIZE(struct, c, 4)
+CHECK_ALIGN(struct, c, 1)
+
+union d {char x; int : 20;};
+CHECK_SIZE(union, d, 3)
+CHECK_ALIGN(union, d, 1)
+
+// Bit-field packing
+struct __attribute__((packed)) e {int x : 4, y : 30, z : 30;};
+CHECK_SIZE(struct, e, 8)
+CHECK_ALIGN(struct, e, 1)
+
+// Alignment on bit-fields
+struct f {__attribute((aligned(8))) int x : 30, y : 30, z : 30;};
+CHECK_SIZE(struct, f, 24)
+CHECK_ALIGN(struct, f, 8)
diff --git a/clang/test/Sema/struct-packed-align.c b/clang/test/Sema/struct-packed-align.c
index f759e37a1df..6398cd3993c 100644
--- a/clang/test/Sema/struct-packed-align.c
+++ b/clang/test/Sema/struct-packed-align.c
@@ -69,3 +69,23 @@ struct packedtest {
   int ted_likes_cheese;
   void *args[] __attribute__((packed));
 };
+
+// Packed union
+union __attribute__((packed)) au4 {char c; int x;};
+extern int h1[sizeof(union au4) == 4 ? 1 : -1];
+extern int h2[__alignof(union au4) == 1 ? 1 : -1];
+
+// Aligned union
+union au5 {__attribute__((aligned(4))) char c;};
+extern int h1[sizeof(union au5) == 4 ? 1 : -1];
+extern int h2[__alignof(union au5) == 4 ? 1 : -1];
+
+// Alignment+packed
+struct as6 {char c; __attribute__((packed, aligned(2))) int x;};
+extern int i1[sizeof(struct as6) == 6 ? 1 : -1];
+extern int i2[__alignof(struct as6) == 2 ? 1 : -1];
+
+union au6 {char c; __attribute__((packed, aligned(2))) int x;};
+extern int k1[sizeof(union au6) == 4 ? 1 : -1];
+extern int k2[__alignof(union au6) == 2 ? 1 : -1];
+