[Sema] Fold VLA types in compound literals to constant arrays.

[efriedma]

Summary

Similar to variables with an initializer, this is never valid in standard C, so we can safely constant-fold as an extension. I ran into this construct in a couple proprietary codebases.

While I'm here, drive-by fix for 090dd647: we should only fold variables with VLA types, not arbitrary variably modified types.

[aaron.ballman]

clang/test/Sema/vla.c

134

Doesn't this violate the constraints in C17 6.5.2.5p1, "The type name shall specify a complete object type or an array of unknown size, but not a variable-length array type"?

[efriedma]

clang/test/Sema/vla.c

134

Yes, this is a constraint violation. This patch downgrades the error to a warning, for compatibility with older versions of clang.

[aaron.ballman]

clang/test/Sema/vla.c

134

It was an error in older versions of Clang, so downgrading it to a warning can't make code *more* compatible with older versions. This also makes it harder to port C code to other compilers because Clang happily accepts code that the C standard says should be rejected.

I'm not strongly opposed to the change, but I also don't like ignoring constraint violations in the standard as that comes awfully close to what -fpermissive does (and at least with that awful flag you have to opt *into* the less strictly conforming behavior instead of getting it by default as with extensions).

I'm curious if @rsmith has thoughts here?

[efriedma]

clang/test/Sema/vla.c

134

It was an error in older versions of Clang

https://godbolt.org/z/rvbffY

[aaron.ballman]

clang/test/Sema/vla.c

134

Oh, I hadn't realized this changed *that* recently! Is this breaking some significant amounts of code now that we err on it (or regressing performance by not folding the VLA)?

My point still stands about disliking when we ignore constraint violations. To expound a bit, in this particular case, I think the standard is being over-constraining because we reasonably *can* fold this to a more efficient form. Normally, I'd suggest filing a DR with WG14 over this and treating the constraint as UB we can extend. However, I think the "solution" that comes out of WG14 would (likely) be to make compound literal expressions of VLA types be undefined behavior rather than a constraint violation (because I don't see many other options in the standard wording for a more targeted fix to allow *just* this safe usage). Given the security concerns around misuse of VLAs already, I think that would be a worse world to live in even if it gets us what we want with this specific case in mind. This is where my caution is coming from. Knowing a bit more about the experience in the proprietary code bases would be helpful, if it's something you can share more details about.

[rsmith]

clang/test/Sema/vla.c

134

Is this breaking some significant amounts of code now that we err on it (or regressing performance by not folding the VLA)?

We only ever constant-fold VLAs to constant-length arrays in cases where a VLA would be invalid, so there's no regressing-performance concern. (We used to do such folding, but it resulted in our miscompiling certain testcases, which is why Clang's behavior changed recently.) As a result, there's no risk of someone seeing a surprise VLA here, that should have been a constraint violation.

Given the above, I don't think that changing this from a constraint violation to UB in the C standard would make a difference. Either way, I think a strictly conforming program cannot contain such a construct, and a conforming implementation can, as an extension, define the meaning of programs containing such a construct.

It's common for us to have conforming extensions that permit us to accept more real-world code. Accepting with an on-by-default warning is often the best tradeoff between accepting real-world code and diagnosing non-portable constructs, especially in cases where past versions of GCC (<= 4.4) and Clang accepted the code as an extension. I'm inclined to think that's the best we can do here.

If we want to more strongly discourage use of this extension (and I don't have any particular preferences on whether we should do so), we could promote the warning to DefaultError (but leave the extension in, so that people can still build legacy codebases), but I think we should do that consistently across all the places we fold VLAs to constant arrays, not only for compound literals.

[efriedma]

clang/test/Sema/vla.c

134

The pattern I saw looked something like the following, at global scope, where it couldn't really be confused for a VLA:

const int x = 2;
int *p = (int[x]){0};

As for the frequency of it showing up, I think I got confused. Looking again, there were multiple codebases with new -Wgnu-folding-constant warnings, but only one codebase with this specific construct. So that makes the argument a bit weaker.

That said, we're doing this folding for variables already, so extending it to compound literals doesn't seem like a big deal. And the warning is on by default.

[aaron.ballman]

LGTM, though I think we should consider changing the diagnostic text (for all of the VLA folding cases) as a follow-up.

clang/test/Sema/vla.c

134

Given the above, I don't think that changing this from a constraint violation to UB in the C standard would make a difference. Either way, I think a strictly conforming program cannot contain such a construct, and a conforming implementation can, as an extension, define the meaning of programs containing such a construct.

I think changing from a constraint violation to UB in the C standard would make a difference, but subtly. When something is a constraint violation, the implicit expectation is that implementations diagnose it because the committee felt it was a sufficiently bad construct to warrant forcing implementations to tell users not to use it. When something is UB, there is no such expectation -- some UB is explicitly for implementation extensions, others are because it would be impossible to diagnose, etc. So I agree that the outcome is the same, but the messaging to implementers (and other readers of the standard) is quite different.

It's common for us to have conforming extensions that permit us to accept more real-world code. Accepting with an on-by-default warning is often the best tradeoff between accepting real-world code and diagnosing non-portable constructs, especially in cases where past versions of GCC (<= 4.4) and Clang accepted the code as an extension. I'm inclined to think that's the best we can do here.

Agreed, but "real-world code", to me, isn't "someone wrote it once" so much as "this is being relied upon and it's an important use case." For example, I am highly sympathetic to a standard library (or other popular system libraries) that relied on older compiler behavior (accidentally or purposefully), but less so for "regular user code" unless it's a popular idiom.

If we want to more strongly discourage use of this extension (and I don't have any particular preferences on whether we should do so), we could promote the warning to DefaultError (but leave the extension in, so that people can still build legacy codebases), but I think we should do that consistently across all the places we fold VLAs to constant arrays, not only for compound literals.

100% agreed that anything we do here should be done consistently. I think I'd feel most comfortable if the diagnostic made it more obvious that this code is not valid C code, like ISO C forbids use of a variable length array in this context; folding to a constant array as an extension. I think it's reasonable to not promote it to DefaultError because I think each of these cases is not introducing some potentially unsafe usage so much as enabling questionable code to compile with reasonable results. (If I'm wrong about that, I'd be curious to know of it.)

As for this specific case of defining the behavior... if this was the first VLA constant folding extension, I'd argue it's not motivated well enough, but because we have plenty of other instances of doing the same thing, I'm fine adding the extension here.

clang/include/clang/Sema/Sema.h

public:
   void diagnoseExprIntendedAsTemplateName(Scope *S, ExprResult TemplateName,
                                           SourceLocation Less,
                                           SourceLocation Greater);
 
   Decl *ActOnDeclarator(Scope *S, Declarator &D);
 
   NamedDecl *HandleDeclarator(Scope *S, Declarator &D,
                               MultiTemplateParamsArg TemplateParameterLists);
+  bool tryToFixVariablyModifiedVarType(TypeSourceInfo *&TInfo,
+                                       QualType &T, SourceLocation Loc,
+                                       unsigned FailedFoldDiagID);
   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,

clang/lib/Sema/SemaDecl.cpp

TryToFixInvalidVariablyModifiedTypeSourceInfo(TypeSourceInfo *TInfo,
   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 Sema::tryToFixVariablyModifiedVarType(TypeSourceInfo *&TInfo,
   bool SizeIsNegative;
   llvm::APSInt Oversized;
   TypeSourceInfo *FixedTInfo = TryToFixInvalidVariablyModifiedTypeSourceInfo(
-      TInfo, S.Context, SizeIsNegative, Oversized);
+      TInfo, Context, SizeIsNegative, Oversized);
   if (FixedTInfo) {
-    S.Diag(Loc, diag::ext_vla_folded_to_constant);
+    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);
+    Diag(Loc, diag::err_typecheck_negative_array_size);
   else if (Oversized.getBoolValue())
-    S.Diag(Loc, diag::err_array_too_large) << Oversized.toString(10);
+    Diag(Loc, diag::err_array_too_large) << Oversized.toString(10);
   else if (FailedFoldDiagID)
-    S.Diag(Loc, FailedFoldDiagID);
+    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
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
+  // If this variable has a VLA 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(),
+  if (D.hasInitializer() && R->isVariableArrayType())
+    tryToFixVariablyModifiedVarType(TInfo, R, D.getIdentifierLoc(),
                                     /*DiagID=*/0);
 
   bool IsMemberSpecialization = false;
   bool IsVariableTemplateSpecialization = false;
   bool IsPartialSpecialization = false;
   bool IsVariableTemplate = false;
   VarDecl *NewVD = nullptr;
   VarTemplateDecl *NewTemplate = nullptr;
if (!InvalidDecl && getLangOpts().CPlusPlus && !II && BitWidth &&
     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()) {
     if (!tryToFixVariablyModifiedVarType(
-            *this, TInfo, T, Loc, diag::err_typecheck_field_variable_size))
+            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;
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()) {
     if (!tryToFixVariablyModifiedVarType(
-            *this, TInfo, T, Loc, diag::err_typecheck_ivar_variable_size))
+            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.

clang/lib/Sema/SemaExpr.cpp

Sema::BuildCompoundLiteralExpr(SourceLocation LParenLoc, TypeSourceInfo *TInfo,
   QualType literalType = TInfo->getType();
 
   if (literalType->isArrayType()) {
     if (RequireCompleteSizedType(
             LParenLoc, Context.getBaseElementType(literalType),
             diag::err_array_incomplete_or_sizeless_type,
             SourceRange(LParenLoc, LiteralExpr->getSourceRange().getEnd())))
       return ExprError();
-    if (literalType->isVariableArrayType())
-      return ExprError(Diag(LParenLoc, diag::err_variable_object_no_init)
-        << SourceRange(LParenLoc, LiteralExpr->getSourceRange().getEnd()));
+    if (literalType->isVariableArrayType()) {
+      if (!tryToFixVariablyModifiedVarType(TInfo, literalType, LParenLoc,
+                                           diag::err_variable_object_no_init)) {
+        return ExprError();
+      }
+    }
   } else if (!literalType->isDependentType() &&
              RequireCompleteType(LParenLoc, literalType,
                diag::err_typecheck_decl_incomplete_type,
                SourceRange(LParenLoc, LiteralExpr->getSourceRange().getEnd())))
     return ExprError();
 
   InitializedEntity Entity
     = InitializedEntity::InitializeCompoundLiteralInit(TInfo);

clang/test/Sema/vla.c

jump_over_a4:;
 
   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}};
+
+  // expected-warning@+1{{variable length array folded to constant array as an extension}}
+  char (*a9)[] = (char[2][ksize]) {{1,2,3,4},{4,3,2,1}};
+
+  char (*a10)[ksize] = 0;
 }