r223185 - Fix incorrect codegen for devirtualized calls to virtual overloaded operators.
Nico Weber
thakis at chromium.org
Tue Dec 2 17:23:15 PST 2014
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 <nicolasweber at gmx.de> 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
> 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/20141202/22ef0acd/attachment.html>
More information about the cfe-commits
mailing list