[PATCH] D77754: [MS] Fix packed struct layout for arrays of aligned non-record types

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added a reviewer: rjmccall.
Herald added a subscriber: jfb.
Herald added a project: clang.

In particular, this affects Clang's vectors. Users encounter this issue
when a struct contains an __m128 type.

Fixes PR45420


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77754

Files:
  clang/lib/AST/ASTContext.cpp
  clang/test/Layout/ms-aligned-array.c


Index: clang/test/Layout/ms-aligned-array.c
===================================================================
--- /dev/null
+++ clang/test/Layout/ms-aligned-array.c
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fno-rtti -emit-llvm-only -triple x86_64-pc-win32 -fms-extensions -fdump-record-layouts -fsyntax-only %s 2>/dev/null \
+// RUN:            | FileCheck %s -check-prefix CHECK
+
+// Before PR45420, we would only find the alignment on this record. Afterwards,
+// we can see the alignment on the typedef through the array type.
+// FIXME: What about other type sugar, like _Atomic? This would only matter in a
+// packed struct context.
+struct __declspec(align(16)) AlignedStruct { int x; };
+typedef int  __declspec(align(16)) AlignedInt;
+
+#define CHECK_SIZE(X, Align) \
+  _Static_assert(__alignof(struct X) == Align, "should be aligned");
+
+#pragma pack(push, 2)
+
+struct A {
+  struct AlignedStruct a[1];
+};
+CHECK_SIZE(A, 16);
+
+struct B {
+  char b;
+  AlignedInt a[1];
+};
+CHECK_SIZE(B, 16);
+
+struct C {
+  char b;
+  AlignedInt a[];
+};
+CHECK_SIZE(C, 16);
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct AlignedStruct
+// CHECK-NEXT:          0 |   int x
+// CHECK-NEXT:            | [sizeof=16, align=16]
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct A
+// CHECK-NEXT:          0 |   struct AlignedStruct [1] a
+// CHECK-NEXT:            | [sizeof=16, align=16]
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct B
+// CHECK-NEXT:          0 |   char b
+// CHECK-NEXT:         16 |   AlignedInt [1] a
+// CHECK-NEXT:            | [sizeof=32, align=16]
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:          0 | struct C
+// CHECK-NEXT:          0 |   char b
+// CHECK-NEXT:         16 |   AlignedInt [] a
+// CHECK-NEXT:            | [sizeof=16, align=16]
+
+#pragma pack(pop)
+
Index: clang/lib/AST/ASTContext.cpp
===================================================================
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -1892,24 +1892,24 @@
 
   case Type::IncompleteArray:
   case Type::VariableArray:
-    Width = 0;
-    Align = getTypeAlign(cast<ArrayType>(T)->getElementType());
-    break;
-
   case Type::ConstantArray: {
-    const auto *CAT = cast<ConstantArrayType>(T);
+    // Model non-constant sized arrays as size zero, but track the alignment.
+    uint64_t Size = 0;
+    if (const auto *CAT = dyn_cast<ConstantArrayType>(T))
+      Size = CAT->getSize().getZExtValue();
 
-    TypeInfo EltInfo = getTypeInfo(CAT->getElementType());
-    uint64_t Size = CAT->getSize().getZExtValue();
+    TypeInfo EltInfo = getTypeInfo(cast<ArrayType>(T)->getElementType());
     assert((Size == 0 || EltInfo.Width <= (uint64_t)(-1) / Size) &&
            "Overflow in array type bit size evaluation");
     Width = EltInfo.Width * Size;
     Align = EltInfo.Align;
+    AlignIsRequired = EltInfo.AlignIsRequired;
     if (!getTargetInfo().getCXXABI().isMicrosoft() ||
         getTargetInfo().getPointerWidth(0) == 64)
       Width = llvm::alignTo(Width, Align);
     break;
   }
+
   case Type::ExtVector:
   case Type::Vector: {
     const auto *VT = cast<VectorType>(T);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77754.256110.patch
Type: text/x-patch
Size: 3228 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200408/ba1d0f32/attachment-0001.bin>