Fix size of flexible array initializers, and re-enable assertions.

In D123649, I got the formula for getFlexibleArrayInitChars slightly
wrong: the flexible array elements can be contained in the tail padding
of the struct.  Fix the formula to account for that.

With the fixed formula, we run into another issue: in some cases, we
were emitting extra padding for flexible arrray initializers. Fix
CGExprConstant so it uses a packed struct when necessary, to avoid this
extra padding.

Differential Revision: https://reviews.llvm.org/D123826

Author: efriedma-quic

clang/include/clang/AST/Decl.h

@@ -1591,12 +1591,18 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> {
   /// kind?
   QualType::DestructionKind needsDestruction(const ASTContext &Ctx) const;
 
-  /// If this variable declares a struct with a flexible array member, and
-  /// the flexible array member is initialized with one or more elements,
-  /// compute the number of bytes necessary to store those elements.
+  /// Whether this variable has a flexible array member initialized with one
+  /// or more elements. This can only be called for declarations where
+  /// hasInit() is true.
   ///
   /// (The standard doesn't allow initializing flexible array members; this is
   /// a gcc/msvc extension.)
+  bool hasFlexibleArrayInit(const ASTContext &Ctx) const;
+
+  /// If hasFlexibleArrayInit is true, compute the number of additional bytes
+  /// necessary to store those elements. Otherwise, returns zero.
+  ///
+  /// This can only be called for declarations where hasInit() is true.
   CharUnits getFlexibleArrayInitChars(const ASTContext &Ctx) const;
 
   // Implement isa/cast/dyncast/etc.

clang/lib/AST/Decl.cpp

@@ -31,6 +31,7 @@
 #include "clang/AST/PrettyDeclStackTrace.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/Randstruct.h"
+#include "clang/AST/RecordLayout.h"
 #include "clang/AST/Redeclarable.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
@@ -2722,6 +2723,21 @@ VarDecl::needsDestruction(const ASTContext &Ctx) const {
   return getType().isDestructedType();
 }
 
+bool VarDecl::hasFlexibleArrayInit(const ASTContext &Ctx) const {
+  assert(hasInit() && "Expect initializer to check for flexible array init");
+  auto *Ty = getType()->getAs<RecordType>();
+  if (!Ty || !Ty->getDecl()->hasFlexibleArrayMember())
+    return false;
+  auto *List = dyn_cast<InitListExpr>(getInit()->IgnoreParens());
+  if (!List)
+    return false;
+  const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1);
+  auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType());
+  if (!InitTy)
+    return false;
+  return InitTy->getSize() != 0;
+}
+
 CharUnits VarDecl::getFlexibleArrayInitChars(const ASTContext &Ctx) const {
   assert(hasInit() && "Expect initializer to check for flexible array init");
   auto *Ty = getType()->getAs<RecordType>();
@@ -2730,11 +2746,17 @@ CharUnits VarDecl::getFlexibleArrayInitChars(const ASTContext &Ctx) const {
   auto *List = dyn_cast<InitListExpr>(getInit()->IgnoreParens());
   if (!List)
     return CharUnits::Zero();
-  auto FlexibleInit = List->getInit(List->getNumInits() - 1);
+  const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1);
   auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType());
   if (!InitTy)
     return CharUnits::Zero();
-  return Ctx.getTypeSizeInChars(InitTy);
+  CharUnits FlexibleArraySize = Ctx.getTypeSizeInChars(InitTy);
+  const ASTRecordLayout &RL = Ctx.getASTRecordLayout(Ty->getDecl());
+  CharUnits FlexibleArrayOffset =
+      Ctx.toCharUnitsFromBits(RL.getFieldOffset(RL.getFieldCount() - 1));
+  if (FlexibleArrayOffset + FlexibleArraySize < RL.getSize())
+    return CharUnits::Zero();
+  return FlexibleArrayOffset + FlexibleArraySize - RL.getSize();
 }
 
 MemberSpecializationInfo *VarDecl::getMemberSpecializationInfo() const {

clang/lib/CodeGen/CGDecl.cpp

@@ -342,7 +342,7 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D,
   if (!Init) {
     if (!getLangOpts().CPlusPlus)
       CGM.ErrorUnsupported(D.getInit(), "constant l-value expression");
-    else if (!D.getFlexibleArrayInitChars(getContext()).isZero())
+    else if (D.hasFlexibleArrayInit(getContext()))
       CGM.ErrorUnsupported(D.getInit(), "flexible array initializer");
     else if (HaveInsertPoint()) {
       // Since we have a static initializer, this global variable can't
@@ -354,17 +354,12 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D,
     return GV;
   }
 
-#if 0
-  // FIXME: The following check doesn't handle flexible array members
-  // inside tail padding (which don't actually increase the size of
-  // the struct).
 #ifndef NDEBUG
   CharUnits VarSize = CGM.getContext().getTypeSizeInChars(D.getType()) +
                       D.getFlexibleArrayInitChars(getContext());
   CharUnits CstSize = CharUnits::fromQuantity(
       CGM.getDataLayout().getTypeAllocSize(Init->getType()));
   assert(VarSize == CstSize && "Emitted constant has unexpected size");
-#endif
 #endif
 
   // The initializer may differ in type from the global. Rewrite

clang/lib/CodeGen/CGExprConstant.cpp

@@ -439,22 +439,33 @@ llvm::Constant *ConstantAggregateBuilder::buildFrom(
     // Can't emit as an array, carry on to emit as a struct.
   }
 
+  // The size of the constant we plan to generate.  This is usually just
+  // the size of the initialized type, but in AllowOversized mode (i.e.
+  // flexible array init), it can be larger.
   CharUnits DesiredSize = Utils.getSize(DesiredTy);
+  if (Size > DesiredSize) {
+    assert(AllowOversized && "Elems are oversized");
+    DesiredSize = Size;
+  }
+
+  // The natural alignment of an unpacked LLVM struct with the given elements.
   CharUnits Align = CharUnits::One();
   for (llvm::Constant *C : Elems)
     Align = std::max(Align, Utils.getAlignment(C));
+
+  // The natural size of an unpacked LLVM struct with the given elements.
   CharUnits AlignedSize = Size.alignTo(Align);
 
   bool Packed = false;
   ArrayRef<llvm::Constant*> UnpackedElems = Elems;
   llvm::SmallVector<llvm::Constant*, 32> UnpackedElemStorage;
-  if ((DesiredSize < AlignedSize && !AllowOversized) ||
-      DesiredSize.alignTo(Align) != DesiredSize) {
-    // The natural layout would be the wrong size; force use of a packed layout.
+  if (DesiredSize < AlignedSize || DesiredSize.alignTo(Align) != DesiredSize) {
+    // The natural layout would be too big; force use of a packed layout.
     NaturalLayout = false;
     Packed = true;
   } else if (DesiredSize > AlignedSize) {
-    // The constant would be too small. Add padding to fix it.
+    // The natural layout would be too small. Add padding to fix it. (This
+    // is ignored if we choose a packed layout.)
     UnpackedElemStorage.assign(Elems.begin(), Elems.end());
     UnpackedElemStorage.push_back(Utils.getPadding(DesiredSize - Size));
     UnpackedElems = UnpackedElemStorage;
@@ -482,7 +493,7 @@ llvm::Constant *ConstantAggregateBuilder::buildFrom(
     // If we're using the packed layout, pad it out to the desired size if
     // necessary.
     if (Packed) {
-      assert((SizeSoFar <= DesiredSize || AllowOversized) &&
+      assert(SizeSoFar <= DesiredSize &&
              "requested size is too small for contents");
       if (SizeSoFar < DesiredSize)
         PackedElems.push_back(Utils.getPadding(DesiredSize - SizeSoFar));

clang/lib/CodeGen/CodeGenModule.cpp

@@ -4615,7 +4615,7 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
         T = D->getType();
 
       if (getLangOpts().CPlusPlus) {
-        if (!InitDecl->getFlexibleArrayInitChars(getContext()).isZero())
+        if (InitDecl->hasFlexibleArrayInit(getContext()))
           ErrorUnsupported(D, "flexible array initializer");
         Init = EmitNullConstant(T);
         NeedsGlobalCtor = true;
@@ -4631,17 +4631,12 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
       if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
         DelayedCXXInitPosition.erase(D);
 
-#if 0
-      // FIXME: The following check doesn't handle flexible array members
-      // inside tail padding (which don't actually increase the size of
-      // the struct).
 #ifndef NDEBUG
       CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) +
                           InitDecl->getFlexibleArrayInitChars(getContext());
       CharUnits CstSize = CharUnits::fromQuantity(
           getDataLayout().getTypeAllocSize(Init->getType()));
       assert(VarSize == CstSize && "Emitted constant has unexpected size");
-#endif
 #endif
     }
   }

clang/test/CodeGen/flexible-array-init.c

@@ -18,7 +18,5 @@ struct __attribute((packed, aligned(4))) { char a; int x; char z[]; } d = { 1, 2
 struct __attribute((packed, aligned(4))) { char a; int x; char z[]; } e = { 1, 2, { 13, 15, 17, 19 } };
 // CHECK: @e ={{.*}} <{ i8, i32, [4 x i8] }> <{ i8 1, i32 2, [4 x i8] c"\0D\0F\11\13" }>
 
-// FIXME: This global should be 6 bytes, not 8.  Not likely to matter in most
-// cases, but still a bug.
 struct { int x; char y[]; } f = { 1, { 13, 15 } };
-// CHECK: @f ={{.*}} global { i32, [2 x i8] } { i32 1, [2 x i8] c"\0D\0F" }
+// CHECK: @f ={{.*}} global <{ i32, [2 x i8] }> <{ i32 1, [2 x i8] c"\0D\0F" }>

clang/test/CodeGenCXX/flexible-array-init.cpp

@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL1 %s
 // RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL2 %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm-only -verify -DFAIL3 %s
 
 struct A { int x; int y[]; };
 A a = { 1, 7, 11 };
@@ -9,7 +10,7 @@ A a = { 1, 7, 11 };
 A b = { 1, { 13, 15 } };
 // CHECK: @b ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 13, i32 15] }
 
-int f();
+char f();
 #ifdef FAIL1
 A c = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}}
 #endif
@@ -18,3 +19,7 @@ void g() {
   static A d = { f(), { f(), f() } }; // expected-error {{cannot compile this flexible array initializer yet}}
 }
 #endif
+#ifdef FAIL3
+struct B { int x; char y; char z[]; };
+B e = {f(), f(), f(), f()}; // expected-error {{cannot compile this flexible array initializer yet}}
+#endif