[Sema] Fold VLAs to constant arrays in a few more contexts

[erik.pilkington]

Summary

D89523 removed support for promoting VLAs to constant arrays when the bounds isn't an ICE, since this can result in miscompiling a conforming program that assumes that the array is a VLA. Promoting VLAs for fields is still supported, since clang doesn't support VLAs in fields, so no conforming program could have a field VLA.

This change is really disruptive for us (hundreds of projects are failing to compile with this change), so I think we should carve out two more cases where we promote VLAs which can't miscompile a conforming program:

1. When the VLA appears in an ivar -- this seems like a corollary to the field thing
2. When the VLA has an initializer -- VLAs can't have an initializer

Thanks for taking a look!

[erik.pilkington]

Ping!

[rsmith]

The direction here seems fine to me.

clang/include/clang/Sema/Sema.h

2478 ↗

(On Diff #303204)

Can we set a HasInitializer flag on Declarator instead (much like we provide a FunctionDefinitionKind)? This flag seems a bit ad-hoc (and also doesn't cover C++ braced initializers as named).

[erik.pilkington]

Use a bit on Declarator instead of an ad-hoc bool parameter.

[erik.pilkington]

Ping!

[rsmith]

Thanks, that's very clean.

clang/lib/Sema/SemaDecl.cpp

6027

Typo "it we"

6899–6901

Is there a reason to make this C-specific? We support VLAs in C++ as an extension; it'd seem reasonable to do this folding as an extension too, if it's as simple as moving the new code out of the if (!C++).

clang/test/Sema/vla.c

104

Can you also add a positive test? Eg, a goto over one of the folded-to-constant cases, to show that we can jump over those. (As-is, this test ensures that we produce the warning, but not that we actually treat the array variable as not being a VLA.)

[thakis]

Looks like this breaks tests on Windows: http://45.33.8.238/win/29181/step_7.txt

PTAL, and revert while you investigate if it takes a while to fix.

[erik.pilkington]

In D90871#2433766, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/29181/step_7.txt

PTAL, and revert while you investigate if it takes a while to fix.

Thanks for letting me know, should be fixed by: 4fa0dbd6885bc4a48ceecdbb0fca9638792762cf

[thakis]

In D90871#2433804, @erik.pilkington wrote:

In D90871#2433766, @thakis wrote:

Looks like this breaks tests on Windows: http://45.33.8.238/win/29181/step_7.txt

PTAL, and revert while you investigate if it takes a while to fix.

Thanks for letting me know, should be fixed by: 4fa0dbd6885bc4a48ceecdbb0fca9638792762cf

Thanks for the fix! But just adding a triple means the existing test in that file is no longer tested with the win api, which is not ideal. (But probably also not a huge deal.)

clang/include/clang/Sema/DeclSpec.h

private:
   unsigned ObjCIvar : 1;
 
   /// Indicates whether this is an Objective-C 'weak' property.
   unsigned ObjCWeakProperty : 1;
 
   /// Indicates whether the InlineParams / InlineBindings storage has been used.
   unsigned InlineStorageUsed : 1;
 
+  /// Indicates whether this declarator has an initializer.
+  unsigned HasInitializer : 1;
+
   /// Attrs - Attributes.
   ParsedAttributes Attrs;
 
   /// The asm label, if specified.
   Expr *AsmLabel;
 
   /// \brief The constraint-expression specified by the trailing
   /// requires-clause, or null if no such clause was specified.

 public:
   Declarator(const DeclSpec &ds, DeclaratorContext C)
       : DS(ds), Range(ds.getSourceRange()), Context(C),
         InvalidType(DS.getTypeSpecType() == DeclSpec::TST_error),
         GroupingParens(false), FunctionDefinition(static_cast<unsigned>(
                                    FunctionDefinitionKind::Declaration)),
         Redeclaration(false), Extension(false), ObjCIvar(false),
         ObjCWeakProperty(false), InlineStorageUsed(false),
-        Attrs(ds.getAttributePool().getFactory()), AsmLabel(nullptr),
-        TrailingRequiresClause(nullptr),
+        HasInitializer(false), Attrs(ds.getAttributePool().getFactory()),
+        AsmLabel(nullptr), TrailingRequiresClause(nullptr),
         InventedTemplateParameterList(nullptr) {}
 
   ~Declarator() {
     clear();
   }
   /// getDeclSpec - Return the declaration-specifier that this declarator was
   /// declared with.
   const DeclSpec &getDeclSpec() const { return DS; }
void clear() {
     BindingGroup.clear();
 
     for (unsigned i = 0, e = DeclTypeInfo.size(); i != e; ++i)
       DeclTypeInfo[i].destroy();
     DeclTypeInfo.clear();
     Attrs.clear();
     AsmLabel = nullptr;
     InlineStorageUsed = false;
+    HasInitializer = false;
     ObjCIvar = false;
     ObjCWeakProperty = false;
     CommaLoc = SourceLocation();
     EllipsisLoc = SourceLocation();
   }
 
   /// mayOmitIdentifier - Return true if the identifier is either optional or
   /// not allowed.  This is true for typenames, prototypes, and template
public:
   bool isFunctionDefinition() const {
     return getFunctionDefinitionKind() != FunctionDefinitionKind::Declaration;
   }
 
   FunctionDefinitionKind getFunctionDefinitionKind() const {
     return (FunctionDefinitionKind)FunctionDefinition;
   }
 
+  void setHasInitializer(bool Val = true) { HasInitializer = Val; }
+  bool hasInitializer() const { return HasInitializer; }
+
   /// Returns true if this declares a real member and not a friend.
   bool isFirstDeclarationOfMember() {
     return getContext() == DeclaratorContext::Member &&
            !getDeclSpec().isFriendSpecified();
   }
 
   /// Returns true if this declares a static member.  This cannot be called on a
   /// declarator outside of a MemberContext because we won't know until

clang/lib/Parse/ParseDecl.cpp

void pop() {
         P.Actions.ActOnCXXExitDeclInitializer(S, ThisDecl);
         if (S)
           P.ExitScope();
       }
       ThisDecl = nullptr;
     }
   };
 
-  // Inform the current actions module that we just parsed this declarator.
+  enum class InitKind { Uninitialized, Equal, CXXDirect, CXXBraced };
+  InitKind TheInitKind;
+  // If a '==' or '+=' is found, suggest a fixit to '='.
+  if (isTokenEqualOrEqualTypo())
+    TheInitKind = InitKind::Equal;
+  else if (Tok.is(tok::l_paren))
+    TheInitKind = InitKind::CXXDirect;
+  else if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace) &&
+           (!CurParsedObjCImpl || !D.isFunctionDeclarator()))
+    TheInitKind = InitKind::CXXBraced;
+  else
+    TheInitKind = InitKind::Uninitialized;
+  if (TheInitKind != InitKind::Uninitialized)
+    D.setHasInitializer();
+
+  // Inform Sema that we just parsed this declarator.
   Decl *ThisDecl = nullptr;
   Decl *OuterDecl = nullptr;
   switch (TemplateInfo.Kind) {
   case ParsedTemplateInfo::NonTemplate:
     ThisDecl = Actions.ActOnDeclarator(getCurScope(), D);
     break;
 
   case ParsedTemplateInfo::Template:
if (Tok.is(tok::semi)) {
         ThisDecl =
             Actions.ActOnTemplateDeclarator(getCurScope(), FakedParamLists, D);
       }
     }
     break;
     }
   }
 
+  switch (TheInitKind) {
   // Parse declarator '=' initializer.
-  // If a '==' or '+=' is found, suggest a fixit to '='.
-  if (isTokenEqualOrEqualTypo()) {
+  case InitKind::Equal: {
     SourceLocation EqualLoc = ConsumeToken();
 
     if (Tok.is(tok::kw_delete)) {
       if (D.isFunctionDeclarator())
         Diag(ConsumeToken(), diag::err_default_delete_in_multiple_declaration)
           << 1 /* delete */;
       else
         Diag(ConsumeToken(), diag::err_deleted_non_function);
if (Tok.is(tok::kw_delete)) {
             D.getContext() == DeclaratorContext::SelectionInit)
           StopTokens.push_back(tok::r_paren);
         SkipUntil(StopTokens, StopAtSemi | StopBeforeMatch);
         Actions.ActOnInitializerError(ThisDecl);
       } else
         Actions.AddInitializerToDecl(ThisDecl, Init.get(),
                                      /*DirectInit=*/false);
     }
-  } else if (Tok.is(tok::l_paren)) {
+    break;
+  }
+  case InitKind::CXXDirect: {
     // Parse C++ direct initializer: '(' expression-list ')'
     BalancedDelimiterTracker T(*this, tok::l_paren);
     T.consumeOpen();
 
     ExprVector Exprs;
     CommaLocsTy CommaLocs;
 
     InitializerScopeRAII InitScope(*this, D, ThisDecl);
if (ParseExpressionList(Exprs, CommaLocs, ExpressionStarts)) {
       InitScope.pop();
 
       ExprResult Initializer = Actions.ActOnParenListExpr(T.getOpenLocation(),
                                                           T.getCloseLocation(),
                                                           Exprs);
       Actions.AddInitializerToDecl(ThisDecl, Initializer.get(),
                                    /*DirectInit=*/true);
     }
-  } else if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace) &&
-             (!CurParsedObjCImpl || !D.isFunctionDeclarator())) {
+    break;
+  }
+  case InitKind::CXXBraced: {
     // Parse C++0x braced-init-list.
     Diag(Tok, diag::warn_cxx98_compat_generalized_initializer_lists);
 
     InitializerScopeRAII InitScope(*this, D, ThisDecl);
 
     PreferredType.enterVariableInit(Tok.getLocation(), ThisDecl);
     ExprResult Init(ParseBraceInitializer());
 
     InitScope.pop();
 
     if (Init.isInvalid()) {
       Actions.ActOnInitializerError(ThisDecl);
     } else
       Actions.AddInitializerToDecl(ThisDecl, Init.get(), /*DirectInit=*/true);
-
-  } else {
+    break;
+  }
+  case InitKind::Uninitialized: {
     Actions.ActOnUninitializedDecl(ThisDecl);
+    break;
+  }
   }
 
   Actions.FinalizeDeclaration(ThisDecl);
   return OuterDecl ? OuterDecl : ThisDecl;
 }
 
 /// ParseSpecifierQualifierList
 ///        specifier-qualifier-list:

clang/lib/Sema/SemaDecl.cpp

TryToFixInvalidVariablyModifiedTypeSourceInfo(TypeSourceInfo *TInfo,
   if (FixedTy.isNull())
     return nullptr;
   TypeSourceInfo *FixedTInfo = Context.getTrivialTypeSourceInfo(FixedTy);
   FixInvalidVariablyModifiedTypeLoc(TInfo->getTypeLoc(),
                                     FixedTInfo->getTypeLoc());
   return FixedTInfo;
 }
 
+/// Attempt to fold a variable-sized type to a constant-sized type, returning
+/// true if we were successful.
+static bool tryToFixVariablyModifiedVarType(Sema &S, TypeSourceInfo *&TInfo,
+                                            QualType &T, SourceLocation Loc,
+                                            unsigned FailedFoldDiagID) {
+  bool SizeIsNegative;
+  llvm::APSInt Oversized;
+  TypeSourceInfo *FixedTInfo = TryToFixInvalidVariablyModifiedTypeSourceInfo(
+      TInfo, S.Context, SizeIsNegative, Oversized);
+  if (FixedTInfo) {
+    S.Diag(Loc, diag::ext_vla_folded_to_constant);
+    TInfo = FixedTInfo;
+    T = FixedTInfo->getType();
+    return true;
+  }
+
+  if (SizeIsNegative)
+    S.Diag(Loc, diag::err_typecheck_negative_array_size);
+  else if (Oversized.getBoolValue())
+    S.Diag(Loc, diag::err_array_too_large) << Oversized.toString(10);
+  else if (FailedFoldDiagID)
+    S.Diag(Loc, FailedFoldDiagID);
+  return false;
+}
+
 /// Register the given locally-scoped extern "C" declaration so
 /// that it can be found later for redeclarations. We include any extern "C"
 /// declaration that is not visible in the translation unit here, not just
 /// function-scope declarations.
 void
 Sema::RegisterLocallyScopedExternCDecl(NamedDecl *ND, Scope *S) {
   if (!getLangOpts().CPlusPlus &&
       ND->getLexicalDeclContext()->getRedeclContext()->isTranslationUnit())
if (!DC->isRecord() && S->getFnParent() == nullptr) {
     // appear in the declaration specifiers in an external declaration.
     // Global Register+Asm is a GNU extension we support.
     if (SC == SC_Auto || (SC == SC_Register && !D.getAsmLabel())) {
       Diag(D.getIdentifierLoc(), diag::err_typecheck_sclass_fscope);
       D.setInvalidType();
     }
   }
 
+  // If this variable has a variable-modified type and an initializer, try to
+  // fold to a constant-sized type. This is otherwise invalid.
+  if (D.hasInitializer() && R->isVariablyModifiedType())
+    tryToFixVariablyModifiedVarType(*this, TInfo, R, D.getIdentifierLoc(),
+                                    /*DiagID=*/0);
+
   bool IsMemberSpecialization = false;
   bool IsVariableTemplateSpecialization = false;
   bool IsPartialSpecialization = false;
   bool IsVariableTemplate = false;
   VarDecl *NewVD = nullptr;
   VarTemplateDecl *NewTemplate = nullptr;
   TemplateParameterList *TemplateParams = nullptr;
   if (!getLangOpts().CPlusPlus) {
     NewVD = VarDecl::Create(Context, DC, D.getBeginLoc(), D.getIdentifierLoc(),
                             II, R, TInfo, SC);
 
     if (R->getContainedDeducedType())
       ParsingInitForAutoVars.insert(NewVD);
 
     if (D.isInvalidType())
if (!InvalidDecl && getLangOpts().CPlusPlus && !II && BitWidth &&
       T.hasQualifiers()) {
     InvalidDecl = true;
     Diag(Loc, diag::err_anon_bitfield_qualifiers);
   }
 
   // C99 6.7.2.1p8: A member of a structure or union may have any type other
   // than a variably modified type.
   if (!InvalidDecl && T->isVariablyModifiedType()) {
-    bool SizeIsNegative;
-    llvm::APSInt Oversized;
-
-    TypeSourceInfo *FixedTInfo =
-      TryToFixInvalidVariablyModifiedTypeSourceInfo(TInfo, Context,
-                                                    SizeIsNegative,
-                                                    Oversized);
-    if (FixedTInfo) {
-      Diag(Loc, diag::ext_vla_folded_to_constant);
-      TInfo = FixedTInfo;
-      T = FixedTInfo->getType();
-    } else {
-      if (SizeIsNegative)
-        Diag(Loc, diag::err_typecheck_negative_array_size);
-      else if (Oversized.getBoolValue())
-        Diag(Loc, diag::err_array_too_large)
-          << Oversized.toString(10);
-      else
-        Diag(Loc, diag::err_typecheck_field_variable_size);
+    if (!tryToFixVariablyModifiedVarType(
+            *this, TInfo, T, Loc, diag::err_typecheck_field_variable_size))
       InvalidDecl = true;
-    }
-  }
 
   // Fields can not have abstract class types
   if (!InvalidDecl && RequireNonAbstractType(Loc, T,
                                              diag::err_abstract_type_in_decl,
                                              AbstractFieldType))
     InvalidDecl = true;
 
   bool ZeroWidth = false;
Decl *Sema::ActOnIvar(Scope *S,
   }
   if (T->isReferenceType()) {
     Diag(Loc, diag::err_ivar_reference_type);
     D.setInvalidType();
   }
   // C99 6.7.2.1p8: A member of a structure or union may have any type other
   // than a variably modified type.
   else if (T->isVariablyModifiedType()) {
-    Diag(Loc, diag::err_typecheck_ivar_variable_size);
+    if (!tryToFixVariablyModifiedVarType(
+            *this, TInfo, T, Loc, diag::err_typecheck_ivar_variable_size))
       D.setInvalidType();
   }
 
   // Get the visibility (access control) for this ivar.
   ObjCIvarDecl::AccessControl ac =
     Visibility != tok::objc_not_keyword ? TranslateIvarVisibility(Visibility)
                                         : ObjCIvarDecl::None;
   // Must set ivar's DeclContext to its enclosing interface.
   ObjCContainerDecl *EnclosingDecl = cast<ObjCContainerDecl>(CurContext);

clang/test/CXX/basic/basic.types/p10.cpp

struct ArrBad {
   S s[3]; // expected-note {{data member 's' of non-literal type 'S [3]'}}
 };
 constexpr int f(ArrBad) { return 0; } // expected-error {{1st parameter type 'ArrBad' is not a literal type}}
 
 constexpr int arb(int n) {
   int a[n]; // expected-error {{variable of non-literal type 'int [n]' cannot be defined in a constexpr function}}
 }
 // expected-warning@+1 {{variable length array folded to constant array as an extension}}
-constexpr long Overflow[ // expected-error {{constexpr variable cannot have non-literal type 'long const[(1 << 30) << 2]'}}
-    (1 << 30) << 2]{};   // expected-warning {{requires 34 bits to represent}}
+constexpr long Overflow[(1 << 30) << 2]{}; // expected-warning {{requires 34 bits to represent}}
 
 namespace inherited_ctor {
   struct A { constexpr A(int); };
   struct B : A {
     B();
     using A::A;
   };
   constexpr int f(B) { return 0; } // ok

clang/test/Sema/decl-in-prototype.c

 
 // Only warn once, even if we create two declarations.
 void f(struct q *, struct __attribute__((aligned(4))) q *); // expected-warning {{will not be visible outside}}
 
 // This enum inside the function pointer parameter shouldn't leak into the
 // function.
 enum { BB = 0 };
 void enum_in_fun_in_fun(void (*fp)(enum { AA, BB } e)) { // expected-warning {{will not be visible}}
-  SA(1, AA == 5); // expected-error {{variable-sized object may not be initialized}}
+  SA(1, AA == 5); // expected-warning{{variable length array folded to constant array as an extension}}
   SA(2, BB == 0);
 }
 
 void f7() {
   extern void ext(struct S { enum E7 { a, b } o; } p); // expected-warning 2 {{will not be visible}}
   ext(a); // expected-error {{use of undeclared identifier}}
 }
 

clang/test/Sema/vla.c

void pr44406() {
   int z[(int)(1.0 * 2)]; // expected-note {{bypasses initialization of variable length array}}
 L:;
 }
 
 const int pr44406_a = 32;
 typedef struct {
   char c[pr44406_a]; // expected-warning {{folded to constant array as an extension}}
 } pr44406_s;
+
+void test_fold_to_constant_array() {
+  const int ksize = 4;
+
+  goto jump_over_a1; // expected-error{{cannot jump from this goto statement to its label}}
+  char a1[ksize]; // expected-note{{variable length array}}
+ jump_over_a1:;
+
+  goto jump_over_a2;
+  char a2[ksize] = "foo"; // expected-warning{{variable length array folded to constant array as an extension}}
+ jump_over_a2:;
+
+  goto jump_over_a3;
+  char a3[ksize] = {}; // expected-warning {{variable length array folded to constant array as an extension}} expected-warning{{use of GNU empty initializer}}
+ jump_over_a3:;
+
+  goto jump_over_a4; // expected-error{{cannot jump from this goto statement to its label}}
+  char a4[ksize][2]; // expected-note{{variable length array}}
+ jump_over_a4:;
+
+  char a5[ksize][2] = {}; // expected-warning {{variable length array folded to constant array as an extension}} expected-warning{{use of GNU empty initializer}}
+
+  int a6[ksize] = {1,2,3,4}; // expected-warning{{variable length array folded to constant array as an extension}}
+
+  // expected-warning@+1{{variable length array folded to constant array as an extension}}
+  int a7[ksize] __attribute__((annotate("foo"))) = {1,2,3,4};
+
+  // expected-warning@+1{{variable length array folded to constant array as an extension}}
+  char a8[2][ksize] = {{1,2,3,4},{4,3,2,1}};
+}

clang/test/SemaCXX/vla.cpp

void f(int n) {
     pod<int> a[n];
     error<int> b[n]; // expected-note {{instantiation}}
     incomplete c[n]; // expected-error {{incomplete}}
   }
 }
 
 void pr23151(int (&)[*]) { // expected-error {{variable length array must be bound in function definition}}
 }
+
+void test_fold() {
+  char a1[(unsigned long)(int *)0+1]{}; // expected-warning{{variable length array folded to constant array as an extension}}
+  char a2[(unsigned long)(int *)0+1] = {}; // expected-warning{{variable length array folded to constant array as an extension}}
+  char a3[(unsigned long)(int *)0+1];
+}

clang/test/SemaObjC/variable-size-ivar.m

+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+const int ksize = 42;
+int size = 42;
+
+@interface X
+{
+  int arr1[ksize]; // expected-warning{{variable length array folded to constant array}}
+  int arr2[size]; // expected-error{{instance variables must have a constant size}}
+  int arr3[ksize-43]; // expected-error{{array size is negative}}
+}
+@end