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