Promote NPCs used as arguments to variadic functions

[rnk]

Summary

Make it possible to pass NULL through variadic functions on platforms
where NULL has an integer type that is more narrow than a pointer. On
such platforms we widen null pointer constants to a pointer-sized
integer.

Fixes PR20949.

[rnk]

Note that Linux headers sometimes define NULL to 0 on x86_64, which causes the same problem. This change will "fix" the problem by widening the int to an intptr_t.

[thakis]

lgtm, thanks!

For 32bit, this should never make a difference in practice. For 64bit, passing "0" to a varargs function will now be a 8 byte write instead of a 4 byte write, but arguments are 8-byte aligned there, and nothing should break if the padding bytes for 0s are now zero instead of random (and for pointers, it helps).

[rnk]

Woops, forgot to cc cfe-commits.

[dblaikie]

It's usually best to throw out a Phab review & start again if you don't cc
a mailing list when you first create it. (because phab won't re-send the
original mail - which means the mailing list has no context (no patch
description and more importantly, not attached patch))

[rnk]

• Move changes to CodeGen

[majnemer]

Perhaps a warning should be added so code doesn't "miscompile" when compiled with GCC?

Is there a reason to restrict this just to null pointer constants?

lib/CodeGen/CGCall.cpp

2601 ↗

(On Diff #14050)

This should be removed.

[rnk]

In D5480#13, @majnemer wrote:

Perhaps a warning should be added so code doesn't "miscompile" when compiled with GCC?

I don't think the warning is actionable for the user. How do we avoid false positives on printf("%d\n", 0)?

Users are typically saavy enough to know that passing '0' to a varargs function will be interpreted as an int, or at least be able to debug the crash once it happens. The subtle case is when passing NULL, which 99% of the time just works with GCC's stddef.h. In the 1% corner case where NULL is defined incorrectly (literal 0 and not 0L), I can live the fact that clang and gcc will have subtle behavior differences.

Is there a reason to restrict this just to null pointer constants?

I don't see why we should implicitly widen all ints to i64 in varargs contexts.

[rnk]

remove errs

[rnk]

ping

[rsmith]

I wonder whether it makes sense to restrict this to 64-bit MSVC platforms, since it's not really a problem elsewhere (other platforms have a sane NULL) and it presumably generates marginally worse code. Plus if we start doing this on, say, Mac OS, it'll make it harder for people to spot they have a bug until they port to another system.

lib/CodeGen/CGCall.cpp

2721 ↗

(On Diff #14349)

Is there any way we can check that the current target permits this? (That is, that widening an integer argument to pointer width will never change the calling convention.)

3024 ↗

(On Diff #14349)

Could just CreateZExt here. Presumably we should never coerce to a narrower type?

[rnk]

In D5480#19, @rsmith wrote:

I wonder whether it makes sense to restrict this to 64-bit MSVC platforms, since it's not really a problem elsewhere (other platforms have a sane NULL) and it presumably generates marginally worse code. Plus if we start doing this on, say, Mac OS, it'll make it harder for people to spot they have a bug until they port to another system.

OK, but there are other triples (WoA) that use the itanium environment that might have broken headers, so how about Windows platforms where pointer size != integer size? (aarch64, x86_64, ppc64)

lib/CodeGen/CGCall.cpp

2721 ↗

(On Diff #14349)

There is no such predicate currently. I don't think there is value in adding a predicate which will always return true for all current targets.

3024 ↗

(On Diff #14349)

Sure.

[rnk]

• Restrict it to windows.

[rsmith]

LGTM

test/CodeGenCXX/varargs.cpp

18–19 ↗

(On Diff #14674)

The comment is a lie...

[rnk]

Closed by commit rL219456 (authored by @rnk).

cfe/trunk/lib/CodeGen/CGCall.cpp

if (L.getAlignment() >= getContext().getTypeAlignInChars(type)) {
       args.add(RValue::getAggregate(tmp), type);
     }
     return;
   }
 
   args.add(EmitAnyExprToTemp(E), type);
 }
 
+QualType CodeGenFunction::getVarArgType(const Expr *Arg) {
+  // System headers on Windows define NULL to 0 instead of 0LL on Win64. MSVC
+  // implicitly widens null pointer constants that are arguments to varargs
+  // functions to pointer-sized ints.
+  if (!getTarget().getTriple().isOSWindows())
+    return Arg->getType();
+
+  if (Arg->getType()->isIntegerType() &&
+      getContext().getTypeSize(Arg->getType()) <
+          getContext().getTargetInfo().getPointerWidth(0) &&
+      Arg->isNullPointerConstant(getContext(),
+                                 Expr::NPC_ValueDependentIsNotNull)) {
+    return getContext().getIntPtrType();
+  }
+
+  return Arg->getType();
+}
+
 // In ObjC ARC mode with no ObjC ARC exception safety, tell the ARC
 // optimizer it can aggressively ignore unwind edges.
 void
 CodeGenFunction::AddObjCARCExceptionMetadata(llvm::Instruction *Inst) {
   if (CGM.getCodeGenOpts().OptimizationLevel != 0 &&
       !CGM.getCodeGenOpts().ObjCAutoRefCountExceptions)
     Inst->setMetadata("clang.arc.no_objc_arc_exceptions",
                       CGM.getNoObjCARCExceptionsMetadata());
case ABIArgInfo::Direct: {
           ArgInfo.getDirectOffset() == 0) {
         assert(NumIRArgs == 1);
         llvm::Value *V;
         if (RV.isScalar())
           V = RV.getScalarVal();
         else
           V = Builder.CreateLoad(RV.getAggregateAddr());
 
+        // We might have to widen integers, but we should never truncate.
+        if (ArgInfo.getCoerceToType() != V->getType() &&
+            V->getType()->isIntegerTy())
+          V = Builder.CreateZExt(V, ArgInfo.getCoerceToType());
+
         // If the argument doesn't match, perform a bitcast to coerce it.  This
         // can happen due to trivial type mismatches.
         if (FirstIRArg < IRFuncTy->getNumParams() &&
             V->getType() != IRFuncTy->getParamType(FirstIRArg))
           V = Builder.CreateBitCast(V, IRFuncTy->getParamType(FirstIRArg));
         IRCallArgs[FirstIRArg] = V;
         break;
       }

cfe/trunk/lib/CodeGen/CodeGenFunction.h

#endif
     // Either we've emitted all the call args, or we have a call to variadic
     // function.
     assert(
         (Arg == ArgEnd || !CallArgTypeInfo || CallArgTypeInfo->isVariadic()) &&
         "Extra arguments in non-variadic function!");
 
     // If we still have any arguments, emit them using the type of the argument.
     for (; Arg != ArgEnd; ++Arg)
-      ArgTypes.push_back(Arg->getType());
+      ArgTypes.push_back(getVarArgType(*Arg));
 
     EmitCallArgs(Args, ArgTypes, ArgBeg, ArgEnd, CalleeDecl, ParamsToSkip,
                  ForceColumnInfo);
   }
 
   void EmitCallArgs(CallArgList &Args, ArrayRef<QualType> ArgTypes,
                     CallExpr::const_arg_iterator ArgBeg,
                     CallExpr::const_arg_iterator ArgEnd,
                     const FunctionDecl *CalleeDecl = nullptr,
                     unsigned ParamsToSkip = 0, bool ForceColumnInfo = false);
 
 private:
+  QualType getVarArgType(const Expr *Arg);
+
   const TargetCodeGenInfo &getTargetHooks() const {
     return CGM.getTargetCodeGenInfo();
   }
 
   void EmitDeclMetadata();
 
   CodeGenModule::ByrefHelpers *
   buildByrefHelpers(llvm::StructType &byrefType,

cfe/trunk/test/CodeGen/variadic-null-win64.c

+// RUN: %clang_cc1 %s -emit-llvm -o - -triple x86_64-windows-msvc | FileCheck %s --check-prefix=WINDOWS
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple x86_64-linux | FileCheck %s --check-prefix=LINUX
+
+// Make it possible to pass NULL through variadic functions on platforms where
+// NULL has an integer type that is more narrow than a pointer. On such
+// platforms we widen null pointer constants to a pointer-sized integer.
+
+#define NULL 0
+
+void v(const char *f, ...);
+void f(const char *f) {
+  v(f, 1, 2, 3, NULL);
+}
+// WINDOWS: define void @f(i8* %f)
+// WINDOWS: call void (i8*, ...)* @v(i8* {{.*}}, i32 1, i32 2, i32 3, i64 0)
+// LINUX: define void @f(i8* %f)
+// LINUX: call void (i8*, ...)* @v(i8* {{.*}}, i32 1, i32 2, i32 3, i32 0)