Sema: Treat zero sized arrays as flexible array members

[majnemer]

Summary

Zero sized arrays are more or less the pre-standard version of flexible
array members. It makes sense to mark structs which contain them as
hasFlexibleArrayMember.

Doing this has the side effect of resolving PR21040, a crash involving
one record inheriting from a base which is terminated with a zero sized
array field.

[andreadb]

Thanks for this patch.

I am not knowledgeable enough in this area to give a LGTM, but personally I like this approach.
I always thought that GCC was too permissive in general when it came to using zero-length arrays.
I agree that we should allow declaring a zero-length array as the sole element of a structure. I am not sure if it makes sense to have a zero-length array in a union.

-Andrea

[rsmith]

Generally, I think this is a good direction, but seems large enough to deserve a release notes change. Have you tested this against much existing code?

test/SemaCXX/zero-length-arrays.cpp

13

Can we find out what rdar://problem/10228639 was?

[majnemer]

I haven't tested it much beyond self hosting and targeted test cases.

test/SemaCXX/zero-length-arrays.cpp

13

interaction between having a trailing zero-sized array in a struct (C++ extension) and whether that implies having a default constructor.
the usual sort of thing. language extension interaction w/ actual language standard. things get odd.

[rsmith]

It'd be useful to figure out where we differ from the GCC documentation with this patch applied:

https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

test/SemaCXX/zero-length-arrays.cpp

13

Sounds like there was no expectation that we actually accept this then?

[kcc]

Richard, ping?

include/clang/AST/ASTContext.h

public:
 
   /// \brief Return the innermost element type of a type (which needn't
   /// actually be an array type).
   QualType getBaseElementType(QualType QT) const;
 
   /// \brief Return number of constant array elements.
   uint64_t getConstantArrayElementCount(const ConstantArrayType *CA) const;
 
+  /// \brief Determine whether the given type is an incomplete or zero-length
+  /// array type.
+  bool isIncompleteOrZeroLengthArrayType(QualType T) const;
+
   /// \brief Perform adjustment on the parameter type of a function.
   ///
   /// This routine adjusts the given parameter type @p T to the actual
   /// parameter type used by semantic analysis (C99 6.7.5.3p[7,8],
   /// C++ [dcl.fct]p3). The adjusted parameter type is returned.
   QualType getAdjustedParameterType(QualType T) const;
   
   /// \brief Retrieve the parameter type as adjusted for use in the signature

lib/AST/ASTContext.cpp

ASTContext::getConstantArrayElementCount(const ConstantArrayType *CA)  const {
   do {
     ElementCount *= CA->getSize().getZExtValue();
     CA = dyn_cast_or_null<ConstantArrayType>(
       CA->getElementType()->getAsArrayTypeUnsafe());
   } while (CA);
   return ElementCount;
 }
 
+bool ASTContext::isIncompleteOrZeroLengthArrayType(QualType T) const {
+  if (T->isIncompleteArrayType())
+    return true;
+
+  while (const ConstantArrayType *ArrayT = getAsConstantArrayType(T)) {
+    if (!ArrayT->getSize())
+      return true;
+
+    T = ArrayT->getElementType();
+  }
+
+  return false;
+}
+
 /// getFloatingRank - Return a relative rank for floating point types.
 /// This routine will assert if passed a built-in type that isn't a float.
 static FloatingRank getFloatingRank(QualType T) {
   if (const ComplexType *CT = T->getAs<ComplexType>())
     return getFloatingRank(CT->getElementType());
 
   assert(T->getAs<BuiltinType>() && "getFloatingRank(): not a floating type");
   switch (T->getAs<BuiltinType>()->getKind()) {

lib/Sema/SemaDecl.cpp

void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
   SmallVector<FieldDecl*, 32> RecFields;
 
   bool ARCErrReported = false;
   for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
        i != end; ++i) {
     FieldDecl *FD = cast<FieldDecl>(*i);
 
     // Get the type for the field.
-    const Type *FDTy = FD->getType().getTypePtr();
+    QualType FDTy = FD->getType();
 
     if (!FD->isAnonymousStructOrUnion()) {
       // Remember all fields written by the user.
       RecFields.push_back(FD);
     }
 
     // If the field is already invalid for some reason, don't emit more
     // diagnostics about it.
for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
     //   array.
     if (FDTy->isFunctionType()) {
       // Field declared as a function.
       Diag(FD->getLocation(), diag::err_field_declared_as_function)
         << FD->getDeclName();
       FD->setInvalidDecl();
       EnclosingDecl->setInvalidDecl();
       continue;
-    } else if (FDTy->isIncompleteArrayType() && Record && 
+    } else if (Context.isIncompleteOrZeroLengthArrayType(FDTy) && Record &&
                ((i + 1 == Fields.end() && !Record->isUnion()) ||
-                ((getLangOpts().MicrosoftExt ||
-                  getLangOpts().CPlusPlus) &&
+                ((getLangOpts().MicrosoftExt || getLangOpts().CPlusPlus) &&
                  (i + 1 == Fields.end() || Record->isUnion())))) {
       // Flexible array member.
       // Microsoft and g++ is more permissive regarding flexible array.
       // It will accept flexible array in union and also
       // as the sole element of a struct/class.
       unsigned DiagID = 0;
       if (Record->isUnion())
         DiagID = getLangOpts().MicrosoftExt
                      ? diag::ext_flexible_array_union_ms
                      : getLangOpts().CPlusPlus
                            ? diag::ext_flexible_array_union_gnu
                            : diag::err_flexible_array_union;
       else if (Fields.size() == 1)
         DiagID = getLangOpts().MicrosoftExt
                      ? diag::ext_flexible_array_empty_aggregate_ms
                      : getLangOpts().CPlusPlus
                            ? diag::ext_flexible_array_empty_aggregate_gnu
                            : NumNamedMembers < 1
                                  ? diag::err_flexible_array_empty_aggregate
                                  : 0;
 
-      if (DiagID)
+      if (DiagID && FDTy->isIncompleteArrayType())
         Diag(FD->getLocation(), DiagID) << FD->getDeclName()
                                         << Record->getTagKind();
       // While the layout of types that contain virtual bases is not specified
       // by the C++ standard, both the Itanium and Microsoft C++ ABIs place
       // virtual bases after the derived members.  This would make a flexible
       // array member declared at the end of an object not adjacent to the end
       // of the type.
       if (CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(Record))

lib/Sema/SemaDeclCXX.cpp

for (auto *C : Indirect->chain()) {
       if (Field && isInactiveUnionMember(Field))
         return true;
     }
     return false;
   }
 };
 }
 
-/// \brief Determine whether the given type is an incomplete or zero-lenfgth
-/// array type.
-static bool isIncompleteOrZeroLengthArrayType(ASTContext &Context, QualType T) {
-  if (T->isIncompleteArrayType())
-    return true;
-  
-  while (const ConstantArrayType *ArrayT = Context.getAsConstantArrayType(T)) {
-    if (!ArrayT->getSize())
-      return true;
-    
-    T = ArrayT->getElementType();
-  }
-  
-  return false;
-}
-
 static bool CollectFieldInitializer(Sema &SemaRef, BaseAndFieldInfo &Info,
                                     FieldDecl *Field, 
                                     IndirectFieldDecl *Indirect = nullptr) {
   if (Field->isInvalidDecl())
     return false;
 
   // Overwhelmingly common case: we have a direct initializer for this field.
   if (CXXCtorInitializer *Init =
else
       Init = new (SemaRef.Context) CXXCtorInitializer(SemaRef.Context, Field,
                                                       SourceLocation(),
                                                       SourceLocation(), DIE,
                                                       SourceLocation());
     return Info.addFieldInitializer(Init);
   }
 
   // Don't initialize incomplete or zero-length arrays.
-  if (isIncompleteOrZeroLengthArrayType(SemaRef.Context, Field->getType()))
+  if (SemaRef.Context.isIncompleteOrZeroLengthArrayType(Field->getType()))
     return false;
 
   // Don't try to build an implicit initializer if there were semantic
   // errors in any of the initializers (and therefore we might be
   // missing some that the user actually wrote).
   if (Info.AnyErrorsInInits)
     return false;
 
Sema::MarkBaseAndMemberDestructorsReferenced(SourceLocation Location,
   // emitted, and we currently don't say.
   
   // Non-static data members.
   for (auto *Field : ClassDecl->fields()) {
     if (Field->isInvalidDecl())
       continue;
     
     // Don't destroy incomplete or zero-length arrays.
-    if (isIncompleteOrZeroLengthArrayType(Context, Field->getType()))
+    if (Context.isIncompleteOrZeroLengthArrayType(Field->getType()))
       continue;
 
     QualType FieldType = Context.getBaseElementType(Field->getType());
     
     const RecordType* RT = FieldType->getAs<RecordType>();
     if (!RT)
       continue;
     

test/Analysis/dtors-in-dtor-cfg-output.cpp

 public:
   ~TestOrder();
 };
 
 TestOrder::~TestOrder() {}
 
 class TestArray {
   A a[2];
-  A b[0];
 public:
   ~TestArray();
 };
 
 TestArray::~TestArray() {}
 
 // CHECK:  [B2 (ENTRY)]
 // CHECK:    Succs (1): B1

test/CodeGenObjC/arc-captured-32bit-block-var-layout.m

// CHECK: block variable layout: BL_NON_OBJECT_WORD:1, BL_UNRETAINED:4, BL_OPERATOR:0
 
    c();
 }
 
 // Test3 for array of stuff.
 void arr3() {
   struct S {
    int a;
-    __unsafe_unretained id unsafe_unretained_var[0];
+    __unsafe_unretained id unsafe_unretained_var[7];
  } imported_s;
 
-// CHECK: block variable layout: BL_OPERATOR:0
+// CHECK: block variable layout: BL_NON_OBJECT_WORD:1, BL_UNRETAINED:7, BL_OPERATOR:0
     void (^c)() = ^{
       int i = imported_s.a;
     };    
 
    c();
 }
 
 

test/CodeGenObjC/arc-captured-block-var-layout.m

// CHECK-LP64: block variable layout: BL_NON_OBJECT_WORD:1, BL_UNRETAINED:4, BL_OPERATOR:0
 
    c();
 }
 
 // Test3 for array of stuff.
 void arr3() {
   struct S {
    int a;
-    __unsafe_unretained id unsafe_unretained_var[0];
+    __unsafe_unretained id unsafe_unretained_var[7];
  } imported_s;
 
-// CHECK-LP64: block variable layout: BL_OPERATOR:0
+// CHECK-LP64: block variable layout: BL_NON_OBJECT_WORD:1, BL_UNRETAINED:7, BL_OPERATOR:0
     void (^c)() = ^{
       int i = imported_s.a;
     };    
 
    c();
 }
 
 

test/SemaCXX/flexible-array-test.cpp

 struct FlexNonTrivDtor {
   int n;
   NonTrivDtor ntd[]; // expected-error {{flexible array member 'ntd' of type 'NonTrivDtor []' with non-trivial destruction}}
   ~FlexNonTrivDtor() {
     for (int i = n; i != 0; --i)
       ntd[i-1].~NonTrivDtor();
   }
 };
+
+namespace PR21040 {
+struct A {
+  int a0[0];
+};
+
+struct B : private A { // expected-error {{base class 'A' has a flexible array member}}
+  int b;
+};
+
+B bb;
+}

test/SemaCXX/zero-length-arrays.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 // <rdar://problem/10228639>
 class Foo {
   ~Foo();
   Foo(const Foo&);
 public:
   Foo(int);
 };
 
 class Bar {
   int foo_count;
-  Foo foos[0];
-  Foo foos2[0][2];
-  Foo foos3[2][0];
+  Foo foos[0]; // expected-error-re {{flexible array member {{.*}} with non-trivial destruction}}
 
 public:
   Bar(): foo_count(0) { }    
   ~Bar() { }
 };
 
 void testBar() {
   Bar b;
   Bar b2(b);
   b = b2;
 }