[PATCH] D103611: Correct the behavior of va_arg checking in C++

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
aaron.ballman added reviewers: rsmith, eli.friedman, rjmccall.
aaron.ballman requested review of this revision.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103611

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/varargs.cpp


Index: clang/test/SemaCXX/varargs.cpp
===================================================================
--- clang/test/SemaCXX/varargs.cpp
+++ clang/test/SemaCXX/varargs.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++03 -verify %s
+// RUN: %clang_cc1 -std=c++03 -Wno-c++11-extensions -verify %s
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 __builtin_va_list ap;
@@ -28,6 +28,30 @@
   };
 }
 
+// Ensure the correct behavior for promotable type UB checking.
+void promotable(int a, ...) {
+  enum Unscoped { One };
+  (void)__builtin_va_arg(ap, Unscoped); // ok
+
+  enum class Scoped { Two };
+  (void)__builtin_va_arg(ap, Scoped); // ok
+
+  enum Fixed : int { Three };
+  (void)__builtin_va_arg(ap, Fixed); // ok
+
+  enum FixedSmall : char { Four };
+  (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 { Five };
+  (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.
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15750,7 +15750,27 @@
     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() to
+      // test this. However, in C++, that checks for *same type*, which causes
+      // false positives when passing an enumeration type to va_arg. Instead,
+      // attempt to merge the types and see how well that works.
+      QualType Merged =
+          Context.mergeTypes(PromoteType, TInfo->getType(),
+                             /*OfBlockPointer*/ false, /*Unqualified*/ true);
+      if (!Merged.isNull())
         PromoteType = QualType();
     }
     if (TInfo->getType()->isSpecificBuiltinType(BuiltinType::Float))


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103611.349524.patch
Type: text/x-patch
Size: 3302 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210603/e3a60199/attachment.bin>