[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

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/Sema.h

     // Any additional cases recognized here should also be handled by
     // diagnoseExprIntendedAsTemplateName.
     return false;
   }
   void diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
                                           SourceLocation Less,
                                           SourceLocation Greater);
 
-  Decl *ActOnDeclarator(Scope *S, Declarator &D);
+  Decl *ActOnDeclarator(Scope *S, Declarator &D, bool NextTokIsEqual = false);
 
   NamedDecl *HandleDeclarator(Scope *S, Declarator &D,
-                              MultiTemplateParamsArg TemplateParameterLists);
+                              MultiTemplateParamsArg TemplateParameterLists,
+                              bool NextTokIsEqual = false);
   void RegisterLocallyScopedExternCDecl(NamedDecl *ND, Scope *S);
   bool DiagnoseClassNameShadow(DeclContext *DC, DeclarationNameInfo Info);
   bool diagnoseQualifiedDeclaration(CXXScopeSpec &SS, DeclContext *DC,
                                     DeclarationName Name, SourceLocation Loc,
                                     bool IsTemplateId);
   void
   diagnoseIgnoredQualifiers(unsigned DiagID, unsigned Quals,
                             SourceLocation FallbackLoc,

                                     LookupResult &Previous);
   NamedDecl* ActOnTypedefNameDecl(Scope* S, DeclContext* DC, TypedefNameDecl *D,
                                   LookupResult &Previous, bool &Redeclaration);
   NamedDecl *ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC,
                                      TypeSourceInfo *TInfo,
                                      LookupResult &Previous,
                                      MultiTemplateParamsArg TemplateParamLists,
                                      bool &AddToScope,
-                                     ArrayRef<BindingDecl *> Bindings = None);
+                                     ArrayRef<BindingDecl *> Bindings = None,
+                                     bool NextTokIsEqual = false);
   NamedDecl *
   ActOnDecompositionDeclarator(Scope *S, Declarator &D,
                                MultiTemplateParamsArg TemplateParamLists);
   // Returns true if the variable declaration is a redeclaration
   bool CheckVariableDeclaration(VarDecl *NewVD, LookupResult &Previous);
   void CheckVariableDeclarationType(VarDecl *NewVD);
   bool DeduceVariableDeclarationType(VarDecl *VDecl, bool DirectInit,
                                      Expr *Init);

clang/lib/Parse/ParseDecl.cpp

         P.Actions.ActOnCXXExitDeclInitializer(S, ThisDecl);
         if (S)
           P.ExitScope();
       }
       ThisDecl = nullptr;
     }
   };
 
-  // Inform the current actions module that we just parsed this declarator.
+  // 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);
+    ThisDecl = Actions.ActOnDeclarator(getCurScope(), D,
+                                       /*HasInit=*/Tok.is(tok::equal));
     break;
 
   case ParsedTemplateInfo::Template:
   case ParsedTemplateInfo::ExplicitSpecialization: {
     ThisDecl = Actions.ActOnTemplateDeclarator(getCurScope(),
                                                *TemplateInfo.TemplateParams,
                                                D);
     if (VarTemplateDecl *VT = dyn_cast_or_null<VarTemplateDecl>(ThisDecl)) {

clang/lib/Sema/SemaDecl.cpp

     CXXScopeSpec &SS = Chunk.Mem.Scope();
     if (S.RebuildNestedNameSpecifierInCurrentInstantiation(SS))
       return true;
   }
 
   return false;
 }
 
-Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D) {
+Decl *Sema::ActOnDeclarator(Scope *S, Declarator &D, bool NextTokIsEqual) {
   D.setFunctionDefinitionKind(FDK_Declaration);
-  Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg());
+  Decl *Dcl = HandleDeclarator(S, D, MultiTemplateParamsArg(), NextTokIsEqual);
 
   if (OriginalLexicalContext && OriginalLexicalContext->isObjCContainer() &&
       Dcl && Dcl->getDeclContext()->isFileContext())
     Dcl->setTopLevelDeclInObjCContainer();
 
   if (getLangOpts().OpenCL)
     setCurrentOpenCLExtensionForDecl(Dcl);
 

         SpecLoc.getNestedNameSpecifier()->getAsType()))
     Diag(Loc, diag::err_decltype_in_declarator)
       << SpecLoc.getTypeLoc().getSourceRange();
 
   return false;
 }
 
 NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D,
-                                  MultiTemplateParamsArg TemplateParamLists) {
+                                  MultiTemplateParamsArg TemplateParamLists,
+                                  bool NextTokIsEqual) {
   // TODO: consider using NameInfo for diagnostic.
   DeclarationNameInfo NameInfo = GetNameForDeclarator(D);
   DeclarationName Name = NameInfo.getName();
 
   // All of these full declarators require an identifier.  If it doesn't have
   // one, the ParsedFreeStandingDeclSpec action should be used.
   if (D.isDecompositionDeclarator()) {
     return ActOnDecompositionDeclarator(S, D, TemplateParamLists);

 
     New = ActOnTypedefDeclarator(S, D, DC, TInfo, Previous);
   } else if (R->isFunctionType()) {
     New = ActOnFunctionDeclarator(S, D, DC, TInfo, Previous,
                                   TemplateParamLists,
                                   AddToScope);
   } else {
     New = ActOnVariableDeclarator(S, D, DC, TInfo, Previous, TemplateParamLists,
-                                  AddToScope);
+                                  AddToScope, /*Bindings=*/None, NextTokIsEqual);
   }
 
   if (!New)
     return nullptr;
 
   // If this has an identifier and is not a function template specialization,
   // add it to the scope stack.
   if (New->getDeclName() && AddToScope)

   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 it 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())

       return false;
   }
   return true;
 }
 
 NamedDecl *Sema::ActOnVariableDeclarator(
     Scope *S, Declarator &D, DeclContext *DC, TypeSourceInfo *TInfo,
     LookupResult &Previous, MultiTemplateParamsArg TemplateParamLists,
-    bool &AddToScope, ArrayRef<BindingDecl *> Bindings) {
+    bool &AddToScope, ArrayRef<BindingDecl *> Bindings,
+    bool NextTokIsEqual) {
   QualType R = TInfo->getType();
   DeclarationName Name = GetNameForDeclarator(D).getName();
 
   IdentifierInfo *II = Name.getAsIdentifierInfo();
 
   if (D.isDecompositionDeclarator()) {
     // Take the name of the first declarator as our name for diagnostic
     // purposes.

   }
 
   bool IsMemberSpecialization = false;
   bool IsVariableTemplateSpecialization = false;
   bool IsPartialSpecialization = false;
   bool IsVariableTemplate = false;
   VarDecl *NewVD = nullptr;
   VarTemplateDecl *NewTemplate = nullptr;
   TemplateParameterList *TemplateParams = nullptr;
   if (!getLangOpts().CPlusPlus) {
+    if (NextTokIsEqual && R->isVariablyModifiedType())
+      tryToFixVariablyModifiedVarType(*this, TInfo, R, D.getIdentifierLoc(),
+                                      /*DiagID=*/0);
+
     NewVD = VarDecl::Create(Context, DC, D.getBeginLoc(), D.getIdentifierLoc(),
                             II, R, TInfo, SC);
 
     if (R->getContainedDeducedType())
       ParsingInitForAutoVars.insert(NewVD);
 
     if (D.isInvalidType())
       NewVD->setInvalidDecl();

       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;
 

   }
   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);
-    D.setInvalidType();
+    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/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:;
+
+  char a2[ksize] = "foo"; // expected-warning{{variable length array folded to constant array as an extension}}
+
+  char a3[ksize] = {}; // expected-warning {{variable length array folded to constant array as an extension}} expected-warning{{use of GNU empty initializer}}
+
+  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/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