BlinkGCPlugin: Handle omitted trace() in intermediary classes.

As a result of introducing templated traceImpl() function, the callee part of
"Base::trace()" call (i.e. Base::trace) may not be fully resolved at the time
of the plugin execution. This has caused another issue in cases where trace()
is intentionally omitted because the class has no fields to trace.

Consider the following example:

    class A : public GarbageCollected<A> { trace() { ... } };
    class B : public A { /* No trace() */ };
    class C : public B { trace() { B::trace(visitor); } };

The "B::trace()" call in C is actually A::trace(). If the callee part "B::trace"
is unresolved, we only know that "there is a function call which will be
resolved to A::trace()". We still want to mark B as traced when we see such
a function call.

This patch fixes this trace call detection logic so we can handle this case.
The new logic keeps going up the class hierarchy while the class is not
required to have a trace method, and if we find a class that matches the class
of the (unresolved) callee part, we mark the direct base class of the original
class as traced.

BUG=462511
R=kouhei@chromium.org
CC=zerny@chromium.org, oilpan-reviews@chromium.org

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

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

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -325,8 +325,12 @@ class CheckDispatchVisitor : public RecursiveASTVisitor<CheckDispatchVisitor> {
 // - A base is traced if a base-qualified call to a trace method is found.
 class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
  public:
-  CheckTraceVisitor(CXXMethodDecl* trace, RecordInfo* info)
-      : trace_(trace), info_(info), delegates_to_traceimpl_(false) {}
+  CheckTraceVisitor(CXXMethodDecl* trace, RecordInfo* info, RecordCache* cache)
+      : trace_(trace),
+        info_(info),
+        cache_(cache),
+        delegates_to_traceimpl_(false) {
+  }
 
   bool delegates_to_traceimpl() const { return delegates_to_traceimpl_; }
 
@@ -541,12 +545,48 @@ class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
     if (!IsTraceCallName(func_name))
       return false;
 
-    RecordInfo::Bases::iterator iter = info_->GetBases().find(callee_record);
-    if (iter == info_->GetBases().end())
-      return false;
+    for (auto& base : info_->GetBases()) {
+      // We want to deal with omitted trace() function in an intermediary
+      // class in the class hierarchy, e.g.:
+      //     class A : public GarbageCollected<A> { trace() { ... } };
+      //     class B : public A { /* No trace(); have nothing to trace. */ };
+      //     class C : public B { trace() { B::trace(visitor); } }
+      // where, B::trace() is actually A::trace(), and in some cases we get
+      // A as |callee_record| instead of B. We somehow need to mark B as
+      // traced if we find A::trace() call.
+      //
+      // To solve this, here we keep going up the class hierarchy as long as
+      // they are not required to have a trace method. The implementation is
+      // a simple DFS, where |base_records| represents the set of base classes
+      // we need to visit.
+
+      std::vector<CXXRecordDecl*> base_records;
+      base_records.push_back(base.first);
+
+      while (!base_records.empty()) {
+        CXXRecordDecl* base_record = base_records.back();
+        base_records.pop_back();
+
+        if (base_record == callee_record) {
+          // If we find a matching trace method, pretend the user has written
+          // a correct trace() method of the base; in the example above, we
+          // find A::trace() here and mark B as correctly traced.
+          base.second.MarkTraced();
+          return true;
+        }
 
-    iter->second.MarkTraced();
-    return true;
+        if (RecordInfo* base_info = cache_->Lookup(base_record)) {
+          if (!base_info->RequiresTraceMethod()) {
+            // If this base class is not required to have a trace method, then
+            // the actual trace method may be defined in an ancestor.
+            for (auto& inner_base : base_info->GetBases())
+              base_records.push_back(inner_base.first);
+          }
+        }
+      }
+    }
+
+    return false;
   }
 
   bool CheckTraceFieldCall(CXXMemberCallExpr* call) {
@@ -609,7 +649,8 @@ class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
   };
 
   // Nested checking for weak callbacks.
-  CheckTraceVisitor(RecordInfo* info) : trace_(0), info_(info) {}
+  CheckTraceVisitor(RecordInfo* info)
+      : trace_(nullptr), info_(info), cache_(nullptr) {}
 
   bool IsWeakCallback() { return !trace_; }
 
@@ -643,6 +684,7 @@ class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
 
   CXXMethodDecl* trace_;
   RecordInfo* info_;
+  RecordCache* cache_;
   bool delegates_to_traceimpl_;
 };
 
@@ -1387,7 +1429,7 @@ class BlinkGCPluginConsumer : public ASTConsumer {
       }
     }
 
-    CheckTraceVisitor visitor(trace, parent);
+    CheckTraceVisitor visitor(trace, parent, &cache_);
     visitor.TraverseCXXMethodDecl(trace);
 
     // Skip reporting if this trace method is a just delegate to

tools/clang/blink_gc_plugin/tests/traceimpl_omitted_trace.cpp

+// Copyright 2015 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 "traceimpl_omitted_trace.h"
+
+// Nothing to define here.

tools/clang/blink_gc_plugin/tests/traceimpl_omitted_trace.h

+// Copyright 2015 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 TRACEIMPL_OMITTED_TRACE_H_
+#define TRACEIMPL_OMITTED_TRACE_H_
+
+#include "heap/stubs.h"
+
+namespace blink {
+
+class A : public GarbageCollected<A> {
+ public:
+  virtual void trace(Visitor* visitor) { traceImpl(visitor); }
+  virtual void trace(InlinedGlobalMarkingVisitor visitor) {
+    traceImpl(visitor);
+  }
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor) {}
+};
+
+class B : public A {
+  // trace() isn't necessary because we've got nothing to trace here.
+};
+
+class C : public B {
+ public:
+  void trace(Visitor* visitor) override { traceImpl(visitor); }
+  void trace(InlinedGlobalMarkingVisitor visitor) override {
+    traceImpl(visitor);
+  }
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor) {
+    // B::trace() is actually A::trace(), and in certain cases we only get
+    // limited information like "there is a function call that will be resolved
+    // to A::trace()". We still want to mark B as traced.
+    B::trace(visitor);
+  }
+};
+
+}
+
+#endif  // TRACEIMPL_OMITTED_TRACE_H_

tools/clang/blink_gc_plugin/tests/traceimpl_omitted_trace.txt

+