Remove va_start diagnostic false positive with enumerations

[aaron.ballman]

Summary

r267338 improved the diagnostic checking for undefined behavior with va_start(), but it had some false positives regarding enumerations. The underlying type for an enumeration in C is either char, signed int, or unsigned int. In the case the underlying type is chosen to be char (such as when passing -fshort-enums or using attribute((packed)) on the enum declaration), the enumeration can result in undefined behavior. However, when the underlying type is signed int or unsigned int (or long long as an extension), there is no undefined behavior because the types are compatible. This patch silences diagnostics for the latter while retaining the diagnostics for the former.

This patch addresses PR29140.

[dim]

This works for me. I had two test cases from the FreeBSD source tree which resulted in warnings, e.g.:

cddl/contrib/opensolaris/lib/libnvpair/libnvpair.c:388:15: error: passing an object that undergoes default argument promotion to 'va_start' has undefined behavior
      [-Werror,-Wvarargs]
        va_start(ap, which);
                     ^
cddl/contrib/opensolaris/lib/libnvpair/libnvpair.c:382:66: note: parameter of type 'enum nvlist_prtctl_fmt' is declared here
nvlist_prtctl_dofmt(nvlist_prtctl_t pctl, enum nvlist_prtctl_fmt which, ...)
                                                                 ^
1 error generated.

This fix resolves both of them.

[aaron.ballman]

Ping

[aaron.ballman]

In D23921#531883, @aaron.ballman wrote:

Ping

Ping

[dim]

FWIW, I have already imported this fix into FreeBSD two weeks ago: https://svnweb.freebsd.org/changeset/base/304960
It has been working fine for us.

[aaron.ballman]

Commit in r281609

[torokati44]

I see this has been reverted in r281612, but I can no longer access the build log serving as a reason linked in the message: https://www.mail-archive.com/[e-mail]/msg35386.html
We have a few false-positive warnings that (I think) would be silenced by this change. I'm just wondering if something like this, if not this, could be included anyway? Not critical of course, it just would be nice.

lib/Sema/SemaChecking.cpp

     Diag(TheCall->getArg(1)->getLocStart(),
          diag::warn_second_arg_of_va_start_not_last_named_param);
   else if (IsCRegister || Type->isReferenceType() ||
-           Type->isPromotableIntegerType() ||
-           Type->isSpecificBuiltinType(BuiltinType::Float)) {
+           Type->isSpecificBuiltinType(BuiltinType::Float) || [=] {
+             // Promotable integers are UB, but enumerations need a bit of
+             // extra checking to see what their promotable type actually is.
+             if (!Type->isPromotableIntegerType())
+               return false;
+             if (!Type->isEnumeralType())
+               return true;
+             const EnumDecl *ED = Type->getAs<EnumType>()->getDecl();
+             return !(ED &&
+                      Context.typesAreCompatible(ED->getPromotionType(), Type));
+           }()) {
     unsigned Reason = 0;
     if (Type->isReferenceType())  Reason = 1;
     else if (IsCRegister)         Reason = 2;

test/Sema/varargs.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify %s -triple i386-pc-unknown
 // RUN: %clang_cc1 -fsyntax-only -verify %s -triple x86_64-apple-darwin9
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -DMS -verify %s
 
 void f1(int a)
 {

   __builtin_va_start(ap, i); // expected-warning {{passing a parameter declared with the 'register' keyword to 'va_start' has undefined behavior}}
   __builtin_va_end(ap);
 }
+
+enum __attribute__((packed)) E1 {
+  one1
+};
+
+void f13(enum E1 e, ...) {
+  __builtin_va_list va;
+  __builtin_va_start(va, e);
+#ifndef MS
+  // In Microsoft compatibility mode, all enum types are int, but in
+  // non-ms-compatibility mode, this enumeration type will undergo default
+  // argument promotions.
+  // expected-note@-7 {{parameter of type 'enum E1' is declared here}}
+  // expected-warning@-6 {{passing an object that undergoes default argument promotion to 'va_start' has undefined behavior}}
+#endif
+  __builtin_va_end(va);
+}