aaron.ballman 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@-2{{variable length arrays are a C99 feature}}
+#endif
}
----------------
efriedma wrote:
> 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.
> 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.
Yeah, that's a different way of delineating than I was thinking originally and
it's worth more thought. I was thinking the separation would be "this is a VLA"
(for people who want to avoid all VLA stack allocations due to the security
concerns) and "this is a portability concern" (for people who want to port to
older language standards, C++, other compilers).
>> the purpose to -Wvla is to identify nonportable code
> The standard committee can't magically make something "portable"; portability
> depends on implementations.
Agreed, but you cut the quote short: "because it's using a VLA." was
weight-bearing. `-Wvla` warning on things which are not VLAs according to the
standard is largely why we fixed the standard to make it clear this construct
wasn't a VLA as far as `__STDC_NO_VLA__` support is concerned. But you're point
is also valid that we probably need something about the portability of the
syntax rather than only diagnose based on the semantics.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D132952/new/
https://reviews.llvm.org/D132952
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
