BlinkGCPlugin: Handle omitted trace() in intermediary classes.

tools/clang/blink_gc_plugin/BlinkGCPlugin.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.
 
 // This clang plugin checks various invariants of the Blink garbage
 // collection infrastructure.
 //
 // Errors are described at:
 // http://www.chromium.org/developers/blink-gc-plugin-errors
 
@@ skipping 307 matching lines @@
  private:
   RecordInfo* receiver_;
   bool dispatched_to_receiver_;
 };
 
 // This visitor checks a tracing method by traversing its body.
 // - A member field is considered traced if it is referenced in the body.
 // - 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_; }
 
   bool VisitMemberExpr(MemberExpr* member) {
     // In weak callbacks, consider any occurrence as a correct usage.
     // TODO: We really want to require that isAlive is checked on manually
     // processed weak fields.
     if (IsWeakCallback()) {
       if (FieldDecl* field = dyn_cast<FieldDecl>(member->getMemberDecl()))
         FoundField(field);
@@ skipping 194 matching lines @@
       callee_record = trace_decl->getParent();
       func_name = callee->getMemberName().getAsString();
     }
 
     if (!callee_record)
       return false;
 
     if (!IsTraceCallName(func_name))
       return false;
 
-    RecordInfo::Bases::iterator iter = info_->GetBases().find(callee_record);
-    if (iter == info_->GetBases().end())
-      return false;
+    for (RecordInfo::Bases::iterator iter = info_->GetBases().begin();
+         iter != info_->GetBases().end(); ++iter) {

[kouhei (in TOK), 2015/03/02 04:24:13]

Nit: use foreach?

[Yuta Kitamura, 2015/03/02 07:36:49]

Done.

+      // 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.
 
-    iter->second.MarkTraced();
-    return true;
+      std::vector<CXXRecordDecl*> base_records;
+      base_records.push_back(iter->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.
+          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 (RecordInfo::Bases::iterator inner_iter =
+                     base_info->GetBases().begin();
+                 inner_iter != base_info->GetBases().end(); ++inner_iter) {
+              base_records.push_back(inner_iter->first);
+            }
+          }
+        }
+      }
+    }
+
+    return false;
   }
 
   bool CheckTraceFieldCall(CXXMemberCallExpr* call) {
     return CheckTraceFieldCall(call->getMethodDecl()->getNameAsString(),
                                call->getRecordDecl(),
                                call->getArg(0));
   }
 
   bool CheckTraceFieldCall(string name, CXXRecordDecl* callee, Expr* arg) {
     if (name != kTraceName || !Config::IsVisitor(callee->getName()))
@@ skipping 42 matching lines @@
         return false;
       }
       return true;
     }
    private:
     MemberExpr* member_;
     FieldDecl* field_;
   };
 
   // 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_; }
 
   void MarkTraced(RecordInfo::Fields::iterator it) {
     // In a weak callback we can't mark strong fields as traced.
     if (IsWeakCallback() && !it->second.edge()->IsWeakMember())
       return;
     it->second.MarkTraced();
   }
 
@@ skipping 13 matching lines @@
       }
     } else {
       RecordInfo::Fields::iterator it = info_->GetFields().find(field);
       if (it != info_->GetFields().end())
         MarkTraced(it);
     }
   }
 
   CXXMethodDecl* trace_;
   RecordInfo* info_;
+  RecordCache* cache_;
   bool delegates_to_traceimpl_;
 };
 
 // This visitor checks that the fields of a class and the fields of
 // its part objects don't define GC roots.
 class CheckGCRootsVisitor : public RecursiveEdgeVisitor {
  public:
   typedef std::vector<FieldPoint*> RootPath;
   typedef std::set<RecordInfo*> VisitingSet;
   typedef std::vector<RootPath> Errors;
@@ skipping 724 matching lines @@
     if (trace_type == Config::TRACE_METHOD) {
       for (RecordInfo::Bases::iterator it = parent->GetBases().begin();
            it != parent->GetBases().end();
            ++it) {
         RecordInfo* base = it->second.info();
         if (CXXMethodDecl* other = base->InheritsNonVirtualTrace())
           ReportOverriddenNonVirtualTrace(parent, trace, other);
       }
     }
 
-    CheckTraceVisitor visitor(trace, parent);
+    CheckTraceVisitor visitor(trace, parent, &cache_);
     visitor.TraverseCXXMethodDecl(trace);
 
     // Skip reporting if this trace method is a just delegate to
     // traceImpl (or traceAfterDispatchImpl) method. We will report on
     // CheckTraceMethod on traceImpl method.
     if (visitor.delegates_to_traceimpl())
       return;
 
     for (RecordInfo::Bases::iterator it = parent->GetBases().begin();
          it != parent->GetBases().end();
@@ skipping 589 matching lines @@
 
  private:
   BlinkGCPluginOptions options_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<BlinkGCPluginAction> X(
     "blink-gc-plugin",
     "Check Blink GC invariants");