Fix issue #2235 - IR struct packedness (#2247)

Unnaturally aligned aggregates were potentially not marked as packed,
leading to LLVM inserting additional padding and screwing up the memory
layout.

ddmd/mtype.d

@@ -8308,13 +8308,6 @@ extern (C++) final class TypeStruct : Type
     AliasThisRec att = RECfwdref;
     CPPMANGLE cppmangle = CPPMANGLE.def;
 
-    version(IN_LLVM)
-    {
-        // cache the hasUnalignedFields check
-        // 0 = not checked, 1 = aligned, 2 = unaligned
-        int unaligned;
-    }
-
     extern (D) this(StructDeclaration sym)
     {
         super(Tstruct);

ddmd/mtype.h

@@ -757,12 +757,6 @@ public:
     AliasThisRec att;
     CPPMANGLE cppmangle;
 
-#if IN_LLVM
-    // cache the hasUnalignedFields check
-    // 0 = not checked, 1 = aligned, 2 = unaligned
-    int32_t unaligned;
-#endif
-
     const char *kind();
     d_uns64 size(Loc loc);
     unsigned alignsize();

gen/llvmhelpers.cpp

@@ -1301,42 +1301,6 @@ void DtoSetFuncDeclIntrinsicName(TemplateInstance *ti, TemplateDeclaration *td,
 
 ////////////////////////////////////////////////////////////////////////////////
 
-bool hasUnalignedFields(Type *t) {
-  t = t->toBasetype();
-  if (t->ty == Tsarray) {
-    assert(t->nextOf()->size() % t->nextOf()->alignsize() == 0);
-    return hasUnalignedFields(t->nextOf());
-  }
-  if (t->ty != Tstruct) {
-    return false;
-  }
-
-  TypeStruct *ts = static_cast<TypeStruct *>(t);
-  if (ts->unaligned) {
-    return (ts->unaligned == 2);
-  }
-
-  StructDeclaration *sym = ts->sym;
-
-  // go through all the fields and try to find something unaligned
-  ts->unaligned = 2;
-  for (unsigned i = 0; i < sym->fields.dim; i++) {
-    VarDeclaration *f = static_cast<VarDeclaration *>(sym->fields.data[i]);
-    unsigned a = f->type->alignsize() - 1;
-    if (((f->offset + a) & ~a) != f->offset) {
-      return true;
-    }
-    if (f->type->toBasetype()->ty == Tstruct && hasUnalignedFields(f->type)) {
-      return true;
-    }
-  }
-
-  ts->unaligned = 1;
-  return false;
-}
-
-////////////////////////////////////////////////////////////////////////////////
-
 size_t getMemberSize(Type *type) {
   const dinteger_t dSize = type->size();
   llvm::Type *const llType = DtoType(type);

gen/llvmhelpers.h

@@ -127,9 +127,6 @@ LLConstant *DtoTypeInfoOf(Type *ty, bool base = true);
 // target stuff
 void findDefaultTarget();
 
-/// Returns true if there is any unaligned type inside the aggregate.
-bool hasUnalignedFields(Type *t);
-
 /// Returns a pointer to the given member field of an aggregate.
 ///
 /// 'src' is a pointer to the start of the memory of an 'ad' instance.

ir/irtypeaggr.cpp

@@ -203,16 +203,24 @@ IrTypeAggr::IrTypeAggr(AggregateDeclaration *ad)
       aggr(ad) {}
 
 bool IrTypeAggr::isPacked(AggregateDeclaration *ad) {
-  if (ad->isUnionDeclaration()) {
-    return true;
-  }
-  for (unsigned i = 0; i < ad->fields.dim; i++) {
-    VarDeclaration *vd = static_cast<VarDeclaration *>(ad->fields.data[i]);
-    unsigned a = vd->type->alignsize() - 1;
-    if (((vd->offset + a) & ~a) != vd->offset) {
+  const auto aggregateSize = (ad->sizeok == SIZEOKdone ? ad->structsize : ~0u);
+
+  // For unions, only a subset of the fields are actually used for the IR type -
+  // don't care.
+  for (const auto field : ad->fields) {
+    // The aggregate's size and the field offset need to be multiples of the
+    // field's natural alignment, otherwise the aggregate type is unnaturally
+    // 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) {
       return true;
     }
   }
+
   return false;
 }
 

ir/irtypestruct.cpp

@@ -45,12 +45,7 @@ IrTypeStruct *IrTypeStruct::get(StructDeclaration *sd) {
     return t;
   }
 
-  t->packed = sd->alignment == 1;
-  if (!t->packed) {
-    // Unfortunately, the previous check is not enough in case the struct
-    // contains an align declaration. See issue 726.
   t->packed = isPacked(sd);
-  }
 
   // For ldc.dcomptetypes.Pointer!(uint n,T),
   // emit { T addrspace(gIR->dcomputetarget->mapping[n])* }

tests/codegen/gh2235.d

@@ -0,0 +1,29 @@
+// RUN: %ldc -output-ll -of=%t.ll %s && FileCheck %s < %t.ll
+// RUN: %ldc -run %s
+
+// CHECK: %gh2235.Foo = type <{
+align(2) struct Foo {
+    long y;
+    byte z;
+}
+
+// CHECK: %gh2235.Bar = type <{
+class Bar {
+    union {
+        bool b;
+        Foo foo;
+    }
+    byte x;
+
+    void set(Foo f) {
+        x = 99;
+        foo = f;
+    }
+}
+
+void main() {
+    Bar bar = new Bar();
+    Foo f;
+    bar.set(f);
+    assert(bar.x == 99);
+}