BlinkGCPlugin: Fix detection of trace dispatching after inlined tracing.

This patch actually fixes three different issues, as I could not separate the
updated tests in a meaningful way.

1. CheckDispatchVisitor needs to look for UnresolvedMemberExpr*, since inlined
tracing introduces unresolved member references.

This visitor looks at every function call within a trace dispatching method,
and returns whether there is a call delegating to the class in question. We
need to take care of UnresolvedMemberExpr* cases in addition to MemberExpr*.

2. RecordInfo::DetermineTracingMethods() should return traceImpl() instead of
trace() as trace_dispatch_method_.

The plugin inspects the definition of trace_dispatch_method_ in order to detect
missing or inappropriate delegation to subclasses. Therefore, the method should
be something that has actual delegation calls in its body (i.e. traceImpl())
instead of a trampoline stub (i.e. trace()).

3. A silly typo in Config::GetTraceMethodType(). Really silly. Not sure why
I did this way.

This patch comes with updated tests for traceAfterDispatchImpl(). Now the tests
are more closer to real cases, using traceImpl to implement trace dispatching.

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

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

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

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -315,6 +315,20 @@ class CheckDispatchVisitor : public RecursiveASTVisitor<CheckDispatchVisitor> {
     return true;
   }
 
+  bool VisitUnresolvedMemberExpr(UnresolvedMemberExpr* member) {
+    for (Decl* decl : member->decls()) {
+      if (CXXMethodDecl* method = dyn_cast<CXXMethodDecl>(decl)) {
+        if (method->getParent() == receiver_->record() &&
+            Config::GetTraceMethodType(method) ==
+            Config::TRACE_AFTER_DISPATCH_METHOD) {
+          dispatched_to_receiver_ = true;
+          return true;
+        }
+      }
+    }
+    return true;
+  }
+
  private:
   RecordInfo* receiver_;
   bool dispatched_to_receiver_;
@@ -1407,7 +1421,8 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   // Determine what type of tracing method this is (dispatch or trace).
   void CheckTraceOrDispatchMethod(RecordInfo* parent, CXXMethodDecl* method) {
     Config::TraceMethodType trace_type = Config::GetTraceMethodType(method);
-    if (trace_type != Config::TRACE_METHOD ||
+    if (trace_type == Config::TRACE_AFTER_DISPATCH_METHOD ||
+        trace_type == Config::TRACE_AFTER_DISPATCH_IMPL_METHOD ||
         !parent->GetTraceDispatchMethod()) {
       CheckTraceMethod(parent, method, trace_type);
     }

tools/clang/blink_gc_plugin/Config.h

@@ -236,7 +236,7 @@ class Config {
     if (name == kTraceAfterDispatchName)
       return TRACE_AFTER_DISPATCH_METHOD;
     if (name == kTraceImplName)
-      return TRACE_AFTER_DISPATCH_METHOD;
+      return TRACE_IMPL_METHOD;
     if (name == kTraceAfterDispatchImplName)
       return TRACE_AFTER_DISPATCH_IMPL_METHOD;
 

tools/clang/blink_gc_plugin/RecordInfo.cpp

@@ -427,39 +427,50 @@ void RecordInfo::DetermineTracingMethods() {
   if (Config::IsGCBase(name_))
     return;
   CXXMethodDecl* trace = nullptr;
+  CXXMethodDecl* trace_impl = nullptr;
   CXXMethodDecl* trace_after_dispatch = nullptr;
   bool has_adjust_and_mark = false;
   bool has_is_heap_object_alive = false;
-  for (CXXRecordDecl::method_iterator it = record_->method_begin();
-       it != record_->method_end();
-       ++it) {
-    switch (Config::GetTraceMethodType(*it)) {
+  for (Decl* decl : record_->decls()) {
+    CXXMethodDecl* method = dyn_cast<CXXMethodDecl>(decl);
+    if (!method) {
+      if (FunctionTemplateDecl* func_template =
+          dyn_cast<FunctionTemplateDecl>(decl))
+        method = dyn_cast<CXXMethodDecl>(func_template->getTemplatedDecl());
+    }
+    if (!method)
+      continue;
+
+    switch (Config::GetTraceMethodType(method)) {
       case Config::TRACE_METHOD:
-        trace = *it;
+        trace = method;
         break;
       case Config::TRACE_AFTER_DISPATCH_METHOD:
-        trace_after_dispatch = *it;
+        trace_after_dispatch = method;
         break;
       case Config::TRACE_IMPL_METHOD:
+        trace_impl = method;
+        break;
       case Config::TRACE_AFTER_DISPATCH_IMPL_METHOD:
         break;
       case Config::NOT_TRACE_METHOD:
-        if (it->getNameAsString() == kFinalizeName) {
-          finalize_dispatch_method_ = *it;
-        } else if (it->getNameAsString() == kAdjustAndMarkName) {
+        if (method->getNameAsString() == kFinalizeName) {
+          finalize_dispatch_method_ = method;
+        } else if (method->getNameAsString() == kAdjustAndMarkName) {
           has_adjust_and_mark = true;
-        } else if (it->getNameAsString() == kIsHeapObjectAliveName) {
+        } else if (method->getNameAsString() == kIsHeapObjectAliveName) {
           has_is_heap_object_alive = true;
         }
         break;
     }
   }
+
   // Record if class defines the two GCMixin methods.
   has_gc_mixin_methods_ =
       has_adjust_and_mark && has_is_heap_object_alive ? kTrue : kFalse;
   if (trace_after_dispatch) {
     trace_method_ = trace_after_dispatch;
-    trace_dispatch_method_ = trace;
+    trace_dispatch_method_ = trace_impl ? trace_impl : trace;
   } else {
     // TODO: Can we never have a dispatch method called trace without the same
     // class defining a traceAfterDispatch method?

tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl.cpp

@@ -6,9 +6,11 @@
 
 namespace blink {
 
-void TraceAfterDispatchInlinedBase::trace(Visitor* visitor) {
-  // Implement a simple form of manual dispatching, because BlinkGCPlugin gets
-  // angry if dispatching statements are missing.
+template <typename VisitorDispatcher>
+inline void TraceAfterDispatchInlinedBase::traceImpl(
+    VisitorDispatcher visitor) {
+  // Implement a simple form of manual dispatching, because BlinkGCPlugin
+  // checks if the tracing is dispatched to all derived classes.
   //
   // This function has to be implemented out-of-line, since we need to know the
   // definition of derived classes here.
@@ -21,6 +23,15 @@ void TraceAfterDispatchInlinedBase::trace(Visitor* visitor) {
 }
 
 void TraceAfterDispatchExternBase::trace(Visitor* visitor) {
+  traceImpl(visitor);
+}
+
+void TraceAfterDispatchExternBase::trace(InlinedGlobalMarkingVisitor visitor) {
+  traceImpl(visitor);
+}
+
+template <typename VisitorDispatcher>
+inline void TraceAfterDispatchExternBase::traceImpl(VisitorDispatcher visitor) {
   if (tag_ == DERIVED) {
     static_cast<TraceAfterDispatchExternDerived*>(this)->traceAfterDispatch(
         visitor);
@@ -33,6 +44,11 @@ void TraceAfterDispatchExternBase::traceAfterDispatch(Visitor* visitor) {
   traceAfterDispatchImpl(visitor);
 }
 
+void TraceAfterDispatchExternBase::traceAfterDispatch(
+    InlinedGlobalMarkingVisitor visitor) {
+  traceAfterDispatchImpl(visitor);
+}
+
 template <typename VisitorDispatcher>
 inline void TraceAfterDispatchExternBase::traceAfterDispatchImpl(
     VisitorDispatcher visitor) {
@@ -43,6 +59,11 @@ void TraceAfterDispatchExternDerived::traceAfterDispatch(Visitor* visitor) {
   traceAfterDispatchImpl(visitor);
 }
 
+void TraceAfterDispatchExternDerived::traceAfterDispatch(
+    InlinedGlobalMarkingVisitor visitor) {
+  traceAfterDispatchImpl(visitor);
+}
+
 template <typename VisitorDispatcher>
 inline void TraceAfterDispatchExternDerived::traceAfterDispatchImpl(
     VisitorDispatcher visitor) {

tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl.h

@@ -23,11 +23,18 @@ class TraceAfterDispatchInlinedBase
  public:
   explicit TraceAfterDispatchInlinedBase(ClassTag tag) : tag_(tag) {}
 
-  void trace(Visitor* visitor);
+  void trace(Visitor* visitor) { traceImpl(visitor); }
+  void trace(InlinedGlobalMarkingVisitor visitor) { traceImpl(visitor); }
 
   void traceAfterDispatch(Visitor* visitor) { traceAfterDispatchImpl(visitor); }
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor) {
+    traceAfterDispatchImpl(visitor);
+  }
 
  private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor);
+
   template <typename VisitorDispatcher>
   void traceAfterDispatchImpl(VisitorDispatcher visitor) {
     visitor->trace(x_base_);
@@ -42,6 +49,9 @@ class TraceAfterDispatchInlinedDerived : public TraceAfterDispatchInlinedBase {
   TraceAfterDispatchInlinedDerived() : TraceAfterDispatchInlinedBase(DERIVED) {}
 
   void traceAfterDispatch(Visitor* visitor) { traceAfterDispatchImpl(visitor); }
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor) {
+    traceAfterDispatchImpl(visitor);
+  }
 
  private:
   template <typename VisitorDispatcher>
@@ -59,10 +69,15 @@ class TraceAfterDispatchExternBase
   explicit TraceAfterDispatchExternBase(ClassTag tag) : tag_(tag) {}
 
   void trace(Visitor* visitor);
+  void trace(InlinedGlobalMarkingVisitor visitor);
 
   void traceAfterDispatch(Visitor* visitor);
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor);
 
  private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor);
+
   template <typename VisitorDispatcher>
   void traceAfterDispatchImpl(VisitorDispatcher visitor);
 
@@ -75,6 +90,7 @@ class TraceAfterDispatchExternDerived : public TraceAfterDispatchExternBase {
   TraceAfterDispatchExternDerived() : TraceAfterDispatchExternBase(DERIVED) {}
 
   void traceAfterDispatch(Visitor* visitor);
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor);
 
  private:
   template <typename VisitorDispatcher>

tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl_error.cpp

@@ -6,24 +6,37 @@
 
 namespace blink {
 
-void TraceAfterDispatchInlinedBase::trace(Visitor* visitor) {
-  // Implement a simple form of manual dispatching, because BlinkGCPlugin gets
-  // angry if dispatching statements are missing.
+template <typename VisitorDispatcher>
+inline void TraceAfterDispatchInlinedBase::traceImpl(
+    VisitorDispatcher visitor) {
+  // Implement a simple form of manual dispatching, because BlinkGCPlugin
+  // checks if the tracing is dispatched to all derived classes.
   //
   // This function has to be implemented out-of-line, since we need to know the
   // definition of derived classes here.
   if (tag_ == DERIVED) {
-    static_cast<TraceAfterDispatchInlinedDerived*>(this)->traceAfterDispatch(
-        visitor);
+    // Missing dispatch call:
+    // static_cast<TraceAfterDispatchInlinedDerived*>(this)->traceAfterDispatch(
+    //     visitor);
   } else {
     traceAfterDispatch(visitor);
   }
 }
 
 void TraceAfterDispatchExternBase::trace(Visitor* visitor) {
+  traceImpl(visitor);
+}
+
+void TraceAfterDispatchExternBase::trace(InlinedGlobalMarkingVisitor visitor) {
+  traceImpl(visitor);
+}
+
+template <typename VisitorDispatcher>
+inline void TraceAfterDispatchExternBase::traceImpl(VisitorDispatcher visitor) {
   if (tag_ == DERIVED) {
-    static_cast<TraceAfterDispatchExternDerived*>(this)->traceAfterDispatch(
-        visitor);
+    // Missing dispatch call:
+    // static_cast<TraceAfterDispatchExternDerived*>(this)->traceAfterDispatch(
+    //     visitor);
   } else {
     traceAfterDispatch(visitor);
   }
@@ -33,6 +46,11 @@ void TraceAfterDispatchExternBase::traceAfterDispatch(Visitor* visitor) {
   traceAfterDispatchImpl(visitor);
 }
 
+void TraceAfterDispatchExternBase::traceAfterDispatch(
+    InlinedGlobalMarkingVisitor visitor) {
+  traceAfterDispatchImpl(visitor);
+}
+
 template <typename VisitorDispatcher>
 inline void TraceAfterDispatchExternBase::traceAfterDispatchImpl(
     VisitorDispatcher visitor) {
@@ -43,6 +61,11 @@ void TraceAfterDispatchExternDerived::traceAfterDispatch(Visitor* visitor) {
   traceAfterDispatchImpl(visitor);
 }
 
+void TraceAfterDispatchExternDerived::traceAfterDispatch(
+    InlinedGlobalMarkingVisitor visitor) {
+  traceAfterDispatchImpl(visitor);
+}
+
 template <typename VisitorDispatcher>
 inline void TraceAfterDispatchExternDerived::traceAfterDispatchImpl(
     VisitorDispatcher visitor) {

tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl_error.h

@@ -23,11 +23,18 @@ class TraceAfterDispatchInlinedBase
  public:
   explicit TraceAfterDispatchInlinedBase(ClassTag tag) : tag_(tag) {}
 
-  void trace(Visitor* visitor);
+  void trace(Visitor* visitor) { traceImpl(visitor); }
+  void trace(InlinedGlobalMarkingVisitor visitor) { traceImpl(visitor); }
 
   void traceAfterDispatch(Visitor* visitor) { traceAfterDispatchImpl(visitor); }
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor) {
+    traceAfterDispatchImpl(visitor);
+  }
 
  private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor);
+
   template <typename VisitorDispatcher>
   void traceAfterDispatchImpl(VisitorDispatcher visitor) {
     // No trace call; should get a warning.
@@ -42,6 +49,9 @@ class TraceAfterDispatchInlinedDerived : public TraceAfterDispatchInlinedBase {
   TraceAfterDispatchInlinedDerived() : TraceAfterDispatchInlinedBase(DERIVED) {}
 
   void traceAfterDispatch(Visitor* visitor) { traceAfterDispatchImpl(visitor); }
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor) {
+    traceAfterDispatchImpl(visitor);
+  }
 
  private:
   template <typename VisitorDispatcher>
@@ -58,10 +68,15 @@ class TraceAfterDispatchExternBase
   explicit TraceAfterDispatchExternBase(ClassTag tag) : tag_(tag) {}
 
   void trace(Visitor* visitor);
+  void trace(InlinedGlobalMarkingVisitor visitor);
 
   void traceAfterDispatch(Visitor* visitor);
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor);
 
  private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor);
+
   template <typename VisitorDispatcher>
   void traceAfterDispatchImpl(VisitorDispatcher visitor);
 
@@ -74,6 +89,7 @@ class TraceAfterDispatchExternDerived : public TraceAfterDispatchExternBase {
   TraceAfterDispatchExternDerived() : TraceAfterDispatchExternBase(DERIVED) {}
 
   void traceAfterDispatch(Visitor* visitor);
+  void traceAfterDispatch(InlinedGlobalMarkingVisitor visitor);
 
  private:
   template <typename VisitorDispatcher>

tools/clang/blink_gc_plugin/tests/trace_after_dispatch_impl_error.txt

+trace_after_dispatch_impl_error.cpp:10:1: warning: [blink-gc] Missing dispatch to class 'TraceAfterDispatchInlinedDerived' in manual trace dispatch.
+inline void TraceAfterDispatchInlinedBase::traceImpl(
+^
+trace_after_dispatch_impl_error.cpp:35:1: warning: [blink-gc] Missing dispatch to class 'TraceAfterDispatchExternDerived' in manual trace dispatch.
+inline void TraceAfterDispatchExternBase::traceImpl(VisitorDispatcher visitor) {
+^
 In file included from trace_after_dispatch_impl_error.cpp:5:
-./trace_after_dispatch_impl_error.h:32:3: warning: [blink-gc] Class 'TraceAfterDispatchInlinedBase' has untraced fields that require tracing.
+./trace_after_dispatch_impl_error.h:39:3: warning: [blink-gc] Class 'TraceAfterDispatchInlinedBase' has untraced fields that require tracing.
   void traceAfterDispatchImpl(VisitorDispatcher visitor) {
   ^
-./trace_after_dispatch_impl_error.h:37:3: note: [blink-gc] Untraced field 'x_base_' declared here:
+./trace_after_dispatch_impl_error.h:44:3: note: [blink-gc] Untraced field 'x_base_' declared here:
   Member<X> x_base_;
   ^
-./trace_after_dispatch_impl_error.h:48:3: warning: [blink-gc] Base class 'TraceAfterDispatchInlinedBase' of derived class 'TraceAfterDispatchInlinedDerived' requires tracing.
+./trace_after_dispatch_impl_error.h:58:3: warning: [blink-gc] Base class 'TraceAfterDispatchInlinedBase' of derived class 'TraceAfterDispatchInlinedDerived' requires tracing.
   void traceAfterDispatchImpl(VisitorDispatcher visitor) {
   ^
-./trace_after_dispatch_impl_error.h:48:3: warning: [blink-gc] Class 'TraceAfterDispatchInlinedDerived' has untraced fields that require tracing.
-./trace_after_dispatch_impl_error.h:52:3: note: [blink-gc] Untraced field 'x_derived_' declared here:
+./trace_after_dispatch_impl_error.h:58:3: warning: [blink-gc] Class 'TraceAfterDispatchInlinedDerived' has untraced fields that require tracing.
+./trace_after_dispatch_impl_error.h:62:3: note: [blink-gc] Untraced field 'x_derived_' declared here:
   Member<X> x_derived_;
   ^
-trace_after_dispatch_impl_error.cpp:37:1: warning: [blink-gc] Class 'TraceAfterDispatchExternBase' has untraced fields that require tracing.
+trace_after_dispatch_impl_error.cpp:55:1: warning: [blink-gc] Class 'TraceAfterDispatchExternBase' has untraced fields that require tracing.
 inline void TraceAfterDispatchExternBase::traceAfterDispatchImpl(
 ^
-./trace_after_dispatch_impl_error.h:69:3: note: [blink-gc] Untraced field 'x_base_' declared here:
+./trace_after_dispatch_impl_error.h:84:3: note: [blink-gc] Untraced field 'x_base_' declared here:
   Member<X> x_base_;
   ^
-trace_after_dispatch_impl_error.cpp:47:1: warning: [blink-gc] Base class 'TraceAfterDispatchExternBase' of derived class 'TraceAfterDispatchExternDerived' requires tracing.
+trace_after_dispatch_impl_error.cpp:70:1: warning: [blink-gc] Base class 'TraceAfterDispatchExternBase' of derived class 'TraceAfterDispatchExternDerived' requires tracing.
 inline void TraceAfterDispatchExternDerived::traceAfterDispatchImpl(
 ^
-trace_after_dispatch_impl_error.cpp:47:1: warning: [blink-gc] Class 'TraceAfterDispatchExternDerived' has untraced fields that require tracing.
-./trace_after_dispatch_impl_error.h:82:3: note: [blink-gc] Untraced field 'x_derived_' declared here:
+trace_after_dispatch_impl_error.cpp:70:1: warning: [blink-gc] Class 'TraceAfterDispatchExternDerived' has untraced fields that require tracing.
+./trace_after_dispatch_impl_error.h:98:3: note: [blink-gc] Untraced field 'x_derived_' declared here:
   Member<X> x_derived_;
   ^
-6 warnings generated.
+8 warnings generated.