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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
