[Clang][Sema] DR722 (nullptr and varargs) and missing -Wvarargs diagnostics

[MitalAshok]

Summary

nullptr passed to a variadic function in C++ now converted to void*, per CWG722
va_arg on array types and half precision floats now diagnosed.

[MitalAshok]

There is one observable difference by explicitly casting nullptr_t to void*: prvalues of type nullptr_t aren't read from (which seemed unintentional):

static void* f(int x, ...) {
    __builtin_va_list args;
    __builtin_va_start(args, x);
    void* arg = __builtin_va_arg(args, void*);
    __builtin_va_end(args);
    return arg;
}

int main() {
    int x;
    void* x_ptr = &x;
    void* result = f(0, __builtin_bit_cast(decltype(nullptr), x_ptr));
    __builtin_printf("%p\n%p\n%p\n", x_ptr, result, nullptr);
}

https://godbolt.org/z/Pfv8Wbhxj

An object of type nullptr_t is passed to the function, but when it is read with void* arg = __builtin_va_arg(args, void*);, it is not a null pointer.

clang/test/Sema/format-strings-pedantic.c

21

In C2x, nullptr passed as an argument and retrieved via va_arg(ap, void *) is valid (See C2x 7.16.1.1p2) and this is the same as printf("%p", (void*) nullptr), but this should still be fine as a "pedantic" warning

clang/www/cxx_dr_status.html

4376

It may be better to mark this as "Yes", since in old clang versions passing nullptr would work by virtue of it having representation of a null void* pointer, and it only affected diagnostics

[MitalAshok]

clang/lib/Sema/SemaExpr.cpp

17323

This warns if you call va_arg(ap, double[2]). However this might be valid since the actual argument only has to be a "compatible type" and I think double _Complex is compatible with double[2]. I think we should warn anyways, since array rvalues are tricky to work with, and the user probably passed a double[2] and should retrieve the double*.

[MitalAshok]

Target clang 18 instead

[aaron.ballman]

Thank you for working on this! FWIW, precommit Ci found a relevant test failure that should also be addressed.

clang/lib/Sema/SemaExpr.cpp

1062–1073

Oh fun, C and C++ solved this issue differently. In C++, there's an implicit conversion so the nullptr_t is converted to void * on the call, and in C there's an allowance for va_arg to handle nullptr_t as a void *.

And without further changes, C++ picks that rule up automatically once it rebases onto C23: http://eel.is/c++draft/support#cstdarg.syn-1

17323

_Complex double and double[2] are not compatible types in C.

C2x 6.2.7p1: Two types are compatible types if they are the same. Additional rules for determining whether two types are compatible are described in 6.7.2 for type specifiers, in 6.7.3 for type qualifiers, and in 6.7.6 for declarators. ... (The rest is talking about structures, enumerations, and unions).

_Complex is a type specifier, so....

C2x 6.7.2p7: Each of the comma-separated multisets designates the same type, except that for bit-fields, it is implementation-defined whether the specifier int designates the same type as signed int or the same type as unsigned int.

(The multisets it describes do not include any array types.)

clang/test/Sema/format-strings-pedantic.c

21

I don't think this should be diagnosed pedantically; the code is correct.

[MitalAshok]

No longer warn on printf("%p", nullptr)

[MitalAshok]

C99 7.15.1.1p2 explicitly allows a char* to be retrieved as a void* with va_arg, so the -Wformat-pedantic warning was incorrect.

[aaron.ballman]

Precommit CI found a related issue that should be addressed.

clang/www/cxx_dr_status.html

4376

I think it's better to use Clang 18, otherwise people will think there's a diagnostic bug with earlier versions. We usually use "Yes" to mean "we've always supported this correctly, as best we can tell".

[MitalAshok]

Fix scanf("%p", (char**)ptr) for -Wformat-pedantic

Also added more tests for %p and -Wformat

[MitalAshok]

clang/test/Sema/format-pointer.c

39 ↗

(On Diff #547874)

Should this be a pedantic warning?

If this reads a non-null pointer, the bit pattern of np will be messed up but np will still read as a nullptr.
If it reads a null pointer, it should be fine.

[aaron.ballman]

clang/lib/AST/FormatString.cpp

484–487 ↗

(On Diff #547874)

We can simplify a bit further these days.

522–534 ↗

(On Diff #547874)

Fixes to match the usual coding style rules.

Wow, Phab's diff engine struggles with that edit. Basically: no else after a return, remove braces on single-line if, renamed to PointeeTy.

clang/lib/Sema/SemaExpr.cpp

1062–1072

I think this code should live in Sema::DefaultArgumentPromotion() because it's related specifically to default argument promotions: https://eel.is/c++draft/expr#call-11.sentence-5

17318–17324

Hmmm... the existing code seems wrong to me because it's not paying any attention to FLT_EVAL_METHOD, but I think it probably should? CC @jcranmer-intel @zahiraam for opinions.

Actually, I wonder if the correct approach here is to split Sema::DefaultArgumentPromotion() up so that we can calculate what the default argument promoted type is of the expression independent of performing the actual promotion, and call the promotion type calculation logic here?

clang/test/Sema/format-pointer.c

39 ↗

(On Diff #547874)

That's a fun scenario, good test case!

C23 7.26.6.2p10: Except in the case of a % specifier, the input item (or, in the case of a %n directive, the count of input characters) is converted to a type appropriate to the conversion specifier. If the input item is not a matching sequence, the execution of the directive fails: this condition is a matching failure. Unless assignment suppression was indicated by a *, the result of the conversion is placed in the object pointed to by the first argument following the format argument that has not already received a conversion result. If this object does not have an appropriate type, or if the result of the conversion cannot be represented in the object, the behavior is undefined.

p12, the p specifier: Matches an implementation-defined set of sequences, which should be the same as the set of sequences that may be produced by the %p conversion of the fprintf function. The corresponding argument shall be a pointer to a pointer of void. The input item is converted to a pointer value in an implementation-defined manner. If the input item is a value converted earlier during the same program execution, the pointer that results shall compare equal to that value; otherwise the behavior of the %p conversion is undefined.

The corresponding argument is not a pointer to a pointer of void, so it's UB, not just pedantically so.

[MitalAshok]

Address feedback; Reuse DefaultArgumentPromotion instead of duplicating its functionality when building VaArgExpr

There is the added bonus for it working with dependent types somewhat (template<typename T> __builtin_va_arg(ap, T[2]); will warn in the template instead of waiting for it to be instantiated).
I can add the check for dependent types back if this is unwanted.

I don't think there are any edge cases where this will give a false warning.

[MitalAshok]

clang-format

[jcranmer-intel]

clang/lib/Sema/SemaExpr.cpp

17318–17324

C23 6.5.2.2p6 [draft N3096] says "trailing arguments that have type float are promoted to double". FLT_EVAL_METHOD shouldn't need to apply here, since it's largely about reflecting that things like x87 using 80-bit precision internally, and not actual argument passing.

[aaron.ballman]

Precommit CI still has a related failure, it seems.

clang/lib/Sema/SemaExpr.cpp

17318–17324

Ah that's good to know, thank you!

[MitalAshok]

Use C's definition of type compatibility even in C++ mode (to not warn about enums promoted to their underlying types)

[MitalAshok]

clang-format

[MitalAshok]

clang/test/SemaCXX/varargs.cpp

34

Unscoped1 is promoted to int when passed to a variadic function.

The underlying type for Unscoped1 is unsigned int, so only Unscoped1 and unsigned int are compatible, not Unscoped1 and int. An Unscoped1 passed to a variadic function must be retrieved via va_arg(ap, int).

[MitalAshok]

clang/test/SemaCXX/varargs.cpp

34

Although I guess the warning is now wrong because even though void f(int x, ...) { std::va_list ap; va_start(ap, x); va_arg(ap, Unscoped1); } f(0, Unscoped1{2}) would be UB, f(0, 2u) would not be UB.

The user still should be warned about it, so I could create a new warning "second argument to 'va_arg' is of promotable enumeration type 'Unscoped1'; this va_arg may have undefined behavior because arguments of this enumeration type will be promoted to 'int', not the underlying type 'unsigned int'", and maybe suggest a fix Unscoped1{va_arg(ap, unsigned)}.

Or we could ignore it and pretend that int and enums with underlying types unsigned are compatible for the purposes of va_arg

[aaron.ballman]

clang/test/SemaCXX/varargs.cpp

34

I think we shouldn't warn in this case because of C23 7.16.1.1p2:

If type is not compatible with the type of the actual next argument (as promoted according to
the default argument promotions), the behavior is undefined, except for the following cases:
...
one type is compatible with a signed integer type, the other type is compatible with the
corresponding unsigned integer type, and the value is representable in both types;

Coupled with C23 6.7.2.2p13:

For all enumerations without a fixed underlying type, each enumerated type shall be compatible
with char or a signed or an unsigned integer type that is not bool or a bit-precise integer type. The
choice of type is implementation-defined., but shall be capable of representing the values of all
the members of the enumeration.

WDYT?

[MitalAshok]

enums now considered compatible with unsigned/signed versions of their underlying type for the purposes of -Wvarargs

[MitalAshok]

clang-format

[MitalAshok]

clang/test/SemaCXX/varargs.cpp

34

This seems to have changed very recently between N3096 (April C23 draft) and N3149 (July C23 draft) by N3112.

For reference, the old wording (also in C17) read:

one type is a signed integer type, the other type is the corresponding unsigned integer type,
and the value is representable in both types;

So with the current rules, yes they would be compatible and this shouldn't warn. I've changed it so it checks types with corresponding signedness.

[aaron.ballman]

I think this is looking close to good, just had some minor nits and an extra test case to consider.

clang/lib/Sema/SemaExpr.cpp

17182

17191–17195

This code is correct, but let's add an explicit test for the subtle edge case with _Atomic types. getUnqualifiedType() does not strip atomic qualifiers, so int and _Atomic int should remain incompatible.

clang/test/Sema/format-pointer.c

1–8 ↗

(On Diff #553056)

clang/test/SemaCXX/varargs.cpp

34

This seems to have changed very recently between N3096 (April C23 draft) and N3149 (July C23 draft) by N3112

Yes, this changed during ballot comment resolution at the June 2023 meeting; it was US-127 (see https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3148.doc for the paper trail)

So with the current rules, yes they would be compatible and this shouldn't warn. I've changed it so it checks types with corresponding signedness.

Thank you!

clang/docs/ReleaseNotes.rst

 C++23 Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 C++2c Feature Support
 ^^^^^^^^^^^^^^^^^^^^^
 
 Resolutions to C++ Defect Reports
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Implemented `DR722 <https://wg21.link/CWG722>`_ which promotes ``nullptr`` to ``void*``
+  when passed to a C-style variadic function.
 
 C Language Changes
 ------------------
 
 C2x Feature Support
 ^^^^^^^^^^^^^^^^^^^
 
 Non-comprehensive list of changes in this release

clang/lib/Sema/SemaExpr.cpp

 /// when we're in an unevaluated context.
 Sema::VarArgKind Sema::isValidVarArgType(const QualType &Ty) {
   if (Ty->isIncompleteType()) {
     // C++11 [expr.call]p7:
     //   After these conversions, if the argument does not have arithmetic,
     //   enumeration, pointer, pointer to member, or class type, the program
     //   is ill-formed.
     //
-    // Since we've already performed array-to-pointer and function-to-pointer
-    // decay, the only such type in C++ is cv void. This also handles
-    // initializer lists as variadic arguments.
+    // Since we've already performed null pointer conversion, array-to-pointer
+    // decay and function-to-pointer decay, the only such type in C++ is cv
+    // void. This also handles initializer lists as variadic arguments.
     if (Ty->isVoidType())
       return VAK_Invalid;
 
     if (Ty->isObjCObjectType())
       return VAK_Invalid;
     return VAK_Valid;
   }
 
if (ExprRes.isInvalid())
     return ExprError();
 
   // Copy blocks to the heap.
   if (ExprRes.get()->getType()->isBlockPointerType())
     maybeExtendBlockObject(ExprRes);
 
   E = ExprRes.get();
 
+  // C2x 6.5.2.2p6:
+  //   The integer promotions are performed on each trailing argument, and
+  //   trailing arguments that have type float are promoted to double. These are
+  //   called the default argument promotions. No other conversions are
+  //   performed implicitly.
+
+  // C++ [expr.call]p7, per DR722:
+  //   An argument that has (possibly cv-qualified) type std::nullptr_t is
+  //   converted to void* ([conv.ptr]).
+  if (E->getType()->isNullPtrType() && getLangOpts().CPlusPlus) {
+    E = ImpCastExprToType(E, Context.VoidPtrTy, CK_NullToPointer).get();
+  }
+
   // Diagnostics regarding non-POD argument types are
   // emitted along with format string checking in Sema::CheckFunctionCall().
   if (isValidVarArgType(E->getType()) == VAK_Undefined) {
     // Turn this into a trap.
     CXXScopeSpec SS;
     SourceLocation TemplateKWLoc;
     UnqualifiedId Name;
     Name.setIdentifier(PP.getIdentifierInfo("__builtin_trap"),
ExprResult Sema::ActOnVAArg(SourceLocation BuiltinLoc, Expr *E, ParsedType Ty,
                             SourceLocation RPLoc) {
   TypeSourceInfo *TInfo;
   GetTypeFromParser(Ty, &TInfo);
   return BuildVAArgExpr(BuiltinLoc, E, TInfo, RPLoc);
 }
 
 ExprResult Sema::BuildVAArgExpr(SourceLocation BuiltinLoc,
                                 Expr *E, TypeSourceInfo *TInfo,
                                 SourceLocation RPLoc) {
   Expr *OrigExpr = E;
   bool IsMS = false;
 
   // CUDA device code does not support varargs.
   if (getLangOpts().CUDA && getLangOpts().CUDAIsDevice) {
     if (const FunctionDecl *F = dyn_cast<FunctionDecl>(CurContext)) {
       CUDAFunctionTarget T = IdentifyCUDATarget(F);
       if (T == CFT_Global || T == CFT_Device || T == CFT_HostDevice)
         return ExprError(Diag(E->getBeginLoc(), diag::err_va_arg_in_device));
     }
   }
 
   // NVPTX does not support va_arg expression.
   if (getLangOpts().OpenMP && getLangOpts().OpenMPIsTargetDevice &&
       Context.getTargetInfo().getTriple().isNVPTX())
     targetDiag(E->getBeginLoc(), diag::err_va_arg_in_device);
 
   // It might be a __builtin_ms_va_list. (But don't ever mark a va_arg()
   // as Microsoft ABI on an actual Microsoft platform, where
   // __builtin_ms_va_list and __builtin_va_list are the same.)
   if (!E->isTypeDependent() && Context.getTargetInfo().hasBuiltinMSVaList() &&
if (Context.isPromotableIntegerType(TInfo->getType())) {
             UnderlyingType->isUnsignedIntegerType()
                 ? Context.getCorrespondingSignedType(UnderlyingType)
                 : Context.getCorrespondingUnsignedType(UnderlyingType);
         if (Context.typesAreCompatible(PromoteType, UnderlyingType,
                                        /*CompareUnqualified*/ true))
           PromoteType = QualType();
       }
     }
-    if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))
+    if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float) ||
+        TInfo->getType()->isSpecificBuiltinType(BuiltinType::Half))
       PromoteType = Context.DoubleTy;
+    if (TInfo->getType()->isNullPtrType() && getLangOpts().CPlusPlus)
+      PromoteType = Context.VoidPtrTy;
+    if (TInfo->getType()->isArrayType())
+      PromoteType = Context.getArrayDecayedType(TInfo->getType());
     if (!PromoteType.isNull())
       DiagRuntimeBehavior(TInfo->getTypeLoc().getBeginLoc(), E,
                   PDiag(diag::warn_second_parameter_to_va_arg_never_compatible)
                           << TInfo->getType()
                           << PromoteType
                           << TInfo->getTypeLoc().getSourceRange());
   }
 

clang/test/CXX/drs/dr7xx.cpp

struct X {
         (void)a.n; // FIXME: expected-error {{declared in enclosing}}
         (void)(a.*&A::n); // FIXME: expected-error {{declared in enclosing}}
       }
     };
   }
 #endif
 }
 
+namespace dr722 { // dr722: 18
+#if __cplusplus >= 201103L
+  int x = __builtin_printf("%p", nullptr);
+  void f(__builtin_va_list args) {
+    __builtin_va_arg(args, decltype(nullptr)); // expected-warning {{second argument to 'va_arg' is of promotable type 'decltype(nullptr)' (aka 'std::nullptr_t'); this va_arg has undefined behavior because arguments will be promoted to 'void *'}}
+  }
+#endif
+}
+
 namespace dr727 { // dr727: partial
   struct A {
     template<typename T> struct C; // expected-note 6{{here}}
     template<typename T> void f(); // expected-note {{here}}
     template<typename T> static int N; // expected-error 0-1{{C++14}} expected-note 6{{here}}
 
     template<> struct C<int>;
     template<> void f<int>();

clang/test/Sema/format-strings-pedantic.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
 // RUN: %clang_cc1 -xobjective-c -fblocks -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
 // RUN: %clang_cc1 -xc++ -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
+// RUN: %clang_cc1 -std=c2x -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
 
 __attribute__((format(printf, 1, 2)))
 int printf(const char *restrict, ...);
 
 int main(void) {
   printf("%p", (int *)0); // expected-warning {{format specifies type 'void *' but the argument has type 'int *'}}
   printf("%p", (void *)0);
 
 #ifdef __OBJC__
   printf("%p", ^{}); // expected-warning {{format specifies type 'void *' but the argument has type 'void (^)(void)'}}
   printf("%p", (id)0); // expected-warning {{format specifies type 'void *' but the argument has type 'id'}}
 #endif
 
 #ifdef __cplusplus
-  printf("%p", nullptr); // expected-warning {{format specifies type 'void *' but the argument has type 'std::nullptr_t'}}
+  printf("%p", nullptr);
+#elif !__is_identifier(nullptr)
+  printf("%p", nullptr); // expected-warning {{format specifies type 'void *' but the argument has type 'nullptr_t'}}
 #endif
 }

clang/test/SemaCXX/varargs.cpp

struct Foo {
     // expected-error@+1 {{default argument references parameter 'a'}}
     void meth(int a, int b = (__builtin_va_start(ap, a), 0)) {}
   };
 }
 
 // Ensure the correct behavior for promotable type UB checking.
 void promotable(int a, ...) {
   enum Unscoped1 { One = 0x7FFFFFFF };
   (void)__builtin_va_arg(ap, Unscoped1); // ok
 
   enum Unscoped2 { Two = 0xFFFFFFFF };
   (void)__builtin_va_arg(ap, Unscoped2); // ok
 
   enum class Scoped { Three };
   (void)__builtin_va_arg(ap, Scoped); // ok
 
   enum Fixed : int { Four };
   (void)__builtin_va_arg(ap, Fixed); // ok
 
   enum FixedSmall : char { Five };
   (void)__builtin_va_arg(ap, FixedSmall); // expected-warning {{second argument to 'va_arg' is of promotable type 'FixedSmall'; this va_arg has undefined behavior because arguments will be promoted to 'int'}}
 
   enum FixedLarge : long long { Six };
   (void)__builtin_va_arg(ap, FixedLarge); // ok
 
   // Ensure that qualifiers are ignored.
   (void)__builtin_va_arg(ap, const volatile int);  // ok
 
   // Ensure that signed vs unsigned doesn't matter either.
   (void)__builtin_va_arg(ap, unsigned int);
 
   (void)__builtin_va_arg(ap, bool); // expected-warning {{second argument to 'va_arg' is of promotable type 'bool'; this va_arg has undefined behavior because arguments will be promoted to 'int'}}
+
+  (void)__builtin_va_arg(ap, float); // expected-warning {{second argument to 'va_arg' is of promotable type 'float'; this va_arg has undefined behavior because arguments will be promoted to 'double'}}
+  (void)__builtin_va_arg(ap, __fp16); // expected-warning {{second argument to 'va_arg' is of promotable type '__fp16'; this va_arg has undefined behavior because arguments will be promoted to 'double'}}
+
+#if __cplusplus >= 201103L
+  (void)__builtin_va_arg(ap, decltype(nullptr)); // expected-warning {{second argument to 'va_arg' is of promotable type 'decltype(nullptr)' (aka 'std::nullptr_t'); this va_arg has undefined behavior because arguments will be promoted to 'void *'}}
+#endif
+
+  (void)__builtin_va_arg(ap, int[3]); // expected-warning {{second argument to 'va_arg' is of promotable type 'int[3]'; this va_arg has undefined behavior because arguments will be promoted to 'int *'}}
+  (void)__builtin_va_arg(ap, const int[3]); // expected-warning {{second argument to 'va_arg' is of promotable type 'const int[3]'; this va_arg has undefined behavior because arguments will be promoted to 'const int *'}}
 }
 
 #if __cplusplus >= 201103L
 // We used to have bugs identifying the correct enclosing function scope in a
 // lambda.
 
 void fixed_lambda_varargs_function(int a, ...) {
   [](int b) {

clang/www/cxx_dr_status.html

<tr id="721">
     <td>CD2</td>
     <td>Where must a variable be initialized to be used in a constant expression?</td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="722">
     <td><a href="https://cplusplus.github.io/CWG/issues/722.html">722</a></td>
     <td>CD2</td>
     <td>Can <TT>nullptr</TT> be passed to an ellipsis?</td>
-    <td class="none" align="center">Unknown</td>
+    <td class="unreleased" align="center">Clang 18</td>
   </tr>
   <tr id="726">
     <td><a href="https://cplusplus.github.io/CWG/issues/726.html">726</a></td>
     <td>CD2</td>
     <td>Atomic and non-atomic objects in the memory model</td>
     <td class="none" align="center">Unknown</td>
   </tr>
   <tr id="727">