[PATCH] D156565: Diagnose use of VLAs in C++ by default

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156565#4654818 <https://reviews.llvm.org/D156565#4654818>, @aaron.ballman wrote:

> In D156565#4654773 <https://reviews.llvm.org/D156565#4654773>, @nathanchance wrote:
>
>> Is it expected that this introduces a warning for C code, as the commit message and tests appear to only affect C++? A trivial example from the Linux kernel:
>>
>> https://elixir.bootlin.com/linux/v6.5.8/source/tools/lib/bpf/btf_dump.c#L1678
>>
>>   #include <stddef.h>
>>   #include <stdio.h>
>>   #include <string.h>
>>   
>>   void foo(char *orig_name, char **cached_name, size_t dup_cnt)
>>   {
>>           const size_t max_len = 256;
>>           char new_name[max_len];
>>   
>>           snprintf(new_name, max_len, "%s___%zu", orig_name, dup_cnt);
>>           *cached_name = strdup(new_name);
>>   }
>>
>>
>>
>>   $ clang -std=gnu89 -Wall -fsyntax-only test.c
>>   test.c:8:16: warning: variable length arrays are a C99 feature [-Wvla-extension]
>>       8 |         char new_name[max_len];
>>         |                       ^~~~~~~
>>   1 warning generated.
>
> No, that's unintended, I'll get that fixed. Thanks for letting me know!

Unfortunately, this will require complicating the diagnostic groups slightly more. We don't have facilities that allow us to say that a single diagnostic is grouped under `-Wall` in one language mode but not another, and we don't have a way for diagnostic groups to share the same string (so we can't have `def VLACxxExtension : DiagGroup<"vla-extension", [VLAUseStaticAssert]>;` and `def VLAExtension : DiagGroup<"vla-extension", [VLACxxExtension]>;`).

I think I will address this by adding `-Wvla-cxx-extension` and putting the C++ warnings under it, and that warning group will be added to `-Wall`. e.g.,

  diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
  index dcdae38013d2..4cb792132d6e 100644
  --- a/clang/include/clang/Basic/DiagnosticGroups.td
  +++ b/clang/include/clang/Basic/DiagnosticGroups.td
  @@ -849,7 +849,8 @@ def VariadicMacros : DiagGroup<"variadic-macros">;
   def VectorConversion : DiagGroup<"vector-conversion">;      // clang specific
   def VexingParse : DiagGroup<"vexing-parse">;
   def VLAUseStaticAssert : DiagGroup<"vla-extension-static-assert">;
  -def VLAExtension : DiagGroup<"vla-extension", [VLAUseStaticAssert]>;
  +def VLACxxExtension : DiagGroup<"vla-cxx-extension", [VLAUseStaticAssert]>;
  +def VLAExtension : DiagGroup<"vla-extension", [VLACxxExtension]>;
   def VLA : DiagGroup<"vla", [VLAExtension]>;
   def VolatileRegisterVar : DiagGroup<"volatile-register-var">;
   def Visibility : DiagGroup<"visibility">;
  @@ -1086,7 +1087,8 @@ def Consumed       : DiagGroup<"consumed">;
   // warning should be active _only_ when -Wall is passed in, mark it as
   // DefaultIgnore in addition to putting it here.
   def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool,
  -                            MisleadingIndentation, PackedNonPod, VLAExtension]>;
  +                            MisleadingIndentation, PackedNonPod,
  +                            VLACxxExtension]>;
  
   // Warnings that should be in clang-cl /w4.
   def : DiagGroup<"CL4", [All, Extra]>;
  diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
  index a4c1cb08de94..3bcbb003d6de 100644
  --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
  +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
  @@ -141,9 +141,9 @@ def ext_vla : Extension<"variable length arrays are a C99 feature">,
   // language modes, we warn as an extension but add the warning group to -Wall.
   def ext_vla_cxx : ExtWarn<
     "variable length arrays in C++ are a Clang extension">,
  -  InGroup<VLAExtension>;
  +  InGroup<VLACxxExtension>;
   def ext_vla_cxx_in_gnu_mode : Extension<ext_vla_cxx.Summary>,
  -  InGroup<VLAExtension>;
  +  InGroup<VLACxxExtension>;
   def ext_vla_cxx_static_assert : ExtWarn<
     "variable length arrays in C++ are a Clang extension; did you mean to use "
     "'static_assert'?">, InGroup<VLAUseStaticAssert>;

Any concerns with this approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565