[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

2543–2547

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.

2553–2559

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

34

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/docs/ReleaseNotes.rst

- ``-Winteger-overflow`` will diagnose overflow in more cases. This fixes
   `Issue 58944 <https://github.com/llvm/llvm-project/issues/58944>`_.
 - Clang has an internal limit of 2GB of preprocessed source code per
   compilation, including source reachable through imported AST files such as
   PCH or modules. When Clang hits this limit, it now produces notes mentioning
   which header and source files are consuming large amounts of this space.
   ``#pragma clang __debug sloc_usage`` can also be used to request this report.
 - Clang no longer permits the keyword 'bool' in a concept declaration as a
   concepts-ts compatibility extension.
+- Added ``-Wvla-stack-allocation`` to only give warnings about VLA that need 
+  stack allocation.
+
+  .. code-block:: C
+
+    void f(int n) {
+      int a[n]; // VLA that needs stack allocation
+    }
+
+    void g(int n, int a[n]) {} // VLA that don't need stack allocation
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 - It's now possible to set the crash diagnostics directory through
   the environment variable ``CLANG_CRASH_DIAGNOSTICS_DIR``.
   The ``-fcrash-diagnostics-dir`` flag takes precedence.
 - When using header modules, inclusion of a private header and violations of
   the `use-declaration rules

clang/include/clang/Basic/DiagnosticGroups.td

 def PropertyAccessDotSyntax: DiagGroup<"property-access-dot-syntax">;
 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 VLAPortability : DiagGroup<"vla-portability">;
+def VLAStackAlloc : DiagGroup<"vla-stack-alloc">;
+def VLA : DiagGroup<"vla", [VLAExtension, VLAPortability, VLAStackAlloc]>;
 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">,
-  InGroup<VLA>, DefaultIgnore;
+def warn_vla_portability : Warning<"variable length array used">,
+  InGroup<VLAPortability>, DefaultIgnore;
+def warn_vla_stack_alloc : Warning<"variable length array that may require stack allocation used">,
+  InGroup<VLAStackAlloc>, 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/SemaDecl.cpp

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 (R->isVariableArrayType())
+    Diag(D.getIdentifierLoc(), diag::warn_vla_stack_alloc);
+
   // 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->isVariableArrayType())
     tryToFixVariablyModifiedVarType(TInfo, R, D.getIdentifierLoc(),
                                     /*DiagID=*/0);
 
   bool IsMemberSpecialization = false;
   bool IsVariableTemplateSpecialization = false;

clang/lib/Sema/SemaType.cpp

QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
   // VLAs always produce at least a -Wvla 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;
+    VLADiag = diag::warn_vla_portability;
     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 {
     VLADiag = diag::ext_vla;
     VLAIsError = false;
   }
 
   llvm::APSInt ConstVal(Context.getTypeSize(Context.getSizeType()));
   if (!ArraySize) {
     if (ASM == ArrayType::Star) {
       Diag(Loc, VLADiag);
       if (VLAIsError)
         return QualType();
 
       T = Context.getVariableArrayType(T, nullptr, ASM, Quals, Brackets);
     } else {
       T = Context.getIncompleteArrayType(T, ASM, Quals);
     }
   } else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) {
     T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets);
   } else {

clang/test/C/drs/dr3xx.c

  * Definition of variably modified types
  */
 void dr311(int x) {
   typedef int vla[x]; /* expected-warning {{variable length array}} */
 
   /* Ensure that a constant array of variable-length arrays are still
    * considered a variable-length array.
    */
-  vla y[3]; /* expected-warning {{variable length array}} */
+  vla y[3]; /* expected-warning {{variable length array}} 
+               expected-warning {{variable length array that may require stack allocation used}}
+            */
 }
 
 /* WG14 DR313: yes
  * Incomplete arrays of VLAs
  */
 void dr313(int i) {
   int c[][i] = { 0 }; /* expected-error {{variable-sized object may not be initialized}}
                          expected-warning {{variable length array}}
typedef int dr320_t[dr320_v]; /* c89only-warning {{variable length arrays are a C99 feature}}
                                  c99andup-warning {{variable length array used}}
                                */
 void dr320(int okay[dr320_v]) { /* c89only-warning {{variable length arrays are a C99 feature}}
                                    c99andup-warning {{variable length array used}}
                                  */
   typedef int type[dr320_v]; /* c89only-warning {{variable length arrays are a C99 feature}}
                                 c99andup-warning {{variable length array used}}
                               */
-  extern type bad;  /* expected-error {{variable length array declaration cannot have 'extern' linkage}} */
+  extern type bad;  /* expected-error {{variable length array declaration cannot have 'extern' linkage}} 
+                       expected-warning {{variable length array that may require stack allocation used}}
+                    */
 
   /* C99 6.7.5.2p2, second sentence. */
-  static type fine; /* expected-error {{variable length array declaration cannot have 'static' storage duration}} */
+  static type fine; /* expected-error {{variable length array declaration cannot have 'static' storage duration}} 
+                       expected-warning {{variable length array that may require stack allocation used}}
+                    */
 }
 
 /* WG14 DR321: yes
  * Wide character code values for members of the basic character set
  */
 #define DR321 (\
     ' ' == L' ' && '\t' == L'\t' && '\v' == L'\v' && '\r' == L'\r' &&           \
     '\n' == L'\n' &&                                                            \

clang/test/Sema/cast.c

 // RUN: %clang_cc1 -fsyntax-only -triple x86_64-unknown-unknown %s -verify -Wvla
 
 int array[(long)(char *)0]; // expected-warning {{variable length array used}} \
                             // expected-warning {{variable length array folded to constant array as an extension}} \
-                            // expected-note {{this conversion is not allowed in a constant expression}}
+                            // expected-note {{this conversion is not allowed in a constant expression}} \
+                            // expected-warning {{variable length array that may require stack allocation used}}
 
 typedef struct { unsigned long bits[(((1) + (64) - 1) / (64))]; } cpumask_t;
 cpumask_t x;
 void foo(void) {
   (void)x;
 }
 void bar(void) {
   char* a;

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=c89 -fsyntax-only -verify=expected,c89 -Wvla %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify=expected,c99 -Wvla %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify=expected,only-stack-allocation -Wvla-stack-alloc -Wno-vla-portability %s
 
 void test1(int n) {
-  int v[n]; // expected-warning {{variable length array}}
+  int v[n]; /* 
+    c89-warning {{variable length arrays are a C99 feature}}
+    c89-warning {{variable length array that may require stack allocation used}}
+    c99-warning {{variable length array used}}
+    c99-warning {{variable length array that may require stack allocation 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]) { /* 
+  c89-warning {{variable length arrays are a C99 feature}}
+  c99-warning {{variable length array used}}
+*/
 }
 
-void test3(int n, int v[n]); // expected-warning {{variable length array}}
+void test3(int n, int v[n]); /* 
+  c89-warning {{variable length arrays are a C99 feature}}
+  c99-warning {{variable length array used}}
+*/
 
+int test4_num;
+typedef int Test4[test4_num]; /* 
+  c89-warning {{variable length arrays are a C99 feature}} 
+  c99-warning {{variable length array used}}
+  expected-error {{variable length array declaration not allowed at file scope}}
+*/
+
+void func() {
+  typedef int test5[test4_num]; /* 
+    c89-warning {{variable length arrays are a C99 feature}} 
+    c99-warning {{variable length array used}} 
+  */
+
+  int test6[test4_num]; /*  
+    c89-warning {{variable length arrays are a C99 feature}}
+    c89-warning {{variable length array that may require stack allocation used}}
+    c99-warning {{variable length array used}}
+    c99-warning {{variable length array that may require stack allocation used}}
+    only-stack-allocation-warning {{variable length array that may require stack allocation used}}
+  */
+
+  // FIXME: warn twice
+  (void)sizeof(int[test4_num]); /* 
+    c89-warning {{variable length arrays are a C99 feature}}
+    c89-warning {{variable length arrays are a C99 feature}}
+    c99-warning {{variable length array used}}
+    c99-warning {{variable length array used}}
+  */
+
+  int (*test7)[test4_num]; /*
+    c89-warning {{variable length arrays are a C99 feature}}
+    c99-warning {{variable length array used}}
+  */
+}

clang/test/SemaCXX/warn-vla.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wvla %s
 
 void test1(int n) { // expected-note {{here}}
-  int v[n]; // expected-warning {{variable length array}} expected-note {{parameter 'n'}}
+  int v[n]; /* expected-warning {{variable length array}} 
+               expected-note {{parameter 'n'}}
+               expected-warning {{variable length array that may require stack allocation used}}
+            */
 }
 
 void test2(int n, int v[n]) { // expected-warning {{variable length array}} expected-note {{parameter 'n'}} expected-note {{here}}
 }
 
 void test3(int n, int v[n]); // expected-warning {{variable length array}} expected-note {{parameter 'n'}} expected-note {{here}}
 
 template<typename T>
 void test4(int n) { // expected-note {{here}}
-  int v[n]; // expected-warning {{variable length array}} expected-note {{parameter 'n'}}
+  int v[n]; /* expected-warning {{variable length array}} 
+               expected-note {{parameter 'n'}}
+               expected-warning {{variable length array that may require stack allocation used}}
+            */
 }
 
 template<typename T>
 void test5(int n, int v[n]) { // expected-warning {{variable length array}} expected-note {{parameter 'n'}} expected-note {{here}}
 }
 
 template<typename T>
 void test6(int n, int v[n]); // expected-warning {{variable length array}} expected-note {{parameter 'n'}} expected-note {{here}}
 
 template<typename T>
 void test7(int n, T v[n]) { // expected-warning {{variable length array}} expected-note {{parameter 'n'}} expected-note {{here}}
 }