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

[Tao Liang via Phabricator via cfe-commits]

Origami404 added a subscriber: mizvekov.
Origami404 added a comment.

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 <https://github.com/llvm/llvm-project/issues/57098#issuecomment-1265130710>, @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 <https://github.com/llvm/llvm-project/issues/59570>. 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137343