r227073 - Don't let virtual calls and dynamic casts call Sema::MarkVTableUsed().

Nico Weber nicolasweber at gmx.de
Sun Jan 25 22:23:36 PST 2015


Author: nico
Date: Mon Jan 26 00:23:36 2015
New Revision: 227073

URL: http://llvm.org/viewvc/llvm-project?rev=227073&view=rev
Log:
Don't let virtual calls and dynamic casts call Sema::MarkVTableUsed().

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 <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 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.

Modified:
    cfe/trunk/include/clang/Sema/Sema.h
    cfe/trunk/lib/Sema/Sema.cpp
    cfe/trunk/lib/Sema/SemaCast.cpp
    cfe/trunk/lib/Sema/SemaDeclCXX.cpp
    cfe/trunk/lib/Sema/SemaExpr.cpp
    cfe/trunk/lib/Sema/SemaInit.cpp
    cfe/trunk/test/CXX/special/class.dtor/p9.cpp
    cfe/trunk/test/CodeGenCXX/key-function-vtable.cpp
    cfe/trunk/test/SemaCXX/devirtualize-vtable-marking.cpp
    cfe/trunk/test/SemaCXX/microsoft-dtor-lookup.cpp
    cfe/trunk/test/SemaCXX/warn-weak-vtables.cpp
    cfe/trunk/test/SemaTemplate/virtual-member-functions.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Jan 26 00:23:36 2015
@@ -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,

Modified: cfe/trunk/lib/Sema/Sema.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/Sema.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/Sema.cpp (original)
+++ cfe/trunk/lib/Sema/Sema.cpp Mon Jan 26 00:23:36 2015
@@ -337,18 +337,6 @@ ExprResult Sema::ImpCastExprToType(Expr
   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);

Modified: cfe/trunk/lib/Sema/SemaCast.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaCast.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCast.cpp Mon Jan 26 00:23:36 2015
@@ -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<CXXRecordDecl>(SrcRecord->getDecl()));
     return;
   }
 
@@ -682,8 +676,6 @@ void CastOperation::CheckDynamicCast() {
       << 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

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Jan 26 00:23:36 2015
@@ -1745,18 +1745,6 @@ void Sema::BuildBasePathArray(const CXXB
     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

Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Mon Jan 26 00:23:36 2015
@@ -11621,7 +11621,7 @@ void Sema::MarkFunctionReferenced(Source
     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() &&
@@ -11641,7 +11641,7 @@ void Sema::MarkFunctionReferenced(Source
         DefineImplicitLambdaToBlockPointerConversion(Loc, Conversion);
       else
         DefineImplicitLambdaToFunctionPointerConversion(Loc, Conversion);
-    } else if (MethodDecl->isVirtual())
+    } else if (MethodDecl->isVirtual() && getLangOpts().AppleKext)
       MarkVTableUsed(Loc, MethodDecl->getParent());
   }
 

Modified: cfe/trunk/lib/Sema/SemaInit.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaInit.cpp (original)
+++ cfe/trunk/lib/Sema/SemaInit.cpp Mon Jan 26 00:23:36 2015
@@ -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<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 :

Modified: cfe/trunk/test/CXX/special/class.dtor/p9.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/special/class.dtor/p9.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/test/CXX/special/class.dtor/p9.cpp (original)
+++ cfe/trunk/test/CXX/special/class.dtor/p9.cpp Mon Jan 26 00:23:36 2015
@@ -89,4 +89,11 @@ namespace test3 {
     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;
+  }
 }

Modified: cfe/trunk/test/CodeGenCXX/key-function-vtable.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/key-function-vtable.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenCXX/key-function-vtable.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/key-function-vtable.cpp Mon Jan 26 00:23:36 2015
@@ -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

Modified: cfe/trunk/test/SemaCXX/devirtualize-vtable-marking.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/devirtualize-vtable-marking.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/devirtualize-vtable-marking.cpp (original)
+++ cfe/trunk/test/SemaCXX/devirtualize-vtable-marking.cpp Mon Jan 26 00:23:36 2015
@@ -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;
 }
 }
 

Modified: cfe/trunk/test/SemaCXX/microsoft-dtor-lookup.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/microsoft-dtor-lookup.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/microsoft-dtor-lookup.cpp (original)
+++ cfe/trunk/test/SemaCXX/microsoft-dtor-lookup.cpp Mon Jan 26 00:23:36 2015
@@ -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;
 }
 
 }

Modified: cfe/trunk/test/SemaCXX/warn-weak-vtables.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-weak-vtables.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-weak-vtables.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-weak-vtables.cpp Mon Jan 26 00:23:36 2015
@@ -20,15 +20,14 @@ void f() {
     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 @@ 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<typename T> struct TemplVirt {
@@ -72,8 +70,8 @@ template<> struct TemplVirt<long> { // e
   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;
 }

Modified: cfe/trunk/test/SemaTemplate/virtual-member-functions.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/virtual-member-functions.cpp?rev=227073&r1=227072&r2=227073&view=diff
==============================================================================
--- cfe/trunk/test/SemaTemplate/virtual-member-functions.cpp (original)
+++ cfe/trunk/test/SemaTemplate/virtual-member-functions.cpp Mon Jan 26 00:23:36 2015
@@ -110,12 +110,12 @@ namespace PR7114 {
 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