From 2acaa49f61e4e22c068afa10ec36430dfdbd9e05 Mon Sep 17 00:00:00 2001 From: George Burgess IV Date: Thu, 23 Feb 2017 22:07:35 +0000 Subject: [PATCH] [CodeGen] Fix ExtParameterInfo bugs in C++ CodeGen code. This patch makes use of the prefix/suffix ABI argument distinction that was introduced in r295870, so that we now emit ExtParameterInfo at the correct offset for member calls that have added ABI arguments. I don't see a good way to test the generated param info, since we don't actually seem to use it in CGFunctionInfo outside of Swift. Any suggestions/thoughts for how to better test this are welcome. :) This patch also fixes a small bug with inheriting constructors: if we decide not to pass args into an base class ctor, we would still generate ExtParameterInfo as though we did. The added test-case is for that behavior. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@296024 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGCall.cpp | 42 ++++++++++++++++++++++------- lib/CodeGen/CGClass.cpp | 9 ++++--- lib/CodeGen/CGExprCXX.cpp | 21 +++++++++++---- lib/CodeGen/CGVTables.cpp | 3 ++- lib/CodeGen/CodeGenTypes.h | 7 +++-- lib/CodeGen/MicrosoftCXXABI.cpp | 2 +- test/CodeGenObjCXX/arc-attrs-abi.mm | 30 ++++++++++++++++++--- 7 files changed, 88 insertions(+), 26 deletions(-) diff --git a/lib/CodeGen/CGCall.cpp b/lib/CodeGen/CGCall.cpp index b65b659fcc..428098da26 100644 --- a/lib/CodeGen/CGCall.cpp +++ b/lib/CodeGen/CGCall.cpp @@ -362,18 +362,31 @@ getExtParameterInfosForCall(const FunctionProtoType *proto, } /// Arrange a call to a C++ method, passing the given arguments. +/// +/// ExtraPrefixArgs is the number of ABI-specific args passed after the `this` +/// parameter. +/// ExtraSuffixArgs is the number of ABI-specific args passed at the end of +/// args. +/// PassProtoArgs indicates whether `args` has args for the parameters in the +/// given CXXConstructorDecl. const CGFunctionInfo & CodeGenTypes::arrangeCXXConstructorCall(const CallArgList &args, const CXXConstructorDecl *D, CXXCtorType CtorKind, - unsigned ExtraArgs) { + unsigned ExtraPrefixArgs, + unsigned ExtraSuffixArgs, + bool PassProtoArgs) { // FIXME: Kill copy. SmallVector ArgTypes; for (const auto &Arg : args) ArgTypes.push_back(Context.getCanonicalParamType(Arg.Ty)); + // +1 for implicit this, which should always be args[0]. + unsigned TotalPrefixArgs = 1 + ExtraPrefixArgs; + CanQual FPT = GetFormalType(D); - RequiredArgs Required = RequiredArgs::forPrototypePlus(FPT, 1 + ExtraArgs, D); + RequiredArgs Required = + RequiredArgs::forPrototypePlus(FPT, TotalPrefixArgs + ExtraSuffixArgs, D); GlobalDecl GD(D, CtorKind); CanQualType ResultType = TheCXXABI.HasThisReturn(GD) ? ArgTypes.front() @@ -382,8 +395,14 @@ CodeGenTypes::arrangeCXXConstructorCall(const CallArgList &args, : Context.VoidTy; FunctionType::ExtInfo Info = FPT->getExtInfo(); - auto ParamInfos = getExtParameterInfosForCall(FPT.getTypePtr(), 1 + ExtraArgs, - ArgTypes.size()); + llvm::SmallVector ParamInfos; + // If the prototype args are elided, we should only have ABI-specific args, + // which never have param info. + if (PassProtoArgs && FPT->hasExtParameterInfos()) { + // ABI-specific suffix arguments are treated the same as variadic arguments. + addExtParameterInfosForCall(ParamInfos, FPT.getTypePtr(), TotalPrefixArgs, + ArgTypes.size()); + } return arrangeLLVMFunctionInfo(ResultType, /*instanceMethod=*/true, /*chainCall=*/false, ArgTypes, Info, ParamInfos, Required); @@ -627,15 +646,20 @@ CodeGenTypes::arrangeBuiltinFunctionDeclaration(CanQualType resultType, } /// Arrange a call to a C++ method, passing the given arguments. +/// +/// numPrefixArgs is the number of ABI-specific prefix arguments we have. It +/// does not count `this`. const CGFunctionInfo & CodeGenTypes::arrangeCXXMethodCall(const CallArgList &args, const FunctionProtoType *proto, - RequiredArgs required) { - unsigned numRequiredArgs = - (proto->isVariadic() ? required.getNumRequiredArgs() : args.size()); - unsigned numPrefixArgs = numRequiredArgs - proto->getNumParams(); + RequiredArgs required, + unsigned numPrefixArgs) { + assert(numPrefixArgs + 1 <= args.size() && + "Emitting a call with less args than the required prefix?"); + // Add one to account for `this`. It's a bit awkward here, but we don't count + // `this` in similar places elsewhere. auto paramInfos = - getExtParameterInfosForCall(proto, numPrefixArgs, args.size()); + getExtParameterInfosForCall(proto, numPrefixArgs + 1, args.size()); // FIXME: Kill copy. auto argTypes = getArgTypesForCall(Context, args); diff --git a/lib/CodeGen/CGClass.cpp b/lib/CodeGen/CGClass.cpp index a6f350e393..a305ae62ca 100644 --- a/lib/CodeGen/CGClass.cpp +++ b/lib/CodeGen/CGClass.cpp @@ -1979,7 +1979,7 @@ static bool canEmitDelegateCallArgs(CodeGenFunction &CGF, // Likewise if they're inalloca. const CGFunctionInfo &Info = - CGF.CGM.getTypes().arrangeCXXConstructorCall(Args, Ctor, Type, 0); + CGF.CGM.getTypes().arrangeCXXConstructorCall(Args, Ctor, Type, 0, 0); if (Info.usesInAlloca()) return false; } @@ -2021,10 +2021,11 @@ void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D, return; } + bool PassPrototypeArgs = true; // Check whether we can actually emit the constructor before trying to do so. if (auto Inherited = D->getInheritedConstructor()) { - if (getTypes().inheritingCtorHasParams(Inherited, Type) && - !canEmitDelegateCallArgs(*this, D, Type, Args)) { + PassPrototypeArgs = getTypes().inheritingCtorHasParams(Inherited, Type); + if (PassPrototypeArgs && !canEmitDelegateCallArgs(*this, D, Type, Args)) { EmitInlinedInheritingCXXConstructorCall(D, Type, ForVirtualBase, Delegating, Args); return; @@ -2040,7 +2041,7 @@ void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D, llvm::Constant *CalleePtr = CGM.getAddrOfCXXStructor(D, getFromCtorType(Type)); const CGFunctionInfo &Info = CGM.getTypes().arrangeCXXConstructorCall( - Args, D, Type, ExtraArgs.Prefix + ExtraArgs.Suffix); + Args, D, Type, ExtraArgs.Prefix, ExtraArgs.Suffix, PassPrototypeArgs); CGCallee Callee = CGCallee::forDirect(CalleePtr, D); EmitCall(Info, Callee, ReturnValueSlot(), Args); diff --git a/lib/CodeGen/CGExprCXX.cpp b/lib/CodeGen/CGExprCXX.cpp index 29c307bbef..e597f46905 100644 --- a/lib/CodeGen/CGExprCXX.cpp +++ b/lib/CodeGen/CGExprCXX.cpp @@ -24,7 +24,15 @@ using namespace clang; using namespace CodeGen; -static RequiredArgs +namespace { +struct MemberCallInfo { + RequiredArgs ReqArgs; + // Number of prefix arguments for the call. Ignores the `this` pointer. + unsigned PrefixSize; +}; +} + +static MemberCallInfo commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD, llvm::Value *This, llvm::Value *ImplicitParam, QualType ImplicitParamTy, const CallExpr *CE, @@ -48,6 +56,7 @@ commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD, const FunctionProtoType *FPT = MD->getType()->castAs(); RequiredArgs required = RequiredArgs::forPrototypePlus(FPT, Args.size(), MD); + unsigned PrefixSize = Args.size() - 1; // And the rest of the call args. if (RtlArgs) { @@ -65,7 +74,7 @@ commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD, FPT->getNumParams() == 0 && "No CallExpr specified for function with non-zero number of arguments"); } - return required; + return {required, PrefixSize}; } RValue CodeGenFunction::EmitCXXMemberOrOperatorCall( @@ -75,9 +84,10 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorCall( const CallExpr *CE, CallArgList *RtlArgs) { const FunctionProtoType *FPT = MD->getType()->castAs(); CallArgList Args; - RequiredArgs required = commonEmitCXXMemberOrOperatorCall( + MemberCallInfo CallInfo = commonEmitCXXMemberOrOperatorCall( *this, MD, This, ImplicitParam, ImplicitParamTy, CE, Args, RtlArgs); - auto &FnInfo = CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required); + auto &FnInfo = CGM.getTypes().arrangeCXXMethodCall( + Args, FPT, CallInfo.ReqArgs, CallInfo.PrefixSize); return EmitCall(FnInfo, Callee, ReturnValue, Args); } @@ -425,7 +435,8 @@ CodeGenFunction::EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E, // And the rest of the call args EmitCallArgs(Args, FPT, E->arguments()); - return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required), + return EmitCall(CGM.getTypes().arrangeCXXMethodCall(Args, FPT, required, + /*PrefixSize=*/0), Callee, ReturnValue, Args); } diff --git a/lib/CodeGen/CGVTables.cpp b/lib/CodeGen/CGVTables.cpp index 24c7f15a08..947997935d 100644 --- a/lib/CodeGen/CGVTables.cpp +++ b/lib/CodeGen/CGVTables.cpp @@ -284,6 +284,7 @@ void CodeGenFunction::EmitCallAndReturnForThunk(llvm::Constant *CalleePtr, if (isa(MD)) CGM.getCXXABI().adjustCallArgsForDestructorThunk(*this, CurGD, CallArgs); + unsigned PrefixArgs = CallArgs.size() - 1; // Add the rest of the arguments. for (const ParmVarDecl *PD : MD->parameters()) EmitDelegateCallArg(CallArgs, PD, SourceLocation()); @@ -292,7 +293,7 @@ void CodeGenFunction::EmitCallAndReturnForThunk(llvm::Constant *CalleePtr, #ifndef NDEBUG const CGFunctionInfo &CallFnInfo = CGM.getTypes().arrangeCXXMethodCall( - CallArgs, FPT, RequiredArgs::forPrototypePlus(FPT, 1, MD)); + CallArgs, FPT, RequiredArgs::forPrototypePlus(FPT, 1, MD), PrefixArgs); assert(CallFnInfo.getRegParm() == CurFnInfo->getRegParm() && CallFnInfo.isNoReturn() == CurFnInfo->isNoReturn() && CallFnInfo.getCallingConvention() == CurFnInfo->getCallingConvention()); diff --git a/lib/CodeGen/CodeGenTypes.h b/lib/CodeGen/CodeGenTypes.h index 2ce6591e4e..f0b97ebde1 100644 --- a/lib/CodeGen/CodeGenTypes.h +++ b/lib/CodeGen/CodeGenTypes.h @@ -303,11 +303,14 @@ public: const CGFunctionInfo &arrangeCXXConstructorCall(const CallArgList &Args, const CXXConstructorDecl *D, CXXCtorType CtorKind, - unsigned ExtraArgs); + unsigned ExtraPrefixArgs, + unsigned ExtraSuffixArgs, + bool PassProtoArgs = true); const CGFunctionInfo &arrangeCXXMethodCall(const CallArgList &args, const FunctionProtoType *type, - RequiredArgs required); + RequiredArgs required, + unsigned numPrefixArgs); const CGFunctionInfo &arrangeMSMemberPointerThunk(const CXXMethodDecl *MD); const CGFunctionInfo &arrangeMSCtorClosure(const CXXConstructorDecl *CD, CXXCtorType CT); diff --git a/lib/CodeGen/MicrosoftCXXABI.cpp b/lib/CodeGen/MicrosoftCXXABI.cpp index 2bb37834f5..7fc8f3c23b 100644 --- a/lib/CodeGen/MicrosoftCXXABI.cpp +++ b/lib/CodeGen/MicrosoftCXXABI.cpp @@ -3934,7 +3934,7 @@ MicrosoftCXXABI::getAddrOfCXXCtorClosure(const CXXConstructorDecl *CD, CGM.getAddrOfCXXStructor(CD, StructorType::Complete); CGCallee Callee = CGCallee::forDirect(CalleePtr, CD); const CGFunctionInfo &CalleeInfo = CGM.getTypes().arrangeCXXConstructorCall( - Args, CD, Ctor_Complete, ExtraArgs.Prefix + ExtraArgs.Suffix); + Args, CD, Ctor_Complete, ExtraArgs.Prefix, ExtraArgs.Suffix); CGF.EmitCall(CalleeInfo, Callee, ReturnValueSlot(), Args); Cleanups.ForceCleanup(); diff --git a/test/CodeGenObjCXX/arc-attrs-abi.mm b/test/CodeGenObjCXX/arc-attrs-abi.mm index 7570038b1b..1fb32f8c00 100644 --- a/test/CodeGenObjCXX/arc-attrs-abi.mm +++ b/test/CodeGenObjCXX/arc-attrs-abi.mm @@ -1,8 +1,8 @@ -// RUN: %clang_cc1 -triple x86_64-apple -emit-llvm -fobjc-arc -o - %s -// RUN: %clang_cc1 -triple x86_64-windows -emit-llvm -fobjc-arc -o - %s +// RUN: %clang_cc1 -triple x86_64-apple -emit-llvm -fobjc-arc -o - %s -std=c++11 | FileCheck %s --check-prefix=ITANIUM +// RUN: %clang_cc1 -triple x86_64-windows -emit-llvm -fobjc-arc -o - %s -std=c++11 // -// Test caess where we weren't properly adding parameter infos declarations, -// which caused assertions to fire. Hence, no CHECKs. +// Test cases where we weren't properly adding extended parameter info, which +// caused assertions to fire. Hence, minimal CHECKs. struct VirtualBase { VirtualBase(__attribute__((ns_consumed)) id x); @@ -13,3 +13,25 @@ struct WithVirtualBase : virtual VirtualBase { WithVirtualBase::WithVirtualBase(__attribute__((ns_consumed)) id x) : VirtualBase(x) {} + + +struct VirtualBase2 { + VirtualBase2(__attribute__((ns_consumed)) id x, void *y); +}; + +// In this case, we don't actually end up passing the `id` param from +// WithVirtualBaseLast's ctor to WithVirtualBaseMid's. So, we shouldn't emit +// ext param info for `id` to `Mid`. Itanium-only check since MSABI seems to +// emit the construction code inline. +struct WithVirtualBaseMid : virtual VirtualBase2 { + // Ensure we only pass in `this` and a vtable. Otherwise this test is useless. + // ITANIUM: define {{.*}} void @_ZN18WithVirtualBaseMidCI212VirtualBase2EP11objc_objectPv({{.*}}, {{.*}}) + using VirtualBase2::VirtualBase2; +}; +struct WithVirtualBaseLast : WithVirtualBaseMid { + using WithVirtualBaseMid::WithVirtualBaseMid; +}; + +void callLast(__attribute__((ns_consumed)) id x) { + WithVirtualBaseLast{x, (void*)0}; +} -- 2.50.1