[clang] Treat variable-length array of incomplete element type as incomplete type.

[sammccall]

The fix doesn't look obviously correct: the side effect of marking the destructor reference seems important if we actually generate code. It's not obvious to me why the type can only be incomplete if there are errors.

This was introduced between clang 8 and clang 9, I would guess by f8ccf052935adaf405e581fd31e8bc634cc5bbc7.
@erik.pilkington
Looking at that patch, mostly this function was just a rename, but there's a new callsite for array-types that seems to be what we're hitting here.
Maybe a slightly less-invasive version would be to guard that callsite with "and if the element type is complete"?

[hokein]

• jrefine the fix of the crash: if the element type of an variable-length array

is incomplete, the array type is incomplete;

• simplify the testcase

[hokein]

In D99165#2644243, @sammccall wrote:

The fix doesn't look obviously correct: the side effect of marking the destructor reference seems important if we actually generate code. It's not obvious to me why the type can only be incomplete if there are errors.

This was introduced between clang 8 and clang 9, I would guess by f8ccf052935adaf405e581fd31e8bc634cc5bbc7.
@erik.pilkington
Looking at that patch, mostly this function was just a rename, but there's a new callsite for array-types that seems to be what we're hitting here.
Maybe a slightly less-invasive version would be to guard that callsite with "and if the element type is complete"?

ok, after taking a closer look on the crash, you're right. fixing checkDestructorReference or guarding at that particular callsite is not a root fix.

I managed to reduce a smaller case ForwardClass array[var_size] = {0};, and the crash occurs when clang is performing an initializer check on an incomplete-type var decl, this should not be happened --
because an incomplete type of decl should be marked invalid in clang (e.g. ForwardClass array[var_size];, ForwardClass var;), but for the crash case, the type of variable-length array is not treated as an incomplete type (this is the bug),
thus the var decl is considered valid, and clang performs the initializer check on it.

So I think the right solution is to fix type of variable-length array -- if the element type of the variable-length array is incomplete, then the type of array is incomplete.

clang/lib/AST/Type.cpp

2234

This comment is pretty old, and was added in 2dfdb820ca550f75769f6850bc27f825f1dce4f7 in 2009. I don't think this is correct anymore, variable-length array is allowed in C++, and from http://eel.is/c++draft/basic.types.general#5,

an array of unknown bound or of incomplete element type, is an incompletely-defined object type.36 Incompletely-defined object types and cv void are incomplete types

[sammccall]

Thanks for digging!

clang/lib/AST/Type.cpp

2234

This comment is pretty old, and was added in 2dfdb820ca550f75769f6850bc27f825f1dce4f7 in 2009. I don't think this is correct anymore

Agree, VLA in C++ were supported in clang in e7545b33ff79 (5 years ago)

variable-length array is allowed in C++

Not by the standard! It's an extension, copied from gcc.

an array of unknown bound or of incomplete element type

Yeah, it makes sense to treat VLA like array here.
(I was confused, but "unknown bound" doesn't refer to VLAs, that's rather extern int x[]; which is IncompleteArray)

[hokein]

clang/lib/AST/Type.cpp

2234

Not by the standard! It's an extension, copied from gcc.

oh, right. VLA is allowed by C99 standard, but not in C++ standard, weird..

clang/lib/AST/Type.cpp

case Record: {
     // A tagged type (struct/union/enum/class) is incomplete if the decl is a
     // forward declaration, but not a full definition (C99 6.2.5p22).
     RecordDecl *Rec = cast<RecordType>(CanonicalType)->getDecl();
     if (Def)
       *Def = Rec;
     return !Rec->isCompleteDefinition();
   }
   case ConstantArray:
+  case VariableArray:
     // An array is incomplete if its element type is incomplete
     // (C++ [dcl.array]p1).
-    // We don't handle variable arrays (they're not allowed in C++) or
-    // dependent-sized arrays (dependent types are never treated as incomplete).
+    // We don't handle dependent-sized arrays (dependent types are never treated
+    // as incomplete).
     return cast<ArrayType>(CanonicalType)->getElementType()
              ->isIncompleteType(Def);
   case IncompleteArray:
     // An array of unknown size is an incomplete type (C99 6.2.5p22).
     return true;
   case MemberPointer: {
     // Member pointers in the MS ABI have special behavior in
     // RequireCompleteType: they attach a MSInheritanceAttr to the CXXRecordDecl

clang/test/SemaCXX/cxx0x-initializer-aggregates.cpp

 namespace array_addressof {
   using T = int[5];
   T *p = &T{1,2,3,4,5}; // expected-error {{taking the address of a temporary object of type 'array_addressof::T' (aka 'int [5]')}}
 }
 
 namespace PR24816 {
   struct { int i; } ne = {{0, 1}}; // expected-error{{excess elements in scalar initializer}}
 }
+
+namespace no_crash {
+class Foo; // expected-note {{forward declaration}}
+void test(int size) {
+  Foo array[size] = {0}; // expected-error {{variable has incomplete type}}
+}
+}