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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17255
+  // Check if these are compatible types according to the C rules even in C++
+  // because va_arg is defined in C in terms of C compatibile types
+  static auto IsCompatible = [&](QualType L, QualType R) {
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17264-17268
+  QualType PromotedType = PromotedExpr.get()->getType().getUnqualifiedType();
+  QualType VAArgType = VAArg->getType().getUnqualifiedType();
+  // If these types are compatible, it was not promoted to an incompatible type.
+  if (IsCompatible(PromotedType, VAArgType))
+    return QualType();
----------------
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.


================
Comment at: clang/test/Sema/format-pointer.c:1-8
+// RUN: %clang_cc1 -xc -Wformat %s -verify
+// RUN: %clang_cc1 -xc -Wformat -std=c2x %s -verify
+// RUN: %clang_cc1 -xc++ -Wformat %s -verify
+// RUN: %clang_cc1 -xobjective-c -Wformat -fblocks %s -verify
+// RUN: %clang_cc1 -xobjective-c++ -Wformat -fblocks %s -verify
+// RUN: %clang_cc1 -xc -std=c2x -Wformat %s -pedantic -verify=expected,pedantic
+// RUN: %clang_cc1 -xc++ -Wformat %s -pedantic -verify=expected,pedantic
----------------



================
Comment at: clang/test/SemaCXX/varargs.cpp:34
   enum Unscoped1 { One = 0x7FFFFFFF };
-  (void)__builtin_va_arg(ap, Unscoped1); // ok
+  (void)__builtin_va_arg(ap, Unscoped1); // expected-warning {{second argument to 'va_arg' is of promotable type 'Unscoped1'; this va_arg has undefined behavior because arguments will be promoted to 'int'}}
 
----------------
MitalAshok wrote:
> aaron.ballman wrote:
> > MitalAshok wrote:
> > > MitalAshok wrote:
> > > > 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).
> > > > 
> > > 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
> > 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?
> This seems to have changed very recently between [[ https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf | N3096 ]] (April C23 draft) and [[ https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3149.zip | N3149 ]] (July C23 draft) by [[ https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3112.pdf | 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.
> 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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156054