Fix a codegen bug for variadic functions with pass_object_size params

[george.burgess.iv]

Summary

Currently, we get assertion failures/segfaults for variadic functions with pass_object_size params, e.g.:

void foo(void *const __attribute__((pass_object_size(0))) a, ...) {}

This is because we didn't consider the parameter injected by pass_object_size "required" when generating the CGFunctionInfo for foo. This patch teaches clang that said injected params are required.

(N.B. Only one user of appendParameterTypes is patched here because the other user, arrangeCXXStructorDeclaration, considers all args to be required).

[rsmith]

lib/CodeGen/CGCall.cpp

142–143 ↗

(On Diff #48538)

Would it make sense for RequiredArgs::forPrototypePlus to do this computation? Are the other callers of that function also getting the wrong number of required arguments in this case?

[george.burgess.iv]

Addressed all feedback

lib/CodeGen/CGCall.cpp

142–143 ↗

(On Diff #48538)

...There was a reason I didn't do that. I don't remember what it was, so either it disappeared, or I was just being dumb, but there was a reason. Either way, I moved this logic into RequiredArgs, and ended up fixing a few more bugs in the process. Yay for (fixing) bugs!

[george.burgess.iv]

Ping :)

[george.burgess.iv]

Ping :)

[george.burgess.iv]

Ping :)

[george.burgess.iv]

Ping :)

[george.burgess.iv]

(: ƃuıd

[rsmith]

lib/CodeGen/CGExprCXX.cpp

57–58 ↗

(On Diff #51374)

Comment doesn't seem to match the code; Args.size() here includes only the 'this' pointer and the possible destructor VTT parameter. Delete comment?

331 ↗

(On Diff #51374)

Seems inconsistent to pass E->getDirectCallee() here but not above. Since you can't take the address of a pass_object_size function, do we need to pass the callee in either place?

[george.burgess.iv]

Addressed all feedback.

lib/CodeGen/CGExprCXX.cpp

331 ↗

(On Diff #51374)

Good point. :)

cfe/trunk/include/clang/CodeGen/CGFunctionInfo.h

 // LLVM source types and ABI-coerced types for function arguments and
 // return values.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_CODEGEN_CGFUNCTIONINFO_H
 #define LLVM_CLANG_CODEGEN_CGFUNCTIONINFO_H
 
+#include "clang/AST/Attr.h"
 #include "clang/AST/CanonicalType.h"
 #include "clang/AST/CharUnits.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/Support/TrailingObjects.h"
 #include <cassert>
 
 namespace clang {
-class Decl;
-
 namespace CodeGen {
 
 /// ABIArgInfo - Helper class to encapsulate information about how a
 /// specific C type should be passed to or returned from a function.
 class ABIArgInfo {
 public:
   enum Kind : uint8_t {
     /// Direct - Pass the argument directly using the normal converted LLVM
public:
   RequiredArgs(All_t _) : NumRequired(~0U) {}
   explicit RequiredArgs(unsigned n) : NumRequired(n) {
     assert(n != ~0U);
   }
 
   /// Compute the arguments required by the given formal prototype,
   /// given that there may be some additional, non-formal arguments
   /// in play.
+  ///
+  /// If FD is not null, this will consider pass_object_size params in FD.
   static RequiredArgs forPrototypePlus(const FunctionProtoType *prototype,
-                                       unsigned additional) {
+                                       unsigned additional,
+                                       const FunctionDecl *FD) {
     if (!prototype->isVariadic()) return All;
+    if (FD)
+      additional += std::count_if(FD->param_begin(), FD->param_end(),
+                                  [](const ParmVarDecl *PVD) {
+                                    return PVD->hasAttr<PassObjectSizeAttr>();
+                                  });
     return RequiredArgs(prototype->getNumParams() + additional);
   }
 
-  static RequiredArgs forPrototype(const FunctionProtoType *prototype) {
-    return forPrototypePlus(prototype, 0);
+  static RequiredArgs forPrototype(const FunctionProtoType *prototype,
+                                   const FunctionDecl *FD) {
+    return forPrototypePlus(prototype, 0, FD);
   }
 
-  static RequiredArgs forPrototype(CanQual<FunctionProtoType> prototype) {
-    return forPrototype(prototype.getTypePtr());
+  static RequiredArgs forPrototype(CanQual<FunctionProtoType> prototype,
+                                   const FunctionDecl *FD) {
+    return forPrototype(prototype.getTypePtr(), FD);
   }
 
   static RequiredArgs forPrototypePlus(CanQual<FunctionProtoType> prototype,
-                                       unsigned additional) {
-    return forPrototypePlus(prototype.getTypePtr(), additional);
+                                       unsigned additional,
+                                       const FunctionDecl *FD) {
+    return forPrototypePlus(prototype.getTypePtr(), additional, FD);
   }
 
   bool allowsOptionalArgs() const { return NumRequired != ~0U; }
   unsigned getNumRequiredArgs() const {
     assert(allowsOptionalArgs());
     return NumRequired;
   }
 

cfe/trunk/lib/CodeGen/CGCall.cpp

 /// Arrange the LLVM function layout for a value of the given function
 /// type, on top of any implicit parameters already stored.
 static const CGFunctionInfo &
 arrangeLLVMFunctionInfo(CodeGenTypes &CGT, bool instanceMethod,
                         SmallVectorImpl<CanQualType> &prefix,
                         CanQual<FunctionProtoType> FTP,
                         const FunctionDecl *FD) {
   SmallVector<FunctionProtoType::ExtParameterInfo, 16> paramInfos;
-  RequiredArgs required = RequiredArgs::forPrototypePlus(FTP, prefix.size());
+  RequiredArgs Required =
+      RequiredArgs::forPrototypePlus(FTP, prefix.size(), FD);
   // FIXME: Kill copy.
   appendParameterTypes(CGT, prefix, paramInfos, FTP, FD);
   CanQualType resultType = FTP->getReturnType().getUnqualifiedType();
 
   return CGT.arrangeLLVMFunctionInfo(resultType, instanceMethod,
                                      /*chainCall=*/false, prefix,
                                      FTP->getExtInfo(), paramInfos,
-                                     required);
+                                     Required);
 }
 
 /// Arrange the argument and result information for a value of the
 /// given freestanding function type.
 const CGFunctionInfo &
 CodeGenTypes::arrangeFreeFunctionType(CanQual<FunctionProtoType> FTP,
                                       const FunctionDecl *FD) {
   SmallVector<CanQualType, 16> argTypes;
CodeGenTypes::arrangeCXXConstructorCall(const CallArgList &args,
                                         CXXCtorType CtorKind,
                                         unsigned ExtraArgs) {
   // FIXME: Kill copy.
   SmallVector<CanQualType, 16> ArgTypes;
   for (const auto &Arg : args)
     ArgTypes.push_back(Context.getCanonicalParamType(Arg.Ty));
 
   CanQual<FunctionProtoType> FPT = GetFormalType(D);
-  RequiredArgs Required = RequiredArgs::forPrototypePlus(FPT, 1 + ExtraArgs);
+  RequiredArgs Required = RequiredArgs::forPrototypePlus(FPT, 1 + ExtraArgs, D);
   GlobalDecl GD(D, CtorKind);
   CanQualType ResultType = TheCXXABI.HasThisReturn(GD)
                                ? ArgTypes.front()
                                : TheCXXABI.hasMostDerivedReturn(GD)
                                      ? CGM.getContext().VoidPtrTy
                                      : Context.VoidTy;
 
   FunctionType::ExtInfo Info = FPT->getExtInfo();

 }
 
 const CGFunctionInfo &
 CodeGenTypes::arrangeBlockFunctionDeclaration(const FunctionProtoType *proto,
                                               const FunctionArgList &params) {
   auto paramInfos = getExtParameterInfosForCall(proto, 1, params.size());
   auto argTypes = getArgTypesForDeclaration(Context, params);
 
-  return arrangeLLVMFunctionInfo(GetReturnType(proto->getReturnType()),
-                                 /*instanceMethod*/ false, /*chainCall*/ false,
-                                 argTypes, proto->getExtInfo(), paramInfos,
-                                 RequiredArgs::forPrototypePlus(proto, 1));
+  return arrangeLLVMFunctionInfo(
+      GetReturnType(proto->getReturnType()),
+      /*instanceMethod*/ false, /*chainCall*/ false, argTypes,
+      proto->getExtInfo(), paramInfos,
+      RequiredArgs::forPrototypePlus(proto, 1, nullptr));
 }
 
 const CGFunctionInfo &
 CodeGenTypes::arrangeBuiltinFunctionCall(QualType resultType,
                                          const CallArgList &args) {
   // FIXME: Kill copy.
   SmallVector<CanQualType, 16> argTypes;
   for (const auto &Arg : args)

cfe/trunk/lib/CodeGen/CGExprCXX.cpp

commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD,
   Args.add(RValue::get(This), MD->getThisType(CGF.getContext()));
 
   // If there is an implicit parameter (e.g. VTT), emit it.
   if (ImplicitParam) {
     Args.add(RValue::get(ImplicitParam), ImplicitParamTy);
   }
 
   const FunctionProtoType *FPT = MD->getType()->castAs<FunctionProtoType>();
-  RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size());
+  RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size(), MD);
 
   // And the rest of the call args.
   if (CE) {
     // Special case: skip first argument of CXXOperatorCall (it is "this").
     unsigned ArgsToSkip = isa<CXXOperatorCallExpr>(CE) ? 1 : 0;
     CGF.EmitCallArgs(Args, FPT, drop_begin(CE->arguments(), ArgsToSkip),
                      CE->getDirectCallee());
   } else {
CodeGenFunction::EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E,
   CallArgList Args;
 
   QualType ThisType = 
     getContext().getPointerType(getContext().getTagDeclType(RD));
 
   // Push the this ptr.
   Args.add(RValue::get(ThisPtrForCall), ThisType);
 
-  RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, 1);
-  
+  RequiredArgs required =
+      RequiredArgs::forPrototypePlus(FPT, 1, /*FD=*/nullptr);
   // And the rest of the call args
-  EmitCallArgs(Args, FPT, E->arguments(), E->getDirectCallee());
+  EmitCallArgs(Args, FPT, E->arguments());
   return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required),
                   Callee, ReturnValue, Args);
 }
 
 RValue
 CodeGenFunction::EmitCXXOperatorMemberCallExpr(const CXXOperatorCallExpr *E,
                                                const CXXMethodDecl *MD,
                                                ReturnValueSlot ReturnValue) {

cfe/trunk/lib/CodeGen/CGVTables.cpp

void CodeGenFunction::EmitCallAndReturnForThunk(llvm::Value *Callee,
 
   // Add the rest of the arguments.
   for (const ParmVarDecl *PD : MD->params())
     EmitDelegateCallArg(CallArgs, PD, PD->getLocStart());
 
   const FunctionProtoType *FPT = MD->getType()->getAs<FunctionProtoType>();
 
 #ifndef NDEBUG
-  const CGFunctionInfo &CallFnInfo =
-    CGM.getTypes().arrangeCXXMethodCall(CallArgs, FPT,
-                                       RequiredArgs::forPrototypePlus(FPT, 1));
+  const CGFunctionInfo &CallFnInfo = CGM.getTypes().arrangeCXXMethodCall(
+      CallArgs, FPT, RequiredArgs::forPrototypePlus(FPT, 1, MD));
   assert(CallFnInfo.getRegParm() == CurFnInfo->getRegParm() &&
          CallFnInfo.isNoReturn() == CurFnInfo->isNoReturn() &&
          CallFnInfo.getCallingConvention() == CurFnInfo->getCallingConvention());
   assert(isa<CXXDestructorDecl>(MD) || // ignore dtor return types
          similar(CallFnInfo.getReturnInfo(), CallFnInfo.getReturnType(),
                  CurFnInfo->getReturnInfo(), CurFnInfo->getReturnType()));
   assert(CallFnInfo.arg_size() == CurFnInfo->arg_size());
   for (unsigned i = 0, e = CurFnInfo->arg_size(); i != e; ++i)

cfe/trunk/test/CodeGen/pass-object-size.c

void test13() {
 
   // CHECK: @llvm.objectsize
   ObjectSize0(p);
 
   // CHECK-NOT: @llvm.objectsize
   ObjectSize0(++p);
   ObjectSize0(p++);
 }
+
+// There was a bug where variadic functions with pass_object_size would cause
+// problems in the form of failed assertions.
+void my_sprintf(char *const c __attribute__((pass_object_size(0))), ...) {}
+
+// CHECK-LABEL: define void @test14
+void test14(char *c) {
+  // CHECK: @llvm.objectsize
+  // CHECK: call void (i8*, i64, ...) @my_sprintf
+  my_sprintf(c);
+
+  // CHECK: @llvm.objectsize
+  // CHECK: call void (i8*, i64, ...) @my_sprintf
+  my_sprintf(c, 1, 2, 3);
+}

cfe/trunk/test/CodeGenCXX/pass-object-size.cpp

namespace delegate {
   struct A {
     A(void *const p __attribute__((pass_object_size(0))));
   };
   A::A(void *const p __attribute__((pass_object_size(0)))) {}
   // Ensure that we forward the size through a delegating constructor call.
   // CHECK: define void @_ZN8delegate1AC1EPvU17pass_object_size0({{[^,]*}}, i8*{{[^,]*}}, i64{{[^,]*}})
   // CHECK: call void @_ZN8delegate1AC2EPvU17pass_object_size0({{[^,]*}}, i8*{{[^,]*}}, i64{{[^,]*}})
 }
+
+namespace variadic {
+// We had an issue where variadic member/operator calls with pass_object_size
+// would cause crashes.
+
+struct AsCtor {
+  AsCtor(const char *const c __attribute__((pass_object_size(0))), double a,
+         ...) {}
+};
+
+struct AsMember {
+  void bar(const char *const c __attribute__((pass_object_size(0))), double a,
+           ...) {}
+  void operator()(const char *const c __attribute__((pass_object_size(0))),
+                  double a, ...) {}
+};
+
+// CHECK-LABEL: define void @_ZN8variadic4testEv()
+void test() {
+  // CHECK-RE: call{{[^@]+}}@_ZN8variadic6AsCtorC1EPKcU17pass_object_size0dz
+  AsCtor("a", 1.0);
+  // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMember3barEPKcU17pass_object_size0dz
+  AsMember{}.bar("a", 1.0);
+  // CHECK-RE: call{{[^@]+}}@_ZN8variadic8AsMemberclEPKcU17pass_object_size0dz
+  AsMember{}("a", 1.0);
+}
+}