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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
