Allow flexible array initialization in C++.

[efriedma]

Summary

Flexible array initialization is a C/C++ extension implemented in many compilers to allow initializing the flexible array tail of a struct type that contains a flexible array. In clang, this is currently restricted to C. But this construct is used in the Microsoft SDK headers, so I'd like to extend it to C++.

For now, this doesn't handle dynamic initialization; probably not hard to implement, but it's extra code, and I don't think it's necessary for the expected uses. And I'm not handling constant evaluation; it should fail gracefully.

I've added some additional code to assert that flexible array init works the way we expect it to in both C and C++.

[erichkeane]

clang/lib/CodeGen/CGDecl.cpp

346

Can you write a test for this with a 'fixme'? I don't see a reason why we shouldn't support this eventually.

clang/lib/CodeGen/CodeGenModule.cpp

4617

Same comment here.

clang/test/SemaCXX/constant-expression-cxx11.cpp

2388

I would expect this to be an error, not the static-assert. The constexpr variable means 'initializable as a constant expression'.

I'm guessing the problem is ACTUALLY that we support constexpr init, but not the operator[]. I think I'd like to have us have the initialization fail here, since it isn't otherwise usable.

[efriedma]

clang/test/SemaCXX/constant-expression-cxx11.cpp

2388

I think we end up computing the initializer "correctly", but have no way to actually access the elements in constant evaluation. Seems to come out of CGExprConstant lowering okay, but I guess we don't really need that to work at the moment; we fall back to the old CGExprConstant direct lowering code.

I'll add a bailout to constant evaluation.

[efriedma]

Address review comments

[efriedma]

Apparently the assertions I added are triggering in the LLVM test-suite. Commented them out in 6cf0b1b3da3e8662baf030a2d741e3becaaab2b0; I'll come up with a proper fix soon.

[nickdesaulniers]

For posterity, posting the link to the failure this caused in Linaro's TCWG's CI of kernel builds w/ clang: https://lore.kernel.org/llvm/[e-mail]/.

I'm guessing D123826 fixes this?

[efriedma]

6cf0b1b3 was a temporary fix to stop the crash. D123826 is the full fix to make the assertion actually work correctly.

[ahatanak]

clang/lib/CodeGen/CodeGenModule.cpp

4638

This assertion fails when the following code is compiled:

typedef unsigned char uint8_t;
typedef uint8_t uint8_a16 __attribute__((aligned(16)));

void foo1() {
  static const uint8_a16 array1[] = { 1 };
}

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4638

sizeof(uint8_a16[1]) is 16, but we currently emit a one-byte global. So it seems like there's an underlying bug exposed by the assertion.

gcc thinks this is nonsense, and just prints an error.

[ahatanak]

clang/lib/CodeGen/CodeGenModule.cpp

4638

It seems to me that we should disallow arrays that have an element whose alignment is larger than its size, just as gcc does.

But I see arrays like that declared in a couple of clang's regression tests (e.g., Sema/attr-aligned.c). I'm not sure whether there was a reason for not rejecting it or it was just an oversight.

[efriedma]

clang/lib/CodeGen/CodeGenModule.cpp

4638

I'd guess oversight; it's originally a gcc extension, and using it to insert padding into arrays doesn't really make sense.

[ahatanak]

We found another case where the assertion fails.

$ cat test.cpp

struct S {
  int len;
  int i[];
};

S value{0, 0};

$ clang++ -std=c++11 -c test.cpp

CstSize is only 4 while VarSize is 8.

[ahatanak]

The assertion also fails when the following code is compiled:

struct S { int i[]; };
S value{0};

According to C99, flexible array members have to belong to structs that have more than one named member. But clang allows that as a GNU extension although gcc seems to reject it.

[efriedma]

The assertion is correct; without it, your code is getting miscompiled.

[aaron.ballman]

In D123649#4362402, @efriedma wrote:

The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be asserting in practice: https://godbolt.org/z/cs5bhvPGr

[ahatanak]

Looks like Init's type is wrong. It should be { i32, [1 x i32] } instead of { i32, [0 x i32] } in the case where S has two members.

[efriedma]

In D123649#4362418, @aaron.ballman wrote:

In D123649#4362402, @efriedma wrote:

The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be asserting in practice: https://godbolt.org/z/cs5bhvPGr

I wasn't trying to suggest our behavior is correct here, just that the assertion is correctly indicating an underlying issue with the generated IR.

[aaron.ballman]

In D123649#4362756, @efriedma wrote:

In D123649#4362418, @aaron.ballman wrote:

In D123649#4362402, @efriedma wrote:

The assertion is correct; without it, your code is getting miscompiled.

The assertion may be correct in its intent, but we still should not be asserting in practice: https://godbolt.org/z/cs5bhvPGr

I wasn't trying to suggest our behavior is correct here, just that the assertion is correctly indicating an underlying issue with the generated IR.

Ah, sorry! I misunderstood. :-)

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.
+  ///
+  /// (The standard doesn't allow initializing flexible array members; this is
+  /// a gcc/msvc extension.)
+  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 {
   void anchor() override;
 

clang/lib/AST/Decl.cpp

if (Eval->HasConstantDestruction)
       return QualType::DK_none;
 
   if (isNoDestroy(Ctx))
     return QualType::DK_none;
 
   return getType().isDestructedType();
 }
 
+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);
+  auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType());
+  if (!InitTy)
+    return CharUnits::Zero();
+  return Ctx.getTypeSizeInChars(InitTy);
+}
+
 MemberSpecializationInfo *VarDecl::getMemberSpecializationInfo() const {
   if (isStaticDataMember())
     // FIXME: Remove ?
     // return getASTContext().getInstantiatedFromStaticDataMember(this);
     return getASTContext().getTemplateOrSpecializationInfo(this)
         .dyn_cast<MemberSpecializationInfo *>();
   return nullptr;
 }

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())
+      CGM.ErrorUnsupported(D.getInit(), "flexible array init");
     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;
   }
 
+#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
+
   // 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;
 
     GV = new llvm::GlobalVariable(

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())
+          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);
+
+#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
     }
   }
 
   llvm::Type* InitType = Init->getType();
   llvm::Constant *Entry =
       GetAddrOfGlobalVar(D, InitType, ForDefinition_t(!IsTentative));
 
   // Strip off pointer casts if we got them.

clang/lib/Sema/SemaInit.cpp

bool InitListChecker::CheckFlexibleArrayInit(const InitializedEntity &Entity,
                                              FieldDecl *Field,
                                              bool TopLevelObject) {
   // Handle GNU flexible array initializers.
   unsigned FlexArrayDiag;
   if (isa<InitListExpr>(InitExpr) &&
       cast<InitListExpr>(InitExpr)->getNumInits() == 0) {
     // Empty flexible array init always allowed as an extension
     FlexArrayDiag = diag::ext_flexible_array_init;
-  } else if (SemaRef.getLangOpts().CPlusPlus) {
-    // Disallow flexible array init in C++; it is not required for gcc
-    // compatibility, and it needs work to IRGen correctly in general.
-    FlexArrayDiag = diag::err_flexible_array_init;
   } else if (!TopLevelObject) {
     // Disallow flexible array init on non-top-level object
     FlexArrayDiag = diag::err_flexible_array_init;
   } else if (Entity.getKind() != InitializedEntity::EK_Variable) {
     // Disallow flexible array init on anything which is not a variable.
     FlexArrayDiag = diag::err_flexible_array_init;
   } else if (cast<VarDecl>(Entity.getDecl())->hasLocalStorage()) {
     // Disallow flexible array init on local variables.

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

+// RUN: %clang_cc1 -triple i386-unknown-unknown -emit-llvm -o - %s | FileCheck %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] }
+
+struct B { int x; int y[]; };
+B b = { 1, { 13, 15 } };
+// CHECK: @b ={{.*}} global { i32, [2 x i32] } { i32 1, [2 x i32] [i32 13, i32 15] }

clang/test/SemaCXX/constant-expression-cxx11.cpp

namespace flexible_array {
   static_assert(a.arr[1], ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 
   constexpr A b[] = {{1}, {2}, {3}}; // expected-warning {{flexible array member}}
   static_assert(b[0].x == 1, "");
   static_assert(b[1].x == 2, "");
   static_assert(b[2].x == 3, "");
   static_assert(b[2].arr[0], ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 
-  // If we ever start to accept this, we'll need to ensure we can
-  // constant-evaluate it properly.
-  constexpr A c = {1, 2, 3}; // expected-error {{initialization of flexible array member}}
+  // Flexible array initialization is currently not supported by constant
+  // evaluation. Make sure we emit a sane error message, for now.
+  constexpr A c = {1, 2, 3}; // expected-warning {{flexible array initialization is a GNU extension}}
+  static_assert(c.arr[0] == 1, ""); // expected-error {{constant expression}} expected-note {{array member without known bound}}
 }
 
 void local_constexpr_var() {
   constexpr int a = 0; // expected-note {{address of non-static constexpr variable 'a' may differ on each invocation of the enclosing function; add 'static' to give it a constant address}}
   constexpr const int *p = &a; // expected-error {{constant expression}} expected-note {{pointer to 'a' is not a constant expression}}
 }