Fix packed struct layout regression (issue ldc-developers#2346)

kinke · Sep 24, 2017 · c4ecdbe · c4ecdbe
1 parent 5a9c7a1
commit c4ecdbe
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 4 deletions.
diff --git a/ir/irtypeaggr.cpp b/ir/irtypeaggr.cpp
@@ -203,7 +203,11 @@ IrTypeAggr::IrTypeAggr(AggregateDeclaration *ad)
       aggr(ad) {}
 
 bool IrTypeAggr::isPacked(AggregateDeclaration *ad) {
+  // If the aggregate's size is unknown, any field with natural alignment > 1
+  // will make it packed.
   const auto aggregateSize = (ad->sizeok == SIZEOKdone ? ad->structsize : ~0u);
+  const auto aggregateOverallAlignment =
+      ad->sizeok == SIZEOKdone ? ad->alignsize : 1;
 
   // For unions, only a subset of the fields are actually used for the IR type -
   // don't care.
@@ -213,10 +217,12 @@ bool IrTypeAggr::isPacked(AggregateDeclaration *ad) {
     // aligned, and LLVM would insert padding.
     const auto naturalFieldAlignment = field->type->alignsize();
     const auto mask = naturalFieldAlignment - 1;
-
-    // If the aggregate's size is unknown, any field with natural alignment > 1
-    // will make it packed.
-    if ((aggregateSize & mask) != 0 || (field->offset & mask) != 0) {
+    // Additionally, the aggregate's overall alignment must not be lower than
+    // a field's natural one, otherwise LLVM would use a higher overall LL
+    // struct alignment, causing additional padding in aggregates containing
+    // this aggregate.
+    if (naturalFieldAlignment > aggregateOverallAlignment ||
+        (aggregateSize & mask) != 0 || (field->offset & mask) != 0) {
       return true;
     }
   }

diff --git a/tests/codegen/gh2346.d b/tests/codegen/gh2346.d
@@ -0,0 +1,53 @@
+// RUN: %ldc -output-ll -of=%t.ll %s && FileCheck %s < %t.ll
+
+// Make sure the LL struct is packed due to an unnatural overall alignment of 1.
+// CHECK-DAG: %gh2346.UnalignedUInt = type <{ i32 }>
+struct UnalignedUInt {
+  align(1) uint a;
+}
+static assert(UnalignedUInt.alignof == 1);
+static assert(UnalignedUInt.sizeof == 4);
+
+// Then there's no need to pack naturally aligned containers.
+// CHECK-DAG: %gh2346.Container = type { i8, %gh2346.UnalignedUInt }
+struct Container {
+    ubyte one;
+    UnalignedUInt two;
+}
+static assert(Container.alignof == 1);
+static assert(Container.sizeof == 5);
+static assert(Container.two.offsetof == 1);
+
+// CHECK-DAG: %gh2346.UnalignedUInt2 = type <{ i32 }>
+struct UnalignedUInt2 {
+  align(2) uint a;
+}
+static assert(UnalignedUInt2.alignof == 2);
+static assert(UnalignedUInt2.sizeof == 4);
+
+// CHECK-DAG: %gh2346.Container2 = type { i8, [1 x i8], %gh2346.UnalignedUInt2 }
+struct Container2 {
+    ubyte one;
+    UnalignedUInt2 two;
+}
+static assert(Container2.alignof == 2);
+static assert(Container2.sizeof == 6);
+static assert(Container2.two.offsetof == 2);
+
+// CHECK-DAG: %gh2346.PackedContainer2 = type <{ i8, %gh2346.UnalignedUInt2 }>
+struct PackedContainer2 {
+    ubyte one;
+    align(1) UnalignedUInt2 two;
+}
+static assert(PackedContainer2.alignof == 1);
+static assert(PackedContainer2.sizeof == 5);
+static assert(PackedContainer2.two.offsetof == 1);
+
+// CHECK-DAG: %gh2346.WeirdContainer = type { i8, [1 x i8], %gh2346.UnalignedUInt, [2 x i8] }
+align(4) struct WeirdContainer {
+    ubyte one;
+    align(2) UnalignedUInt two;
+}
+static assert(WeirdContainer.alignof == 2);
+static assert(WeirdContainer.sizeof == 8);
+static assert(WeirdContainer.two.offsetof == 2);