From 69fc16616d5d160fd316c53ba35f9a13aeb7741f Mon Sep 17 00:00:00 2001 From: Nico Weber Date: Mon, 26 Jan 2015 06:23:36 +0000 Subject: [PATCH] Don't let virtual calls and dynamic casts call Sema::MarkVTableUsed(). MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. (*) Note that Sema::MarkVTableUsed() doesn't cause the emission of a vtable. Its main user-visible effect is that it instantiates virtual member functions of template classes, to make sure that if codegen decides to write a vtable all the entries in the vtable are defined. While this shouldn't change the behavior of codegen (other than being faster), it does make clang more permissive: virtual methods of templates (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 struct OwnPtr { ~OwnPtr() { static_assert((sizeof(T) > 0), "TypeMustBeComplete"); } }; class ScriptLoader; struct Base { virtual ~Base(); }; struct Sub : public Base { virtual void someFun() const {} OwnPtr 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 Sema::DefineUsedVTables() skips vtables for classes with key functions) and cl (which is my motivation for this change) – this fixes PR20337. See this issue and the review thread for some discussions about optimizations. 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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@227073 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Sema/Sema.h | 2 -- lib/Sema/Sema.cpp | 12 --------- lib/Sema/SemaCast.cpp | 8 ------ lib/Sema/SemaDeclCXX.cpp | 12 --------- lib/Sema/SemaExpr.cpp | 4 +-- lib/Sema/SemaInit.cpp | 9 ------- test/CXX/special/class.dtor/p9.cpp | 7 +++++ test/CodeGenCXX/key-function-vtable.cpp | 2 +- test/SemaCXX/devirtualize-vtable-marking.cpp | 15 ++++++----- test/SemaCXX/microsoft-dtor-lookup.cpp | 3 ++- test/SemaCXX/warn-weak-vtables.cpp | 26 +++++++++---------- .../SemaTemplate/virtual-member-functions.cpp | 4 +-- 12 files changed, 35 insertions(+), 69 deletions(-) diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index a53eda41e1..f6d185e491 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -5182,8 +5182,6 @@ public: // 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, diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp index b9aaf1616d..ded6c303af 100644 --- a/lib/Sema/Sema.cpp +++ b/lib/Sema/Sema.cpp @@ -337,18 +337,6 @@ ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty, 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()) - T = Pointer->getPointeeType(); - if (const RecordType *RecordTy = T->getAs()) - MarkVTableUsed(E->getLocStart(), - cast(RecordTy->getDecl())); - } - if (ImplicitCastExpr *ImpCast = dyn_cast(E)) { if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) { ImpCast->setType(Ty); diff --git a/lib/Sema/SemaCast.cpp b/lib/Sema/SemaCast.cpp index a4c2d9b51c..3e06a6477d 100644 --- a/lib/Sema/SemaCast.cpp +++ b/lib/Sema/SemaCast.cpp @@ -665,12 +665,6 @@ void CastOperation::CheckDynamicCast() { } 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(SrcRecord->getDecl())); return; } @@ -682,8 +676,6 @@ void CastOperation::CheckDynamicCast() { << SrcPointee.getUnqualifiedType() << SrcExpr.get()->getSourceRange(); SrcExpr = ExprError(); } - Self.MarkVTableUsed(OpRange.getBegin(), - cast(SrcRecord->getDecl())); // dynamic_cast is not available with -fno-rtti. // As an exception, dynamic_cast to void* is available because it doesn't diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index 1b9ccee9db..79732c354c 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -1745,18 +1745,6 @@ void Sema::BuildBasePathArray(const CXXBasePaths &Paths, BasePathArray.push_back(const_cast(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 diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 4114b56713..8b9950847f 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -11621,7 +11621,7 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func, Destructor = cast(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(Func)) { if (MethodDecl->isOverloadedOperator() && @@ -11641,7 +11641,7 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func, DefineImplicitLambdaToBlockPointerConversion(Loc, Conversion); else DefineImplicitLambdaToFunctionPointerConversion(Loc, Conversion); - } else if (MethodDecl->isVirtual()) + } else if (MethodDecl->isVirtual() && getLangOpts().AppleKext) MarkVTableUsed(Loc, MethodDecl->getParent()); } diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp index e7a8e9cacd..9aca373d97 100644 --- a/lib/Sema/SemaInit.cpp +++ b/lib/Sema/SemaInit.cpp @@ -5880,15 +5880,6 @@ InitializationSequence::Perform(Sema &S, &BasePath, IgnoreBaseAccess)) return ExprError(); - if (S.BasePathInvolvesVirtualBase(BasePath)) { - QualType T = SourceType; - if (const PointerType *Pointer = T->getAs()) - T = Pointer->getPointeeType(); - if (const RecordType *RecordTy = T->getAs()) - S.MarkVTableUsed(CurInit.get()->getLocStart(), - cast(RecordTy->getDecl())); - } - ExprValueKind VK = Step->Kind == SK_CastDerivedToBaseLValue ? VK_LValue : diff --git a/test/CXX/special/class.dtor/p9.cpp b/test/CXX/special/class.dtor/p9.cpp index a03fcdb249..42a4236a4a 100644 --- a/test/CXX/special/class.dtor/p9.cpp +++ b/test/CXX/special/class.dtor/p9.cpp @@ -89,4 +89,11 @@ namespace test3 { virtual ~B() {} static void operator delete(void*); }; + + void f() { +#ifdef MSABI + // expected-note@+2 {{implicit default constructor for 'test3::B' first required here}} +#endif + B use_vtable; + } } diff --git a/test/CodeGenCXX/key-function-vtable.cpp b/test/CodeGenCXX/key-function-vtable.cpp index c443b5277e..80ce497505 100644 --- a/test/CodeGenCXX/key-function-vtable.cpp +++ b/test/CodeGenCXX/key-function-vtable.cpp @@ -41,7 +41,7 @@ struct X1 : X0 { 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 diff --git a/test/SemaCXX/devirtualize-vtable-marking.cpp b/test/SemaCXX/devirtualize-vtable-marking.cpp index fc3e8ce770..1b32182c4f 100644 --- a/test/SemaCXX/devirtualize-vtable-marking.cpp +++ b/test/SemaCXX/devirtualize-vtable-marking.cpp @@ -1,29 +1,32 @@ // RUN: %clang_cc1 -verify -std=c++11 %s - +// expected-no-diagnostics template struct OwnPtr { T *p; ~OwnPtr() { - // expected-error@+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(this)->m(); } OwnPtr 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; } } diff --git a/test/SemaCXX/microsoft-dtor-lookup.cpp b/test/SemaCXX/microsoft-dtor-lookup.cpp index 412749f707..312598e286 100644 --- a/test/SemaCXX/microsoft-dtor-lookup.cpp +++ b/test/SemaCXX/microsoft-dtor-lookup.cpp @@ -23,8 +23,9 @@ struct VC : A, B { 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; } } diff --git a/test/SemaCXX/warn-weak-vtables.cpp b/test/SemaCXX/warn-weak-vtables.cpp index 671ff297cf..d306653871 100644 --- a/test/SemaCXX/warn-weak-vtables.cpp +++ b/test/SemaCXX/warn-weak-vtables.cpp @@ -20,15 +20,14 @@ void f() { virtual void f() { } }; - A *a; - a->f(); + A a; } // Use the vtables -void uses(A &a, B &b, C &c) { - a.f(); - b.f(); - c.f(); +void uses_abc() { + A a; + B b; + C c; } // @@ -52,10 +51,9 @@ public: Parent::~Parent() {} -void uses(Parent &p, Derived &d, VeryDerived &vd) { - p.getFoo(); - d.getFoo(); - vd.getFoo(); +void uses_derived() { + Derived d; + VeryDerived vd; } template struct TemplVirt { @@ -72,8 +70,8 @@ template<> struct TemplVirt { // expected-warning{{'TemplVirt' has n virtual void f() {} }; -void uses(TemplVirt& f, TemplVirt& b, TemplVirt& l) { - f.f(); - b.f(); - l.f(); +void uses_templ() { + TemplVirt f; + TemplVirt b; + TemplVirt l; } diff --git a/test/SemaTemplate/virtual-member-functions.cpp b/test/SemaTemplate/virtual-member-functions.cpp index c2fc2634ed..4044f9e513 100644 --- a/test/SemaTemplate/virtual-member-functions.cpp +++ b/test/SemaTemplate/virtual-member-functions.cpp @@ -110,12 +110,12 @@ namespace PR7114 { namespace DynamicCast { struct Y {}; template struct X : virtual Y { - virtual void foo() { T x; } // expected-error {{variable has incomplete type 'void'}} + virtual void foo() { T x; } }; template struct X2 : virtual Y { virtual void foo() { T x; } }; - Y* f(X* x) { return dynamic_cast(x); } // expected-note {{in instantiation of member function 'DynamicCast::X::foo' requested here}} + Y* f(X* x) { return dynamic_cast(x); } Y* f2(X* x) { return dynamic_cast(x); } } -- 2.40.0