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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156565#4654820 , @cor3ntin wrote:

>> Any concerns with this approach?
>
> Sounds reasonable to me

Thanks for the double-check! This should now be fixed in 
https://github.com/llvm/llvm-project/commit/f8d448d5e587a23886c3226957f880146a4d8c69,
 sorry for the hassle!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

> Any concerns with this approach?

Sounds reasonable to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @aaron.ballman 
wrote:

> In 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 
>>   #include 
>>   #include 
>>   
>>   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;
  +  InGroup;
   def ext_vla_cxx_in_gnu_mode : Extension,
  -  InGroup;
  +  InGroup;
   def ext_vla_cxx_static_assert : ExtWarn<
 "variable length arrays in C++ are a Clang extension; did you mean to use "
 "'static_assert'?">, InGroup;

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In 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 
>   #include 
>   #include 
>   
>   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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

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 
  #include 
  #include 
  
  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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7339c0f782d5: Diagnose use of VLAs in C++ by default 
(authored by aaron.ballman).

Changed prior to commit:
  https://reviews.llvm.org/D156565?vs=557575&id=557810#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/lambdas.cpp
  clang/test/CXX/basic/basic.types/p10.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.type/p2.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp
  clang/test/Misc/warning-wall.c
  clang/test/OpenMP/debug-info-openmp-array.cpp
  clang/test/OpenMP/for_reduction_codegen.cpp
  clang/test/OpenMP/for_reduction_codegen_UDR.cpp
  clang/test/OpenMP/master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/parallel_ast_print.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_for_codegen.cpp
  clang/test/OpenMP/parallel_masked_ast_print.cpp
  clang/test/OpenMP/parallel_master_ast_print.cpp
  clang/test/OpenMP/parallel_master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_constant_device_codegen.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_data_use_device_addr_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen_01.cpp
  clang/test/OpenMP/target_defaultmap_messages.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_firstprivate_codegen.cpp
  clang/test/OpenMP/target_has_device_addr_codegen_01.cpp
  clang/test/OpenMP/target_in_reduction_codegen.cpp
  clang/test/OpenMP/target_map_codegen_12.cpp
  clang/test/OpenMP/target_map_codegen_18a.cpp
  clang/test/OpenMP/target_map_codegen_18b.cpp
  clang/test/OpenMP/target_map_codegen_18c.cpp
  clang/test/OpenMP/target_map_codegen_18d.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_defaultmap_messages.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_generic_loop_depend_codegen.cpp
  clang/test/OpenMP/target_private_codegen.cpp
  clang/test/OpenMP/target_reduction_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_dis

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Ping


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 557575.
aaron.ballman added a comment.

Updated based on review feedback. Specifically:

- Added test cases, fixed handling of parenthesized expressions.


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

https://reviews.llvm.org/D156565

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/lambdas.cpp
  clang/test/CXX/basic/basic.types/p10.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.type/p2.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp
  clang/test/Misc/warning-wall.c
  clang/test/OpenMP/debug-info-openmp-array.cpp
  clang/test/OpenMP/for_reduction_codegen.cpp
  clang/test/OpenMP/for_reduction_codegen_UDR.cpp
  clang/test/OpenMP/master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/parallel_ast_print.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_for_codegen.cpp
  clang/test/OpenMP/parallel_masked_ast_print.cpp
  clang/test/OpenMP/parallel_master_ast_print.cpp
  clang/test/OpenMP/parallel_master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_constant_device_codegen.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_data_use_device_addr_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen_01.cpp
  clang/test/OpenMP/target_defaultmap_messages.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_firstprivate_codegen.cpp
  clang/test/OpenMP/target_has_device_addr_codegen_01.cpp
  clang/test/OpenMP/target_in_reduction_codegen.cpp
  clang/test/OpenMP/target_map_codegen_12.cpp
  clang/test/OpenMP/target_map_codegen_18a.cpp
  clang/test/OpenMP/target_map_codegen_18b.cpp
  clang/test/OpenMP/target_map_codegen_18c.cpp
  clang/test/OpenMP/target_map_codegen_18d.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_defaultmap_messages.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_generic_loop_depend_codegen.cpp
  clang/test/OpenMP/target_private_codegen.cpp
  clang/test/OpenMP/target_reduction_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_schedule_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_codegen.cpp
  clang/test/OpenMP/target_team

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as not done.
aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+  VLADiag = getLangOpts().GNUMode

jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > aaron.ballman wrote:
> > > > jyknight wrote:
> > > > > Not sure whether to actually care, since C++98 is so old now, but, 
> > > > > having `-Wno-vla-extension-static-assert` work in C++98 mode seems 
> > > > > like it'd be quite useful, exactly because the usage cannot trivially 
> > > > > be replaced by a static_assert. So it's a bit unfortunate that we 
> > > > > don't distinguish it there.
> > > > > 
> > > > > Perhaps we should emit the same diagnostics in both 98/11, but with 
> > > > > slightly different text in 98?
> > > > That's effectively what we're doing here, right?
> > > > 
> > > > C++98 is told "variable length arrays (in C++) are a Clang extension" 
> > > > and in C++11 they're told "variable length arrays (in C++) are a Clang 
> > > > extension; did you mean to use 'static_assert'?"
> > > > 
> > > > Or am I misunderstanding you?
> > > In this patch, in C++98 mode, we diagnose everything under the flag 
> > > -Wvla-extension, and never under the flag -Wvla-extension-static-assert. 
> > > My point was that we ought to diagnose static-assert-like-VLAs under the 
> > > -Wvla-extension-static-assert flag even in C++98 mode.
> > Ah okay! That does make sense but would be hard to pull off. Because the 
> > existing code is based around picking which diagnostic ID to use, all of 
> > the related diagnostic IDs need to take the same kind of streaming 
> > arguments, that makes passing additional arguments to control `%select` 
> > behavior really awkward in this case. So I'd have to add an entire new 
> > diagnostic ID for the C++98 case and I'm not certain it's really worth it. 
> > e.g.
> > ```
> > def ext_vla : Extension<"variable length arrays are a C99 feature">,
> >   InGroup;
> > // In C++ language modes, we warn by default as an extension, while in GNU++
> > // 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;
> > def ext_vla_cxx_in_gnu_mode : Extension,
> >   InGroup;
> > def ext_vla_cxx_static_assert : ExtWarn<
> >   "variable length arrays in C++ are a Clang extension; did you mean to use 
> > "
> >   "'static_assert'?">, InGroup;
> > def ext_vla_cxx_in_gnu_mode_static_assert : Extension<
> >   ext_vla_cxx_static_assert.Summary>, InGroup;
> > def ext_vla_cxx98_static_assert : ExtWarn<
> >   ext_vla_cxx.Summary>, InGroup;
> > def ext_vla_cxx98_in_gnu_mode_static_assert : Extension<
> >   ext_vla_cxx98_static_assert.Summary>, InGroup;
> > def warn_vla_used : Warning<"variable length array used">,
> >   InGroup, DefaultIgnore;
> > ```
> > It's not the end of the world, but do you think it's worth it?
> Yea, that's basically what I meant.
> 
> But, actually, the message is not great in C++11 mode either -- "use 
> static_assert" isn't really what's meant, since the only reason it's 
> diagnosed here at all is because the value (accidentally or intentionally) 
> _isn't_ actually a constant expression and thus wouldn't work under 
> static_assert.
> 
> A message like: `variable length arrays in C++ are a Clang extension; this 
> looks like it was intended to be a pre-C++11 emulation of static_assert, but 
> doesn't have a constant condition` would be correct in both language modes, 
> and maybe be more to the point for the issue.
> 
> But, actually, the message is not great in C++11 mode either -- "use 
> static_assert" isn't really what's meant, since the only reason it's 
> diagnosed here at all is because the value (accidentally or intentionally) 
> _isn't_ actually a constant expression and thus wouldn't work under 
> static_assert.

Good point!!

> A message like: `variable length arrays in C++ are a Clang extension; this 
> looks like it was intended to be a pre-C++11 emulation of static_assert, but 
> doesn't have a constant condition` would be correct in both language modes, 
> and maybe be more to the point for the issue.

Oof, that's a mouthful. Stepping back a bit, I think the only reason we want a 
second diagnostic group here is so people can be alerted to using a VLA while 
turning off diagnostics for static_assert emulation. From that perspective, we 
don't need different wording for the diagnostics so much as different 
diagnostic groups for the same diagnostic wording. e.g.,
```
void old_style_static_assert(int n) {
  int array1[n != 12 ? 1 : -1]; // warning: variable length arrays in C++ are a 
Clang extension [-Wvla-extension-static-assert]
  int array2[n]; // warning: variable length arrays in C++ are a Clang 
extension [-Wvla-extension]
}
```
Do you th

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+  VLADiag = getLangOpts().GNUMode

aaron.ballman wrote:
> jyknight wrote:
> > aaron.ballman wrote:
> > > jyknight wrote:
> > > > Not sure whether to actually care, since C++98 is so old now, but, 
> > > > having `-Wno-vla-extension-static-assert` work in C++98 mode seems like 
> > > > it'd be quite useful, exactly because the usage cannot trivially be 
> > > > replaced by a static_assert. So it's a bit unfortunate that we don't 
> > > > distinguish it there.
> > > > 
> > > > Perhaps we should emit the same diagnostics in both 98/11, but with 
> > > > slightly different text in 98?
> > > That's effectively what we're doing here, right?
> > > 
> > > C++98 is told "variable length arrays (in C++) are a Clang extension" and 
> > > in C++11 they're told "variable length arrays (in C++) are a Clang 
> > > extension; did you mean to use 'static_assert'?"
> > > 
> > > Or am I misunderstanding you?
> > In this patch, in C++98 mode, we diagnose everything under the flag 
> > -Wvla-extension, and never under the flag -Wvla-extension-static-assert. My 
> > point was that we ought to diagnose static-assert-like-VLAs under the 
> > -Wvla-extension-static-assert flag even in C++98 mode.
> Ah okay! That does make sense but would be hard to pull off. Because the 
> existing code is based around picking which diagnostic ID to use, all of the 
> related diagnostic IDs need to take the same kind of streaming arguments, 
> that makes passing additional arguments to control `%select` behavior really 
> awkward in this case. So I'd have to add an entire new diagnostic ID for the 
> C++98 case and I'm not certain it's really worth it. e.g.
> ```
> def ext_vla : Extension<"variable length arrays are a C99 feature">,
>   InGroup;
> // In C++ language modes, we warn by default as an extension, while in GNU++
> // 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;
> def ext_vla_cxx_in_gnu_mode : Extension,
>   InGroup;
> def ext_vla_cxx_static_assert : ExtWarn<
>   "variable length arrays in C++ are a Clang extension; did you mean to use "
>   "'static_assert'?">, InGroup;
> def ext_vla_cxx_in_gnu_mode_static_assert : Extension<
>   ext_vla_cxx_static_assert.Summary>, InGroup;
> def ext_vla_cxx98_static_assert : ExtWarn<
>   ext_vla_cxx.Summary>, InGroup;
> def ext_vla_cxx98_in_gnu_mode_static_assert : Extension<
>   ext_vla_cxx98_static_assert.Summary>, InGroup;
> def warn_vla_used : Warning<"variable length array used">,
>   InGroup, DefaultIgnore;
> ```
> It's not the end of the world, but do you think it's worth it?
Yea, that's basically what I meant.

But, actually, the message is not great in C++11 mode either -- "use 
static_assert" isn't really what's meant, since the only reason it's diagnosed 
here at all is because the value (accidentally or intentionally) _isn't_ 
actually a constant expression and thus wouldn't work under static_assert.

A message like: `variable length arrays in C++ are a Clang extension; this 
looks like it was intended to be a pre-C++11 emulation of static_assert, but 
doesn't have a constant condition` would be correct in both language modes, and 
maybe be more to the point for the issue.



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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149
+def ext_vla_cxx_static_assert : ExtWarn<
+  "variable length arrays in C++ are a Clang extension; did you mean to use "
+  "'static_assert'?">, InGroup;

aaron.ballman wrote:
> tahonermann wrote:
> > I find the "did you mean to use 'static_assert'" phrasing awkward here 
> > since it is highly unlikely that someone intended to write 
> > `static_assert(x)` and instead wrote `int my_assert[x ? 1 : -1]`. Perhaps 
> > something like this?
> > 
> > "variable length arrays in C++ are a Clang extension; 'static_assert' can 
> > be used in this case".
> Eh, I'm on the fence. We're consistent about asking users "did you mean X?" 
> in our diagnostics and this follows the same pattern. "Did you mean to use X" 
> is not "is this a typo for X?" but "were you aware you could do X instead?" 
> So yeah, the wording is a bit awkward, but it's consistent with other 
> diagnostics and not really wrong either. Do you have strong feelings?
No, I don't have strong feelings on this.

I did recognize use of the widely used "did you mean" pattern, but my 
impression with those has been that they are generally employed in situations 
where similar syntax is involved (e.g., misspelled name, `.` vs `->`, `return` 
vs `co_return`, etc...) or where there might be some missing syntax (e.g., `()` 
following a function name or a missing `;`); cases where the programmer is 
likely to response "oh, yes I did!". This feels different since it is 
suggesting use of a distinct feature.


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149
+def ext_vla_cxx_static_assert : ExtWarn<
+  "variable length arrays in C++ are a Clang extension; did you mean to use "
+  "'static_assert'?">, InGroup;

tahonermann wrote:
> I find the "did you mean to use 'static_assert'" phrasing awkward here since 
> it is highly unlikely that someone intended to write `static_assert(x)` and 
> instead wrote `int my_assert[x ? 1 : -1]`. Perhaps something like this?
> 
> "variable length arrays in C++ are a Clang extension; 'static_assert' can be 
> used in this case".
Eh, I'm on the fence. We're consistent about asking users "did you mean X?" in 
our diagnostics and this follows the same pattern. "Did you mean to use X" is 
not "is this a typo for X?" but "were you aware you could do X instead?" So 
yeah, the wording is a bit awkward, but it's consistent with other diagnostics 
and not really wrong either. Do you have strong feelings?



Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+  VLADiag = getLangOpts().GNUMode

jyknight wrote:
> aaron.ballman wrote:
> > jyknight wrote:
> > > Not sure whether to actually care, since C++98 is so old now, but, having 
> > > `-Wno-vla-extension-static-assert` work in C++98 mode seems like it'd be 
> > > quite useful, exactly because the usage cannot trivially be replaced by a 
> > > static_assert. So it's a bit unfortunate that we don't distinguish it 
> > > there.
> > > 
> > > Perhaps we should emit the same diagnostics in both 98/11, but with 
> > > slightly different text in 98?
> > That's effectively what we're doing here, right?
> > 
> > C++98 is told "variable length arrays (in C++) are a Clang extension" and 
> > in C++11 they're told "variable length arrays (in C++) are a Clang 
> > extension; did you mean to use 'static_assert'?"
> > 
> > Or am I misunderstanding you?
> In this patch, in C++98 mode, we diagnose everything under the flag 
> -Wvla-extension, and never under the flag -Wvla-extension-static-assert. My 
> point was that we ought to diagnose static-assert-like-VLAs under the 
> -Wvla-extension-static-assert flag even in C++98 mode.
Ah okay! That does make sense but would be hard to pull off. Because the 
existing code is based around picking which diagnostic ID to use, all of the 
related diagnostic IDs need to take the same kind of streaming arguments, that 
makes passing additional arguments to control `%select` behavior really awkward 
in this case. So I'd have to add an entire new diagnostic ID for the C++98 case 
and I'm not certain it's really worth it. e.g.
```
def ext_vla : Extension<"variable length arrays are a C99 feature">,
  InGroup;
// In C++ language modes, we warn by default as an extension, while in GNU++
// 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;
def ext_vla_cxx_in_gnu_mode : Extension,
  InGroup;
def ext_vla_cxx_static_assert : ExtWarn<
  "variable length arrays in C++ are a Clang extension; did you mean to use "
  "'static_assert'?">, InGroup;
def ext_vla_cxx_in_gnu_mode_static_assert : Extension<
  ext_vla_cxx_static_assert.Summary>, InGroup;
def ext_vla_cxx98_static_assert : ExtWarn<
  ext_vla_cxx.Summary>, InGroup;
def ext_vla_cxx98_in_gnu_mode_static_assert : Extension<
  ext_vla_cxx98_static_assert.Summary>, InGroup;
def warn_vla_used : Warning<"variable length array used">,
  InGroup, DefaultIgnore;
```
It's not the end of the world, but do you think it's worth it?



Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:34
+   off-note {{function parameter 'n' with 
unknown value cannot be used in a constant expression}}
+}

tahonermann wrote:
> Perhaps add a test where the conditional expression is parenthesized.
>   int array4[(n ? 1 : -1)]; 
> 
> Adding tests for one side being of size 0 might be useful to demonstrate that 
> those are not intended to trigger the "use 'static_assert'" diagnostic. I 
> know some compilers don't diagnose on a size of 0 and so the static assert 
> idiom generally requires a negative number.
>   int array5[n ? 1 : 0]; // ok?
Good call on the additional test cases!


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:147-149
+def ext_vla_cxx_static_assert : ExtWarn<
+  "variable length arrays in C++ are a Clang extension; did you mean to use "
+  "'static_assert'?">, InGroup;

I find the "did you mean to use 'static_assert'" phrasing awkward here since it 
is highly unlikely that someone intended to write `static_assert(x)` and 
instead wrote `int my_assert[x ? 1 : -1]`. Perhaps something like this?

"variable length arrays in C++ are a Clang extension; 'static_assert' can be 
used in this case".



Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:34
+   off-note {{function parameter 'n' with 
unknown value cannot be used in a constant expression}}
+}

Perhaps add a test where the conditional expression is parenthesized.
  int array4[(n ? 1 : -1)]; 

Adding tests for one side being of size 0 might be useful to demonstrate that 
those are not intended to trigger the "use 'static_assert'" diagnostic. I know 
some compilers don't diagnose on a size of 0 and so the static assert idiom 
generally requires a negative number.
  int array5[n ? 1 : 0]; // ok?


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+  VLADiag = getLangOpts().GNUMode

aaron.ballman wrote:
> jyknight wrote:
> > Not sure whether to actually care, since C++98 is so old now, but, having 
> > `-Wno-vla-extension-static-assert` work in C++98 mode seems like it'd be 
> > quite useful, exactly because the usage cannot trivially be replaced by a 
> > static_assert. So it's a bit unfortunate that we don't distinguish it there.
> > 
> > Perhaps we should emit the same diagnostics in both 98/11, but with 
> > slightly different text in 98?
> That's effectively what we're doing here, right?
> 
> C++98 is told "variable length arrays (in C++) are a Clang extension" and in 
> C++11 they're told "variable length arrays (in C++) are a Clang extension; 
> did you mean to use 'static_assert'?"
> 
> Or am I misunderstanding you?
In this patch, in C++98 mode, we diagnose everything under the flag 
-Wvla-extension, and never under the flag -Wvla-extension-static-assert. My 
point was that we ought to diagnose static-assert-like-VLAs under the 
-Wvla-extension-static-assert flag even in C++98 mode.


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/SemaCXX/offsetof.cpp:31
+  int array1[__builtin_offsetof(HasArray, array[i])]; // expected-warning 
{{variable length arrays are a Clang extension}} \
+ new-interp-note 
{{function parameter 'i' with unknown value cannot be used in a constant 
expression}}
 }

aaron.ballman wrote:
> jyknight wrote:
> > Weird that new-interp adds the diagnostic for C++98 mode. I wonder if that 
> > indicates a bug (e.g. if in new-interp we accidentally use C++11 rules for 
> > C++98)?
> CC @tbaeder 
https://github.com/llvm/llvm-project/pull/67990


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin accepted this revision.
cor3ntin added a comment.
This revision is now accepted and ready to land.

LGTM!


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 557531.
aaron.ballman marked 4 inline comments as done.
aaron.ballman added a comment.

Updated based on review feedback. Specifically:

- Updated a FIXME comment in a test to clarify what's happening.
- Reworded diagnostic to add "in C++"


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

https://reviews.llvm.org/D156565

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/lambdas.cpp
  clang/test/CXX/basic/basic.types/p10.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.type/p2.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp
  clang/test/Misc/warning-wall.c
  clang/test/OpenMP/debug-info-openmp-array.cpp
  clang/test/OpenMP/for_reduction_codegen.cpp
  clang/test/OpenMP/for_reduction_codegen_UDR.cpp
  clang/test/OpenMP/master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/parallel_ast_print.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_for_codegen.cpp
  clang/test/OpenMP/parallel_masked_ast_print.cpp
  clang/test/OpenMP/parallel_master_ast_print.cpp
  clang/test/OpenMP/parallel_master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_constant_device_codegen.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_data_use_device_addr_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen_01.cpp
  clang/test/OpenMP/target_defaultmap_messages.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_firstprivate_codegen.cpp
  clang/test/OpenMP/target_has_device_addr_codegen_01.cpp
  clang/test/OpenMP/target_in_reduction_codegen.cpp
  clang/test/OpenMP/target_map_codegen_12.cpp
  clang/test/OpenMP/target_map_codegen_18a.cpp
  clang/test/OpenMP/target_map_codegen_18b.cpp
  clang/test/OpenMP/target_map_codegen_18c.cpp
  clang/test/OpenMP/target_map_codegen_18d.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_defaultmap_messages.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_generic_loop_depend_codegen.cpp
  clang/test/OpenMP/target_private_codegen.cpp
  clang/test/OpenMP/target_reduction_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_schedule_codegen.cpp
  
clang/test/OpenMP/target_t

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked 4 inline comments as done.
aaron.ballman added a subscriber: tbaeder.
aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:143
+def ext_vla_cxx : ExtWarn<
+  "variable length arrays are a Clang extension">, InGroup;
+def ext_vla_cxx_in_gnu_mode : Extension,

jyknight wrote:
> Clarify text: "variable length arrays in C++ are a Clang extension" and same 
> below.
> 
> I also think we should be (consistently) using the term "variable-length 
> array" with a dash instead of "variable length array", but if you change 
> that, it should be in a follow-on which changes it in all the messages.
> Clarify text: "variable length arrays in C++ are a Clang extension" and same 
> below.

Sure, that's reasonable, thanks!

> I also think we should be (consistently) using the term "variable-length 
> array" with a dash instead of "variable length array", but if you change 
> that, it should be in a follow-on which changes it in all the messages.

Agreed that we should be consistent, but the standard term of art is `variable 
length array` despite "variable-length array" being arguably more correct, and 
that usage is the more common in Clang (273 uses of "variable length array" 
compared to 64 uses of "variable-length array").



Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+  VLADiag = getLangOpts().GNUMode

jyknight wrote:
> Not sure whether to actually care, since C++98 is so old now, but, having 
> `-Wno-vla-extension-static-assert` work in C++98 mode seems like it'd be 
> quite useful, exactly because the usage cannot trivially be replaced by a 
> static_assert. So it's a bit unfortunate that we don't distinguish it there.
> 
> Perhaps we should emit the same diagnostics in both 98/11, but with slightly 
> different text in 98?
That's effectively what we're doing here, right?

C++98 is told "variable length arrays (in C++) are a Clang extension" and in 
C++11 they're told "variable length arrays (in C++) are a Clang extension; did 
you mean to use 'static_assert'?"

Or am I misunderstanding you?



Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp:39
+  using T = int[n]; // expected-error {{variable length array declaration not 
allowed at file scope}} \
+   expected-warning {{variable length arrays are a Clang 
extension}} \
+   expected-note {{read of non-const variable 'n' is not 
allowed in a constant expression}}

jyknight wrote:
> That we get a warning _and_ an error for this using statement now is 
> non-ideal. I see that we are doing this in two different places, 
> though...first in CheckType for the VLA, then diagnosing if it's used at file 
> scope in CheckDecl. So maybe not worth fixing.
> 
Yeah, agreed that this is unfortunate, but it comes from the split between the 
type and the declaration checking code and that's going to be hard to work 
around.



Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:124
+  typedef int arr[strcmp("bar", "foo") + 4 * strncmp("foo", "bar", 4)]; // 
expected-warning {{variable length array folded to constant array as an 
extension}} \
+   
expected-warning {{variable length arrays are a Clang extension}} \
+   
expected-note {{non-constexpr function 'strcmp' cannot be used in a constant 
expression}}

jyknight wrote:
> Another unfortunate case, similar to the error case earlier, of BOTH warning 
> about about it being a variable-length but then also warning about it 
> actually being constant array as an extension.
Yup, same kind of issue here as above in terms of the cause.



Comment at: clang/test/SemaCXX/offsetof.cpp:31
+  int array1[__builtin_offsetof(HasArray, array[i])]; // expected-warning 
{{variable length arrays are a Clang extension}} \
+ new-interp-note 
{{function parameter 'i' with unknown value cannot be used in a constant 
expression}}
 }

jyknight wrote:
> Weird that new-interp adds the diagnostic for C++98 mode. I wonder if that 
> indicates a bug (e.g. if in new-interp we accidentally use C++11 rules for 
> C++98)?
CC @tbaeder 



Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:13
+
+// FIXME: it's not clear why C++98 mode does not emit the extra notes in the
+// same way that C++11 mode does.

jyknight wrote:
> I think this is "expected" due to the divergence between the use of the 
> constant-expression evaluation in C++11-and-later and "ICE" code in previous.
Ah you're exactly right about that, thank you!


C

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

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Overall looks good, just a few nits from looking things over.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:143
+def ext_vla_cxx : ExtWarn<
+  "variable length arrays are a Clang extension">, InGroup;
+def ext_vla_cxx_in_gnu_mode : Extension,

Clarify text: "variable length arrays in C++ are a Clang extension" and same 
below.

I also think we should be (consistently) using the term "variable-length array" 
with a dash instead of "variable length array", but if you change that, it 
should be in a follow-on which changes it in all the messages.



Comment at: clang/lib/Sema/SemaType.cpp:2617
+  } else if (getLangOpts().CPlusPlus) {
+if (getLangOpts().CPlusPlus11 && IsStaticAssertLike(ArraySize, Context))
+  VLADiag = getLangOpts().GNUMode

Not sure whether to actually care, since C++98 is so old now, but, having 
`-Wno-vla-extension-static-assert` work in C++98 mode seems like it'd be quite 
useful, exactly because the usage cannot trivially be replaced by a 
static_assert. So it's a bit unfortunate that we don't distinguish it there.

Perhaps we should emit the same diagnostics in both 98/11, but with slightly 
different text in 98?



Comment at: clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp:39
+  using T = int[n]; // expected-error {{variable length array declaration not 
allowed at file scope}} \
+   expected-warning {{variable length arrays are a Clang 
extension}} \
+   expected-note {{read of non-const variable 'n' is not 
allowed in a constant expression}}

That we get a warning _and_ an error for this using statement now is non-ideal. 
I see that we are doing this in two different places, though...first in 
CheckType for the VLA, then diagnosing if it's used at file scope in CheckDecl. 
So maybe not worth fixing.




Comment at: clang/test/SemaCXX/cxx1z-noexcept-function-type.cpp:124
+  typedef int arr[strcmp("bar", "foo") + 4 * strncmp("foo", "bar", 4)]; // 
expected-warning {{variable length array folded to constant array as an 
extension}} \
+   
expected-warning {{variable length arrays are a Clang extension}} \
+   
expected-note {{non-constexpr function 'strcmp' cannot be used in a constant 
expression}}

Another unfortunate case, similar to the error case earlier, of BOTH warning 
about about it being a variable-length but then also warning about it actually 
being constant array as an extension.



Comment at: clang/test/SemaCXX/offsetof.cpp:31
+  int array1[__builtin_offsetof(HasArray, array[i])]; // expected-warning 
{{variable length arrays are a Clang extension}} \
+ new-interp-note 
{{function parameter 'i' with unknown value cannot be used in a constant 
expression}}
 }

Weird that new-interp adds the diagnostic for C++98 mode. I wonder if that 
indicates a bug (e.g. if in new-interp we accidentally use C++11 rules for 
C++98)?



Comment at: clang/test/SemaCXX/vla-ext-diag.cpp:13
+
+// FIXME: it's not clear why C++98 mode does not emit the extra notes in the
+// same way that C++11 mode does.

I think this is "expected" due to the divergence between the use of the 
constant-expression evaluation in C++11-and-later and "ICE" code in previous.


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 557483.
aaron.ballman added a comment.

Updated based on feedback on the RFC. Specifically, this variant recognizes 
`int array[cond ? 1 : -1];` or similar constructs and recommends using a 
`static_assert` instead. This new diagnostic is under its own warning group 
(`-Wvla-extension-static-assert`) so that users who want to be alerted to use 
of VLAs but not static_assert-like VLAs can do so, but the new warning group is 
still under `-Wvla-extension` and thus is enabled similarly.


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

https://reviews.llvm.org/D156565

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/lambdas.cpp
  clang/test/CXX/basic/basic.types/p10.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.type/p2.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp
  clang/test/Misc/warning-wall.c
  clang/test/OpenMP/debug-info-openmp-array.cpp
  clang/test/OpenMP/for_reduction_codegen.cpp
  clang/test/OpenMP/for_reduction_codegen_UDR.cpp
  clang/test/OpenMP/master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/parallel_ast_print.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_for_codegen.cpp
  clang/test/OpenMP/parallel_masked_ast_print.cpp
  clang/test/OpenMP/parallel_master_ast_print.cpp
  clang/test/OpenMP/parallel_master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_constant_device_codegen.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_data_use_device_addr_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen_01.cpp
  clang/test/OpenMP/target_defaultmap_messages.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_firstprivate_codegen.cpp
  clang/test/OpenMP/target_has_device_addr_codegen_01.cpp
  clang/test/OpenMP/target_in_reduction_codegen.cpp
  clang/test/OpenMP/target_map_codegen_12.cpp
  clang/test/OpenMP/target_map_codegen_18a.cpp
  clang/test/OpenMP/target_map_codegen_18b.cpp
  clang/test/OpenMP/target_map_codegen_18c.cpp
  clang/test/OpenMP/target_map_codegen_18d.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_defaultmap_messages.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_generic_loop_depend_codegen.cpp
  clang/test/OpenMP/target_private_codegen.cpp
  clang/test/OpenMP/target_reduction_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenM

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I put up an RFC at 
https://discourse.llvm.org/t/rfc-diagnosing-use-of-vlas-in-c/73109 to reach a 
slightly wider audience for awareness.


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

In D156565#4605027 , @shafik wrote:

> In D156565#4599812 , @aaron.ballman 
> wrote:
>
>> Enable the diagnostic by default in C++ language modes, and under -Wall in 
>> GNU++ language modes.
>
> I am happy with that approach.

Ditto! Some of the GCC folks also expressed a desire for this direction on 
their side.
Given that, and how surprising VLAs can be to c++ folks, It seems reasonable to 
go ahead with this patch


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

In D156565#4599812 , @aaron.ballman 
wrote:

> Enable the diagnostic by default in C++ language modes, and under -Wall in 
> GNU++ language modes.

I am happy with that approach.


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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman updated this revision to Diff 551596.
aaron.ballman added a comment.

Enable the diagnostic by default in C++ language modes, and under -Wall in 
GNU++ language modes.


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

https://reviews.llvm.org/D156565

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/Analysis/lambdas.cpp
  clang/test/CXX/basic/basic.types/p10.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.type/p2.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp
  clang/test/Misc/warning-wall.c
  clang/test/OpenMP/debug-info-openmp-array.cpp
  clang/test/OpenMP/for_reduction_codegen.cpp
  clang/test/OpenMP/for_reduction_codegen_UDR.cpp
  clang/test/OpenMP/master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/parallel_ast_print.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_for_codegen.cpp
  clang/test/OpenMP/parallel_masked_ast_print.cpp
  clang/test/OpenMP/parallel_master_ast_print.cpp
  clang/test/OpenMP/parallel_master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_constant_device_codegen.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_data_use_device_addr_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen_01.cpp
  clang/test/OpenMP/target_defaultmap_messages.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_firstprivate_codegen.cpp
  clang/test/OpenMP/target_has_device_addr_codegen_01.cpp
  clang/test/OpenMP/target_in_reduction_codegen.cpp
  clang/test/OpenMP/target_map_codegen_12.cpp
  clang/test/OpenMP/target_map_codegen_18a.cpp
  clang/test/OpenMP/target_map_codegen_18b.cpp
  clang/test/OpenMP/target_map_codegen_18c.cpp
  clang/test/OpenMP/target_map_codegen_18d.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_defaultmap_messages.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_codegen.cpp
  clang/test/OpenMP/target_parallel_for_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_codegen.cpp
  clang/test/OpenMP/target_parallel_for_simd_depend_codegen.cpp
  clang/test/OpenMP/target_parallel_generic_loop_depend_codegen.cpp
  clang/test/OpenMP/target_private_codegen.cpp
  clang/test/OpenMP/target_reduction_codegen.cpp
  clang/test/OpenMP/target_simd_codegen.cpp
  clang/test/OpenMP/target_simd_depend_codegen.cpp
  clang/test/OpenMP/target_teams_codegen.cpp
  clang/test/OpenMP/target_teams_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_depend_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_depend_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_dist_schedule_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_schedule_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_collapse_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_par

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156565#4580716 , @Endill wrote:

> In D156565#4547909 , @aaron.ballman 
> wrote:
>
>> In D156565#4543503 , 
>> @aaron.ballman wrote:
>>
>>> In D156565#4543414 , @jrtc27 
>>> wrote:
>>>
 Given GCC defines GNU C++ and regards this as a feature (unless you use 
 things like -pedantic to ask for ISO C++), does it make sense to enable 
 this for GNU C++?
>>>
>>> I think GCC should enable -Wvla by default in GNU C++ as well, for the same 
>>> reasons I'm proposing it for Clang. I've filed an issue for it at 
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110848
>>
>> The GCC conversation is leaning towards only diagnosing by default in C++ 
>> mode but not in GNU++ mode. I'm still trying to persuade them to diagnose in 
>> both modes one last time, but if it looks like they're firm about not 
>> diagnosing in GNU++ mode, I can live with that (for now). It at least 
>> improves our security posture a bit, so it's definitely a win.
>
> I think that we should warn by default in GNU mode regardless of GCC 
> decision. As for the porting concern, I think it falls into "comprehensive 
> diagnostics" selling point you mentioned earlier, which I totally agree with.

The current discussion on the GCC issue is to diagnose by default in C++ mode 
and add `-Wvla` to `-Wall` in GNU++ mode, which perhaps is a nice compromise. 
I'm waiting to see if any further discussion happens on that issue, but if 
folks have opinions on that approach, I'd love to hear them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Vlad Serebrennikov via Phabricator via cfe-commits]

Endill added a comment.

In D156565#4547909 , @aaron.ballman 
wrote:

> In D156565#4543503 , @aaron.ballman 
> wrote:
>
>> In D156565#4543414 , @jrtc27 wrote:
>>
>>> Given GCC defines GNU C++ and regards this as a feature (unless you use 
>>> things like -pedantic to ask for ISO C++), does it make sense to enable 
>>> this for GNU C++?
>>
>> I think GCC should enable -Wvla by default in GNU C++ as well, for the same 
>> reasons I'm proposing it for Clang. I've filed an issue for it at 
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110848
>
> The GCC conversation is leaning towards only diagnosing by default in C++ 
> mode but not in GNU++ mode. I'm still trying to persuade them to diagnose in 
> both modes one last time, but if it looks like they're firm about not 
> diagnosing in GNU++ mode, I can live with that (for now). It at least 
> improves our security posture a bit, so it's definitely a win.

I think that we should warn by default in GNU mode regardless of GCC decision. 
As for the porting concern, I think it falls into "comprehensive diagnostics" 
selling point you mentioned earlier, which I totally agree with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156565#4543503 , @aaron.ballman 
wrote:

> In D156565#4543414 , @jrtc27 wrote:
>
>> Given GCC defines GNU C++ and regards this as a feature (unless you use 
>> things like -pedantic to ask for ISO C++), does it make sense to enable this 
>> for GNU C++?
>
> I think GCC should enable -Wvla by default in GNU C++ as well, for the same 
> reasons I'm proposing it for Clang. I've filed an issue for it at 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110848

The GCC conversation is leaning towards only diagnosing by default in C++ mode 
but not in GNU++ mode. I'm still trying to persuade them to diagnose in both 
modes one last time, but if it looks like they're firm about not diagnosing in 
GNU++ mode, I can live with that (for now). It at least improves our security 
posture a bit, so it's definitely a win.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D156565#4543414 , @jrtc27 wrote:

> Given GCC defines GNU C++ and regards this as a feature (unless you use 
> things like -pedantic to ask for ISO C++), does it make sense to enable this 
> for GNU C++?

I think GCC should enable -Wvla by default in GNU C++ as well, for the same 
reasons I'm proposing it for Clang. I've filed an issue for it at 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110848

> Every deviation from GCC is a point of friction for adopting Clang over GCC.

It is, but one of the selling points of Clang is that we strive to 
comprehensive diagnostics and that sometimes leads to these deviations with 
GCC. We don't make much of a distinction between GNU++ and C++ modes in Clang 
(~10 uses of `GNUMode` in the frontend), but that is an approach I could take 
should it be necessary. However, I strongly prefer to enable this diagnostic in 
all C++ modes -- VLA use in C++ is deeply weird and easy to overlook, making it 
a bug factory: 
https://github.com/llvm/llvm-project/issues?q=is%3Aissue+is%3Aopen+VLA+C%2B%2B


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Given GCC defines GNU C++ and regards this as a feature (unless you use things 
like -pedantic to ask for ISO C++), does it make sense to enable this for GNU 
C++? Every deviation from GCC is a point of friction for adopting Clang over 
GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: ABataev, clang-vendors.
aaron.ballman added a comment.

The patch seems large and scary, but that's because of how often we've had to 
test VLA behavior in C++ code. The OpenMP test cases are mostly opting out of 
VLA warnings mechanically (I verified with @ABataev that this was the desired 
approach) while the non-OpenMP tests were largely updated by hand so that I 
could spot check the diagnostic behavior to ensure it's reasonable. The 
functional changes themselves are quite small.

Adding the clang-vendors group in case this is disruptive for downstreams.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156565

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, erichkeane, clang-language-wg, 
hubert.reinterpretcast, shafik, jyknight.
Herald added subscribers: mattd, pmatos, asb, asavonic, jdoerfert, sbc100, 
dschuff.
Herald added a project: All.
aaron.ballman requested review of this revision.
Herald added subscribers: jplehr, sstefan1, aheejin.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

Clang supports VLAs in C++ as an extension, but we currently only warn on their 
use when you pass `-Wvla`, `-Wvla-extension`, or `-pedantic`. However, VLAs as 
they're expressed in C have been considered by WG21 and rejected, are easy to 
use accidentally to the surprise of users (e.g., 
https://ddanilov.me/default-non-standard-features/), and they have potential 
security implications beyond constant-size arrays 
(https://wiki.sei.cmu.edu/confluence/display/c/ARR32-C.+Ensure+size+arguments+for+variable+length+arrays+are+in+a+valid+range).
 C++ users should strongly consider using other functionality such as 
`std::vector` instead.

This seems like sufficiently compelling evidence to warn users about VLA use by 
default, to this enables the `-Wvla-extension` diagnostic group in C++. The 
warning is still opt-in in C language modes, where support for VLAs is somewhat 
less surprising to users.

Fixes https://github.com/llvm/llvm-project/issues/62836


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156565

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaType.cpp
  clang/test/AST/Interp/literals.cpp
  clang/test/AST/regression-new-expr-crash.cpp
  clang/test/Analysis/lambdas.cpp
  clang/test/CXX/basic/basic.types/p10.cpp
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.typedef/p2-0x.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.type/p2.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/p5.cpp
  clang/test/FixIt/Inputs/nullability.h
  clang/test/OpenMP/debug-info-openmp-array.cpp
  clang/test/OpenMP/for_reduction_codegen.cpp
  clang/test/OpenMP/for_reduction_codegen_UDR.cpp
  clang/test/OpenMP/master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_in_reduction_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/nvptx_target_codegen.cpp
  clang/test/OpenMP/parallel_ast_print.cpp
  clang/test/OpenMP/parallel_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_for_codegen.cpp
  clang/test/OpenMP/parallel_masked_ast_print.cpp
  clang/test/OpenMP/parallel_master_ast_print.cpp
  clang/test/OpenMP/parallel_master_taskloop_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_reduction_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_lastprivate_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_private_codegen.cpp
  clang/test/OpenMP/parallel_master_taskloop_simd_reduction_codegen.cpp
  clang/test/OpenMP/report_default_DSA.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/single_copyprivate_messages.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_codegen.cpp
  clang/test/OpenMP/target_constant_device_codegen.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_data_use_device_addr_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen_01.cpp
  clang/test/OpenMP/target_defaultmap_messages.cpp
  clang/test/OpenMP/target_depend_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_depend_codegen.cpp
  clang/test/OpenMP/target_exit_data_codegen.cpp
  clang/test/OpenMP/target_exit_data_depend_codegen.cpp
  clang/test/OpenMP/target_firstprivate_codegen.cpp
  clang/test/OpenMP/target_has_device_addr_codegen_01.cpp
  clang/test/OpenMP/target_in_reduction_codegen.cpp
  clang/test/OpenMP/target_map_codegen_12.cpp
  clang/test/OpenMP/target_map_codegen_18a.cpp
  clang/test/OpenMP/target_map_codegen_18b.cpp
  clang/test/OpenMP/target_map_codegen_18c.cpp
  clang/test/OpenMP/target_map_codegen_18d.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_codegen.cpp
  clang/test/OpenMP/target_parallel_defaultmap_messages.cpp
  clang/test/OpenMP/target_parallel_depend_codegen.cpp