Blink GC plugin: Check polymorphic requirements on all GC-derived classes.

Prior to this change we only checked the polymorphic requirements on classes
that declared a trace method. Also, this fixes a typo in condition (2) where we
checked on the derived class instead of the left-most class.

R=ager@chromium.org
BUG=

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

Cr-Commit-Position: refs/heads/master@{#294360}

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -951,12 +951,17 @@ 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);
     }
 
+    // Check polymorphic classes that are GC-derived or have a trace method.
+    if (info->record()->hasDefinition() && info->record()->isPolymorphic()) {
+      CXXMethodDecl* trace = info->GetTraceMethod();
+      if (trace || info->IsGCDerived())
+        CheckPolymorphicClass(info, trace);
+    }
+
     {
       CheckFieldsVisitor visitor(options_);
       if (visitor.ContainsInvalidFields(info))
@@ -1010,8 +1015,8 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   // 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.
+  // This ensures that if the vtable is initialized then 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
@@ -1041,7 +1046,7 @@ class BlinkGCPluginConsumer : public ASTConsumer {
         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).
+      // is non-polymorphic and contains no fields).
       if (Config::IsSafePolymorphicBase(name))
         break;
 
@@ -1052,7 +1057,7 @@ class BlinkGCPluginConsumer : public ASTConsumer {
     if (RecordInfo* left_most_info = cache_.Lookup(left_most)) {
 
       // Check condition (1):
-      if (trace->isVirtual()) {
+      if (trace && trace->isVirtual()) {
         if (CXXMethodDecl* trace = left_most_info->GetTraceMethod()) {
           if (trace->isVirtual())
             return;
@@ -1062,7 +1067,7 @@ class BlinkGCPluginConsumer : public ASTConsumer {
       }
 
       // Check condition (2):
-      if (DeclaresVirtualMethods(info->record()))
+      if (DeclaresVirtualMethods(left_most))
         return;
       if (left_most_base) {
         ++it; // Get the base next to the "safe polymorphic base"

tools/clang/blink_gc_plugin/tests/class_does_not_require_finalization.txt

 In file included from class_does_not_require_finalization.cpp:5:
 ./class_does_not_require_finalization.h:18:1: warning: [blink-gc] Class 'DoesNotNeedFinalizer' may not require finalization.
-class DoesNotNeedFinalizer : public GarbageCollectedFinalized<DoesNotNeedFinalizer> {
+class DoesNotNeedFinalizer
 ^
-./class_does_not_require_finalization.h:23:1: warning: [blink-gc] Class 'DoesNotNeedFinalizer2' may not require finalization.
-class DoesNotNeedFinalizer2 : public GarbageCollectedFinalized<DoesNotNeedFinalizer2> {
+./class_does_not_require_finalization.h:24:1: warning: [blink-gc] Class 'DoesNotNeedFinalizer2' may not require finalization.
+class DoesNotNeedFinalizer2
 ^
-./class_does_not_require_finalization.h:34:1: warning: [blink-gc] Class 'DoesNotNeedFinalizer3' may not require finalization.
-class DoesNotNeedFinalizer3 : public GarbageCollectedFinalized<DoesNotNeedFinalizer3>, public HasEmptyDtor {
+./class_does_not_require_finalization.h:36:1: warning: [blink-gc] Class 'DoesNotNeedFinalizer3' may not require finalization.
+class DoesNotNeedFinalizer3
 ^
 3 warnings generated.

tools/clang/blink_gc_plugin/tests/polymorphic_class_with_non_virtual_trace.h

@@ -41,6 +41,21 @@ private:
     Member<HeapObject> m_obj;
 };
 
+template<typename T>
+class TemplatedNonPolymorphicBase
+    : public GarbageCollected<TemplatedNonPolymorphicBase<T> > {
+public:
+    void trace(Visitor* visitor) { visitor->trace(m_obj); }
+private:
+    Member<HeapObject> m_obj;
+};
+
+// Looks OK, but will result in an incorrect object pointer when marking.
+class TemplatedIsNotLeftMostPolymorphic
+    : public TemplatedNonPolymorphicBase<TemplatedIsNotLeftMostPolymorphic>,
+      public PolymorphicBase {
+};
+
 }
 
 #endif

tools/clang/blink_gc_plugin/tests/polymorphic_class_with_non_virtual_trace.txt

@@ -2,4 +2,7 @@ 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.
+./polymorphic_class_with_non_virtual_trace.h:45:1: warning: [blink-gc] Left-most base class 'TemplatedNonPolymorphicBase<blink::TemplatedIsNotLeftMostPolymorphic>' of derived class 'TemplatedIsNotLeftMostPolymorphic' must be polymorphic.
+class TemplatedNonPolymorphicBase
+^
+2 warnings generated.

tools/clang/blink_gc_plugin/tests/virtual_and_trace_after_dispatch.txt

 In file included from virtual_and_trace_after_dispatch.cpp:5:
+./virtual_and_trace_after_dispatch.h:12:1: warning: [blink-gc] Left-most base class 'A' of derived class 'B' must be polymorphic.
+class A : public GarbageCollected<A> {
+^
 ./virtual_and_trace_after_dispatch.h:23:1: warning: [blink-gc] Class 'B' contains or inherits virtual methods but implements manual dispatching.
 class B : public A {
 ^
 ./virtual_and_trace_after_dispatch.h:14:5: note: [blink-gc] Manual dispatch 'trace' declared here:
     void trace(Visitor*);
     ^
-1 warning generated.
+2 warnings generated.