[Bug c/107980] New: va_start incorrectly accepts an arbitrary number of arguments in C2x

[Bug c/107980] New: va_start incorrectly accepts an arbitrary number of arguments in C2x

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

            Bug ID: 107980
           Summary: va_start incorrectly accepts an arbitrary number of
                    arguments in C2x
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: aaron at aaronballman dot com
  Target Milestone: ---

GCC implemented WG14 N2975 which relaxes the requirements for va_start.
However, there's a diagnostic missing for it when the user passes more than two
arguments:
```
#include <stdarg.h>

void func(int i, ...) {
  va_list va;
  va_start(va, i, 23, 4);

  va_end(va);
}

```
is silently accepted by GCC even with -Wall and -pedantic.

I noticed this when I was working on the Clang implementation of N2975. I
noticed that GCC has not changed the definition of __builtin_va_start, which I
think is a good approach (keeping builtin interfaces stable between compiler
and language versions is a nicety). I was considering adding
`__builtin_c23_va_start` to Clang with a signature that accepts `...` so I can
diagnose this case, but I think that will run afoul of 7.16.1.4p4 "Any
additional arguments are not used by the macro and will not be expanded or
evaluated for any reason." without some heroics, so I'm not certain how/if
Clang will address this.

[Bug c/107980] va_start incorrectly accepts an arbitrary number of arguments in C2x

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Hmm from https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf :
```
7.16.1.4 The va_start macro
void va_start(va_list ap, ...);


Any additional arguments are not used by
the macro and will not be expanded or evaluated for any reason.
```


I would assume since it is `arguments` rather than additional argument, that it
could be more than one additional argument.

[Bug c/107980] va_start incorrectly accepts an arbitrary number of arguments in C2x

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #2 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Andrew Pinski from comment #1)
> Hmm from https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3054.pdf :
> ```
> 7.16.1.4 The va_start macro
> void va_start(va_list ap, ...);
> 
> 
> Any additional arguments are not used by
> the macro and will not be expanded or evaluated for any reason.
> ```
> 
> 
> I would assume since it is `arguments` rather than additional argument, that
> it could be more than one additional argument.

Yes, the standard says "arguments" because there can be additional arguments
due to using `...`. However, think about the user experience:
```
// This should always be happily accepted because it's
// idiomatic C before C2x and it's valid C in C2x and later.
va_start(list, some_arg);

// This should be accepted in C2x mode and if you care
// to do such things, warn the user that it's incompatible
// with standards before C2x. Pre-C2x, this is an error.
va_start(list);

// Under what circumstances is this anything other than
// programmer confusion? It's invalid before C2x and the
// extra args are meaningless in C2x and later.
va_start(list, 1, 2, 3, 4);
```

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |diagnostic
            Summary|va_start incorrectly        |va_start does not warn
                   |accepts an arbitrary number |about an arbitrary number
                   |of arguments in C2x         |of arguments in C2x mode

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think we should warn but how to warn is going to have to special case the
macro I think.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> I think we should warn but how to warn is going to have to special case the
> macro I think.

But saying that I do think it is valid C2X code if you take the C2X standard
seperately from the older standards.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #5 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Andrew Pinski from comment #3)
> I think we should warn but how to warn is going to have to special case the
> macro I think.

I was contemplating a perhaps terrible idea of adding some new builtins:
```
unsigned __builtin_va_opt_count(...);
void __builtin_c23_va_start(va_list list, unsigned arg_count);
```
and then doing something along these lines in stdarg.h:
```
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202000L
#define va_start(ap, ...) __builtin_c23_va_start(ap,
__builtin_va_opt_count(__VA_OPT__))
#else
...
#endif
```
but I've not put enough thought into it yet. WDYT about something along those
lines?

(In reply to Andrew Pinski from comment #4)
> (In reply to Andrew Pinski from comment #3)
> > I think we should warn but how to warn is going to have to special case the
> > macro I think.
> 
> But saying that I do think it is valid C2X code if you take the C2X standard
> seperately from the older standards.

I'm sad I noticed this after the NB comment period closed for the US NB because
otherwise I'd leave a comment to make it a constraint violation to pass more
than two arguments to the macro (or at least a recommended practice to
diagnose). Because I agree with you that the letter of the standard makes this
valid in C2x, but given that the extra arguments are not expanded or evaluated,
there is no reason to accept more than two arguments in any C standard version.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Maybe add a _Static_warn builtin instead which is like _Static_assert but
instead of an error, it is a warning.

That is:
#define va_start(ap, ...) (_Static_warn(__builtin_va_opt_count(__VA_OPT__) <=
1, "More than one argument supplied to the macro va_start"),
__builtin_va_start(ap, 0))


THe only issue is that message does not get translated.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Why do you need a builtin for counting number of arguments in __VA_OPT__?
That can be done in the preprocessor.  All one needs is 1, 2 or more arguments.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #8 from Aaron Ballman <aaron at aaronballman dot com> ---
(Sorry, I meant __VA_ARGS__ and not __VA_OPT__; C23 on the brain!)

(In reply to Andrew Pinski from comment #6)
> Maybe add a _Static_warn builtin instead which is like _Static_assert but
> instead of an error, it is a warning.
> 
> That is:
> #define va_start(ap, ...) (_Static_warn(__builtin_va_opt_count(__VA_OPT__)
> <= 1, "More than one argument supplied to the macro va_start"),
> __builtin_va_start(ap, 0))
> 
> 
> THe only issue is that message does not get translated.

Huh, that's a really neat idea to consider! But _Static_assert is a
declaration, not an expression, so we'd have to use a statement expression
there instead of rely on the comma operator. But I could use that for both
diagnosing passing > 2 args as well as passing < 2 args (we sometimes like to
warn "this feature is incompatible with standards before <blah>" in Clang).


(In reply to Jakub Jelinek from comment #7)
> Why do you need a builtin for counting number of arguments in __VA_OPT__?
> That can be done in the preprocessor.  All one needs is 1, 2 or more
> arguments.

Yes, you can beat the preprocessor into doing this for you, but a builtin that
counts the number of arguments in __VA_ARGS__ would solve a problem users have
(https://stackoverflow.com/questions/2124339/c-preprocessor-va-args-number-of-arguments),
so I figured it was better than preprocessor games.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #9 from Aaron Ballman <aaron at aaronballman dot com> ---
Btw, a similar issue in this area is that GCC no longer diagnoses when the user
passes a second argument to va_start in C2x mode and the argument is not the
parameter before the ellipsis. e.g.,
```
#include <stdarg.h>

void diag(int a, int b, ...) {
  va_list list;
  va_start(list, a);
  va_end(list);
}
```
https://godbolt.org/z/daeYbPv6z

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That again is completely valid in C2X.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #11 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Jakub Jelinek from comment #10)
> That again is completely valid in C2X.

Yes, it is. And again, it's still something worth diagnosing (IMO) because it's
utter nonsense code that is invalid with C17 and earlier as well as C++.

I don't believe WG14 intended to make this invalid code valid for C2x.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #12 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
The standard rule about not using extra arguments means that any warnings 
would need to avoid even converting those arguments from pp-tokens to 
tokens; it's OK for them to contain pp-tokens that cannot be converted to 
tokens.

I think the accepted change to the standard was entirely clear about 
ignoring the extra arguments; it wasn't some obscure non-obvious 
consequence that such code would become valid.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Warning for 3 or more arguments using preprocessor stuff only could look like:
#define __va_start_2()
#define __va_start_23(...) __builtin_warning ("too many va_arg arguments");
#define __va_start_1()
#define __va_start_12(v, ...) __va_start_2##__VA_OPT__(3) (__VA_ARGS__)
#define va_start(v, ...) __va_start_1##__VA_OPT__(2) (__VA_ARGS__)
__builtin_va_start (v, 0)
We are C2X here, so we can rely on __VA_OPT__.
As for warning when there are 2 arguments and second argument is not identifier
for the last argument, in the light of #c12 that would be fairly difficult.  In
the preprocessor it is going to be pretty hard to find out whether in the 2
arguments case the second one is a single identifier token, and on the compiler
side __builtin_va_start or __builtin_va_start_c23 would need to be a keyword
with its own parsing which would parse
normally as expression the first argument, but for the rest just warn if it is
more than 2 arguments or second argument is present and is not identifier of
the last argument, but otherwise ignore the tokens.  Still, isn't say:
#define A (
#define B )
va_start (ap, A B B );
va_start (ap, A A A );
valid in C2X?  That ruins both the argument counting and especially any
attempts to decide something after preprocessing.

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #14 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to joseph@codesourcery.com from comment #12)
> The standard rule about not using extra arguments means that any warnings 
> would need to avoid even converting those arguments from pp-tokens to 
> tokens; it's OK for them to contain pp-tokens that cannot be converted to 
> tokens.

Agreed, that's why I filed an NB comment to remove the restriction that the
arguments can't be expanded. Hopefully WG14 will relax that so we can give good
diagnostic behavior.

> I think the accepted change to the standard was entirely clear about 
> ignoring the extra arguments; it wasn't some obscure non-obvious 
> consequence that such code would become valid.

It was not discussed in the paper, or the meeting minutes, or my personal
notes. I may have missed something, but I have no reason to believe WG14
explicitly considered the ramifications of this. Also, I cannot find another
instance where a function-like macro accepts arguments that it is not allowed
to *expand*; that's a novel WG14 invention.

Also, it was a potentially silently breaking change (if you don't mind horribly
contrived examples of breakage): https://godbolt.org/z/T4bG6179f

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #15 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Aaron Ballman from comment #14)
> Also, it was a potentially silently breaking change (if you don't mind
> horribly contrived examples of breakage): https://godbolt.org/z/T4bG6179f

But __COUNTER__ is not part of the standard ....

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[aaron at aaronballman dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #16 from Aaron Ballman <aaron at aaronballman dot com> ---
(In reply to Andrew Pinski from comment #15)
> (In reply to Aaron Ballman from comment #14)
> > Also, it was a potentially silently breaking change (if you don't mind
> > horribly contrived examples of breakage): https://godbolt.org/z/T4bG6179f
> 
> But __COUNTER__ is not part of the standard ....

The code changed meaning regardless and WG14 usually goes out of our way to
avoid changing the meaning of existing, conforming code. (FWIW, I'll freely
admit this example is not compelling; it's just intended as a demonstration
that I don't think WG14 fully thought through the ramifications of what
prohibiting token expansion does in practice.)

[Bug c/107980] va_start does not warn about an arbitrary number of arguments in C2x mode

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107980

--- Comment #17 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
The details of not expanding in cases where it matters whether and how 
many times something is expanded - such as arguments expanding to have 
unbalanced parentheses - may be a non-obvious consequence that wasn't 
considered in WG14.  The basic definition of ignoring the pp-tokens 
without converting them to tokens (and thus not requiring them to parse as 
any particular kind of C language construct) was clear enough from the 
paper (whether or not anyone felt the need to comment on that aspect of 
the definition).