[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