[patch] Don't let virtual calls and dynamic casts call MarkVTableUsed()
Nico Weber
thakis at chromium.org
Tue Jan 13 13:42:55 PST 2015
Tiny update: I missed a MarkVTableUsed() call in SemaInit.cpp. After
removing that, Sema::BasePathInvolvesVirtualBase() is dead, so remove that
too.
On Tue, Jan 13, 2015 at 11:44 AM, Reid Kleckner <rnk at google.com> wrote:
> On Tue, Jan 13, 2015 at 11:24 AM, Reid Kleckner <rnk at google.com> wrote:
>
>> On Tue, Jan 13, 2015 at 11:22 AM, Nico Weber <thakis at chromium.org> wrote:
>>
>>> On Tue, Jan 13, 2015 at 11:11 AM, Reid Kleckner <rnk at google.com> wrote:
>>>
>>>> I'm concerned that if you don't mark the vtable used enough, then
>>>> codegen will crash trying to emit a vtable for a class that it assumed
>>>> would be marked used because it's compiling a virtual call to a method of
>>>> such a class. However, it looks like Rafael completely nuked the
>>>> available_externally vtable emission optimization in r189852, which I
>>>> forgot about. The vtable code still has lots of rigging to allow
>>>> available_externally vtable emission, though.
>>>>
>>>
>>> At the moment, the only thing in coding adding to the DeferredVTables
>>> vector in codegen is getAddrOfVTable(), and that's only called for structor
>>> body emission (and apple kext vcalls). So I think this should be fine.
>>>
>>
>> Right, but we used to call it more, prior to PR13124 and r189852. I'm
>> reading that bug to see if it's something we want to add back.
>>
>
> I'm starting to think that even if we want to bring back the
> available_externally vtable optimization, it should only happen
> optimistically and should not rely on Sema to mark the vtable used. Looking
> at PR13124, though, that seems difficult...
>
> Anyway, I drop my objections. The available_externally vtable optimization
> doesn't exist and turns out to be very hard to implement in Clang today. =/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150113/5f09fa17/attachment.html>
-------------- next part --------------
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h (revision 225799)
+++ include/clang/Sema/Sema.h (working copy)
@@ -5176,8 +5176,6 @@
// FIXME: I don't like this name.
void BuildBasePathArray(const CXXBasePaths &Paths, CXXCastPath &BasePath);
- bool BasePathInvolvesVirtualBase(const CXXCastPath &BasePath);
-
bool CheckDerivedToBaseConversion(QualType Derived, QualType Base,
SourceLocation Loc, SourceRange Range,
CXXCastPath *BasePath = nullptr,
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/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp (revision 225799)
+++ lib/Sema/SemaDeclCXX.cpp (working copy)
@@ -1689,18 +1689,6 @@
BasePathArray.push_back(const_cast<CXXBaseSpecifier*>(Path[I].Base));
}
-/// \brief Determine whether the given base path includes a virtual
-/// base class.
-bool Sema::BasePathInvolvesVirtualBase(const CXXCastPath &BasePath) {
- for (CXXCastPath::const_iterator B = BasePath.begin(),
- BEnd = BasePath.end();
- B != BEnd; ++B)
- if ((*B)->isVirtual())
- return true;
-
- return false;
-}
-
/// CheckDerivedToBaseConversion - Check whether the Derived-to-Base
/// conversion (where Derived and Base are class types) is
/// well-formed, meaning that the conversion is unambiguous (and
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: lib/Sema/SemaInit.cpp
===================================================================
--- lib/Sema/SemaInit.cpp (revision 225799)
+++ lib/Sema/SemaInit.cpp (working copy)
@@ -5820,15 +5820,6 @@
&BasePath, IgnoreBaseAccess))
return ExprError();
- if (S.BasePathInvolvesVirtualBase(BasePath)) {
- QualType T = SourceType;
- if (const PointerType *Pointer = T->getAs<PointerType>())
- T = Pointer->getPointeeType();
- if (const RecordType *RecordTy = T->getAs<RecordType>())
- S.MarkVTableUsed(CurInit.get()->getLocStart(),
- cast<CXXRecordDecl>(RecordTy->getDecl()));
- }
-
ExprValueKind VK =
Step->Kind == SK_CastDerivedToBaseLValue ?
VK_LValue :
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