BlinkGCPlugin: Support overloaded trace() call from template.

With the new setup with InlinedGlobalTracingVisitor, trace() functions are
overloaded and may be called from a traceImpl() function in derived classes.
As traceImpl() is a function template, the trace() overload cannot be resolved
at the time of plugin execution and thus the callee of the trace() call becomes
UnresolvedMemberExpr*.

This patch adds support for detecting Base::trace() call if the
"Base::trace" part is UnresolvedMemberExpr. Specifically, the new code finds
a function looking like trace() from the candidates of UnresolvedMemberExpr,
and if there's one, we treat it as a valid Base::trace() call. The call
needs to give exactly one argument named "visitor" to the function.

A few tests are added to test these cases. A stub for InlinedGlobalTracing-
Visitor is added to tests/heap/stubs.h so it can be used from tests.

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

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

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

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -365,6 +365,11 @@ class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
     Expr* arg = call->getArg(0);
 
     if (UnresolvedMemberExpr* expr = dyn_cast<UnresolvedMemberExpr>(callee)) {
+      // This could be a trace call of a base class, as explained in the
+      // comments of CheckTraceBaseCall().
+      if (CheckTraceBaseCall(call))
+        return true;
+
       // If we find a call to registerWeakMembers which is unresolved we
       // unsoundly consider all weak members as traced.
       // TODO: Find out how to validate weak member tracing for unresolved call.
@@ -403,7 +408,6 @@ class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
   }
 
  private:
-
   CXXRecordDecl* GetDependentTemplatedDecl(CXXDependentScopeMemberExpr* expr) {
     NestedNameSpecifier* qual = expr->getQualifier();
     if (!qual)
@@ -461,42 +465,87 @@ class CheckTraceVisitor : public RecursiveASTVisitor<CheckTraceVisitor> {
   }
 
   bool CheckTraceBaseCall(CallExpr* call) {
-    MemberExpr* callee = dyn_cast<MemberExpr>(call->getCallee());
-    if (!callee)
-      return false;
+    // Checks for "Base::trace(visitor)"-like calls.
+
+    // Checking code for these two variables is shared among MemberExpr* case
+    // and UnresolvedMemberCase* case below.
+    //
+    // For example, if we've got "Base::trace(visitor)" as |call|,
+    // callee_record will be "Base", and func_name will be "trace".
+    CXXRecordDecl* callee_record = nullptr;
+    std::string func_name;
+
+    if (MemberExpr* callee = dyn_cast<MemberExpr>(call->getCallee())) {
+      if (!callee->hasQualifier())
+        return false;
 
-    FunctionDecl* fn = dyn_cast<FunctionDecl>(callee->getMemberDecl());
-    if (!fn || !Config::IsTraceMethod(fn))
-      return false;
+      FunctionDecl* trace_decl =
+          dyn_cast<FunctionDecl>(callee->getMemberDecl());
+      if (!trace_decl || !Config::IsTraceMethod(trace_decl))
+        return false;
+
+      const Type* type = callee->getQualifier()->getAsType();
+      if (!type)
+        return false;
+
+      callee_record = type->getAsCXXRecordDecl();
+      func_name = trace_decl->getName();
+    } else if (UnresolvedMemberExpr* callee =
+               dyn_cast<UnresolvedMemberExpr>(call->getCallee())) {
+      // Callee part may become unresolved if the type of the argument
+      // ("visitor") is a template parameter and the called function is
+      // overloaded (i.e. trace(Visitor*) and
+      // trace(InlinedGlobalMarkingVisitor)).
+      //
+      // Here, we try to find a function that looks like trace() from the
+      // candidate overloaded functions, and if we find one, we assume it is
+      // called here.
+
+      CXXMethodDecl* trace_decl = nullptr;
+      for (NamedDecl* named_decl : callee->decls()) {
+        if (CXXMethodDecl* method_decl = dyn_cast<CXXMethodDecl>(named_decl)) {
+          if (Config::IsTraceMethod(method_decl)) {
+            trace_decl = method_decl;
+            break;
+          }
+        }
+      }
+      if (!trace_decl)
+        return false;
+
+      // Check if the passed argument is named "visitor".
+      if (call->getNumArgs() != 1)
+        return false;
+      DeclRefExpr* arg = dyn_cast<DeclRefExpr>(call->getArg(0));
+      if (!arg || arg->getNameInfo().getAsString() != kVisitorVarName)
+        return false;
+
+      callee_record = trace_decl->getParent();
+      func_name = callee->getMemberName().getAsString();
+    }
 
     if (trace_->getName() == kTraceImplName) {
-      if (fn->getName() != kTraceName)
+      if (func_name != kTraceName)
         return false;
     } else if (trace_->getName() == kTraceAfterDispatchImplName) {
-      if (fn->getName() != kTraceAfterDispatchName)
+      if (func_name != kTraceAfterDispatchName)
         return false;
     } else {
       // Currently, a manually dispatched class cannot have mixin bases (having
       // one would add a vtable which we explicitly check against). This means
       // that we can only make calls to a trace method of the same name. Revisit
       // this if our mixin/vtable assumption changes.
-      if (fn->getName() != trace_->getName())
+      if (func_name != trace_->getName())
         return false;
     }
 
-    CXXRecordDecl* decl = 0;
-    if (callee && callee->hasQualifier()) {
-      if (const Type* type = callee->getQualifier()->getAsType())
-        decl = type->getAsCXXRecordDecl();
-    }
-    if (!decl)
+    if (!callee_record)
+      return false;
+    RecordInfo::Bases::iterator iter = info_->GetBases().find(callee_record);
+    if (iter == info_->GetBases().end())
       return false;
 
-    RecordInfo::Bases::iterator it = info_->GetBases().find(decl);
-    if (it != info_->GetBases().end()) {
-      it->second.MarkTraced();
-    }
-
+    iter->second.MarkTraced();
     return true;
   }
 

tools/clang/blink_gc_plugin/Config.h

@@ -28,6 +28,7 @@ const char kRegisterWeakMembersName[] = "registerWeakMembers";
 const char kHeapAllocatorName[] = "HeapAllocator";
 const char kTraceIfNeededName[] = "TraceIfNeeded";
 const char kVisitorDispatcherName[] = "VisitorDispatcher";
+const char kVisitorVarName[] = "visitor";
 const char kAdjustAndMarkName[] = "adjustAndMark";
 const char kIsHeapObjectAliveName[] = "isHeapObjectAlive";
 

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

@@ -212,11 +212,17 @@ public:
 };
 
 class Visitor : public VisitorHelper<Visitor> {
- public:
+public:
     template<typename T, void (T::*method)(Visitor*)>
     void registerWeakMembers(const T* obj);
 };
 
+class InlinedGlobalMarkingVisitor
+    : public VisitorHelper<InlinedGlobalMarkingVisitor> {
+public:
+     InlinedGlobalMarkingVisitor* operator->() { return this; }
+};
+
 class GarbageCollectedMixin {
 public:
     virtual void adjustAndMark(Visitor*) const = 0;

tools/clang/blink_gc_plugin/tests/traceimpl_overloaded.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_overloaded.h"
+
+namespace blink {
+
+void ExternBase::trace(Visitor* visitor) {
+  traceImpl(visitor);
+}
+
+void ExternBase::trace(InlinedGlobalMarkingVisitor visitor) {
+  traceImpl(visitor);
+}
+
+template <typename VisitorDispatcher>
+inline void ExternBase::traceImpl(VisitorDispatcher visitor) {
+  visitor->trace(x_base_);
+}
+
+void ExternDerived::trace(Visitor* visitor) {
+  traceImpl(visitor);
+}
+
+void ExternDerived::trace(InlinedGlobalMarkingVisitor visitor) {
+  traceImpl(visitor);
+}
+
+template <typename VisitorDispatcher>
+inline void ExternDerived::traceImpl(VisitorDispatcher visitor) {
+  visitor->trace(x_derived_);
+  ExternBase::trace(visitor);
+}
+
+}

tools/clang/blink_gc_plugin/tests/traceimpl_overloaded.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_OVERLOADED_H_
+#define TRACEIMPL_OVERLOADED_H_
+
+#include "heap/stubs.h"
+
+namespace blink {
+
+class X : public GarbageCollected<X> {
+ public:
+  void trace(Visitor*) {}
+  void trace(InlinedGlobalMarkingVisitor) {}
+};
+
+class InlinedBase : public GarbageCollected<InlinedBase> {
+ public:
+  virtual void trace(Visitor* visitor) { traceImpl(visitor); }
+  virtual void trace(InlinedGlobalMarkingVisitor visitor) {
+    traceImpl(visitor);
+  }
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor) { visitor->trace(x_base_); }
+
+  Member<X> x_base_;
+};
+
+class InlinedDerived : public InlinedBase {
+ public:
+  void trace(Visitor* visitor) override { traceImpl(visitor); }
+  void trace(InlinedGlobalMarkingVisitor visitor) override {
+    traceImpl(visitor);
+  }
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor) {
+    visitor->trace(x_derived_);
+    InlinedBase::trace(visitor);
+  }
+
+  Member<X> x_derived_;
+};
+
+class ExternBase : public GarbageCollected<ExternBase> {
+ public:
+  virtual void trace(Visitor*);
+  virtual void trace(InlinedGlobalMarkingVisitor);
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher);
+
+  Member<X> x_base_;
+};
+
+class ExternDerived : public ExternBase {
+ public:
+  void trace(Visitor*) override;
+  void trace(InlinedGlobalMarkingVisitor) override;
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher);
+
+  Member<X> x_derived_;
+};
+
+}
+
+#endif  // TRACEIMPL_OVERLOADED_H_

tools/clang/blink_gc_plugin/tests/traceimpl_overloaded.txt

+

tools/clang/blink_gc_plugin/tests/traceimpl_overloaded_error.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_overloaded_error.h"
+
+namespace blink {
+
+void ExternBase::trace(Visitor* visitor) {
+  traceImpl(visitor);
+}
+
+void ExternBase::trace(InlinedGlobalMarkingVisitor visitor) {
+  traceImpl(visitor);
+}
+
+template <typename VisitorDispatcher>
+inline void ExternBase::traceImpl(VisitorDispatcher visitor) {
+  // Missing visitor->trace(x_base_).
+}
+
+void ExternDerived::trace(Visitor* visitor) {
+  traceImpl(visitor);
+}
+
+void ExternDerived::trace(InlinedGlobalMarkingVisitor visitor) {
+  traceImpl(visitor);
+}
+
+template <typename VisitorDispatcher>
+inline void ExternDerived::traceImpl(VisitorDispatcher visitor) {
+  // Missing visitor->trace(x_derived_) and ExternBase::trace(visitor).
+}
+
+}

tools/clang/blink_gc_plugin/tests/traceimpl_overloaded_error.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_OVERLOADED_ERROR_H_
+#define TRACEIMPL_OVERLOADED_ERROR_H_
+
+#include "heap/stubs.h"
+
+namespace blink {
+
+class X : public GarbageCollected<X> {
+ public:
+  void trace(Visitor*) {}
+  void trace(InlinedGlobalMarkingVisitor) {}
+};
+
+class InlinedBase : public GarbageCollected<InlinedBase> {
+ public:
+  virtual void trace(Visitor* visitor) { traceImpl(visitor); }
+  virtual void trace(InlinedGlobalMarkingVisitor visitor) {
+    traceImpl(visitor);
+  }
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor) {
+    // Missing visitor->trace(x_base_).
+  }
+
+  Member<X> x_base_;
+};
+
+class InlinedDerived : public InlinedBase {
+ public:
+  void trace(Visitor* visitor) override { traceImpl(visitor); }
+  void trace(InlinedGlobalMarkingVisitor visitor) override {
+    traceImpl(visitor);
+  }
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher visitor) {
+    // Missing visitor->trace(x_derived_) and InlinedBase::trace(visitor).
+  }
+
+  Member<X> x_derived_;
+};
+
+class ExternBase : public GarbageCollected<ExternBase> {
+ public:
+  virtual void trace(Visitor*);
+  virtual void trace(InlinedGlobalMarkingVisitor);
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher);
+
+  Member<X> x_base_;
+};
+
+class ExternDerived : public ExternBase {
+ public:
+  void trace(Visitor*) override;
+  void trace(InlinedGlobalMarkingVisitor) override;
+
+ private:
+  template <typename VisitorDispatcher>
+  void traceImpl(VisitorDispatcher);
+
+  Member<X> x_derived_;
+};
+
+}
+
+#endif  // TRACEIMPL_OVERLOADED_ERROR_H_

tools/clang/blink_gc_plugin/tests/traceimpl_overloaded_error.txt

+In file included from traceimpl_overloaded_error.cpp:5:
+./traceimpl_overloaded_error.h:27:3: warning: [blink-gc] Class 'InlinedBase' has untraced fields that require tracing.
+  void traceImpl(VisitorDispatcher visitor) {
+  ^
+./traceimpl_overloaded_error.h:31:3: note: [blink-gc] Untraced field 'x_base_' declared here:
+  Member<X> x_base_;
+  ^
+./traceimpl_overloaded_error.h:43:3: warning: [blink-gc] Base class 'InlinedBase' of derived class 'InlinedDerived' requires tracing.
+  void traceImpl(VisitorDispatcher visitor) {
+  ^
+./traceimpl_overloaded_error.h:43:3: warning: [blink-gc] Class 'InlinedDerived' has untraced fields that require tracing.
+./traceimpl_overloaded_error.h:47:3: note: [blink-gc] Untraced field 'x_derived_' declared here:
+  Member<X> x_derived_;
+  ^
+traceimpl_overloaded_error.cpp:18:1: warning: [blink-gc] Class 'ExternBase' has untraced fields that require tracing.
+inline void ExternBase::traceImpl(VisitorDispatcher visitor) {
+^
+./traceimpl_overloaded_error.h:59:3: note: [blink-gc] Untraced field 'x_base_' declared here:
+  Member<X> x_base_;
+  ^
+traceimpl_overloaded_error.cpp:31:1: warning: [blink-gc] Base class 'ExternBase' of derived class 'ExternDerived' requires tracing.
+inline void ExternDerived::traceImpl(VisitorDispatcher visitor) {
+^
+traceimpl_overloaded_error.cpp:31:1: warning: [blink-gc] Class 'ExternDerived' has untraced fields that require tracing.
+./traceimpl_overloaded_error.h:71:3: note: [blink-gc] Untraced field 'x_derived_' declared here:
+  Member<X> x_derived_;
+  ^
+6 warnings generated.