Don't trap when passing non-POD arguments to variadic functions in MS-compatibility mode

[hans]

Summary

Clang warns (treated as error by default, but still ignored in system headers) when passing non-POD arguments to variadic functions, and generates a trap instruction to crash the program if that code is ever run.

Unfortunately, MSVC happily generates code for such calls without a warning, and there is code in system headers that use it.

This patch makes Clang not insert the trap instruction when in -fms-compatibility mode, while still generating the warning/error message.

[rnk]

Works for me if Richard is OK with it.

It's also worth noting that we already do exactly what MSVC does, which is a bitwise copy of the argument. The callee does *not* destroy the copy, so the program behaves "correctly" despite the UB. We don't need to go further out of our way to make it work.

test/CodeGenCXX/vararg-non-pod-ms-compat.cpp

1 ↗

(On Diff #14077)

Can you add a win64 test case? IIRC we coerce to i64 there, which is compatible with what MSVC does.

[hans]

Adding 64-bit test case as suggested by Reid.

[rsmith]

Looks terrible, go ahead. =)

lib/Sema/SemaExpr.cpp

878–879 ↗

(On Diff #14082)

Maybe this should be a separate VAK_ value, VAK_ValidInMSVC or similar?

[hans]

lib/Sema/SemaExpr.cpp

878–879 ↗

(On Diff #14082)

I used VAK_MSVCUndefined.

[hans]

Closed by commit rL218640 (authored by @hans).

cfe/trunk/include/clang/Sema/Sema.h

VariadicCallType getVariadicCallType(FunctionDecl *FDecl,
                                        Expr *Fn);
 
   // Used for determining in which context a type is allowed to be passed to a
   // vararg function.
   enum VarArgKind {
     VAK_Valid,
     VAK_ValidInCXX11,
     VAK_Undefined,
+    VAK_MSVCUndefined,
     VAK_Invalid
   };
 
   // Determines which VarArgKind fits an expression.
   VarArgKind isValidVarArgType(const QualType &Ty);
 
   /// Check to see if the given expression is a valid argument to a variadic
   /// function, issuing a diagnostic if not.

cfe/trunk/lib/Sema/SemaChecking.cpp

case Sema::VAK_ValidInCXX11:
         S.PDiag(diag::warn_format_conversion_argument_type_mismatch)
           << AT.getRepresentativeTypeName(S.Context) << ExprTy << IsEnum
           << CSR
           << E->getSourceRange(),
         E->getLocStart(), /*IsStringLocation*/false, CSR);
       break;
 
     case Sema::VAK_Undefined:
+    case Sema::VAK_MSVCUndefined:
       EmitFormatDiagnostic(
         S.PDiag(diag::warn_non_pod_vararg_with_format_string)
           << S.getLangOpts().CPlusPlus11
           << ExprTy
           << CallType
           << AT.getRepresentativeTypeName(S.Context)
           << CSR
           << E->getSourceRange(),

cfe/trunk/lib/Sema/SemaExpr.cpp

if (CXXRecordDecl *Record = Ty->getAsCXXRecordDecl())
         return VAK_ValidInCXX11;
 
   if (getLangOpts().ObjCAutoRefCount && Ty->isObjCLifetimeType())
     return VAK_Valid;
 
   if (Ty->isObjCObjectType())
     return VAK_Invalid;
 
+  if (getLangOpts().MSVCCompat)
+    return VAK_MSVCUndefined;
+
   // FIXME: In C++11, these cases are conditionally-supported, meaning we're
   // permitted to reject them. We should consider doing so.
   return VAK_Undefined;
 }
 
 void Sema::checkVariadicArgument(const Expr *E, VariadicCallType CT) {
   // Don't allow one to pass an Objective-C interface to a vararg.
   const QualType &Ty = E->getType();
if (Ty->isRecordType()) {
       // 'c_str' member function, the user probably meant to call that.
       DiagRuntimeBehavior(E->getLocStart(), nullptr,
                           PDiag(diag::warn_pass_class_arg_to_vararg)
                             << Ty << CT << hasCStrMethod(E) << ".c_str()");
     }
     break;
 
   case VAK_Undefined:
+  case VAK_MSVCUndefined:
     DiagRuntimeBehavior(
         E->getLocStart(), nullptr,
         PDiag(diag::warn_cannot_pass_non_pod_arg_to_vararg)
           << getLangOpts().CPlusPlus11 << Ty << CT);
     break;
 
   case VAK_Invalid:
     if (Ty->isObjCObjectType())

cfe/trunk/test/CodeGenCXX/vararg-non-pod-ms-compat.cpp

+// RUN: %clang_cc1 -Wno-error=non-pod-varargs -triple i686-pc-win32 -fms-compatibility -emit-llvm -o - %s | FileCheck %s -check-prefix=X86 -check-prefix=CHECK
+// RUN: %clang_cc1 -Wno-error=non-pod-varargs -triple x86_64-pc-win32 -fms-compatibility -emit-llvm -o - %s | FileCheck %s -check-prefix=X64 -check-prefix=CHECK
+
+struct X {
+  X();
+  ~X();
+  int data;
+};
+
+void vararg(...);
+
+void test(X x) {
+  // CHECK-LABEL: define void @"\01?test@@YAXUX@@@Z"
+
+  // X86: %[[argmem:[^ ]*]] = alloca inalloca <{ %struct.X }>
+  // X86: call void (<{ %struct.X }>*, ...)* bitcast (void (...)* @"\01?vararg@@YAXZZ" to void (<{ %struct.X }>*, ...)*)(<{ %struct.X }>* inalloca %[[argmem]])
+
+  // X64: %[[valptr:[^ ]*]] = getelementptr %struct.X* %{{[^ ]*}}, i32 0, i32 0
+  // X64: %[[val:[^ ]*]] = load i32* %[[valptr]]
+  // X64: call void (...)* @"\01?vararg@@YAXZZ"(i32 %[[val]])
+
+  // CHECK-NOT: llvm.trap
+  vararg(x);
+  // CHECK: ret void
+}
+

cfe/trunk/test/SemaCXX/vararg-non-pod.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -fblocks %s -Wno-error=non-pod-varargs
 
+// Check that the warning is still there under -fms-compatibility.
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks %s -Wno-error=non-pod-varargs -fms-compatibility
+
 extern char version[];
 
 class C {
 public:
   C(int);
   void g(int a, ...);
   static void h(int a, ...);
 };