[PATCH] D132952: [Sema] disable -Wvla for function array parameters

Tue Sep 6 12:46:26 PDT 2022

efriedma added inline comments.

================
Comment at: clang/test/Sema/warn-vla.c:8-12
+void test2(int n, int v[n]) { // c99 no-warning
+#if __STDC_VERSION__ < 199901L
+// expected-warning at -2{{variable length arrays are a C99 feature}}
+#endif
 }
----------------
aaron.ballman wrote:
> efriedma wrote:
> > aaron.ballman wrote:
> > > efriedma wrote:
> > > > aaron.ballman wrote:
> > > > > efriedma wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > inclyc wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > The diagnostic there is rather unfortunate because we're not using a variable-length array in this case.
> > > > > > > > > Emm, I'm not clear about whether we should consider this a VLA, and generates `-Wvla-extensions`. Is `v[n]` literally a variable-length array? (in source code) So it seems to me that we should still report c89 incompatibility warnings?
> > > > > > > > > 
> > > > > > > > C89's grammar only allowed for an integer constant expression to (optionally) appear as the array extent in an array declarator, so there is a compatibility warning needed for that. But I don't think we should issue a warning about this being a VLA in C99 or later. The array *is* a VLA in terms of the form written in the source, but C adjusts the parameter to be a pointer parameter, so as far as the function's type is concerned, it's not a VLA (it's just a self-documenting interface).
> > > > > > > > 
> > > > > > > > Because self-documenting code is nice and because people are worried about accidental use of VLAs that cause stack allocations (which this does not), I think we don't want to scare people off from this construct. But I'm curious what others think as well.
> > > > > > > > But I'm curious what others think as well.
> > > > > > > 
> > > > > > > (Specifically, I'm wondering if others agree that the only warning that should be issued there is a C89 compatibility warning under `-Wvla-extensions` when in C89 mode and via a new `CPre99Compat` diagnostic group when enabled in C99 and later modes.)
> > > > > > > 
> > > > > > > 
> > > > > > I imagine people working with codebases that are also built with compilers that don't support VLAs would still want some form of warning on any VLA type.
> > > > > The tricky part is: that's precisely what WG14 N2992 (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2992.pdf) is clarifying. If your implementation doesn't support VLAs, it's still required to support this syntactic form. So the question becomes: do we want a portability warning to compilers that don't conform to the standard? Maybe we do (if we can find evidence of compilers in such a state), but probably under a specific diagnostic flag rather than -Wvla.
> > > > That only applies to C23, though, right?  None of that wording is there for C11.  (In particular, MSVC claims C11 conformance without any support for VLA types.)
> > > In a strict sense, yes, this is a C23 change. There are no changes to older standards as far as ISO is concerned (and there never have been).
> > > 
> > > In a practical sense, no, this is clarifying how VLAs have always been intended to work. C23 is in a really weird spot in that we had to stop our "defect report" process at the end of C17 because the ISO definition of a defect did not match the WG14 definition of a defect, and ISO got upset. Towards the tail end of the C23 cycle, we introduced a list of extensions to obsolete versions of C (https://www.open-std.org/jtc1/sc22/wg14/www/previous.html) as a stopgap, but the convener would not allow us to retroactively add papers to it. The committee still hasn't gotten used to this new process and as a result, not many papers have made it to the list. We've also not had the chance to discuss https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3002.pdf on having a more reasonable issue tracking process.
> > > 
> > > The end result is that there's a whole pile of papers in C23 that we would have normally treated via the old defect report process but couldn't, and if we had treated them via the old DR process, it would have been more clear why I think this should be a retroactive change back to C99 instead of a C23-only change.
> > Even if the committee can make retroactive changes to standards, that doesn't change the fact that MSVC doesn't support VLAs, and that codebases exist which need to be compiled with both clang and MSVC.
> Such code bases are already supported: https://godbolt.org/z/7n768WKW7 but I take your point that not everyone writes portable code when trying to port between compilers, and so some form of diagnostic would be nice. But I don't think that diagnostic is spelled `-Wvla` because the whole point of the paper was to clarify that such a function signature is mandatory to support even if you claim you don't support VLAs and the purpose to `-Wvla` is to identify nonportable code because it's using a VLA.
> 
> Alternatively, when in `-fms-compatibility` mode, we could disallow VLAs and function signatures which involve one because that's not compatible with MSVC. (No idea just how much code that would break or whether I think this idea has much merit or not, just observing that VLAs are a weird boundary case in terms of MS compatibility because they're not a language extension.)
I think there's some value in having flags for both "stack-allocated variable length array" and "any type spelled using a VLA".  Which one of those is spelled "-Wvla" is up for debate, I guess. I'd be inclined to let the the existing warning flag continue to work the way it has for the past; both clang and gcc have supported it with the existing semantics for a long time.  It's not like there's any shortage of flag names.

When I was referring to "codebases compiled with both clang and MSVC", I wasn't specifically referring to codebases that use clang on Windows; I was also referring to codebases that use MSVC on Windows, and clang on other targets.  How -fms-compatibility works is a separate subject.

> the purpose to -Wvla is to identify nonportable code

The standard committee can't magically make something "portable"; portability depends on implementations.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132952