[PATCH] D137343: [clang] add -Wvla-stack-allocation

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: clang-language-wg, rjmccall.
aaron.ballman added a comment.

In D137343#3978313 <https://reviews.llvm.org/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...



================
Comment at: clang/lib/Sema/SemaType.cpp:2545-2549
+      // 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;
----------------
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.


================
Comment at: clang/lib/Sema/SemaType.cpp:2555-2561
+      // 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;
----------------
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.


================
Comment at: clang/test/Sema/warn-vla.c:33
+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}}
----------------
For example, there's no stack allocation happening here; `test4_num` is being evaluated but not to form an allocation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137343/new/

https://reviews.llvm.org/D137343