[patch] Don't let virtual calls and dynamic casts call MarkVTableUsed()
Nico Weber
thakis at chromium.org
Tue Jan 13 10:40:15 PST 2015
Hi,
clang currently calls MarkVTableUsed() for classes that get their virtual
methods called or that participate in a dynamic_cast. This is unnecessary,
since CodeGen only emits vtables when it generates constructor, destructor,
and vtt code.
Remember that sema's MarkVTableUsed() doesn't actually cause emission of a
vtable, it only marks all methods that are in the vtable as referenced – to
make sure that when codegen decides to write a vtable, everything in there
is actually defined. Since virtual calls and dynamic casts do not cause
codegen to write a vtable, it's not necessary to define methods in the
vtable for them. After this patch, sema only calls MarkVTableUsed() for the
definitions of constructors and destructors – the only two things that make
codegen write a vtable* (look for `getAddrOfVTable` in lib/CodeGen).
They still get defined if they're referenced by a direct call – this only
affects virtual methods that are not called and were only referenced by
MarkVTableUsed(), for example if a different virtual method was called).
For the same reason, this doesn't have an effect on what llvm can or can't
optimize (see also PR20337 comment 10). It does lead to the emission of
fewer unneeded methods, which will speed up builds.
While this shouldn't change the behavior of codegen (other than being
faster), it does make clang more permissive: virtual methods (in particular
destructors) end up being instantiated less often. In particular, classes
that have members that are smart pointers to incomplete types will now get
their implicit virtual destructor instantiated less frequently. For
example, this used to not compile but does now compile:
template <typename T> struct OwnPtr {
~OwnPtr() { static_assert((sizeof(T) > 0), "TypeMustBeComplete"); }
};
class ScriptLoader;
struct Base { virtual ~Base(); };
struct Sub : public Base {
virtual void someFun() const {}
OwnPtr<ScriptLoader> m_loader;
};
void f(Sub *s) { s->someFun(); }
The more permissive behavior matches both gcc (where this is not often
observable, since in practice most things with virtual methods have a key
function) and cl (which is my motivation for this change) – this fixes
PR20337.
This is similar to r213109 in spirit. r225761 was a prerequisite for this
change.
Various tests relied on "a->f()" marking a's vtable as used (in the sema
sense), switch these to just construct a on the stack. This forces
instantiation of the implicit constructor, which will mark the vtable as
used.
* The exception is -fapple-kext mode: In this mode, qualified calls to
virtual functions (`a->Base::f()`) still go through the vtable, and since
the vtable pointer off this doesn't point to Base's vtable, this needs to
reference Base's vtable directly. To keep this working, keep referencing
the vtable for virtual calls in apple kext mode. There's 0 test coverage
for this :-/
Nico
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150113/45413428/attachment.html>
-------------- next part --------------
Index: lib/Sema/Sema.cpp
===================================================================
--- lib/Sema/Sema.cpp (revision 225799)
+++ lib/Sema/Sema.cpp (working copy)
@@ -337,18 +337,6 @@
if (ExprTy == TypeTy)
return E;
- // If this is a derived-to-base cast to a through a virtual base, we
- // need a vtable.
- if (Kind == CK_DerivedToBase &&
- BasePathInvolvesVirtualBase(*BasePath)) {
- QualType T = E->getType();
- if (const PointerType *Pointer = T->getAs<PointerType>())
- T = Pointer->getPointeeType();
- if (const RecordType *RecordTy = T->getAs<RecordType>())
- MarkVTableUsed(E->getLocStart(),
- cast<CXXRecordDecl>(RecordTy->getDecl()));
- }
-
if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) {
ImpCast->setType(Ty);
Index: lib/Sema/SemaCast.cpp
===================================================================
--- lib/Sema/SemaCast.cpp (revision 225799)
+++ lib/Sema/SemaCast.cpp (working copy)
@@ -665,12 +665,6 @@
}
Kind = CK_DerivedToBase;
-
- // If we are casting to or through a virtual base class, we need a
- // vtable.
- if (Self.BasePathInvolvesVirtualBase(BasePath))
- Self.MarkVTableUsed(OpRange.getBegin(),
- cast<CXXRecordDecl>(SrcRecord->getDecl()));
return;
}
@@ -682,8 +676,6 @@
<< SrcPointee.getUnqualifiedType() << SrcExpr.get()->getSourceRange();
SrcExpr = ExprError();
}
- Self.MarkVTableUsed(OpRange.getBegin(),
- cast<CXXRecordDecl>(SrcRecord->getDecl()));
// dynamic_cast is not available with -fno-rtti.
// As an exception, dynamic_cast to void* is available because it doesn't
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 225799)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -11618,7 +11618,7 @@
Destructor = cast<CXXDestructorDecl>(Destructor->getFirstDecl());
if (Destructor->isDefaulted() && !Destructor->isDeleted())
DefineImplicitDestructor(Loc, Destructor);
- if (Destructor->isVirtual())
+ if (Destructor->isVirtual() && getLangOpts().AppleKext)
MarkVTableUsed(Loc, Destructor->getParent());
} else if (CXXMethodDecl *MethodDecl = dyn_cast<CXXMethodDecl>(Func)) {
if (MethodDecl->isOverloadedOperator() &&
@@ -11638,8 +11638,8 @@
DefineImplicitLambdaToBlockPointerConversion(Loc, Conversion);
else
DefineImplicitLambdaToFunctionPointerConversion(Loc, Conversion);
- } else if (MethodDecl->isVirtual())
- MarkVTableUsed(Loc, MethodDecl->getParent());
+ } else if (MethodDecl->isVirtual() && getLangOpts().AppleKext)
+ MarkVTableUsed(Loc, Destructor->getParent());
}
// Recursive functions should be marked when used from another function.
Index: test/CXX/special/class.dtor/p9.cpp
===================================================================
--- test/CXX/special/class.dtor/p9.cpp (revision 225799)
+++ test/CXX/special/class.dtor/p9.cpp (working copy)
@@ -89,4 +89,11 @@
virtual ~B() {}
static void operator delete(void*);
};
+
+ void f() {
+#ifdef MSABI
+ // expected-note at +2 {{implicit default constructor for 'test3::B' first required here}}
+#endif
+ B use_vtable;
+ }
}
Index: test/CodeGenCXX/key-function-vtable.cpp
===================================================================
--- test/CodeGenCXX/key-function-vtable.cpp (revision 225799)
+++ test/CodeGenCXX/key-function-vtable.cpp (working copy)
@@ -41,7 +41,7 @@
inline void X1::f() { }
-void use_X1(X1 *x1) { x1->f(); }
+void use_X1() { X1 x1; }
// FIXME: The checks are extremely difficult to get right when the globals
// aren't alphabetized
Index: test/SemaCXX/devirtualize-vtable-marking.cpp
===================================================================
--- test/SemaCXX/devirtualize-vtable-marking.cpp (revision 225799)
+++ test/SemaCXX/devirtualize-vtable-marking.cpp (working copy)
@@ -1,29 +1,32 @@
// RUN: %clang_cc1 -verify -std=c++11 %s
-
+// expected-no-diagnostics
template <typename T> struct OwnPtr {
T *p;
~OwnPtr() {
- // expected-error at +1 {{invalid application of 'sizeof'}}
static_assert(sizeof(T) > 0, "incomplete T");
delete p;
}
};
namespace use_vtable_for_vcall {
-struct Incomplete; // expected-note {{forward declaration}}
+struct Incomplete;
struct A {
virtual ~A() {}
virtual void m() {}
};
-struct B : A { // expected-note {{in instantiation}}
+struct B : A {
B();
virtual void m() { }
virtual void m2() { static_cast<A *>(this)->m(); }
OwnPtr<Incomplete> m_sqlError;
};
-B *f() {
- return new B();
+void f() {
+ // Since B's constructor is declared out of line, nothing in this file
+ // references a vtable, so the destructor doesn't get built.
+ A *b = new B();
+ b->m();
+ delete b;
}
}
Index: test/SemaCXX/microsoft-dtor-lookup.cpp
===================================================================
--- test/SemaCXX/microsoft-dtor-lookup.cpp (revision 225799)
+++ test/SemaCXX/microsoft-dtor-lookup.cpp (working copy)
@@ -23,8 +23,9 @@
virtual ~VC(); // expected-error {{member 'operator delete' found in multiple base classes of different types}}
};
-void f(VC vc) {
+void f() {
// This marks VC's vtable used.
+ VC vc;
}
}
Index: test/SemaCXX/warn-weak-vtables.cpp
===================================================================
--- test/SemaCXX/warn-weak-vtables.cpp (revision 225799)
+++ test/SemaCXX/warn-weak-vtables.cpp (working copy)
@@ -20,15 +20,14 @@
virtual void f() { }
};
- A *a;
- a->f();
+ A a;
}
// Use the vtables
-void uses(A &a, B<int> &b, C &c) {
- a.f();
- b.f();
- c.f();
+void uses_abc() {
+ A a;
+ B<int> b;
+ C c;
}
// <rdar://problem/9979458>
@@ -52,10 +51,9 @@
Parent::~Parent() {}
-void uses(Parent &p, Derived &d, VeryDerived &vd) {
- p.getFoo();
- d.getFoo();
- vd.getFoo();
+void uses_derived() {
+ Derived d;
+ VeryDerived vd;
}
template<typename T> struct TemplVirt {
@@ -72,8 +70,8 @@
virtual void f() {}
};
-void uses(TemplVirt<float>& f, TemplVirt<bool>& b, TemplVirt<long>& l) {
- f.f();
- b.f();
- l.f();
+void uses_templ() {
+ TemplVirt<float> f;
+ TemplVirt<bool> b;
+ TemplVirt<long> l;
}
Index: test/SemaTemplate/virtual-member-functions.cpp
===================================================================
--- test/SemaTemplate/virtual-member-functions.cpp (revision 225799)
+++ test/SemaTemplate/virtual-member-functions.cpp (working copy)
@@ -110,12 +110,12 @@
namespace DynamicCast {
struct Y {};
template<typename T> struct X : virtual Y {
- virtual void foo() { T x; } // expected-error {{variable has incomplete type 'void'}}
+ virtual void foo() { T x; }
};
template<typename T> struct X2 : virtual Y {
virtual void foo() { T x; }
};
- Y* f(X<void>* x) { return dynamic_cast<Y*>(x); } // expected-note {{in instantiation of member function 'DynamicCast::X<void>::foo' requested here}}
+ Y* f(X<void>* x) { return dynamic_cast<Y*>(x); }
Y* f2(X<void>* x) { return dynamic_cast<Y*>(x); }
}
More information about the cfe-commits
mailing list