D'oh, forgot to say "Over-the-shoulder-reviewed by rnk" in the CL description.
On Tue, Dec 2, 2014 at 5:21 PM, Nico Weber <[email protected]> wrote: > Author: nico > Date: Tue Dec 2 19:21:41 2014 > New Revision: 223185 > > URL: http://llvm.org/viewvc/llvm-project?rev=223185&view=rev > Log: > Fix incorrect codegen for devirtualized calls to virtual overloaded > operators. > > Consider this program: > > struct A { > virtual void operator-() { printf("base\n"); } > }; > struct B final : public A { > virtual void operator-() override { printf("derived\n"); } > }; > > int main() { > B* b = new B; > -static_cast<A&>(*b); > } > > Before this patch, clang saw the virtual call to A::operator-(), figured > out > that it can be devirtualized, and then just called A::operator-() directly, > without going through the vtable. Instead, it should've looked up which > operator-() the call devirtualizes to and should've called that. > > For regular virtual member calls, clang gets all this right already. So > instead of giving EmitCXXOperatorMemberCallee() all the logic that > EmitCXXMemberCallExpr() already has, cut the latter function into two > pieces, > call the second piece EmitCXXMemberOrOperatorMemberCallExpr(), and use it > also > to generate code for calls to virtual member operators. > > This way, virtual overloaded operators automatically don't get > devirtualized > if they have covariant returns (like it was done for regular calls in > r218602), > etc. > > This also happens to fix (or at least improve) codegen for explicit > constructor > calls (`A a; a.A::A()`) in MS mode with -fsanitize-address-field-padding=1. > > (This adjustment for virtual operator calls seems still wrong with the MS > ABI.) > > Modified: > cfe/trunk/lib/CodeGen/CGClass.cpp > cfe/trunk/lib/CodeGen/CGExprCXX.cpp > cfe/trunk/lib/CodeGen/CodeGenFunction.h > cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp > > Modified: cfe/trunk/lib/CodeGen/CGClass.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGClass.cpp?rev=223185&r1=223184&r2=223185&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGClass.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGClass.cpp Tue Dec 2 19:21:41 2014 > @@ -2163,20 +2163,6 @@ CodeGenFunction::CanDevirtualizeMemberFu > return false; > } > > -llvm::Value * > -CodeGenFunction::EmitCXXOperatorMemberCallee(const CXXOperatorCallExpr *E, > - const CXXMethodDecl *MD, > - llvm::Value *This) { > - llvm::FunctionType *fnType = > - CGM.getTypes().GetFunctionType( > - > CGM.getTypes().arrangeCXXMethodDeclaration(MD)); > - > - if (MD->isVirtual() && !CanDevirtualizeMemberFunctionCall(E->getArg(0), > MD)) > - return CGM.getCXXABI().getVirtualFunctionPointer(*this, MD, This, > fnType); > - > - return CGM.GetAddrOfFunction(MD, fnType); > -} > - > void CodeGenFunction::EmitForwardingCallToLambda( > const CXXMethodDecl *callOperator, > CallArgList &callArgs) { > > Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=223185&r1=223184&r2=223185&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original) > +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Tue Dec 2 19:21:41 2014 > @@ -120,9 +120,23 @@ RValue CodeGenFunction::EmitCXXMemberCal > ReturnValue); > } > > - // Compute the object pointer. > + bool HasQualifier = ME->hasQualifier(); > + NestedNameSpecifier *Qualifier = HasQualifier ? ME->getQualifier() : > nullptr; > + bool IsArrow = ME->isArrow(); > const Expr *Base = ME->getBase(); > - bool CanUseVirtualCall = MD->isVirtual() && !ME->hasQualifier(); > + > + return EmitCXXMemberOrOperatorMemberCallExpr( > + CE, MD, ReturnValue, HasQualifier, Qualifier, IsArrow, Base); > +} > + > +RValue CodeGenFunction::EmitCXXMemberOrOperatorMemberCallExpr( > + const CallExpr *CE, const CXXMethodDecl *MD, ReturnValueSlot > ReturnValue, > + bool HasQualifier, NestedNameSpecifier *Qualifier, bool IsArrow, > + const Expr *Base) { > + assert(isa<CXXMemberCallExpr>(CE) || isa<CXXOperatorCallExpr>(CE)); > + > + // Compute the object pointer. > + bool CanUseVirtualCall = MD->isVirtual() && !HasQualifier; > > const CXXMethodDecl *DevirtualizedMethod = nullptr; > if (CanUseVirtualCall && CanDevirtualizeMemberFunctionCall(Base, MD)) { > @@ -153,7 +167,7 @@ RValue CodeGenFunction::EmitCXXMemberCal > } > > llvm::Value *This; > - if (ME->isArrow()) > + if (IsArrow) > This = EmitScalarExpr(Base); > else > This = EmitLValue(Base).getAddress(); > @@ -165,23 +179,28 @@ RValue CodeGenFunction::EmitCXXMemberCal > cast<CXXConstructorDecl>(MD)->isDefaultConstructor()) > return RValue::get(nullptr); > > - if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) > { > - // We don't like to generate the trivial copy/move assignment > operator > - // when it isn't necessary; just produce the proper effect here. > - llvm::Value *RHS = EmitLValue(*CE->arg_begin()).getAddress(); > - EmitAggregateAssign(This, RHS, CE->getType()); > - return RValue::get(This); > - } > + if (!MD->getParent()->mayInsertExtraPadding()) { > + if (MD->isCopyAssignmentOperator() || > MD->isMoveAssignmentOperator()) { > + // We don't like to generate the trivial copy/move assignment > operator > + // when it isn't necessary; just produce the proper effect here. > + // Special case: skip first argument of CXXOperatorCall (it is > "this"). > + unsigned ArgsToSkip = isa<CXXOperatorCallExpr>(CE) ? 1 : 0; > + llvm::Value *RHS = > + EmitLValue(*(CE->arg_begin() + ArgsToSkip)).getAddress(); > + EmitAggregateAssign(This, RHS, CE->getType()); > + return RValue::get(This); > + } > > - if (isa<CXXConstructorDecl>(MD) && > - cast<CXXConstructorDecl>(MD)->isCopyOrMoveConstructor()) { > - // Trivial move and copy ctor are the same. > - assert(CE->getNumArgs() == 1 && "unexpected argcount for trivial > ctor"); > - llvm::Value *RHS = EmitLValue(*CE->arg_begin()).getAddress(); > - EmitAggregateCopy(This, RHS, CE->arg_begin()->getType()); > - return RValue::get(This); > + if (isa<CXXConstructorDecl>(MD) && > + cast<CXXConstructorDecl>(MD)->isCopyOrMoveConstructor()) { > + // Trivial move and copy ctor are the same. > + assert(CE->getNumArgs() == 1 && "unexpected argcount for trivial > ctor"); > + llvm::Value *RHS = EmitLValue(*CE->arg_begin()).getAddress(); > + EmitAggregateCopy(This, RHS, CE->arg_begin()->getType()); > + return RValue::get(This); > + } > + llvm_unreachable("unknown trivial member function"); > } > - llvm_unreachable("unknown trivial member function"); > } > > // Compute the function type we're calling. > @@ -213,13 +232,11 @@ RValue CodeGenFunction::EmitCXXMemberCal > "Destructor shouldn't have explicit parameters"); > assert(ReturnValue.isNull() && "Destructor shouldn't have return > value"); > if (UseVirtualCall) { > - CGM.getCXXABI().EmitVirtualDestructorCall(*this, Dtor, > Dtor_Complete, > - This, CE); > + CGM.getCXXABI().EmitVirtualDestructorCall( > + *this, Dtor, Dtor_Complete, This, cast<CXXMemberCallExpr>(CE)); > } else { > - if (getLangOpts().AppleKext && > - MD->isVirtual() && > - ME->hasQualifier()) > - Callee = BuildAppleKextVirtualCall(MD, ME->getQualifier(), Ty); > + if (getLangOpts().AppleKext && MD->isVirtual() && HasQualifier) > + Callee = BuildAppleKextVirtualCall(MD, Qualifier, Ty); > else if (!DevirtualizedMethod) > Callee = > CGM.getAddrOfCXXStructor(Dtor, StructorType::Complete, FInfo, > Ty); > @@ -239,10 +256,8 @@ RValue CodeGenFunction::EmitCXXMemberCal > } else if (UseVirtualCall) { > Callee = CGM.getCXXABI().getVirtualFunctionPointer(*this, MD, This, > Ty); > } else { > - if (getLangOpts().AppleKext && > - MD->isVirtual() && > - ME->hasQualifier()) > - Callee = BuildAppleKextVirtualCall(MD, ME->getQualifier(), Ty); > + if (getLangOpts().AppleKext && MD->isVirtual() && HasQualifier) > + Callee = BuildAppleKextVirtualCall(MD, Qualifier, Ty); > else if (!DevirtualizedMethod) > Callee = CGM.GetAddrOfFunction(MD, Ty); > else { > @@ -315,20 +330,9 @@ CodeGenFunction::EmitCXXOperatorMemberCa > ReturnValueSlot > ReturnValue) { > assert(MD->isInstance() && > "Trying to emit a member call expr on a static method!"); > - LValue LV = EmitLValue(E->getArg(0)); > - llvm::Value *This = LV.getAddress(); > - > - if ((MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) > && > - MD->isTrivial() && !MD->getParent()->mayInsertExtraPadding()) { > - llvm::Value *Src = EmitLValue(E->getArg(1)).getAddress(); > - QualType Ty = E->getType(); > - EmitAggregateAssign(This, Src, Ty); > - return RValue::get(This); > - } > - > - llvm::Value *Callee = EmitCXXOperatorMemberCallee(E, MD, This); > - return EmitCXXMemberOrOperatorCall(MD, Callee, ReturnValue, This, > - /*ImplicitParam=*/nullptr, > QualType(), E); > + return EmitCXXMemberOrOperatorMemberCallExpr( > + E, MD, ReturnValue, /*HasQualifier=*/false, /*Qualifier=*/nullptr, > + /*IsArrow=*/false, E->getArg(0)); > } > > RValue CodeGenFunction::EmitCUDAKernelCallExpr(const CUDAKernelCallExpr > *E, > > Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.h > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.h?rev=223185&r1=223184&r2=223185&view=diff > > ============================================================================== > --- cfe/trunk/lib/CodeGen/CodeGenFunction.h (original) > +++ cfe/trunk/lib/CodeGen/CodeGenFunction.h Tue Dec 2 19:21:41 2014 > @@ -2329,12 +2329,16 @@ public: > StructorType Type); > RValue EmitCXXMemberCallExpr(const CXXMemberCallExpr *E, > ReturnValueSlot ReturnValue); > + RValue EmitCXXMemberOrOperatorMemberCallExpr(const CallExpr *CE, > + const CXXMethodDecl *MD, > + ReturnValueSlot > ReturnValue, > + bool HasQualifier, > + NestedNameSpecifier > *Qualifier, > + bool IsArrow, const Expr > *Base); > + // Compute the object pointer. > RValue EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E, > ReturnValueSlot ReturnValue); > > - llvm::Value *EmitCXXOperatorMemberCallee(const CXXOperatorCallExpr *E, > - const CXXMethodDecl *MD, > - llvm::Value *This); > RValue EmitCXXOperatorMemberCallExpr(const CXXOperatorCallExpr *E, > const CXXMethodDecl *MD, > ReturnValueSlot ReturnValue); > > Modified: > cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp?rev=223185&r1=223184&r2=223185&view=diff > > ============================================================================== > --- > cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp > (original) > +++ > cfe/trunk/test/CodeGenCXX/devirtualize-virtual-function-calls-final.cpp Tue > Dec 2 19:21:41 2014 > @@ -53,26 +53,32 @@ namespace Test3 { > namespace Test4 { > struct A { > virtual void f(); > + virtual int operator-(); > }; > > struct B final : A { > virtual void f(); > + virtual int operator-(); > }; > > // CHECK-LABEL: define void @_ZN5Test41fEPNS_1BE > void f(B* d) { > // CHECK: call void @_ZN5Test41B1fEv > static_cast<A*>(d)->f(); > + // CHECK: call i32 @_ZN5Test41BngEv > + -static_cast<A&>(*d); > } > } > > namespace Test5 { > struct A { > virtual void f(); > + virtual int operator-(); > }; > > struct B : A { > virtual void f(); > + virtual int operator-(); > }; > > struct C final : B { > @@ -87,6 +93,15 @@ namespace Test5 { > // CHECK-NEXT: call void %[[FUNC]] > static_cast<A*>(d)->f(); > } > + // CHECK-LABEL: define void @_ZN5Test53fopEPNS_1CE > + void fop(C* d) { > + // FIXME: It should be possible to devirtualize this case, but that is > + // not implemented yet. > + // CHECK: getelementptr > + // CHECK-NEXT: %[[FUNC:.*]] = load > + // CHECK-NEXT: call i32 %[[FUNC]] > + -static_cast<A&>(*d); > + } > } > > namespace Test6 { > @@ -165,6 +180,9 @@ namespace Test9 { > virtual A *f() { > return 0; > } > + virtual A *operator-() { > + return 0; > + } > }; > struct RC final : public RA { > virtual C *f() { > @@ -173,6 +191,12 @@ namespace Test9 { > x->b = 2; > return x; > } > + virtual C *operator-() { > + C *x = new C(); > + x->a = 1; > + x->b = 2; > + return x; > + } > }; > // CHECK: define {{.*}} @_ZN5Test91fEPNS_2RCE > A *f(RC *x) { > @@ -187,4 +211,17 @@ namespace Test9 { > // CHECK-NEXT: = call {{.*}} %[[FUNC]] > return static_cast<RA*>(x)->f(); > } > + // CHECK: define {{.*}} @_ZN5Test93fopEPNS_2RCE > + A *fop(RC *x) { > + // FIXME: It should be possible to devirtualize this case, but that is > + // not implemented yet. > + // CHECK: load > + // CHECK: bitcast > + // CHECK: [[F_PTR_RA:%.+]] = bitcast > + // CHECK: [[VTABLE:%.+]] = load {{.+}} [[F_PTR_RA]] > + // CHECK: [[VFN:%.+]] = getelementptr inbounds {{.+}} [[VTABLE]], > i{{[0-9]+}} 1 > + // CHECK-NEXT: %[[FUNC:.*]] = load {{.+}} [[VFN]] > + // CHECK-NEXT: = call {{.*}} %[[FUNC]] > + return -static_cast<RA&>(*x); > + } > } > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
