r177211 - Exploit this-return of a callsite in a this-return function.
Stephen Lin
swlin at post.harvard.edu
Sat Mar 16 23:53:58 PDT 2013
Hi Manman,
I'm a lurker on cfe-commits but I saw your patch, and, in addition to the
issues brought up by Richard, I was wondering if you considered the case of
multiple inheritance: for example, if I understand correctly, the
constructors of a non-polymorphic derived class with multiple bases and a
trivial constructor body would usually have a call to a non-primary base
constructor as the last instruction before returning. (You might be aware
of the that issue already, since it's more or less a special case of the
issue Richard brought up, but just wanted to make sure.)
Best,
Stephen
On Fri, Mar 15, 2013 at 11:12 PM, Manman Ren <mren at apple.com> wrote:
>
> Thanks for the review.
> I will look into the issues.
>
> Manman
>
> On Mar 15, 2013, at 5:28 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Fri, Mar 15, 2013 at 5:11 PM, Manman Ren <mren at apple.com> wrote:
>
>> Author: mren
>> Date: Fri Mar 15 19:11:09 2013
>> New Revision: 177211
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=177211&view=rev
>> Log:
>> Exploit this-return of a callsite in a this-return function.
>>
>> For constructors/desctructors that return 'this', if there exists a
>> callsite
>> that returns 'this' and is immediately before the return instruction, make
>> sure we are using the return value from the callsite.
>>
>> We don't need to keep 'this' alive through the callsite. It also enables
>> optimizations in the backend, such as tail call optimization.
>>
>> rdar://12818789
>>
>> Modified:
>> cfe/trunk/lib/CodeGen/CGCXXABI.h
>> cfe/trunk/lib/CodeGen/CGCall.cpp
>> cfe/trunk/lib/CodeGen/CGClass.cpp
>> cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>> cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
>> cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>> cfe/trunk/test/CodeGenCXX/arm.cpp
>>
>> Modified: cfe/trunk/lib/CodeGen/CGCXXABI.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCXXABI.h?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGCXXABI.h (original)
>> +++ cfe/trunk/lib/CodeGen/CGCXXABI.h Fri Mar 15 19:11:09 2013
>> @@ -91,6 +91,10 @@ public:
>> return *MangleCtx;
>> }
>>
>> + /// Returns true if the given instance method is one of the
>> + /// kinds that the ABI says returns 'this'.
>> + virtual bool HasThisReturn(GlobalDecl GD) const { return false; }
>> +
>> /// Find the LLVM type used to represent the given member pointer
>> /// type.
>> virtual llvm::Type *
>> @@ -209,7 +213,8 @@ public:
>> /// Emit the ABI-specific prolog for the function.
>> virtual void EmitInstanceFunctionProlog(CodeGenFunction &CGF) = 0;
>>
>> - virtual void EmitConstructorCall(CodeGenFunction &CGF,
>> + /// Emit the constructor call. Return the function that is called.
>> + virtual llvm::Value *EmitConstructorCall(CodeGenFunction &CGF,
>> const CXXConstructorDecl *D,
>> CXXCtorType Type, bool ForVirtualBase,
>> bool Delegating,
>>
>> Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGCall.cpp Fri Mar 15 19:11:09 2013
>> @@ -1705,6 +1705,18 @@ void CodeGenFunction::EmitFunctionEpilog
>> llvm_unreachable("Invalid ABI kind for return argument");
>> }
>>
>> + // If this function returns 'this' and the last instruction is a
>> CallInst
>> + // that returns 'this', use the return value from the CallInst. We
>> will not
>> + // need to keep 'this' alive through the callsite. It also enables
>> + // optimizations in the backend, such as tail call optimization.
>> + if (CalleeWithThisReturn && CGM.getCXXABI().HasThisReturn(CurGD)) {
>> + llvm::BasicBlock *IP = Builder.GetInsertBlock();
>> + llvm::CallInst *Callsite;
>> + if (!IP->empty() && (Callsite =
>> dyn_cast<llvm::CallInst>(&IP->back())) &&
>> + Callsite->getCalledFunction() == CalleeWithThisReturn)
>>
>
> This doesn't look right. Suppose we have:
>
> struct A { A(); /* trivial dtor */ };
> struct B : A {
> B() : A() { A(); }
> };
>
> It looks like we'll return the 'this' from the temporary. Why not compare
> the llvm::Value* corresponding to the CallInst itself, rather than
> comparing the callee function? (Or maybe check that the 'this' argument to
> the callee matches the caller's 'this'? That'd also solve the problems
> below.)
>
>
>> + // Create a bitcast of Callsite.
>> + RV = Builder.CreateBitCast(Callsite, RetAI.getCoerceToType());
>> + }
>> llvm::Instruction *Ret = RV ? Builder.CreateRet(RV) :
>> Builder.CreateRetVoid();
>> if (!RetDbgLoc.isUnknown())
>> Ret->setDebugLoc(RetDbgLoc);
>>
>> Modified: cfe/trunk/lib/CodeGen/CGClass.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CGClass.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGClass.cpp Fri Mar 15 19:11:09 2013
>> @@ -1666,8 +1666,11 @@ CodeGenFunction::EmitCXXConstructorCall(
>> }
>>
>> // Non-trivial constructors are handled in an ABI-specific manner.
>> - CGM.getCXXABI().EmitConstructorCall(*this, D, Type, ForVirtualBase,
>> - Delegating, This, ArgBeg, ArgEnd);
>> + llvm::Value *Callee = CGM.getCXXABI().EmitConstructorCall(*this, D,
>> Type,
>> + ForVirtualBase, Delegating, This, ArgBeg,
>> ArgEnd);
>> + if (CGM.getCXXABI().HasThisReturn(CurGD) &&
>> + CGM.getCXXABI().HasThisReturn(GlobalDecl(D, Type)))
>> + CalleeWithThisReturn = Callee;
>>
>
> There's no guarantee that the caller and the callee are constructing the
> same object here.
>
>
>> }
>>
>> void
>> @@ -1756,9 +1759,12 @@ CodeGenFunction::EmitDelegateCXXConstruc
>> EmitDelegateCallArg(DelegateArgs, param);
>> }
>>
>> + llvm::Value *Callee = CGM.GetAddrOfCXXConstructor(Ctor, CtorType);
>> EmitCall(CGM.getTypes().arrangeCXXConstructorDeclaration(Ctor,
>> CtorType),
>> - CGM.GetAddrOfCXXConstructor(Ctor, CtorType),
>> - ReturnValueSlot(), DelegateArgs, Ctor);
>> + Callee, ReturnValueSlot(), DelegateArgs, Ctor);
>> + if (CGM.getCXXABI().HasThisReturn(CurGD) &&
>> + CGM.getCXXABI().HasThisReturn(GlobalDecl(Ctor, CtorType)))
>> + CalleeWithThisReturn = Callee;
>> }
>>
>> namespace {
>> @@ -1825,6 +1831,9 @@ void CodeGenFunction::EmitCXXDestructorC
>> EmitCXXMemberCall(DD, SourceLocation(), Callee, ReturnValueSlot(),
>> This,
>> VTT,
>> getContext().getPointerType(getContext().VoidPtrTy),
>> 0, 0);
>> + if (CGM.getCXXABI().HasThisReturn(CurGD) &&
>> + CGM.getCXXABI().HasThisReturn(GlobalDecl(DD, Type)))
>> + CalleeWithThisReturn = Callee;
>>
>
> There's no guarantee that the caller and callee are destroying the same
> object here.
>
>
>> }
>>
>> namespace {
>>
>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Fri Mar 15 19:11:09 2013
>> @@ -564,6 +564,9 @@ void CodeGenFunction::GenerateCode(Globa
>> SourceRange BodyRange;
>> if (Stmt *Body = FD->getBody()) BodyRange = Body->getSourceRange();
>>
>> + // Reset CalleeWithThisReturn.
>>
>
> Well, obviously. It would be better to explain why this is being done, not
> what is being done.
>
>
>> + CalleeWithThisReturn = 0;
>> +
>> // Emit the standard function prologue.
>> StartFunction(GD, ResTy, Fn, FnInfo, Args, BodyRange.getBegin());
>>
>> @@ -615,6 +618,8 @@ void CodeGenFunction::GenerateCode(Globa
>>
>> // Emit the standard function epilogue.
>> FinishFunction(BodyRange.getEnd());
>> + // Reset CalleeWithThisReturn.
>> + CalleeWithThisReturn = 0;
>>
>> // If we haven't marked the function nothrow through other means, do
>> // a quick pass now to see if we can.
>>
>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original)
>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Fri Mar 15 19:11:09 2013
>> @@ -1131,6 +1131,10 @@ private:
>> CGDebugInfo *DebugInfo;
>> bool DisableDebugInfo;
>>
>> + /// If the current function returns 'this', use the field to keep
>> track of
>> + /// the callee that returns 'this'.
>> + llvm::Value *CalleeWithThisReturn;
>> +
>> /// DidCallStackSave - Whether llvm.stacksave has been called. Used to
>> avoid
>> /// calling llvm.stacksave for multiple VLAs in the same scope.
>> bool DidCallStackSave;
>>
>> Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Fri Mar 15 19:11:09 2013
>> @@ -112,7 +112,7 @@ public:
>>
>> void EmitInstanceFunctionProlog(CodeGenFunction &CGF);
>>
>> - void EmitConstructorCall(CodeGenFunction &CGF,
>> + llvm::Value *EmitConstructorCall(CodeGenFunction &CGF,
>> const CXXConstructorDecl *D,
>> CXXCtorType Type, bool ForVirtualBase,
>> bool Delegating,
>> @@ -177,11 +177,11 @@ public:
>> llvm::Value *readArrayCookieImpl(CodeGenFunction &CGF, llvm::Value
>> *allocPtr,
>> CharUnits cookieSize);
>>
>> -private:
>> /// \brief Returns true if the given instance method is one of the
>> /// kinds that the ARM ABI says returns 'this'.
>> - static bool HasThisReturn(GlobalDecl GD) {
>> - const CXXMethodDecl *MD = cast<CXXMethodDecl>(GD.getDecl());
>> + bool HasThisReturn(GlobalDecl GD) const {
>> + const CXXMethodDecl *MD =
>> dyn_cast_or_null<CXXMethodDecl>(GD.getDecl());
>> + if (!MD) return false;
>> return ((isa<CXXDestructorDecl>(MD) && GD.getDtorType() !=
>> Dtor_Deleting) ||
>> (isa<CXXConstructorDecl>(MD)));
>> }
>> @@ -834,7 +834,7 @@ void ARMCXXABI::EmitInstanceFunctionProl
>> CGF.Builder.CreateStore(getThisValue(CGF), CGF.ReturnValue);
>> }
>>
>> -void ItaniumCXXABI::EmitConstructorCall(CodeGenFunction &CGF,
>> +llvm::Value *ItaniumCXXABI::EmitConstructorCall(CodeGenFunction &CGF,
>> const CXXConstructorDecl *D,
>> CXXCtorType Type, bool
>> ForVirtualBase,
>> bool Delegating,
>> @@ -849,6 +849,7 @@ void ItaniumCXXABI::EmitConstructorCall(
>> // FIXME: Provide a source location here.
>> CGF.EmitCXXMemberCall(D, SourceLocation(), Callee, ReturnValueSlot(),
>> This,
>> VTT, VTTTy, ArgBeg, ArgEnd);
>> + return Callee;
>> }
>>
>> RValue ItaniumCXXABI::EmitVirtualDestructorCall(CodeGenFunction &CGF,
>>
>> Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Fri Mar 15 19:11:09 2013
>> @@ -55,7 +55,7 @@ public:
>>
>> void EmitInstanceFunctionProlog(CodeGenFunction &CGF);
>>
>> - void EmitConstructorCall(CodeGenFunction &CGF,
>> + llvm::Value *EmitConstructorCall(CodeGenFunction &CGF,
>> const CXXConstructorDecl *D,
>> CXXCtorType Type, bool ForVirtualBase,
>> bool Delegating,
>> @@ -238,7 +238,7 @@ void MicrosoftCXXABI::EmitInstanceFuncti
>> }
>> }
>>
>> -void MicrosoftCXXABI::EmitConstructorCall(CodeGenFunction &CGF,
>> +llvm::Value *MicrosoftCXXABI::EmitConstructorCall(CodeGenFunction &CGF,
>> const CXXConstructorDecl *D,
>> CXXCtorType Type, bool
>> ForVirtualBase,
>> bool Delegating,
>> @@ -259,6 +259,7 @@ void MicrosoftCXXABI::EmitConstructorCal
>> CGF.EmitCXXMemberCall(D, SourceLocation(), Callee, ReturnValueSlot(),
>> This,
>> ImplicitParam, ImplicitParamTy,
>> ArgBeg, ArgEnd);
>> + return Callee;
>> }
>>
>> RValue MicrosoftCXXABI::EmitVirtualDestructorCall(CodeGenFunction &CGF,
>>
>> Modified: cfe/trunk/test/CodeGenCXX/arm.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/arm.cpp?rev=177211&r1=177210&r2=177211&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/CodeGenCXX/arm.cpp (original)
>> +++ cfe/trunk/test/CodeGenCXX/arm.cpp Fri Mar 15 19:11:09 2013
>> @@ -56,15 +56,15 @@ namespace test1 {
>> // CHECK: [[THIS:%.*]] = alloca [[A]]*, align 4
>> // CHECK: store [[A]]* {{.*}}, [[A]]** [[THIS]]
>> // CHECK: [[THIS1:%.*]] = load [[A]]** [[THIS]]
>> - // CHECK: call [[A]]* @_ZN5test11AC2Ei(
>> - // CHECK: ret [[A]]* [[THIS1]]
>> + // CHECK: [[THIS2:%.*]] = call [[A]]* @_ZN5test11AC2Ei(
>> + // CHECK: ret [[A]]* [[THIS2]]
>>
>> // CHECK: define linkonce_odr [[A]]* @_ZN5test11AD1Ev([[A]]* %this)
>> unnamed_addr
>> // CHECK: [[THIS:%.*]] = alloca [[A]]*, align 4
>> // CHECK: store [[A]]* {{.*}}, [[A]]** [[THIS]]
>> // CHECK: [[THIS1:%.*]] = load [[A]]** [[THIS]]
>> - // CHECK: call [[A]]* @_ZN5test11AD2Ev(
>> - // CHECK: ret [[A]]* [[THIS1]]
>> + // CHECK: [[THIS2:%.*]] = call [[A]]* @_ZN5test11AD2Ev(
>> + // CHECK: ret [[A]]* [[THIS2]]
>> }
>>
>> // Awkward virtual cases.
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130317/8271e207/attachment.html>
More information about the cfe-commits
mailing list