Blink GC plugin: Require that fields are actually traced by the visitor and identify tracing of dep…

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 289 matching lines @@
 };
 
 // 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) {}
 
-  // Allow recursive traversal by using VisitMemberExpr.
   bool VisitMemberExpr(MemberExpr* member) {
-    // If this member expression references a field decl, mark it as traced.
-    if (FieldDecl* field = dyn_cast<FieldDecl>(member->getMemberDecl())) {
-      if (IsTemplateInstantiation(info_->record())) {
-        // Pointer equality on fields does not work for template instantiations.
-        // The trace method refers to fields of the template definition which
-        // are different from the instantiated fields that need to be traced.
-        const string& name = field->getNameAsString();
+    // 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);
+    }
+    return true;
+  }
+
+  bool VisitCallExpr(CallExpr* call) {
+    // In weak callbacks we don't check calls (see VisitMemberExpr).
+    if (IsWeakCallback())
+      return true;
+
+    // A tracing call will have either a |visitor| or a |m_field| argument.
+    // A registerWeakMembers call will have a |this| argument.
+    if (call->getNumArgs() != 1)
+      return true;
+
+    Expr* callee = call->getCallee();
+    Expr* arg = call->getArg(0);
+
+    if (CXXDependentScopeMemberExpr* expr =
+        dyn_cast<CXXDependentScopeMemberExpr>(callee)) {
+      CheckCXXDependentScopeMemberExpr(expr);
+      return true;
+    }
+
+    if (UnresolvedMemberExpr* expr = dyn_cast<UnresolvedMemberExpr>(callee)) {
+      // 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.
+      if (expr->getMemberName().getAsString() == kRegisterWeakMembersName) {
         for (RecordInfo::Fields::iterator it = info_->GetFields().begin();
              it != info_->GetFields().end();
              ++it) {
-          if (it->first->getNameAsString() == name) {
-            MarkTraced(it);
-            break;
-          }
+          if (it->second.edge()->IsWeakMember())
+            it->second.MarkTraced();
         }
-      } else {
-        RecordInfo::Fields::iterator it = info_->GetFields().find(field);
-        if (it != info_->GetFields().end())
-          MarkTraced(it);
       }
+
+      QualType base = expr->getBaseType();
+      if (!base->isPointerType())
+        return true;
+      CXXRecordDecl* decl = base->getPointeeType()->getAsCXXRecordDecl();
+      if (decl)
+        CheckTraceFieldCall(expr->getMemberName().getAsString(), decl, arg);
       return true;
     }
 
-    // If this is a weak callback function we only check field tracing.
+    if (CXXMemberCallExpr* expr = dyn_cast<CXXMemberCallExpr>(call)) {
+      if (CheckTraceFieldCall(expr) || CheckRegisterWeakMembers(expr))
+        return true;
+    }
+
+    CheckTraceBaseCall(call);
+    return true;
+  }
+
+ private:
+
+  // Trace calls from a templated derived class to its templated super class
+  // result in a DependentScopeMemberExpr because the concrete class depends on
+  // the instantiation of any shared template parameters. In this case we
+  // compare the type names.
+  void CheckCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr* member) {
     if (IsWeakCallback())
-      return true;
+      return;
 
-    // For method calls, check tracing of bases and other special GC methods.
-    if (CXXMethodDecl* fn = dyn_cast<CXXMethodDecl>(member->getMemberDecl())) {
-      const string& name = fn->getNameAsString();
-      // Check weak callbacks.
-      if (name == kRegisterWeakMembersName) {
-        if (fn->isTemplateInstantiation()) {
-          const TemplateArgumentList& args =
-              *fn->getTemplateSpecializationInfo()->TemplateArguments;
-          // The second template argument is the callback method.
-          if (args.size() > 1 &&
-              args[1].getKind() == TemplateArgument::Declaration) {
-            if (FunctionDecl* callback =
-                    dyn_cast<FunctionDecl>(args[1].getAsDecl())) {
-              if (callback->hasBody()) {
-                CheckTraceVisitor nested_visitor(info_);
-                nested_visitor.TraverseStmt(callback->getBody());
-              }
-            }
-          }
-        }
-        return true;
-      }
+    // It is unknown if the member resolves to a method so we string compare.

[haraken, 2014/07/31 15:37:11]

we string compare => we compare strings

+    if (member->getMember().getAsString() != trace_->getName())
+      return;
 
-      // 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 (Config::IsTraceMethod(fn) &&
-          fn->getName() == trace_->getName() &&
-          member->hasQualifier()) {
-        if (const Type* type = member->getQualifier()->getAsType()) {
-          if (CXXRecordDecl* decl = type->getAsCXXRecordDecl()) {
-            RecordInfo::Bases::iterator it = info_->GetBases().find(decl);
-            if (it != info_->GetBases().end())
-              it->second.MarkTraced();
+    NestedNameSpecifier* qual = member->getQualifier();
+    if (!qual)
+      return;
+
+    const Type* type = qual->getAsType();
+    if (!type)
+      return;
+
+    const TemplateSpecializationType* tmpl_type =
+        type->getAs<TemplateSpecializationType>();
+    if (!tmpl_type)
+      return;
+
+    TemplateDecl* tmpl_decl = tmpl_type->getTemplateName().getAsTemplateDecl();
+    if (!tmpl_decl)
+      return;
+
+    CXXRecordDecl* rec = dyn_cast<CXXRecordDecl>(tmpl_decl->getTemplatedDecl());
+    if (!rec)
+      return;
+
+    RecordInfo::Bases::iterator it;
+    for (it = info_->GetBases().begin(); it != info_->GetBases().end(); ++it) {
+      if (it->first->getName() == rec->getName())
+        it->second.MarkTraced();
+    }
+  }
+
+  bool CheckTraceBaseCall(CallExpr* call) {
+    MemberExpr* callee = dyn_cast<MemberExpr>(call->getCallee());
+    if (!callee)
+      return false;
+
+    FunctionDecl* fn = dyn_cast<FunctionDecl>(callee->getMemberDecl());
+    if (!fn || !Config::IsTraceMethod(fn))
+      return false;
+
+    // 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())
+      return false;
+
+    CXXRecordDecl* decl = 0;
+    if (callee && callee->hasQualifier()) {
+      if (const Type* type = callee->getQualifier()->getAsType())
+        decl = type->getAsCXXRecordDecl();
+    }
+    if (!decl)
+      return false;
+
+    RecordInfo::Bases::iterator it = info_->GetBases().find(decl);
+    if (it != info_->GetBases().end()) {
+      it->second.MarkTraced();
+    }
+
+    return true;
+  }
+
+  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()))
+      return false;
+
+    FindFieldVisitor finder;
+    finder.TraverseStmt(arg);
+    if (finder.field())
+      FoundField(finder.field());
+
+    return true;
+  }
+
+  bool CheckRegisterWeakMembers(CXXMemberCallExpr* call) {
+    CXXMethodDecl* fn = call->getMethodDecl();
+    if (fn->getName() != kRegisterWeakMembersName)
+      return false;
+
+    if (fn->isTemplateInstantiation()) {
+      const TemplateArgumentList& args =
+          *fn->getTemplateSpecializationInfo()->TemplateArguments;
+      // The second template argument is the callback method.
+      if (args.size() > 1 &&
+          args[1].getKind() == TemplateArgument::Declaration) {
+        if (FunctionDecl* callback =
+            dyn_cast<FunctionDecl>(args[1].getAsDecl())) {
+          if (callback->hasBody()) {
+            CheckTraceVisitor nested_visitor(info_);
+            nested_visitor.TraverseStmt(callback->getBody());
           }
         }
       }
     }
     return true;
   }
 
- private:
+  class FindFieldVisitor : public RecursiveASTVisitor<FindFieldVisitor> {
+   public:
+    FindFieldVisitor() : member_(0), field_(0) {}
+    MemberExpr* member() const { return member_; }
+    FieldDecl* field() const { return field_; }
+    bool TraverseMemberExpr(MemberExpr* member) {
+      if (FieldDecl* field = dyn_cast<FieldDecl>(member->getMemberDecl())) {
+        member_ = member;
+        field_ = field;
+        return false;
+      }
+      return true;
+    }
+   private:
+    MemberExpr* member_;
+    FieldDecl* field_;
+  };
+
   // Nested checking for weak callbacks.
   CheckTraceVisitor(RecordInfo* info) : trace_(0), info_(info) {}
 
   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();
   }
 
+  void FoundField(FieldDecl* field) {
+    if (IsTemplateInstantiation(info_->record())) {
+      // Pointer equality on fields does not work for template instantiations.
+      // The trace method refers to fields of the template definition which
+      // are different from the instantiated fields that need to be traced.
+      const string& name = field->getNameAsString();
+      for (RecordInfo::Fields::iterator it = info_->GetFields().begin();
+           it != info_->GetFields().end();
+           ++it) {
+        if (it->first->getNameAsString() == name) {
+          MarkTraced(it);
+          break;
+        }
+      }
+    } else {
+      RecordInfo::Fields::iterator it = info_->GetFields().find(field);
+      if (it != info_->GetFields().end())
+        MarkTraced(it);
+    }
+  }
+
   CXXMethodDecl* trace_;
   RecordInfo* info_;
 };
 
 // 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::vector<RootPath> Errors;
@@ skipping 163 matching lines @@
 // garbage collection infrastructure.
 class BlinkGCPluginConsumer : public ASTConsumer {
  public:
   BlinkGCPluginConsumer(CompilerInstance& instance,
                         const BlinkGCPluginOptions& options)
       : instance_(instance),
         diagnostic_(instance.getDiagnostics()),
         options_(options),
         json_(0) {
 
-    // Only check structures in the blink, blink and WebKit namespaces.
-    options_.checked_namespaces.insert("blink");
+    // Only check structures in the blink and WebKit namespaces.
     options_.checked_namespaces.insert("blink");
     options_.checked_namespaces.insert("WebKit");
 
     // Ignore GC implementation files.
     options_.ignored_directories.push_back("/heap/");
 
     // Register warning/error messages.
     diag_class_must_left_mostly_derive_gc_ = diagnostic_.getCustomDiagID(
         getErrorLevel(), kClassMustLeftMostlyDeriveGC);
     diag_class_requires_trace_method_ =
@@ skipping 888 matching lines @@
 
  private:
   BlinkGCPluginOptions options_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<BlinkGCPluginAction> X(
     "blink-gc-plugin",
     "Check Blink GC invariants");