Blink GC plugin: Ensure that left-most vtable is the first thing to be...

Blink GC plugin: Ensure that left-most vtable is the first thing to be initialized for polymorphic classes with trace methods.
    
This is done by ensuring one of two properties for any polymorphic class with a trace method.
1. If the trace method is virtual, then the left-most base class must define a virtual trace method too.
2. If the trace method is non-virtual, then the left-most base class must define some virtual method.

BUG=334149
R=ager@chromium.org

Review URL: https://codereview.chromium.org/455263004

Cr-Commit-Position: refs/heads/master@{#289285}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289285 0039d316-1c4b-4281-b951-d872f2087c98

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -129,6 +129,14 @@ const char kClassDeclaresPureVirtualTrace[] =
     "[blink-gc] Garbage collected class %0"
     " is not permitted to declare a pure-virtual trace method.";
 
+const char kLeftMostBaseMustBePolymorphic[] =
+    "[blink-gc] Left-most base class %0 of derived class %1"
+    " must be polymorphic.";
+
+const char kBaseClassMustDeclareVirtualTrace[] =
+    "[blink-gc] Left-most base class %0 of derived class %1"
+    " must define a virtual trace method.";
+
 struct BlinkGCPluginOptions {
   BlinkGCPluginOptions() : enable_oilpan(false), dump_graph(false) {}
   bool enable_oilpan;
@@ -771,6 +779,10 @@ class BlinkGCPluginConsumer : public ASTConsumer {
         diagnostic_.getCustomDiagID(getErrorLevel(), kClassOverridesNew);
     diag_class_declares_pure_virtual_trace_ = diagnostic_.getCustomDiagID(
         getErrorLevel(), kClassDeclaresPureVirtualTrace);
+    diag_left_most_base_must_be_polymorphic_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kLeftMostBaseMustBePolymorphic);
+    diag_base_class_must_declare_virtual_trace_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kBaseClassMustDeclareVirtualTrace);
 
     // Register note messages.
     diag_base_requires_tracing_note_ = diagnostic_.getCustomDiagID(
@@ -900,6 +912,8 @@ class BlinkGCPluginConsumer : public ASTConsumer {
     if (CXXMethodDecl* trace = info->GetTraceMethod()) {
       if (trace->isPure())
         ReportClassDeclaresPureVirtualTrace(info, trace);
+      if (info->record()->isPolymorphic())
+        CheckPolymorphicClass(info, trace);
     } else if (info->RequiresTraceMethod()) {
       ReportClassRequiresTraceMethod(info);
     }
@@ -932,6 +946,121 @@ class BlinkGCPluginConsumer : public ASTConsumer {
     DumpClass(info);
   }
 
+  CXXRecordDecl* GetDependentTemplatedDecl(const Type& type) {
+    const TemplateSpecializationType* tmpl_type =
+        type.getAs<TemplateSpecializationType>();
+    if (!tmpl_type)
+      return 0;
+
+    TemplateDecl* tmpl_decl = tmpl_type->getTemplateName().getAsTemplateDecl();
+    if (!tmpl_decl)
+      return 0;
+
+    return dyn_cast<CXXRecordDecl>(tmpl_decl->getTemplatedDecl());
+  }
+
+  // The GC infrastructure assumes that if the vtable of a polymorphic
+  // base-class is not initialized for a given object (ie, it is partially
+  // initialized) then the object does not need to be traced. Thus, we must
+  // ensure that any polymorphic class with a trace method does not have any
+  // tractable fields that are initialized before we are sure that the vtable
+  // and the trace method are both defined.  There are two cases that need to
+  // hold to satisfy that assumption:
+  //
+  // 1. If trace is virtual, then it must be defined in the left-most base.
+  // This ensures that if the vtable is initialized and it contains a pointer to
+  // the trace method.
+  //
+  // 2. If trace is non-virtual, then the trace method is defined and we must
+  // ensure that the left-most base defines a vtable. This ensures that the
+  // first thing to be initialized when constructing the object is the vtable
+  // itself.
+  void CheckPolymorphicClass(RecordInfo* info, CXXMethodDecl* trace) {
+    CXXRecordDecl* left_most = info->record();
+    CXXRecordDecl::base_class_iterator it = left_most->bases_begin();
+    CXXRecordDecl* left_most_base = 0;
+    while (it != left_most->bases_end()) {
+      left_most_base = it->getType()->getAsCXXRecordDecl();
+      if (!left_most_base && it->getType()->isDependentType())
+        left_most_base = GetDependentTemplatedDecl(*it->getType());
+
+      // TODO: Find a way to correctly check actual instantiations
+      // for dependent types. The escape below will be hit, eg, when
+      // we have a primary template with no definition and
+      // specializations for each case (such as SupplementBase) in
+      // which case we don't succeed in checking the required
+      // properties.
+      if (!left_most_base || !left_most_base->hasDefinition())
+        return;
+
+      StringRef name = left_most_base->getName();
+      // We know GCMixin base defines virtual trace.
+      if (Config::IsGCMixinBase(name))
+        return;
+
+      // Stop with the left-most prior to a safe polymorphic base (a safe base
+      // is non-polymorphic and contains no fields that need tracing).
+      if (Config::IsSafePolymorphicBase(name))
+        break;
+
+      left_most = left_most_base;
+      it = left_most->bases_begin();
+    }
+
+    if (RecordInfo* left_most_info = cache_.Lookup(left_most)) {
+
+      // Check condition (1):
+      if (trace->isVirtual()) {
+        if (CXXMethodDecl* trace = left_most_info->GetTraceMethod()) {
+          if (trace->isVirtual())
+            return;
+        }
+        ReportBaseClassMustDeclareVirtualTrace(info, left_most);
+        return;
+      }
+
+      // Check condition (2):
+      if (DeclaresVirtualMethods(info->record()))
+        return;
+      if (left_most_base) {
+        ++it; // Get the base next to the "safe polymorphic base"
+        if (it != left_most->bases_end()) {
+          if (CXXRecordDecl* next_base = it->getType()->getAsCXXRecordDecl()) {
+            if (CXXRecordDecl* next_left_most = GetLeftMostBase(next_base)) {
+              if (DeclaresVirtualMethods(next_left_most))
+                return;
+              ReportLeftMostBaseMustBePolymorphic(info, next_left_most);
+              return;
+            }
+          }
+        }
+      }
+      ReportLeftMostBaseMustBePolymorphic(info, left_most);
+    }
+  }
+
+  CXXRecordDecl* GetLeftMostBase(CXXRecordDecl* left_most) {
+    CXXRecordDecl::base_class_iterator it = left_most->bases_begin();
+    while (it != left_most->bases_end()) {
+      if (it->getType()->isDependentType())
+        left_most = GetDependentTemplatedDecl(*it->getType());
+      else
+        left_most = it->getType()->getAsCXXRecordDecl();
+      if (!left_most || !left_most->hasDefinition())
+        return 0;
+      it = left_most->bases_begin();
+    }
+    return left_most;
+  }
+
+  bool DeclaresVirtualMethods(CXXRecordDecl* decl) {
+    CXXRecordDecl::method_iterator it = decl->method_begin();
+    for (; it != decl->method_end(); ++it)
+      if (it->isVirtual() && !it->isPure())
+        return true;
+    return false;
+  }
+
   void CheckLeftMostDerived(RecordInfo* info) {
     CXXRecordDecl* left_most = info->record();
     CXXRecordDecl::base_class_iterator it = left_most->bases_begin();
@@ -1074,16 +1203,12 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   void CheckTraceMethod(RecordInfo* parent,
                         CXXMethodDecl* trace,
                         bool isTraceAfterDispatch) {
-    // A non-virtual trace method must not override another trace.
-    if (!isTraceAfterDispatch && !trace->isVirtual()) {
+    // A trace method must not override any non-virtual trace methods.
+    if (!isTraceAfterDispatch) {
       for (RecordInfo::Bases::iterator it = parent->GetBases().begin();
            it != parent->GetBases().end();
            ++it) {
         RecordInfo* base = it->second.info();
-        // We allow mixin bases to contain a non-virtual trace since it will
-        // never be used for dispatching.
-        if (base->IsGCMixin())
-          continue;
         if (CXXMethodDecl* other = base->InheritsNonVirtualTrace())
           ReportOverriddenNonVirtualTrace(parent, trace, other);
       }
@@ -1484,6 +1609,24 @@ class BlinkGCPluginConsumer : public ASTConsumer {
         << info->record();
   }
 
+  void ReportLeftMostBaseMustBePolymorphic(RecordInfo* derived,
+                                           CXXRecordDecl* base) {
+    SourceLocation loc = base->getLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, diag_left_most_base_must_be_polymorphic_)
+        << base << derived->record();
+  }
+
+  void ReportBaseClassMustDeclareVirtualTrace(RecordInfo* derived,
+                                              CXXRecordDecl* base) {
+    SourceLocation loc = base->getLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, diag_base_class_must_declare_virtual_trace_)
+        << base << derived->record();
+  }
+
   void NoteManualDispatchMethod(CXXMethodDecl* dispatch) {
     SourceLocation loc = dispatch->getLocStart();
     SourceManager& manager = instance_.getSourceManager();
@@ -1574,6 +1717,8 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   unsigned diag_derives_non_stack_allocated_;
   unsigned diag_class_overrides_new_;
   unsigned diag_class_declares_pure_virtual_trace_;
+  unsigned diag_left_most_base_must_be_polymorphic_;
+  unsigned diag_base_class_must_declare_virtual_trace_;
 
   unsigned diag_base_requires_tracing_note_;
   unsigned diag_field_requires_tracing_note_;

tools/clang/blink_gc_plugin/Config.h

@@ -105,13 +105,23 @@ class Config {
     return (IsHashMap(name) || name == "pair") ? 2 : 1;
   }
 
+  static bool IsDummyBase(const std::string& name) {
+    return name == "DummyBase";
+  }
+
+  static bool IsRefCountedBase(const std::string& name) {
+    return name == "RefCounted" ||
+           name == "ThreadSafeRefCounted";
+  }
+
   static bool IsGCMixinBase(const std::string& name) {
     return name == "GarbageCollectedMixin";
   }
 
   static bool IsGCFinalizedBase(const std::string& name) {
     return name == "GarbageCollectedFinalized" ||
-           name == "RefCountedGarbageCollected";
+           name == "RefCountedGarbageCollected" ||
+           name == "ThreadSafeRefCountedGarbageCollected";
   }
 
   static bool IsGCBase(const std::string& name) {
@@ -120,6 +130,12 @@ class Config {
            IsGCMixinBase(name);
   }
 
+  // Returns true of the base classes that do not need a vtable entry for trace
+  // because they cannot possibly initiate a GC during construction.
+  static bool IsSafePolymorphicBase(const std::string& name) {
+    return IsGCBase(name) || IsDummyBase(name) || IsRefCountedBase(name);
+  }
+
   static bool IsAnnotated(clang::Decl* decl, const std::string& anno) {
     clang::AnnotateAttr* attr = decl->getAttr<clang::AnnotateAttr>();
     return attr && (attr->getAnnotation() == anno);

tools/clang/blink_gc_plugin/tests/base_class_must_define_virtual_trace.cpp

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base_class_must_define_virtual_trace.h"
+
+namespace blink {
+
+void PartDerived::trace(Visitor* visitor)
+{
+}
+
+void HeapDerived::trace(Visitor* visitor)
+{
+    visitor->trace(m_part);
+}
+
+
+}

tools/clang/blink_gc_plugin/tests/base_class_must_define_virtual_trace.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_CLASS_MUST_DEFINE_VIRTUAL_TRACE_H_
+#define BASE_CLASS_MUST_DEFINE_VIRTUAL_TRACE_H_
+
+#include "heap/stubs.h"
+
+namespace blink {
+
+class PartBase {
+    DISALLOW_ALLOCATION();
+    // Missing virtual trace.
+};
+
+class PartDerived : public PartBase {
+    DISALLOW_ALLOCATION();
+public:
+    virtual void trace(Visitor*);
+};
+
+class HeapBase : public GarbageCollected<HeapBase> {
+    // Missing virtual trace.
+};
+
+
+class HeapDerived : public HeapBase {
+public:
+    virtual void trace(Visitor*);
+private:
+    PartDerived m_part;
+};
+
+
+}
+
+#endif

tools/clang/blink_gc_plugin/tests/base_class_must_define_virtual_trace.txt

+In file included from base_class_must_define_virtual_trace.cpp:5:
+./base_class_must_define_virtual_trace.h:12:1: warning: [blink-gc] Left-most base class 'PartBase' of derived class 'PartDerived' must define a virtual trace method.
+class PartBase {
+^
+./base_class_must_define_virtual_trace.h:23:1: warning: [blink-gc] Left-most base class 'HeapBase' of derived class 'HeapDerived' must define a virtual trace method.
+class HeapBase : public GarbageCollected<HeapBase> {
+^
+2 warnings generated.

tools/clang/blink_gc_plugin/tests/heap/stubs.h

@@ -216,6 +216,7 @@ public:
 class GarbageCollectedMixin {
     virtual void adjustAndMark(Visitor*) const = 0;
     virtual bool isAlive(Visitor*) const = 0;
+    virtual void trace(Visitor*) { }
 };
 
 template<typename T>

tools/clang/blink_gc_plugin/tests/non_virtual_trace.cpp

@@ -15,4 +15,9 @@ void C::trace(Visitor* visitor)
     B::trace(visitor);
 }
 
+void D::trace(Visitor* visitor)
+{
+    B::trace(visitor);
+}
+
 }

tools/clang/blink_gc_plugin/tests/non_virtual_trace.h

@@ -22,6 +22,11 @@ public:
     void trace(Visitor*); // Cannot override a non-virtual trace.
 };
 
+class D : public B {
+public:
+    virtual void trace(Visitor*); // Cannot override a non-virtual trace.
+};
+
 }
 
 #endif

tools/clang/blink_gc_plugin/tests/non_virtual_trace.txt

+In file included from non_virtual_trace.cpp:5:
+./non_virtual_trace.h:12:1: warning: [blink-gc] Left-most base class 'A' of derived class 'D' must define a virtual trace method.
+class A : public GarbageCollected<A> {
+^
 non_virtual_trace.cpp:13:1: warning: [blink-gc] Class 'C' overrides non-virtual trace of base class 'A'.
 void C::trace(Visitor* visitor)
 ^
 ./non_virtual_trace.h:14:5: note: [blink-gc] Non-virtual trace method declared here:
     void trace(Visitor*);
     ^
-1 warning generated.
+non_virtual_trace.cpp:18:1: warning: [blink-gc] Class 'D' overrides non-virtual trace of base class 'A'.
+void D::trace(Visitor* visitor)
+^
+./non_virtual_trace.h:14:5: note: [blink-gc] Non-virtual trace method declared here:
+    void trace(Visitor*);
+    ^
+3 warnings generated.

tools/clang/blink_gc_plugin/tests/polymorphic_class_with_non_virtual_trace.cpp

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "polymorphic_class_with_non_virtual_trace.h"
+
+namespace blink {
+
+void IsLeftMostPolymorphic::trace(Visitor* visitor)
+{
+    visitor->trace(m_obj);
+}
+
+void IsNotLeftMostPolymorphic::trace(Visitor* visitor)
+{
+    visitor->trace(m_obj);
+}
+
+}

tools/clang/blink_gc_plugin/tests/polymorphic_class_with_non_virtual_trace.h

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef POLYMORPHIC_CLASS_WITH_NON_VIRTUAL_TRACE_H_
+#define POLYMORPHIC_CLASS_WITH_NON_VIRTUAL_TRACE_H_
+
+#include "heap/stubs.h"
+
+namespace blink {
+
+class HeapObject : public GarbageCollected<HeapObject> {
+public:
+    void trace(Visitor*) { }
+};
+
+class NonPolymorphicBase {
+};
+
+class PolymorphicBase {
+public:
+    virtual void foo();
+};
+
+class IsLeftMostPolymorphic
+    : public GarbageCollected<IsLeftMostPolymorphic>,
+      public PolymorphicBase {
+public:
+    void trace(Visitor*);
+private:
+    Member<HeapObject> m_obj;
+};
+
+class IsNotLeftMostPolymorphic
+    : public GarbageCollected<IsNotLeftMostPolymorphic>,
+      public NonPolymorphicBase,
+      public PolymorphicBase {
+public:
+    void trace(Visitor*);
+private:
+    Member<HeapObject> m_obj;
+};
+
+}
+
+#endif

tools/clang/blink_gc_plugin/tests/polymorphic_class_with_non_virtual_trace.txt

+In file included from polymorphic_class_with_non_virtual_trace.cpp:5:
+./polymorphic_class_with_non_virtual_trace.h:17:1: warning: [blink-gc] Left-most base class 'NonPolymorphicBase' of derived class 'IsNotLeftMostPolymorphic' must be polymorphic.
+class NonPolymorphicBase {
+^
+1 warning generated.