Correct the behavior of va_arg checking in C++

[aaron.ballman]

Summary

Clang checks whether the type given to va_arg will automatically cause undefined behavior, but this check was issuing false positives for enumerations in C++. The issue turned out to be because typesAreCompatible() in C++ checks whether the types are *the same*, so this falls back on the type merging logic to see whether the types are mergable or not in both C and C++.

This issue was found by a user on code like:

typedef enum {
  CURLINFO_NONE,
  CURLINFO_EFFECTIVE_URL,
  CURLINFO_LASTONE = 60
} CURLINFO;

...

__builtin_va_arg(list, CURLINFO); // warn about CURLINFO promoting to 'int' being UB in C++ but not C

Given that C++ defers to C for the rules around va_arg, the behavior should be the same in both C and C++ and not diagnose because int and CURLINFO are compatible types.

[efriedma]

I'm a little nervous about using C type merging in C++; it's not designed to support C++ types, so it might end up crashing or something like that. I think I'd prefer to explicitly convert enum types to their underlying type.

[aaron.ballman]

Update based on review feedback.

[aaron.ballman]

In D103611#2797044, @efriedma wrote:

I'm a little nervous about using C type merging in C++; it's not designed to support C++ types, so it might end up crashing or something like that. I think I'd prefer to explicitly convert enum types to their underlying type.

I got lulled into thinking mergeTypes() worked for C++ because it handles references and cites the C++ standard first thing. But I see later that it also claims some C++ types are unreachable. So I switched to the underlying type for the enumeration, good suggestion!

[efriedma]

LGTM

[aaron.ballman]

Changed the test case to speculatively see if the CI pipeline is happy with this (I'm trying to avoid adding a triple to the test).

[aaron.ballman]

Added some more logic and new tests.

The CI pipeline pointed out a valid issue which has now been corrected -- C++ was not properly handling the case where one type was int and the other was unsigned int, which are compatible types in C but not the same type in C++. The test adds some triples and a new test case to ensure that we are able to consistently test this property in both directions.

@efriedma, would you mind taking a second look just to be sure you're still happy with the fix?

[efriedma]

clang/lib/Sema/SemaExpr.cpp

15783

This explanation doesn't seem right. Signed and unsigned types are never considered "compatible".

If I'm understanding correctly, the case this code addresses is promotion according to [conv.prom]p3: "A prvalue of an unscoped enumeration type whose underlying type is not fixed [...]". Somehow, the enum ends up with an unsigned underlying type, but we promote to int? And this doesn't happen in C somehow?

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

15783

This explanation doesn't seem right. Signed and unsigned types are never considered "compatible".

Good point, I think that comment is wrong.

If I'm understanding correctly, the case this code addresses is promotion according to [conv.prom]p3: "A prvalue of an unscoped enumeration type whose underlying type is not fixed [...]". Somehow, the enum ends up with an unsigned underlying type, but we promote to int? And this doesn't happen in C somehow?

That's correct. What I am seeing is:

enum Unscoped { One = 0x7FFFFFFF };

C++:
PromoteType = Builtin (Int)
UnderlyingType = Builtin (UInt)

C:
PromoteType = Builtin (UInt)
UnderlyingType = Builtin (UInt)

enum Unscoped { One = 0xFFFFFFFF };

C++:
PromoteType = Builtin (UInt)
UnderlyingType = Builtin (UInt)

C:
PromoteType = Builtin (UInt)
UnderlyingType = Builtin (UInt)

At least on i386-pc-unknown.

So I think this code is almost correct for that test, but is over-constrained by only doing this in C++. WDYT?

[efriedma]

clang/lib/Sema/SemaExpr.cpp

15775

If we're not going to take advantage of the C notion of compatibility, might as well just check hasSameType().

15783

That makes more sense.

Not sure this particular issue can show up in C; there's a check for C++ in Sema::ActOnEnumBody. But no harm at least.

[aaron.ballman]

Updated the comment and removed a check for C++ mode.

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

15775

We do still make use of that in C. For example, mergeTypes() (called by typesAreCompatible() in C mode) also does work for functions without a prototype (a distinctly C concept), special logic for merging enums and integers, qualifier handling, etc.

15789–15790

I believe I could switch to using hasSameType() here though, if you preferred.

[efriedma]

LGTM

[aaron.ballman]

Thank you for the reviews, @efriedma! I have committed in c92f505346b80fd053ef191bbc66810c9d564b0c

clang/lib/Sema/SemaExpr.cpp

         << TInfo->getTypeLoc().getSourceRange();
     }
 
     // Check for va_arg where arguments of the given type will be promoted
     // (i.e. this va_arg is guaranteed to have undefined behavior).
     QualType PromoteType;
     if (TInfo->getType()->isPromotableIntegerType()) {
       PromoteType = Context.getPromotedIntegerType(TInfo->getType());
-      if (Context.typesAreCompatible(PromoteType, TInfo->getType()))
+      // [cstdarg.syn]p1 defers the C++ behavior to what the C standard says,
+      // and C2x 7.16.1.1p2 says, in part:
+      //   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:
+      //     - both types are pointers to qualified or unqualified versions of
+      //       compatible types;
+      //     - one type is a signed integer type, the other type is the
+      //       corresponding unsigned integer type, and the value is
+      //       representable in both types;
+      //     - one type is pointer to qualified or unqualified void and the
+      //       other is a pointer to a qualified or unqualified character type.
+      // Given that type compatibility is the primary requirement (ignoring
+      // qualifications), you would think we could call typesAreCompatible()
+      // directly to test this. However, in C++, that checks for *same type*,
+      // which causes false positives when passing an enumeration type to
+      // va_arg. Instead, get the underlying type of the enumeration and pass
+      // that.
+      QualType UnderlyingType = TInfo->getType();
+      if (const auto *ET = UnderlyingType->getAs<EnumType>())
+        UnderlyingType = ET->getDecl()->getIntegerType();
+      if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+                                     /*CompareUnqualified*/ true))
         PromoteType = QualType();
+
+      // If the types are still not compatible, we need to test whether the
+      // promoted type and the underlying type are the same except for
+      // signedness. Ask the AST for the correctly corresponding type and see
+      // if that's compatible.
+      if (!PromoteType.isNull() &&
+          PromoteType->isUnsignedIntegerType() !=
+              UnderlyingType->isUnsignedIntegerType()) {
+        UnderlyingType =
+            UnderlyingType->isUnsignedIntegerType()
+                ? Context.getCorrespondingSignedType(UnderlyingType)
+                : Context.getCorrespondingUnsignedType(UnderlyingType);
+        if (Context.typesAreCompatible(PromoteType, UnderlyingType,
+                                       /*CompareUnqualified*/ true))
+          PromoteType = QualType();
+      }
     }
     if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))
       PromoteType = Context.DoubleTy;
     if (!PromoteType.isNull())
       DiagRuntimeBehavior(TInfo->getTypeLoc().getBeginLoc(), E,
                   PDiag(diag::warn_second_parameter_to_va_arg_never_compatible)
                           << TInfo->getType()
                           << PromoteType

clang/test/SemaCXX/varargs.cpp

-// RUN: %clang_cc1 -std=c++03 -verify %s
-// RUN: %clang_cc1 -std=c++11 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -triple i386-pc-unknown -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin9 -verify %s
 
 __builtin_va_list ap;
 
 class string;
 void f(const string& s, ...) {  // expected-note {{parameter of type 'const string &' is declared here}}
   __builtin_va_start(ap, s); // expected-warning {{passing an object of reference type to 'va_start' has undefined behavior}}
 }
 

 void record_context(int a, ...) {
   struct Foo {
     // expected-error@+2 {{'va_start' cannot be used outside a function}}
     // 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);
+}
+
 #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) {
     __builtin_va_start(ap, b); // expected-error {{'va_start' used in function with fixed args}}
   }(42);