[Sema] Fix incorrect packed aligned structure layout

[chill]

Summary

Executing the following program

#include <cassert>
#include <cstddef>

struct S {
  char x;
  int y;
} __attribute__((packed, aligned(8)));

struct alignas(8) T {
  char x;
  int y;
} __attribute__((packed));

int main() {
  assert(offsetof(S, x) == 0);
  assert(offsetof(S, y) == 1);

  assert(offsetof(T, x) == 0);
  assert(offsetof(T, y) == 1);
}

fails with assertion

a.out: a.cc:19: int main(): Assertion `offsetof(T, y) == 1' failed.

The layout if T is incorrect, because it's computed and effectively cached when checking for alignas under-aligning the structure, however, this happens before __attribute__((packed)) is processed.

This patch moves the processing of attributes before the alignas under-alignment check.

[lebedev.ri]

Test?

[rsmith]

This looks fine, but needs a test.

lib/Sema/SemaDecl.cpp

15597

More generally: "before checking the layout".

15672

Do we need to do any attribute processing in this Objective-C interface / implementation / category case?

[chill]

Update: updated comment, added a test.

lib/Sema/SemaDecl.cpp

15672

I think we didn't need it, or else ProcessDeclAttributeList would have been called with a null Record argument.

[chill]

Ping?

[aaron.ballman]

LGTM!

[chill]

Thanks a lot!

[probinson]

This wouldn't be another layout/ABI breakage, would it?

[rsmith]

In D46439#1106929, @probinson wrote:

This wouldn't be another layout/ABI breakage, would it?

Yes, albeit for what I'd expect to be an extremely rare case.

lib/Sema/SemaDecl.cpp

if (CXXRecordDecl *CXXRecord = dyn_cast<CXXRecordDecl>(Record)) {
           }
         }
       }
     }
 
     if (!Completed)
       Record->completeDefinition();
 
+    // Handle attributes before checking the layout.
+    if (Attr)
+      ProcessDeclAttributeList(S, Record, Attr);
+
     // We may have deferred checking for a deleted destructor. Check now.
     if (CXXRecordDecl *CXXRecord = dyn_cast<CXXRecordDecl>(Record)) {
       auto *Dtor = CXXRecord->getDestructor();
       if (Dtor && Dtor->isImplicit() &&
           ShouldDeleteSpecialMember(Dtor, CXXDestructor)) {
         CXXRecord->setImplicitDestructorIsDeleted();
         SetDeclDeleted(Dtor, CXXRecord->getLocation());
       }
if (CheckForZeroSize) {
       // Structs without named members are extension in C (C99 6.7.2.1p7),
       // but are accepted by GCC.
       if (NonBitFields == 0 && !getLangOpts().CPlusPlus) {
         Diag(RecLoc, IsEmpty ? diag::ext_empty_struct_union :
                                diag::ext_no_named_members_in_struct_union)
           << Record->isUnion();
       }
     }
   } else {
     ObjCIvarDecl **ClsFields =
       reinterpret_cast<ObjCIvarDecl**>(RecFields.data());
     if (ObjCInterfaceDecl *ID = dyn_cast<ObjCInterfaceDecl>(EnclosingDecl)) {
       ID->setEndOfDefinitionLoc(RBrac);
       // Add ivar's to class's DeclContext.
       for (unsigned i = 0, e = RecFields.size(); i != e; ++i) {
         ClsFields[i]->setLexicalDeclContext(ID);
         ID->addDecl(ClsFields[i]);
if (ObjCInterfaceDecl *ID = dyn_cast<ObjCInterfaceDecl>(EnclosingDecl)) {
         }
         ClsFields[i]->setLexicalDeclContext(CDecl);
         CDecl->addDecl(ClsFields[i]);
       }
       CDecl->setIvarLBraceLoc(LBrac);
       CDecl->setIvarRBraceLoc(RBrac);
     }
   }
-
-  if (Attr)
-    ProcessDeclAttributeList(S, Record, Attr);
 }
 
 /// Determine whether the given integral value is representable within
 /// the given type T.
 static bool isRepresentableIntegerValue(ASTContext &Context,
                                         llvm::APSInt &Value,
                                         QualType T) {
   assert((T->isIntegralType(Context) || T->isEnumeralType()) &&

test/Layout/itanium-pack-and-align.cpp

+// RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm-only -fdump-record-layouts %s \
+// RUN:            | FileCheck %s
+
+struct S {
+  char x;
+  int y;
+} __attribute__((packed, aligned(8)));
+
+struct alignas(8) T {
+  char x;
+  int y;
+} __attribute__((packed));
+
+S s;
+T t;
+// CHECK:          0 | struct T
+// CHECK-NEXT:          0 |   char x
+// CHECK-NEXT:          1 |   int y
+// CHECK-NEXT:            | [sizeof=8, dsize=8, align=8,
+// CHECK-NEXT:            |  nvsize=8, nvalign=8]
+
+// CHECK:          0 | struct S
+// CHECK-NEXT:          0 |   char x
+// CHECK-NEXT:          1 |   int y
+// CHECK-NEXT:            | [sizeof=8, dsize=8, align=8,
+// CHECK-NETX:            |  nvsize=8, nvalign=8]