From 980e0adbc8f5653fc8b667d3cbc97a1e36fcf2df Mon Sep 17 00:00:00 2001 From: James Y Knight Date: Sat, 19 Jan 2019 02:25:47 -0500 Subject: [PATCH] [opaque pointer types] Fix the CallInfo passed to EmitCall in some edge cases. Currently, EmitCall is emitting a call instruction using a function type derived from the pointee-type of the callee. This *should* be the same as the type created from the CallInfo parameter, but in some cases an incorrect CallInfo was being passed. All of these fixes were discovered by the addition of the assert in EmitCall which verifies that the passed-in CallInfo matches the Callee's function type. As far as I know, these issues caused no bugs at the moment (but would potentially cause problems when removing pointee types.) List of fixes: * arrangeCXXConstructorCall was passing an incorrect value for the number of Required args, when calling an inheriting constructor where the inherited constructor is variadic. (The inheriting constructor doesn't actually get passed any of the user's args, but the code was calculating it as if it did). * arrangeFreeFunctionLikeCall was not including the count of the pass_object_size arguments in the count of required args. * OpenCL uses other address spaces for the "this" pointer. However, commonEmitCXXMemberOrOperatorCall was not annotating the address space on the "this" argument of the call. * Destructor calls were being created with EmitCXXMemberOrOperatorCall instead of EmitCXXDestructorCall in a few places. This was a problem because the calling convention sometimes has destructors returning "this" rather than void, and the latter function knows about that, and sets up the types properly (through calling arrangeCXXStructorDeclaration), while the former does not. * generateObjCGetterBody: the objc_getProperty function returns 'id', but was being called as if it returned the property type. (The return value gets downcast to the propererty type after the call). * OpenMP user-defined reduction functions (#pragma omp declare reduction) can be called with a subclass of the declared type. In such case, the call was being setup as if the function had been actually declared to take the subtype, rather than the base type. --- clang/lib/CodeGen/CGCall.cpp | 51 +++++++++++++------ clang/lib/CodeGen/CGExprCXX.cpp | 30 +++++------ clang/lib/CodeGen/CGObjC.cpp | 6 +-- clang/lib/CodeGen/CodeGenTypes.h | 4 ++ clang/lib/CodeGen/ItaniumCXXABI.cpp | 10 ++-- clang/lib/Sema/SemaOpenMP.cpp | 6 ++- .../CodeGenObjC/getter-property-mismatch.m | 4 +- 7 files changed, 66 insertions(+), 45 deletions(-) diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index 4051cfb820c3..9b675f1fd3c2 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -67,10 +67,17 @@ unsigned CodeGenTypes::ClangCallConvToLLVMCallConv(CallingConv CC) { } /// Derives the 'this' type for codegen purposes, i.e. ignoring method CVR -/// qualification. -static CanQualType GetThisType(ASTContext &Context, const CXXRecordDecl *RD, - const CXXMethodDecl *MD) { - QualType RecTy = Context.getTagDeclType(RD)->getCanonicalTypeInternal(); +/// qualification. Either or both of RD and MD may be null. A null RD indicates +/// that there is no meaningful 'this' type, and a null MD can occur when +/// calling a method pointer. +CanQualType CodeGenTypes::DeriveThisType(const CXXRecordDecl *RD, + const CXXMethodDecl *MD) { + QualType RecTy; + if (RD) + RecTy = Context.getTagDeclType(RD)->getCanonicalTypeInternal(); + else + RecTy = Context.VoidTy; + if (MD) RecTy = Context.getAddrSpaceQualType(RecTy, MD->getMethodQualifiers().getAddressSpace()); return Context.getPointerType(CanQualType::CreateUnsafe(RecTy)); @@ -235,7 +242,7 @@ static CallingConv getCallingConventionForDecl(const Decl *D, bool IsWindows) { /// Arrange the argument and result information for a call to an /// unknown C++ non-static member function of the given abstract type. -/// (Zero value of RD means we don't have any meaningful "this" argument type, +/// (A null RD means we don't have any meaningful "this" argument type, /// so fall back to a generic pointer type). /// The member function must be an ordinary function, i.e. not a /// constructor or destructor. @@ -246,10 +253,7 @@ CodeGenTypes::arrangeCXXMethodType(const CXXRecordDecl *RD, SmallVector argTypes; // Add the 'this' pointer. - if (RD) - argTypes.push_back(GetThisType(Context, RD, MD)); - else - argTypes.push_back(Context.VoidPtrTy); + argTypes.push_back(DeriveThisType(RD, MD)); return ::arrangeLLVMFunctionInfo( *this, true, argTypes, @@ -303,7 +307,7 @@ CodeGenTypes::arrangeCXXStructorDeclaration(const CXXMethodDecl *MD, SmallVector argTypes; SmallVector paramInfos; - argTypes.push_back(GetThisType(Context, MD->getParent(), MD)); + argTypes.push_back(DeriveThisType(MD->getParent(), MD)); bool PassParams = true; @@ -403,8 +407,11 @@ CodeGenTypes::arrangeCXXConstructorCall(const CallArgList &args, unsigned TotalPrefixArgs = 1 + ExtraPrefixArgs; CanQual FPT = GetFormalType(D); - RequiredArgs Required = - RequiredArgs::forPrototypePlus(FPT, TotalPrefixArgs + ExtraSuffixArgs); + RequiredArgs Required = PassProtoArgs + ? RequiredArgs::forPrototypePlus( + FPT, TotalPrefixArgs + ExtraSuffixArgs) + : RequiredArgs::All; + GlobalDecl GD(D, CtorKind); CanQualType ResultType = TheCXXABI.HasThisReturn(GD) ? ArgTypes.front() @@ -530,7 +537,7 @@ const CGFunctionInfo & CodeGenTypes::arrangeUnprototypedMustTailThunk(const CXXMethodDecl *MD) { assert(MD->isVirtual() && "only methods have thunks"); CanQual FTP = GetFormalType(MD); - CanQualType ArgTys[] = { GetThisType(Context, MD->getParent(), MD) }; + CanQualType ArgTys[] = {DeriveThisType(MD->getParent(), MD)}; return arrangeLLVMFunctionInfo(Context.VoidTy, /*instanceMethod=*/false, /*chainCall=*/false, ArgTys, FTP->getExtInfo(), {}, RequiredArgs(1)); @@ -544,7 +551,7 @@ CodeGenTypes::arrangeMSCtorClosure(const CXXConstructorDecl *CD, CanQual FTP = GetFormalType(CD); SmallVector ArgTys; const CXXRecordDecl *RD = CD->getParent(); - ArgTys.push_back(GetThisType(Context, RD, CD)); + ArgTys.push_back(DeriveThisType(RD, CD)); if (CT == Ctor_CopyingClosure) ArgTys.push_back(*FTP->param_type_begin()); if (RD->getNumVBases() > 0) @@ -577,7 +584,7 @@ arrangeFreeFunctionLikeCall(CodeGenTypes &CGT, // extra prefix plus the arguments in the prototype. if (const FunctionProtoType *proto = dyn_cast(fnType)) { if (proto->isVariadic()) - required = RequiredArgs(proto->getNumParams() + numExtraRequiredArgs); + required = RequiredArgs::forPrototypePlus(proto, numExtraRequiredArgs); if (proto->hasExtParameterInfos()) addExtParameterInfosForCall(paramInfos, proto, numExtraRequiredArgs, @@ -802,6 +809,8 @@ CGFunctionInfo *CGFunctionInfo::create(unsigned llvmCC, ArrayRef argTypes, RequiredArgs required) { assert(paramInfos.empty() || paramInfos.size() == argTypes.size()); + assert(!required.allowsOptionalArgs() || + required.getNumRequiredArgs() <= argTypes.size()); void *buffer = operator new(totalSizeToAlloc( @@ -3817,6 +3826,18 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo, const ABIArgInfo &RetAI = CallInfo.getReturnInfo(); llvm::FunctionType *IRFuncTy = Callee.getFunctionType(); + if (!(CallInfo.isVariadic() && CallInfo.getArgStruct())) { + // For an inalloca varargs function, we don't expect CallInfo to match the + // function pointer's type, because the inalloca arg will have the extra + // varargs fields in it. We bitcast to type derived from CallInfo, below. + llvm::FunctionType *IRFuncTyFromInfo = getTypes().GetFunctionType(CallInfo); + if (IRFuncTy != IRFuncTyFromInfo) { + fprintf(stderr, "Function types -- from ptr vs from CallInfo:\n"); + IRFuncTy->dump(); + IRFuncTyFromInfo->dump(); + assert(IRFuncTy == IRFuncTyFromInfo); + } + } // 1. Set up the arguments. diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index 1f650ae419e8..d280833e3edd 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -40,13 +40,11 @@ commonEmitCXXMemberOrOperatorCall(CodeGenFunction &CGF, const CXXMethodDecl *MD, isa(CE)); assert(MD->isInstance() && "Trying to emit a member or operator call expr on a static method!"); - ASTContext &C = CGF.getContext(); // Push the this ptr. const CXXRecordDecl *RD = CGF.CGM.getCXXABI().getThisArgumentTypeForMethod(MD); - Args.add(RValue::get(This), - RD ? C.getPointerType(C.getTypeDeclType(RD)) : C.VoidPtrTy); + Args.add(RValue::get(This), CGF.getTypes().DeriveThisType(RD, MD)); // If there is an implicit parameter (e.g. VTT), emit it. if (ImplicitParam) { @@ -326,9 +324,6 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr( CallLoc, This.getPointer(), C.getRecordType(CalleeDecl->getParent()), /*Alignment=*/CharUnits::Zero(), SkippedChecks); - // FIXME: Uses of 'MD' past this point need to be audited. We may need to use - // 'CalleeDecl' instead. - // C++ [class.virtual]p12: // Explicit qualification with the scope operator (5.1) suppresses the // virtual call mechanism. @@ -337,7 +332,7 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr( // because then we know what the type is. bool UseVirtualCall = CanUseVirtualCall && !DevirtualizedMethod; - if (const CXXDestructorDecl *Dtor = dyn_cast(MD)) { + if (const CXXDestructorDecl *Dtor = dyn_cast(CalleeDecl)) { assert(CE->arg_begin() == CE->arg_end() && "Destructor shouldn't have explicit parameters"); assert(ReturnValue.isNull() && "Destructor shouldn't have return value"); @@ -347,26 +342,29 @@ RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr( cast(CE)); } else { CGCallee Callee; - if (getLangOpts().AppleKext && MD->isVirtual() && HasQualifier) - Callee = BuildAppleKextVirtualCall(MD, Qualifier, Ty); + if (getLangOpts().AppleKext && Dtor->isVirtual() && HasQualifier) + Callee = BuildAppleKextVirtualCall(Dtor, Qualifier, Ty); else if (!DevirtualizedMethod) Callee = CGCallee::forDirect( CGM.getAddrOfCXXStructor(Dtor, StructorType::Complete, FInfo, Ty), GlobalDecl(Dtor, Dtor_Complete)); else { - const CXXDestructorDecl *DDtor = - cast(DevirtualizedMethod); Callee = CGCallee::forDirect( - CGM.GetAddrOfFunction(GlobalDecl(DDtor, Dtor_Complete), Ty), - GlobalDecl(DDtor, Dtor_Complete)); + CGM.GetAddrOfFunction(GlobalDecl(Dtor, Dtor_Complete), Ty), + GlobalDecl(Dtor, Dtor_Complete)); } - EmitCXXMemberOrOperatorCall( - CalleeDecl, Callee, ReturnValue, This.getPointer(), - /*ImplicitParam=*/nullptr, QualType(), CE, nullptr); + + EmitCXXDestructorCall(Dtor, Callee, This.getPointer(), + /*ImplicitParam=*/nullptr, + /*ImplicitParamTy=*/QualType(), nullptr, + getFromDtorType(Dtor_Complete)); } return RValue::get(nullptr); } + // FIXME: Uses of 'MD' past this point need to be audited. We may need to use + // 'CalleeDecl' instead. + CGCallee Callee; if (const CXXConstructorDecl *Ctor = dyn_cast(MD)) { Callee = CGCallee::forDirect( diff --git a/clang/lib/CodeGen/CGObjC.cpp b/clang/lib/CodeGen/CGObjC.cpp index 9a212969f4a3..b70dab71a60f 100644 --- a/clang/lib/CodeGen/CGObjC.cpp +++ b/clang/lib/CodeGen/CGObjC.cpp @@ -1051,9 +1051,9 @@ CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl *classImpl, // FIXME: We shouldn't need to get the function info here, the // runtime already should have computed it to build the function. llvm::CallBase *CallInstruction; - RValue RV = EmitCall( - getTypes().arrangeBuiltinFunctionCall(propType, args), - callee, ReturnValueSlot(), args, &CallInstruction); + RValue RV = EmitCall(getTypes().arrangeBuiltinFunctionCall( + getContext().getObjCIdType(), args), + callee, ReturnValueSlot(), args, &CallInstruction); if (llvm::CallInst *call = dyn_cast(CallInstruction)) call->setTailCall(); diff --git a/clang/lib/CodeGen/CodeGenTypes.h b/clang/lib/CodeGen/CodeGenTypes.h index d7e6edde8319..c5491730cba0 100644 --- a/clang/lib/CodeGen/CodeGenTypes.h +++ b/clang/lib/CodeGen/CodeGenTypes.h @@ -182,6 +182,10 @@ class CodeGenTypes { /// Convert clang calling convention to LLVM callilng convention. unsigned ClangCallConvToLLVMCallConv(CallingConv CC); + /// Derives the 'this' type for codegen purposes, i.e. ignoring method CVR + /// qualification. + CanQualType DeriveThisType(const CXXRecordDecl *RD, const CXXMethodDecl *MD); + /// ConvertType - Convert type T into a llvm::Type. llvm::Type *ConvertType(QualType T); diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp index 453bd2faa7e6..073857699c80 100644 --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp @@ -1566,9 +1566,8 @@ void ItaniumCXXABI::EmitDestructorCall(CodeGenFunction &CGF, Callee = CGCallee::forDirect( CGM.getAddrOfCXXStructor(DD, getFromDtorType(Type)), GD); - CGF.EmitCXXMemberOrOperatorCall(DD, Callee, ReturnValueSlot(), - This.getPointer(), VTT, VTTTy, - nullptr, nullptr); + CGF.EmitCXXDestructorCall(DD, Callee, This.getPointer(), VTT, VTTTy, nullptr, + getFromDtorType(Type)); } void ItaniumCXXABI::emitVTableDefinitions(CodeGenVTables &CGVT, @@ -1766,9 +1765,8 @@ llvm::Value *ItaniumCXXABI::EmitVirtualDestructorCall( CGCallee Callee = CGCallee::forVirtual(CE, GlobalDecl(Dtor, DtorType), This, Ty); - CGF.EmitCXXMemberOrOperatorCall(Dtor, Callee, ReturnValueSlot(), - This.getPointer(), /*ImplicitParam=*/nullptr, - QualType(), CE, nullptr); + CGF.EmitCXXDestructorCall(Dtor, Callee, This.getPointer(), nullptr, + QualType(), nullptr, getFromDtorType(DtorType)); return nullptr; } diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index 6716329e26a4..5ee5eb0b6bf3 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -10747,7 +10747,8 @@ buildDeclareReductionRef(Sema &SemaRef, SourceLocation Loc, SourceRange Range, return D; return nullptr; })) - return SemaRef.BuildDeclRefExpr(VD, Ty, VK_LValue, Loc); + return SemaRef.BuildDeclRefExpr(VD, VD->getType().getNonReferenceType(), + VK_LValue, Loc); if (auto *VD = filterLookupForUDR( Lookups, [&SemaRef, Ty, Loc](ValueDecl *D) -> ValueDecl * { if (!D->isInvalidDecl() && @@ -10765,7 +10766,8 @@ buildDeclareReductionRef(Sema &SemaRef, SourceLocation Loc, SourceRange Range, /*DiagID=*/0) != Sema::AR_inaccessible) { SemaRef.BuildBasePathArray(Paths, BasePath); - return SemaRef.BuildDeclRefExpr(VD, Ty, VK_LValue, Loc); + return SemaRef.BuildDeclRefExpr( + VD, VD->getType().getNonReferenceType(), VK_LValue, Loc); } } } diff --git a/clang/test/CodeGenObjC/getter-property-mismatch.m b/clang/test/CodeGenObjC/getter-property-mismatch.m index fe415d5358d3..cc54fa65196a 100644 --- a/clang/test/CodeGenObjC/getter-property-mismatch.m +++ b/clang/test/CodeGenObjC/getter-property-mismatch.m @@ -15,6 +15,4 @@ @implementation CalDAVAddManagedAttachmentsTaskGroup // CHECK: [[CALL:%.*]] = tail call i8* @objc_getProperty // CHECK: [[ONE:%.*]] = bitcast i8* [[CALL:%.*]] to [[T1:%.*]]* -// CHECK: [[TWO:%.*]] = bitcast [[T1]]* [[ONE]] to [[T2:%.*]]* -// CHECK: ret [[T2]]* [[TWO]] - +// CHECK: ret [[T1]]* [[ONE]]