[C2x] Relaxing requirements for va_start

[aaron.ballman]

Summary

This implements WG14 N2975 relaxing requirements for va_start (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2975.pdf), which does two things:

1. Allows the declaration of a variadic function without any named arguments. e.g., void f(...) is now valid, as in C++.
2. Modified the signature of the va_start macro to be a variadic macro that accepts one or more arguments; the second (and later) arguments are not expanded or evaluated.

I followed the GCC implementation in terms of not modifying the behavior of __builtin_va_start (it still requires exactly two arguments), but this approach has led to several QoI issues that I've documented with FIXME comments in the test. Specifically, the requirement that we do not evaluate *or expand* the second and later arguments means it's no longer possible to issue diagnostics for compatibility with older C versions and C++. I am reaching out to folks in WG14 to see if we can get an NB comment to address these concerns (the US comment period has already closed, so I cannot file the comment myself), so the diagnostic behavior may change in the future.

I took this opportunity to add some documentation for all the related builtins in this area, since there was no documentation for them yet.

[erichkeane]

clang/lib/Parse/ParseDecl.cpp

7356

Is this right? Isnt this also allowing:

foo(int a ...)

I don't think we want to do that, right? GCC diagnoses that as "requires a named argument", not a comma.

clang/lib/Sema/SemaChecking.cpp

7227

This doesn't exactly match the comment, this isn't checking against the literal-0, it is checking against an ICE.

An updated comment would probably be fine here, and perhaps preferential, in order to allow a paren-expr containing 0.

7230

is there value to not jsut doing an early return here? There is a lot of work happening below based on the last arg that we can just skip instead.

[cor3ntin]

clang/lib/Headers/stdarg.h

30

Yeah, i really think there should be some king of diagnostic here.
Maybe __builtin_va_start could take an arbitrary number of arguments and diagnose for more that 2 args or if the second arg is not valid?
is it a matter of being compatible with gcc? do we need to?

[aaron.ballman]

clang/lib/Headers/stdarg.h

30

We try pretty hard to keep our builtins compatible with GCC; that makes everyone's lives easier (most especially standard library authors). If we did anything here, it would be to add a new builtin specific for the new variant. However, that won't save us because of the standard's requirement to not *expand* the variadic arguments. We'd need that builtin to be named va_start and we'd have to get rid of this macro (otherwise, the arguments passed to the builtin will be expanded as they're lexed). But va_start is defined by the standard as being a macro specifically, so we'd still need to make #if defined(va_start) work. Ultimately, I don't think it's worth that level of heroics -- instead, I am hoping WG14 will relax the requirement that the arguments not be expanded or evaluated; that gives us options.

clang/lib/Parse/ParseDecl.cpp

7356

Ah good catch! I'll add a test case for that. In fact, I don't think I need to do anything here in C; the existing logic was already correct.

clang/lib/Sema/SemaChecking.cpp

7227

Ah, fair, we check against 0 explicitly, but we don't care if you got that zero from 0 or from 0 + 0, etc. I'll update the comment.

7230

Hmmm that seems reasonable to me, good call!

[aaron.ballman]

Updating based on review feedback.

• Rolled back parsing changes as they were not needed
• Moved the diagnostic from common kinds to sema kinds because it's now solely in sema
• Reworded a comment
• Added a new test case for void f(int a...)

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

7230

I see below that we still set the return type of hte call to Void. Do we wnat to keep that line, or at least, move it to the top of this function?

clang/test/C/C2x/n2975.c

50

.

[erichkeane]

Give others an extra chance to look at this, but other than the 1 concern, LGTM.

[erichkeane]

clang/test/C/C2x/n2975.c

50

Was going to complain about the error message here, but I'm on the fence whether we should say THIS (the required comma), or something more descriptive, but at least this is better than GCC just failing in normal parsing.

[cor3ntin]

clang/lib/Headers/stdarg.h

30

We probably can reuse the same macro name (we'd still have to handle the 0 case though), but if you'd rather be conforming now and revisit the issue when WG14 remove the limitation that parameters are not expanded, I'm fine with it!

[aaron.ballman]

clang/lib/Headers/stdarg.h

30

For the moment, I'd rather match what GCC is doing (deficiencies and all) until it's clear what actually gets into the final standard. We can revisit once WG14 has said their piece.

clang/lib/Sema/SemaChecking.cpp

7230

I added test coverage and it seems that setting the return type is not needed (by my understanding, CreateBuiltin requires the return type to be passed to it explicitly, and LazilyCreateBuiltin uses the builtin type string to specify *at least* the return type information (even if custom type checking happens for the rest of the signature).

clang/test/C/C2x/n2975.c

50

FWIW, this is existing diagnostic wording and behavior. I think it's reasonable, but if we want to do something better, we can fix it in a follow-up.

[aaron.ballman]

Update based on review feedback.

• Removed code setting the return type for the builtin (it's already set elsewhere)
• Added test coverage verifying the return type for the builtin is correct

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

7230

Great, thanks for checking!

[aaron.ballman]

Adding some more test cases that demonstrate that you can still call these functions as you'd expect and that function pointer assignment works as expected.

clang/docs/LanguageExtensions.rst

 
 Constant evaluation support for the ``__builtin_mem*`` functions is provided
 only for arrays of ``char``, ``signed char``, ``unsigned char``, or ``char8_t``,
 despite these functions accepting an argument of type ``const void*``.
 
 Support for constant expression evaluation for the above builtins can be detected
 with ``__has_feature(cxx_constexpr_string_builtins)``.
 
+Variadic function builtins
+--------------------------
+
+Clang provides several builtins for working with variadic functions from the C
+standard library ``<stdarg.h>`` header:
+
+* ``__builtin_va_list``
+
+A predefined typedef for the target-specific ``va_list`` type.
+
+* ``void __builtin_va_start(__builtin_va_list list, <parameter-name>)``
+
+A builtin function for the target-specific ``va_start`` function-like macro.
+The ``parameter-name`` argument is the name of the parameter preceding the
+ellipsis (``...``) in the function signature. Alternatively, in C2x mode or
+later, it may be the integer literal ``0`` if there is no parameter preceding
+the ellipsis. This function initializes the given ``__builtin_va_list`` object.
+It is undefined behavior to call this function on an already initialized
+``__builin_va_list`` object.
+
+* ``void __builtin_va_end(__builtin_va_list list)``
+
+A builtin function for the target-specific ``va_end`` function-like macro. This
+function finalizes the given ``__builtin_va_list`` object such that it is no
+longer usable unless re-initialized with a call to ``__builtin_va_start`` or
+``__builtin_va_copy``. It is undefined behavior to call this function with a
+``list`` that has not been initialized by either ``__builtin_va_start`` or
+``__builtin_va_copy``.
+
+* ``<type-name> __builtin_va_arg(__builtin_va_list list, <type-name>)``
+
+A builtin function for the target-specific ``va_arg`` function-like macro. This
+function returns the value of the next variadic argument to the call. It is
+undefined behavior to call this builtin when there is no next varadic argument
+to retrieve or if the next variadic argument does not have a type compatible
+with the given ``type-name``. The return type of the function is the
+``type-name`` given as the second argument. It is undefined behavior to call
+this function with a ``list`` that has not been initialized by either
+``__builtin_va_start`` or ``__builtin_va_copy``.
+
+* ``void __builtin_va_copy(__builtin_va_list dest, __builtin_va_list src)``
+
+A builtin function for the target-specific ``va_copy`` function-like macro.
+This function initializes ``dest`` as a copy of ``src``. It is undefined
+behavior to call this function with an already initialized ``dest`` argument.
+
 Memory builtins
 ---------------
 
 Clang provides constant expression evaluation support for builtin forms of the
 following functions from the C standard library headers
 ``<string.h>`` and ``<wchar.h>``:
 
 * ``memcpy``

clang/docs/ReleaseNotes.rst

.. code-block:: c
     (void)(1 ? null_val : 0); // Rejected in C, accepted in C++, Clang rejects
     bool b1 = nullptr;        // Accepted in C, rejected in C++, Clang rejects
     b1 = null_val;            // Accepted in C, rejected in C++, Clang rejects
     null_val = 0;             // Rejected in C, accepted in C++, Clang rejects
 
     void func(nullptr_t);
     func(0);                  // Rejected in C, accepted in C++, Clang rejects
 
+- Implemented `WG14 N2975 <https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2975.pdf>`_,
+  Relax requirements for va_start. In C2x mode, functions can now be declared
+  fully variadic and the ``va_start`` macro no longer requires passing a second
+  argument (though it accepts a second argument for backwards compatibility).
+  If a second argument is passed, it is neither expanded nor evaluated in C2x
+  mode.
+
+  .. code-block:: c
+
+    void func(...) {  // Invalid in C17 and earlier, valid in C2x and later.
+      va_list list;
+      va_start(list); // Invalid in C17 and earlier, valid in C2x and later.
+      va_end(list);
+    }
 
 C++ Language Changes in Clang
 -----------------------------
 - Implemented `DR692 <https://wg21.link/cwg692>`_, `DR1395 <https://wg21.link/cwg1395>`_,
   and `DR1432 <https://wg21.link/cwg1432>`_. The fix for DR1432 is speculative since the
   issue is still open and has no proposed resolution at this time. A speculative fix
   for DR1432 is needed to prevent regressions that would otherwise occur due to DR692.
 - Clang's default C++/ObjC++ standard is now ``gnu++17`` instead of ``gnu++14``.

clang/include/clang/Basic/DiagnosticCommonKinds.td

 def err_enum_template : Error<"enumeration cannot be a template">;
 
 def warn_cxx20_compat_consteval : Warning<
   "'consteval' specifier is incompatible with C++ standards before C++20">,
   InGroup<CXX20Compat>, DefaultIgnore;
 def warn_missing_type_specifier : Warning<
   "type specifier missing, defaults to 'int'">,
   InGroup<ImplicitInt>, DefaultIgnore;
+def warn_c17_compat_missing_comma_before_ellipsis : Warning<
+  "'...' as the only parameter of a function is incompatible with C standards "
+  "before C2x">, DefaultIgnore, InGroup<CPre2xCompat>;
 }
 
 let CategoryName = "Nullability Issue" in {
 
 def warn_nullability_duplicate : Warning<
   "duplicate nullability specifier %0">,
   InGroup<Nullability>;
 

clang/lib/Headers/stdarg.h

 #ifdef __need___va_list
 #undef __need___va_list
 #else
 #define __STDARG_H
 #ifndef _VA_LIST
 typedef __builtin_va_list va_list;
 #define _VA_LIST
 #endif
+
+/* FIXME: This is using the placeholder dates Clang produces for these macros
+   in C2x mode; switch to the correct values once they've been published. */
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202000L
+/* C2x does not require the second parameter for va_start. */
+#define va_start(ap, ...) __builtin_va_start(ap, 0)
+#else
+/* Versions before C2x do require the second parameter. */
 #define va_start(ap, param) __builtin_va_start(ap, param)
+#endif
 #define va_end(ap)          __builtin_va_end(ap)
 #define va_arg(ap, type)    __builtin_va_arg(ap, type)
 
 /* GCC always defines __va_copy, but does not define va_copy unless in c99 mode
  * or -ansi is not specified, since it was not part of C90.
  */
 #define __va_copy(d,s) __builtin_va_copy(d,s)
 

clang/lib/Parse/ParseDecl.cpp

       ParamInfo.push_back(DeclaratorChunk::ParamInfo(ParmII,
                                           ParmDeclarator.getIdentifierLoc(),
                                           Param, std::move(DefArgToks)));
     }
 
     if (TryConsumeToken(tok::ellipsis, EllipsisLoc)) {
       if (!getLangOpts().CPlusPlus) {
         // We have ellipsis without a preceding ',', which is ill-formed
-        // in C. Complain and provide the fix.
-        Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis)
-            << FixItHint::CreateInsertion(EllipsisLoc, ", ");
+        // in C before C2x. Complain and provide the fix. We do not support
+        // this as an extension in earlier C language modes.
+        if (getLangOpts().C2x)
+          Diag(EllipsisLoc,
+               diag::warn_c17_compat_missing_comma_before_ellipsis);
+        else
+          Diag(EllipsisLoc, diag::err_missing_comma_before_ellipsis)
+              << FixItHint::CreateInsertion(EllipsisLoc, ", ");
       } else if (ParmDeclarator.getEllipsisLoc().isValid() ||
                  Actions.containsUnexpandedParameterPacks(ParmDeclarator)) {
         // It looks like this was supposed to be a parameter pack. Warn and
         // point out where the ellipsis should have gone.
         SourceLocation ParmEllipsis = ParmDeclarator.getEllipsisLoc();
         Diag(EllipsisLoc, diag::warn_misplaced_ellipsis_vararg)
           << ParmEllipsis.isValid() << ParmEllipsis;
         if (ParmEllipsis.isValid()) {

clang/lib/Sema/SemaChecking.cpp

 /// for validity.  Emit an error and return true on failure; return false
 /// on success.
 bool Sema::SemaBuiltinVAStart(unsigned BuiltinID, CallExpr *TheCall) {
   Expr *Fn = TheCall->getCallee();
 
   if (checkVAStartABI(*this, BuiltinID, Fn))
     return true;
 
+  // In C2x mode, va_start only needs one argument. However, the builtin still
+  // requires two arguments (which matches the behavior of the GCC builtin),
+  // <stdarg.h> passes `0` as the second argument in C2x mode.
   if (checkArgCount(*this, TheCall, 2))
     return true;
 
   // Type-check the first argument normally.
   if (checkBuiltinArgument(*this, TheCall, 0))
     return true;
 
   // Check that the current function is variadic, and get its last parameter.
   ParmVarDecl *LastParam;
   if (checkVAStartIsInVariadicFunction(*this, Fn, &LastParam))
     return true;
 
   // Verify that the second argument to the builtin is the last argument of the
-  // current function or method.
+  // current function or method. In C2x mode, if the second argument is the
+  // integer literal 0, then we don't bother with this check.
   bool SecondArgIsLastNamedArgument = false;
   const Expr *Arg = TheCall->getArg(1)->IgnoreParenCasts();
+  bool SecondArgIsElided = false;
+  if (Optional<llvm::APSInt> Val =
+          TheCall->getArg(1)->getIntegerConstantExpr(Context);
+      Val)
+    SecondArgIsElided = LangOpts.C2x && *Val == 0;
 
   // These are valid if SecondArgIsLastNamedArgument is false after the next
   // block.
   QualType Type;
   SourceLocation ParamLoc;
   bool IsCRegister = false;
 
   if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Arg)) {
     if (const ParmVarDecl *PV = dyn_cast<ParmVarDecl>(DR->getDecl())) {
       SecondArgIsLastNamedArgument = PV == LastParam;
 
       Type = PV->getType();
       ParamLoc = PV->getLocation();
       IsCRegister =
           PV->getStorageClass() == SC_Register && !getLangOpts().CPlusPlus;
     }
   }
 
-  if (!SecondArgIsLastNamedArgument)
-    Diag(TheCall->getArg(1)->getBeginLoc(),
-         diag::warn_second_arg_of_va_start_not_last_named_param);
-  else if (IsCRegister || Type->isReferenceType() ||
+  if (!SecondArgIsLastNamedArgument) {
+    if (!SecondArgIsElided)
+      Diag(TheCall->getArg(1)->getBeginLoc(),
+           diag::warn_second_arg_of_va_start_not_last_named_param);
+  } else if (IsCRegister || Type->isReferenceType() ||
            Type->isSpecificBuiltinType(BuiltinType::Float) || [=] {
              // Promotable integers are UB, but enumerations need a bit of
              // extra checking to see what their promotable type actually is.
              if (!Context.isPromotableIntegerType(Type))
                return false;
              if (!Type->isEnumeralType())
                return true;
              const EnumDecl *ED = Type->castAs<EnumType>()->getDecl();

clang/lib/Sema/SemaType.cpp

       // have an identifier list. See OpenCL 3.0 6.11/g for more details.
       if (!FTI.NumParams && !FTI.isVariadic &&
           !LangOpts.requiresStrictPrototypes() && !LangOpts.OpenCL) {
         // Simple void foo(), where the incoming T is the result type.
         T = Context.getFunctionNoProtoType(T, EI);
       } else {
         // We allow a zero-parameter variadic function in C if the
         // function is marked with the "overloadable" attribute. Scan
-        // for this attribute now.
-        if (!FTI.NumParams && FTI.isVariadic && !LangOpts.CPlusPlus)
-          if (!D.getDeclarationAttributes().hasAttribute(
-                  ParsedAttr::AT_Overloadable) &&
-              !D.getAttributes().hasAttribute(ParsedAttr::AT_Overloadable) &&
-              !D.getDeclSpec().getAttributes().hasAttribute(
-                  ParsedAttr::AT_Overloadable))
+        // for this attribute now. We also allow it in C2x per WG14 N2975.
+        if (!FTI.NumParams && FTI.isVariadic && !LangOpts.CPlusPlus) {
+          if (LangOpts.C2x)
+            S.Diag(FTI.getEllipsisLoc(),
+                   diag::warn_c17_compat_missing_comma_before_ellipsis);
+          else if (!D.getDeclarationAttributes().hasAttribute(
+                       ParsedAttr::AT_Overloadable) &&
+                   !D.getAttributes().hasAttribute(
+                       ParsedAttr::AT_Overloadable) &&
+                   !D.getDeclSpec().getAttributes().hasAttribute(
+                       ParsedAttr::AT_Overloadable))
             S.Diag(FTI.getEllipsisLoc(), diag::err_ellipsis_first_param);
+        }
 
         if (FTI.NumParams && FTI.Params[0].Param == nullptr) {
           // C99 6.7.5.3p3: Reject int(x,y,z) when it's not a function
           // definition.
           S.Diag(FTI.Params[0].IdentLoc,
                  diag::err_ident_list_in_fn_declaration);
           D.setInvalidType(true);
           // Recover by creating a K&R-style function type, if possible.

clang/test/C/C2x/n2975.c

+// RUN: %clang_cc1 -verify -ffreestanding -Wpre-c2x-compat -std=c2x %s
+
+/* WG14 N2975: partial
+ * Relax requirements for va_start
+ */
+
+#include <stdarg.h>
+
+#define DERP this is an error
+
+void func(...) { // expected-warning {{'...' as the only parameter of a function is incompatible with C standards before C2x}}
+  // Show that va_start doesn't require the second argument in C2x mode.
+  va_list list;
+  va_start(list); // FIXME: it would be nice to issue a portability warning to C17 and earlier here.
+  va_end(list);
+
+  // Show that va_start doesn't expand or evaluate the second argument.
+  va_start(list, DERP);
+  va_end(list);
+
+  // FIXME: it would be kinder to diagnose this instead of silently accepting it.
+  va_start(list, 1, 2);
+  va_end(list);
+
+  // We didn't change the behavior of __builtin_va_start (and neither did GCC).
+  __builtin_va_start(list); // expected-error {{too few arguments to function call, expected 2, have 1}}
+}
+
+// Show that function pointer types also don't need an argument before the
+// ellipsis.
+typedef void (*fp)(...); // expected-warning {{'...' as the only parameter of a function is incompatible with C standards before C2x}}
+
+// Passing something other than the argument before the ... is still not valid.
+void diag(int a, int b, ...) {
+  va_list list;
+  // FIXME: the call to va_start should also diagnose the same way as the call
+  // to __builtin_va_start. However, because va_start is not allowed to expand
+  // or evaluate the second argument, we can't pass it along to
+  // __builtin_va_start to get that diagnostic. So in C17 and earlier, we will
+  // diagnose this use through the macro, but in C2x and later we've lost the
+  // diagnostic entirely. GCC has the same issue currently.
+  va_start(list, a);
+  // However, the builtin itself is under no such constraints regarding
+  // expanding or evaluating the second argument, so it can still diagnose.
+  __builtin_va_start(list, a); // expected-warning {{second argument to 'va_start' is not the last named parameter}}
+  va_end(list);
+}

clang/www/c_status.html

     <tr>
       <td>Improved normal enumerations</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3029.htm">N3029</a></td>
       <td class="unknown" align="center">Unknown</td>
     </tr>
     <tr>
       <td>Relax requirements for va_start</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2975.pdf">N2975</a></td>
-      <td class="none" align="center">No</td>
+      <td class="unreleased" align="center">Clang 16</td>
     </tr>
     <tr>
       <td>Enhanced enumerations</td>
       <td><a href="https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm">N3030</a></td>
       <td class="unknown" align="center">Unknown</td>
     </tr>
     <tr>
       <td>Freestanding C and IEC 60559 conformance scope reduction</td>