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

[efriedma]

Summary

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.

[erichkeane]

Couple nits, otherwise LGTM.

clang/lib/AST/Decl.cpp

2731

2732

2735

Same here

[efriedma]

clang/lib/AST/Decl.cpp

2735

You want something like this?

if (auto *List = dyn_cast<InitListExpr>(getInit()->IgnoreParens())) {
  const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1);
  if (auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType())) {
    return InitTy->getSize() != 0;
  }
}
return false;

[erichkeane]

clang/lib/AST/Decl.cpp

2735

ah, shucks, i missed that this was an early exit (that is, I missed the '!'). Disregard. LGTM.

clang/include/clang/AST/Decl.h

public:
   /// Is destruction of this variable entirely suppressed? If so, the variable
   /// need not have a usable destructor at all.
   bool isNoDestroy(const ASTContext &) const;
 
   /// Would the destruction of this variable have any effect, and if so, what
   /// 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.
   static bool classof(const Decl *D) { return classofKind(D->getKind()); }
   static bool classofKind(Kind K) { return K >= firstVar && K <= lastVar; }
 };
 
 class ImplicitParamDecl : public VarDecl {

clang/lib/AST/Decl.cpp

 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/ODRHash.h"
 #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"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
if (Eval->HasConstantDestruction)
       return QualType::DK_none;
 
   if (isNoDestroy(Ctx))
     return QualType::DK_none;
 
   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>();
   if (!Ty || !Ty->getDecl()->hasFlexibleArrayMember())
     return CharUnits::Zero();
   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 {
   if (isStaticDataMember())
     // FIXME: Remove ?
     // return getASTContext().getInstantiatedFromStaticDataMember(this);
     return getASTContext().getTemplateOrSpecializationInfo(this)
         .dyn_cast<MemberSpecializationInfo *>();

clang/lib/CodeGen/CGDecl.cpp

CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D,
   ConstantEmitter emitter(*this);
   llvm::Constant *Init = emitter.tryEmitForInitializer(D);
 
   // If constant emission failed, then this should be a C++ static
   // initializer.
   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
       // be constant.
       GV->setConstant(false);
 
       EmitCXXGuardedInit(D, GV, /*PerformInit*/true);
     }
     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
   // the global to match the initializer.  (We have to do this
   // because some types, like unions, can't be completely represented
   // in the LLVM type system.)
   if (GV->getValueType() != Init->getType()) {
     llvm::GlobalVariable *OldGV = GV;
 

clang/lib/CodeGen/CGExprConstant.cpp

if (llvm::ArrayType *ATy = dyn_cast<llvm::ArrayType>(DesiredTy)) {
     if (CanEmitArray) {
       return EmitArrayConstant(CGM, ATy, CommonType, ATy->getNumElements(),
                                ArrayElements, Filler);
     }
 
     // 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;
   }
 
   // If we don't have a natural layout, insert padding as necessary.
   // As we go, double-check to see if we can actually just emit Elems
   // as a non-packed struct and do so opportunistically if possible.
for (size_t I = 0; I != Elems.size(); ++I) {
       if (DesiredOffset != SizeSoFar)
         PackedElems.push_back(Utils.getPadding(DesiredOffset - SizeSoFar));
       PackedElems.push_back(Elems[I]);
       SizeSoFar = DesiredOffset + Utils.getSize(Elems[I]);
     }
     // 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));
     }
   }
 
   llvm::StructType *STy = llvm::ConstantStruct::getTypeForElements(
       CGM.getLLVMContext(), Packed ? PackedElems : UnpackedElems, Packed);

clang/lib/CodeGen/CodeGenModule.cpp

else if (!InitExpr) {
     emitter.emplace(*this);
     llvm::Constant *Initializer = emitter->tryEmitForInitializer(*InitDecl);
     if (!Initializer) {
       QualType T = InitExpr->getType();
       if (D->getType()->isReferenceType())
         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;
       } else {
         ErrorUnsupported(D, "static initializer");
         Init = llvm::UndefValue::get(getTypes().ConvertType(T));
       }
     } else {
       Init = Initializer;
       // We don't need an initializer, so remove the entry for the delayed
       // initializer position (just in case this entry was delayed) if we
       // also don't need to register a destructor.
       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
     }
   }
 
   llvm::Type* InitType = Init->getType();
   llvm::Constant *Entry =
       GetAddrOfGlobalVar(D, InitType, ForDefinition_t(!IsTentative));
 
   // Strip off pointer casts if we got them.

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

 // sizeof(d) == 8, so this global should be at least 8 bytes.
 struct __attribute((packed, aligned(4))) { char a; int x; char z[]; } d = { 1, 2, { 13, 15 } };
 // CHECK: @d ={{.*}} <{ i8, i32, [2 x i8], i8 }> <{ i8 1, i32 2, [2 x i8] c"\0D\0F", i8 undef }>,
 
 // This global needs 9 bytes to hold all the flexible array members.
 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

 // 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 };
 // CHECK: @a ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 7, i32 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
 #ifdef FAIL2
 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