[clang] add -Wvla-stack-allocation

[Origami404]

Summary

New behaviors:

-Wvla: produces portability warning for all VLAs like before
-Wvla-portability: not exists (equal to -Wvla)
-Wvla-stack-allocation -Wno-vla: only warns for VLAs that need stack allocation
-Wvla -Wvla-stack-allocation: warn for VLAs that need stack allocation, and give portablility warnings for other VLAs

Note that only one warning can be produced by one VLA because of
implementation complexity. Currently, VLAs are detected in
BuildArrayType, which will be used both by Parser and Tree Transformer.
And it passes the VLA-related warnings as an argument to other
functions. Producing multiple warnings for one VLA needs to change many
functions.

Fixes: https://github.com/llvm/llvm-project/issues/57098
Link: https://reviews.llvm.org/D132952

Co-authored-by: YingChi Long <[e-mail]>

[Origami404]

ping :)

[inclyc]

We may need a release note here so users know about the new changes to -Wvla. See clang/docs/ReleaseNotes.rst!

[Origami404]

Update release notes.

[inclyc]

ping :)

[aaron.ballman]

In D137343#3978313, @inclyc wrote:

ping :)

Thank you for your patience until I could get to this! I've added a few more reviewers for additional opinions on the correct design. I think what you have here is kind of close to what I was expecting, but with differences in the warning flags we expose. I had the impression we were looking for:

-Wvla-stack-allocation -- warns on use of a VLA that involves a stack allocation
-Wvla-portability -- warns on any use of a VM type, except if -Wvla-stack-allocation is also enabled it won't warn on use of VLA that involves a stack allocation (because the other warning already alerts the user to the portability concern).
-Wvla -- groups together -Wvla-stack-allocation and -Wvla-portability so people can turn on all VLA-related diagnostics at once.

The end result is that the use of -Wvla is the same as it's always been, but users can decide to use one of the more fine-grained diagnostics if they want to reduce the scope.

That said, based on the discussion in https://github.com/llvm/llvm-project/issues/59010, there may actually be a third kind of diagnostic we want to consider as well: -Wvla-bounds-[not]-evaluated that triggers on code like:

int foo(void);

void bar(void) {
  sizeof(int[foo()]); // foo is called
  sizeof(sizeof(int[foo()])); // foo is not called
}

void baz(int what[foo()]); // foo is not called
void baz(int what[foo()]) { // foo is called
  extern void whee(int what[foo()]); // foo is not called
}

If this is worth doing, it's probably best done in a separate patch though...

clang/lib/Sema/SemaType.cpp

2545–2549

FWIW, I don't think this is a behavior we need to retain. I think we issued that warning because it was easier than suppressing it, but the error diagnostic suffices to let the user know what's gone wrong in this particular case.

2555–2561

I'm not convinced the logic here is correct yet. Consider cases like:

int foo(void);

void bar(int a, int b[*]); // variable length array used, correct
void bar(int a, int b[a]) { variable length array used, correct
  int x[foo()]; // variable length array that may require stack allocation used, correct

  (void)sizeof(int[foo()]); // variable length array that may require stack allocation used, incorrect and the diagnostic triggers twice

  int (*y)[foo()]; // variable length array that may require stack allocation used, incorrect, this is a pointer to a VLA
}

One thing that may help is to double-check your test cases against the LLVM IR generated by the compiler; if there's an actual stack allocation, then there will be an alloca call in the IR for it.

clang/test/Sema/warn-vla.c

33

For example, there's no stack allocation happening here; test4_num is being evaluated but not to form an allocation.

[Origami404]

int foo(void);

void bar(int a, int b[*]); // variable length array used, correct
void bar(int a, int b[a]) { variable length array used, correct
  int x[foo()]; // variable length array that may require stack allocation used, correct

  (void)sizeof(int[foo()]); // variable length array that may require stack allocation used, incorrect and the diagnostic triggers twice

  int (*y)[foo()]; // variable length array that may require stack allocation used, incorrect, this is a pointer to a VLA
}

Thanks to ballman's example here, it remains me that when non-LCE appears as an array index in a variable declaration, it is still possible to be a part of a pointer/function prototype instead of a VLA. Classifying different VLA usages requires more information about the declaration.

The old VLA checking implements just insert checking for the location in array type construction when a non-ICE is used as the array's length. And it assumes that only one error or warning could be produced by one VLA usage. So the old method seems hard to be extended to support the new checking on VLAs.

I plan to throw away the old method here, trying to use "eval functions" on AST of declarations or LLVM IR to classify different kinds of VLAs.

Since I am preparing for several final examinations next week, I will try to give a new implementation in about a week. And in advance, sorry for my late response next week.

[uecker]

I would still suggest to change the behavior of -Wvla to warn only about VLAs which are allocated on the stack as the most useful warning. Portability warnings should be turned on for -Wc++-compat.

[Origami404]

[clang] add -Wvla-stack-allocation and divide different VLA warning

New behaviors:

-Wvla-stack-allocation

warns on use of a VLA that involves a stack allocation

-Wvla-portability

warns on any use of a VM type, even if -Wvla-stack-allocation is
given.

-Wvla

groups together -Wvla-stack-allocation and -Wvla-portability

Fixes: https://github.com/llvm/llvm-project/issues/57098
Link: https://reviews.llvm.org/D132952

Co-authored-by: YingChi Long <[e-mail]>

Differential Revision: https://reviews.llvm.org/D137343

[Origami404]

Hello, I am back now and will be available anytime next week, so if anyone has any idea on this topic, please at me!

I haven't found a way that "feels right" to achieve consensus yet. But I do have something to share currently.

---

For the semantics of the warning flag, I agree with @uecker that -Wvla-portability should be changed to -Wc++-compat. In my reading of C2x standard, using VM type is acceptable and the only reason we need to issue a warning is that the C++ standard doesn't allow non-constant array length at all. But I have no opinion on the semantics of -Wvla and -Wvla-stack-alloc.

And should -Wvla-stack-alloc fires on static VLAs? (like clang/test/drs/dr3xx.c:164)

---

For implement details, I add a diag at ``, which will be called after a variable declaration. Here we can get the full type of the variable to tell whether the variable has a VLA type or just a VM type.

In the GitHub issue, @mizvekov said:

A general check about any VLA usage at all should probably happen early when we finished parsing and are building VariableArray types, probably in Sema::BuildArrayType.

Otherwise sprinkling around checks for VM types seems messy, so I don't think considering VM for this problem is an efficient approach.

But AFAIK, a VLA type cannot be told just at Sema::BuildArrayType level because the constructing VLA type may just be a part of another VM type but not a VLA type for a VLA. At Sema::BuildArrayType, we seem only can detect VM type instead of VLA type.

---

In Ballman's summary, the -Wvla-stack-alloc should suppress the -Wvla-portability:

-Wvla-portability warns on any use of a VM type, except if -Wvla-stack-allocation is also enabled it won't warn on the use of VLA that involves a stack allocation

I failed to achieve the "except" here because, in the current implementation, VM type-related warnings are checked directly at Sema::BuildArrayType, by checkArraySize which used Sema::VerifyIntegerConstantExpression. But the Sema::VerifyIntegerConstantExpression requires at least one diag at each non-ICE occurrence. I haven't found a way to avoid the diag yet.

---

By the way, I seem to find a bug in the current implementation of VM type warning. For VLA at sizeof context, the warning will be issued twice. Github issue here. It cause the test clang/test/Sema/warn-vla.c:48 giving two warnings.

---

The reason why I set both -Wvla-stack-alloc and -Wvla-portability to DefaultIgnore is that:

• if both of them are not DefaultIgnore, 200 tests will fail
• if only -Wvla-stack-alloc is not DefaultIgnore, 169 tests will fail
• if both of them are DefaultIgnore, only 4 tests will fail

Most of the failed tests seem cannot be avoided because they just use VLAs. If DefaultIgnore is strongly unrecommended, I can go through all the tests and add an expected warning diag for them. It's a bit time-consuming, but I am free all day now.

[rjmccall]

Aaron's suggested design makes the most sense to me, of all the designs I've seen here.

clang/include/clang/Basic/DiagnosticGroups.td

 def PropertyAttr : DiagGroup<"property-attribute-mismatch">;
 def SuperSubClassMismatch : DiagGroup<"super-class-method-mismatch">;
 def OverridingMethodMismatch : DiagGroup<"overriding-method-mismatch">;
 def VariadicMacros : DiagGroup<"variadic-macros">;
 def VectorConversion : DiagGroup<"vector-conversion">;      // clang specific
 def VexingParse : DiagGroup<"vexing-parse">;
 def VLAExtension : DiagGroup<"vla-extension">;
 def VLA : DiagGroup<"vla", [VLAExtension]>;
+def VLAStackAllocation : DiagGroup<"vla-stack-allocation", [VLA]>;
 def VolatileRegisterVar : DiagGroup<"volatile-register-var">;
 def Visibility : DiagGroup<"visibility">;
 def ZeroLengthArray : DiagGroup<"zero-length-array">;
 def GNUZeroLineDirective : DiagGroup<"gnu-zero-line-directive">;
 def GNUZeroVariadicMacroArguments : DiagGroup<"gnu-zero-variadic-macro-arguments">;
 def MisleadingIndentation : DiagGroup<"misleading-indentation">;
 
 // This covers both the deprecated case (in C++98)

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_double_const_requires_fp64 : Warning<
   "double precision constant requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0, "
   "casting to single precision">;
 def err_half_const_requires_fp16 : Error<
   "half precision constant requires cl_khr_fp16">;
 
 // C99 variable-length arrays
 def ext_vla : Extension<"variable length arrays are a C99 feature">,
   InGroup<VLAExtension>;
-def warn_vla_used : Warning<"variable length array used">,
+def warn_vla_portability : Warning<"variable length array used">,
   InGroup<VLA>, DefaultIgnore;
+def warn_vla_stack_allocation : Warning<"variable length array that may require stack allocation used">,
+  InGroup<VLAStackAllocation>, DefaultIgnore;
 def err_vla_in_sfinae : Error<
   "variable length array cannot be formed during template argument deduction">;
 def err_array_star_in_function_definition : Error<
   "variable length array must be bound in function definition">;
 def err_vla_decl_in_file_scope : Error<
   "variable length array declaration not allowed at file scope">;
 def err_vla_decl_has_static_storage : Error<
   "variable length array declaration cannot have 'static' storage duration">;

clang/lib/Sema/SemaType.cpp

 #include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeLocVisitor.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Lookup.h"
QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
   if (!getLangOpts().CPlusPlus11 &&
       ArraySize && !ArraySize->isTypeDependent() &&
       !ArraySize->getType()->isIntegralOrUnscopedEnumerationType()) {
     Diag(ArraySize->getBeginLoc(), diag::err_array_size_non_int)
         << ArraySize->getType() << ArraySize->getSourceRange();
     return QualType();
   }
 
-  // VLAs always produce at least a -Wvla diagnostic, sometimes an error.
+  // VLAs always produce at least a -Wvla-portability or -Wvla-stack-allocation
+  // diagnostic, sometimes an error.
   unsigned VLADiag;
   bool VLAIsError;
   if (getLangOpts().OpenCL) {
     // OpenCL v1.2 s6.9.d: variable length arrays are not supported.
     VLADiag = diag::err_opencl_vla;
     VLAIsError = true;
   } else if (getLangOpts().C99) {
-    VLADiag = diag::warn_vla_used;
+    // after C99, VLA is no longer an extension.
+    if (getCurScope()->getFlags() == Scope::ScopeFlags::DeclScope) {
+      // VLA in file scope typedef will have an error, and should not have a
+      // warning of portability. But for backward compatibility, we preform the
+      // exact same action like before (which will give an warning of "vla
+      // used").
+      VLADiag = diag::warn_vla_portability;
+    } else if (getCurScope()->isFunctionPrototypeScope()) {
+      // VLA in function prototype is acceptable by C2x standard
+      // so just give a protability warning
+      VLADiag = diag::warn_vla_portability;
+    } else {
+      // in other case, a VLA will cause stack allocation
+      // if -Wvla-stack-allocation is ignored, fallback to
+      // -Wvla-protability
+      if (getDiagnostics().isIgnored(diag::warn_vla_stack_allocation, Loc))
+        VLADiag = diag::warn_vla_portability;
+      else
+        VLADiag = diag::warn_vla_stack_allocation;
+    }
     VLAIsError = false;
   } else if (isSFINAEContext()) {
     VLADiag = diag::err_vla_in_sfinae;
     VLAIsError = true;
   } else if (getLangOpts().OpenMP && isInOpenMPTaskUntiedContext()) {
     VLADiag = diag::err_openmp_vla_in_task_untied;
     VLAIsError = true;
   } else {
+    // Before C99 and in most of platform, VLA is an extension.
     VLADiag = diag::ext_vla;
     VLAIsError = false;
   }
 
   llvm::APSInt ConstVal(Context.getTypeSize(Context.getSizeType()));
   if (!ArraySize) {
     if (ASM == ArrayType::Star) {
       Diag(Loc, VLADiag);

clang/test/Sema/warn-vla.c

-// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -Wvla %s
-// RUN: %clang_cc1 -std=c89 -fsyntax-only -verify -Wvla %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify=expected,no-fallback-c99 -Wvla -Wvla-stack-allocation %s
+// RUN: %clang_cc1 -std=c89 -fsyntax-only -verify=expected,no-fallback-c89 -Wvla -Wvla-stack-allocation %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify=expected,fallback -Wvla %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify=expected,only-stack-allocation -Wvla-stack-allocation -Wno-vla %s
 
 void test1(int n) {
-  int v[n]; // expected-warning {{variable length array}}
+  int v[n]; /* no-fallback-c89-warning {{variable length arrays are a C99 feature}}
+               no-fallback-c99-warning {{variable length array that may require stack allocation used}}
+               fallback-warning {{variable length array used}}
+               only-stack-allocation-warning {{variable length array that may require stack allocation used}}
+            */
 }
 
-void test2(int n, int v[n]) { // expected-warning {{variable length array}}
+void test2(int n, int v[n]) { /* no-fallback-c89-warning {{variable length arrays are a C99 feature}}
+                                 no-fallback-c99-warning {{variable length array used}}
+                                 fallback-warning {{variable length array used}}
+                              */
 }
 
-void test3(int n, int v[n]); // expected-warning {{variable length array}}
+void test3(int n, int v[n]); /* no-fallback-c89-warning {{variable length arrays are a C99 feature}}
+                                no-fallback-c99-warning {{variable length array used}}
+                                fallback-warning {{variable length array used}}
+                              */
 
+int test4_num;
+typedef int Test4[test4_num]; /* no-fallback-c89-warning {{variable length arrays are a C99 feature}} 
+                                 no-fallback-c99-warning {{variable length array used}}
+                                 fallback-warning {{variable length array used}}
+                                 expected-error {{variable length array declaration not allowed at file scope}}
+                              */
+
+void test5() {
+  typedef int type[test4_num]; /* no-fallback-c89-warning {{variable length arrays are a C99 feature}} 
+                                  no-fallback-c99-warning {{variable length array that may require stack allocation used}}
+                                  fallback-warning {{variable length array used}}
+                                  only-stack-allocation-warning {{variable length array that may require stack allocation used}}
+                                */
+
+}