Check consistency of manual trace and finalization dispatching.

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 44 matching lines @@
 
 const char kOwnPtrToGCManagedClassNote[] =
     "[blink-gc] OwnPtr field %0 to a GC managed class declared here:";
 
 const char kPartObjectContainsGCRoot[] =
     "[blink-gc] Field %0 with embedded GC root in %1 declared here:";
 
 const char kFieldContainsGCRoot[] =
     "[blink-gc] Field %0 defining a GC root declared here:";
 
+const char kOverriddenNonVirtualTrace[] =
+    "[blink-gc] Class %0 overrides non-virtual trace of base class %1.";
+
+const char kOverriddenNonVirtualTraceNote[] =
+    "[blink-gc] Non-virtual trace method declared here:";
+
+const char kVirtualTraceAndManualDispatch[] =
+    "[blink-gc] Class %0 declares a virtual trace"
+    " but implements manual dispatching.";
+
+const char kVirtualAndManualDispatch[] =
+    "[blink-gc] Class %0 contains or inherits virtual methods"
+    " but implements manual dispatching.";
+
+const char kMissingTraceDispatch[] =
+    "[blink-gc] Missing dispatch to class %0 in manual trace dispatch.";
+
+const char kMissingFinalize[] =
+    "[blink-gc] Class %0 is missing manual finalize dispatch.";
+
+const char kMissingFinalizeDispatch[] =
+    "[blink-gc] Missing dispatch to class %0 in manual finalize dispatch.";
+
 const char kFinalizedFieldNote[] =
     "[blink-gc] Potentially finalized field %0 declared here:";
 
 const char kUserDeclaredDestructorNote[] =
     "[blink-gc] User-declared destructor declared here:";
 
 const char kBaseRequiresFinalizationNote[] =
     "[blink-gc] Base class %0 requiring finalization declared here:";
 
 const char kFieldRequiresFinalizationNote[] =
@@ skipping 94 matching lines @@
       : blacklist_context_(false), cache_(cache) {}
 
   Errors& finalized_fields() { return finalized_fields_; }
 
   bool WalkUpFromCXXOperatorCallExpr(CXXOperatorCallExpr* expr) {
     // Only continue the walk-up if the operator is a blacklisted one.
     switch (expr->getOperator()) {
       case OO_Arrow:
       case OO_Subscript:
         this->WalkUpFromCallExpr(expr);
+      default:
+        return true;
     }
-    return true;
   }
 
   // We consider all non-operator calls to be blacklisted contexts.
   bool WalkUpFromCallExpr(CallExpr* expr) {
     bool prev_blacklist_context = blacklist_context_;
     blacklist_context_ = true;
     for (size_t i = 0; i < expr->getNumArgs(); ++i)
       this->TraverseStmt(expr->getArg(i));
     blacklist_context_ = prev_blacklist_context;
     return true;
@@ skipping 22 matching lines @@
     point->edge()->Accept(&visitor);
     return visitor.might_be_collected();
   }
 
  private:
   bool blacklist_context_;
   Errors finalized_fields_;
   RecordCache* cache_;
 };
 
+// This visitor checks that a method contains within its body, a call to a
+// method on the provided receiver class. This is used to check manual
+// dispatching for trace and finalize methods.
+class CheckDispatchVisitor : public RecursiveASTVisitor<CheckDispatchVisitor> {
+ public:
+  CheckDispatchVisitor(RecordInfo* receiver)
+      : receiver_(receiver), dispatched_to_receiver_(false) { }
+
+  bool dispatched_to_receiver() { return dispatched_to_receiver_; }
+
+  bool VisitMemberExpr(MemberExpr* member) {
+    if (CXXMethodDecl* fn = dyn_cast<CXXMethodDecl>(member->getMemberDecl())) {
+      if (fn->getParent() == receiver_->record())
+        dispatched_to_receiver_ = true;
+    }
+    return true;
+  }
+
+ 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) {}
 
   // Allow recursive traversal by using VisitMemberExpr.
   bool VisitMemberExpr(MemberExpr* member) {
@@ skipping 206 matching lines @@
         diagnostic_.getCustomDiagID(getErrorLevel(), kFieldsRequireTracing);
     diag_class_contains_invalid_fields_ =
         diagnostic_.getCustomDiagID(getErrorLevel(),
                                     kClassContainsInvalidFields);
     diag_class_contains_gc_root_ =
         diagnostic_.getCustomDiagID(getErrorLevel(), kClassContainsGCRoot);
     diag_class_requires_finalization_ = diagnostic_.getCustomDiagID(
         getErrorLevel(), kClassRequiresFinalization);
     diag_finalizer_accesses_finalized_field_ = diagnostic_.getCustomDiagID(
         getErrorLevel(), kFinalizerAccessesFinalizedField);
+    diag_overridden_non_virtual_trace_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kOverriddenNonVirtualTrace);
+    diag_virtual_and_manual_dispatch_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kVirtualAndManualDispatch);
+    diag_missing_trace_dispatch_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kMissingTraceDispatch);
+    diag_missing_finalize_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kMissingFinalize);
+    diag_missing_finalize_dispatch_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kMissingFinalizeDispatch);
 
     // Register note messages.
     diag_field_requires_tracing_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kFieldRequiresTracingNote);
     diag_raw_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kRawPtrToGCManagedClassNote);
     diag_ref_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kRefPtrToGCManagedClassNote);
     diag_own_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kOwnPtrToGCManagedClassNote);
     diag_part_object_contains_gc_root_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kPartObjectContainsGCRoot);
     diag_field_contains_gc_root_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kFieldContainsGCRoot);
     diag_finalized_field_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kFinalizedFieldNote);
     diag_user_declared_destructor_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kUserDeclaredDestructorNote);
     diag_base_requires_finalization_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kBaseRequiresFinalizationNote);
     diag_field_requires_finalization_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kFieldRequiresFinalizationNote);
+    diag_overridden_non_virtual_trace_note_ = diagnostic_.getCustomDiagID(
+        DiagnosticsEngine::Note, kOverriddenNonVirtualTraceNote);
   }
 
   virtual void HandleTranslationUnit(ASTContext& context) {
     CollectVisitor visitor;
     visitor.TraverseDecl(context.getTranslationUnitDecl());
 
     for (RecordVector::iterator it = visitor.record_decls().begin();
          it != visitor.record_decls().end();
          ++it) {
       CheckRecord(cache_.Lookup(*it));
@@ skipping 34 matching lines @@
 
   // Check a class-like object (eg, class, specialization, instantiation).
   void CheckClass(RecordInfo* info) {
     // Don't enforce tracing of stack allocated objects.
     if (!info || info->IsStackAllocated())
       return;
 
     if (info->RequiresTraceMethod() && !info->GetTraceMethod())
       ReportClassRequiresTraceMethod(info);
 
+    if (CXXMethodDecl* dispatch = info->GetTraceDispatchMethod())
+      CheckDispatch(info, dispatch);
+
     {
       CheckFieldsVisitor visitor(options_);
       if (visitor.ContainsInvalidFields(info))
         ReportClassContainsInvalidFields(info, &visitor.invalid_fields());
     }
 
     if (info->IsGCDerived()) {
       CheckGCRootsVisitor visitor;
       if (visitor.ContainsGCRoots(info))
         ReportClassContainsGCRoots(info, &visitor.gc_roots());
 
       if (info->NeedsFinalization())
         CheckFinalization(info);
     }
   }
 
+  void CheckDispatch(RecordInfo* info, CXXMethodDecl* dispatch) {
+    if (info->record()->isPolymorphic())
+      ReportVirtualAndManualDispatch(info);
+
+    CXXRecordDecl* base = dispatch->getParent();
+    // If this class is finalized get its finalize dispatch method.
+    bool is_finalized = info->IsGCFinalized();
+    CXXMethodDecl* finalize = 0;
+    if (is_finalized) {
+      for (CXXRecordDecl::method_iterator it = base->method_begin();
+           it != base->method_end();
+           ++it) {
+        if (it->getNameAsString() == kFinalizeName) {
+          finalize = *it;
+          break;
+        }
+      }
+    }
+
+    // Check that the dispatching class implements finalize if needed.
+    if (is_finalized && !finalize && base == info->record())
+      ReportMissingFinalize(info);
+
+    // If this is a non-abstract class check that it is dispatched to.
+    // TODO: Create a global variant of this local check. We can only check if
+    // the dispatch body is known in this compilation unit.
+    if (info->IsConsideredAbstract())
+      return;
+
+    const FunctionDecl* defn;
+
+    if (dispatch->isDefined(defn)) {
+      CheckDispatchVisitor visitor(info);
+      visitor.TraverseStmt(defn->getBody());
+      if (!visitor.dispatched_to_receiver())
+        ReportMissingTraceDispatch(defn, info);
+    }
+
+    if (is_finalized && finalize && finalize->isDefined(defn)) {
+      CheckDispatchVisitor visitor(info);
+      visitor.TraverseStmt(defn->getBody());
+      if (!visitor.dispatched_to_receiver())
+        ReportMissingFinalizeDispatch(defn, info);
+    }
+  }
+
   void CheckFinalization(RecordInfo* info) {
     // TODO: Should we collect destructors similar to trace methods?
     // TODO: Check overridden finalize().
     CXXDestructorDecl* dtor = info->record()->getDestructor();
 
     // For finalized classes, check the finalization method if possible.
     if (info->IsGCFinalized()) {
       if (dtor && dtor->hasBody()) {
         CheckFinalizerVisitor visitor(&cache_);
         visitor.TraverseCXXMethodDecl(dtor);
@@ skipping 51 matching lines @@
       return;
     }
 
     CheckTraceOrDispatchMethod(parent, method);
   }
 
   // Determine what type of tracing method this is (dispatch or trace).
   void CheckTraceOrDispatchMethod(RecordInfo* parent, CXXMethodDecl* method) {
     bool isTraceAfterDispatch;
     if (Config::IsTraceMethod(method, &isTraceAfterDispatch)) {
-      if (!isTraceAfterDispatch && parent->GetTraceDispatchMethod())
-        CheckTraceDispatchMethod(parent, method);
-      else
-        CheckTraceMethod(parent, method);
+      if (isTraceAfterDispatch || !parent->GetTraceDispatchMethod()) {
+        CheckTraceMethod(parent, method, isTraceAfterDispatch);
+      }
+      // Dispatch methods are checked when we identify subclasses.
     }
   }
 
-  // Check a tracing dispatch (ie, it dispatches to traceAfterDispatch)
-  void CheckTraceDispatchMethod(RecordInfo* parent, CXXMethodDecl* trace) {
-    // TODO: check correct dispatch.
-  }
+  // Check an actual trace method.
+  void CheckTraceMethod(RecordInfo* parent,
+                        CXXMethodDecl* trace,
+                        bool isTraceAfterDispatch) {
+    // A non-virtual trace method must not override another trace.
+    if (!isTraceAfterDispatch && !trace->isVirtual()) {
+      for (RecordInfo::Bases::iterator it = parent->GetBases().begin();
+           it != parent->GetBases().end();
+           ++it) {
+        RecordInfo* base = it->second.info();
+        // We allow mixin bases to contain a non-virtual trace since it is will
+        // never be used for dispatching.
+        if (base->IsUnmixedGCMixin())

[Mads Ager (chromium), 2014/03/18 09:19:32]

Can we find a better name for IsUnmixedGCMixin? I don't understand what that
means.

[zerny-chromium, 2014/03/18 10:03:41]

Changed to just IsGCMixin().

+          continue;
+        if (CXXMethodDecl* other = base->InheritsNonVirtualTrace())
+          ReportOverriddenNonVirtualTrace(parent, trace, other);
+      }
+    }
 
-  // Check an actual trace method.
-  void CheckTraceMethod(RecordInfo* parent, CXXMethodDecl* trace) {
     CheckTraceVisitor visitor(trace, parent);
     visitor.TraverseCXXMethodDecl(trace);
 
     for (RecordInfo::Bases::iterator it = parent->GetBases().begin();
          it != parent->GetBases().end();
          ++it) {
       if (!it->second.IsProperlyTraced())
         ReportBaseRequiresTracing(parent, trace, it->first);
     }
 
@@ skipping 164 matching lines @@
   }
 
   void ReportClassRequiresFinalization(RecordInfo* info) {
     SourceLocation loc = info->record()->getInnerLocStart();
     SourceManager& manager = instance_.getSourceManager();
     FullSourceLoc full_loc(loc, manager);
     diagnostic_.Report(full_loc, diag_class_requires_finalization_)
         << info->record();
   }
 
+  void ReportOverriddenNonVirtualTrace(RecordInfo* info,
+                                       CXXMethodDecl* trace,
+                                       CXXMethodDecl* overridden) {
+    SourceLocation loc = trace->getLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, diag_overridden_non_virtual_trace_)
+        << info->record()
+        << overridden->getParent();
+    NoteOverriddenNonVirtualTrace(overridden);
+  }
+
+  void ReportVirtualAndManualDispatch(RecordInfo* info) {
+    SourceLocation loc = info->record()->getInnerLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, diag_virtual_and_manual_dispatch_)
+        << info->record();
+  }
+
+  void ReportMissingTraceDispatch(const FunctionDecl* dispatch,
+                                  RecordInfo* receiver) {
+    ReportMissingDispatch(dispatch, receiver, diag_missing_trace_dispatch_);
+  }
+
+  void ReportMissingFinalize(RecordInfo* info) {
+    SourceLocation loc = info->record()->getInnerLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, diag_missing_finalize_) << info->record();
+  }
+
+  void ReportMissingFinalizeDispatch(const FunctionDecl* dispatch,
+                                     RecordInfo* receiver) {
+    ReportMissingDispatch(dispatch, receiver, diag_missing_finalize_dispatch_);
+  }
+
+  void ReportMissingDispatch(const FunctionDecl* dispatch,
+                               RecordInfo* receiver,
+                               unsigned error) {
+    SourceLocation loc = dispatch->getLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, error) << receiver->record();
+  }
+
   void NoteFieldRequiresTracing(RecordInfo* holder, FieldDecl* field) {
     NoteField(field, diag_field_requires_tracing_note_);
   }
 
   void NotePartObjectContainsGCRoot(FieldPoint* point) {
     FieldDecl* field = point->field();
     SourceLocation loc = field->getLocStart();
     SourceManager& manager = instance_.getSourceManager();
     FullSourceLoc full_loc(loc, manager);
     diagnostic_.Report(full_loc, diag_part_object_contains_gc_root_note_)
@@ skipping 23 matching lines @@
     NoteField(point->field(), note);
   }
 
   void NoteField(FieldDecl* field, unsigned note) {
     SourceLocation loc = field->getLocStart();
     SourceManager& manager = instance_.getSourceManager();
     FullSourceLoc full_loc(loc, manager);
     diagnostic_.Report(full_loc, note) << field;
   }
 
+  void NoteOverriddenNonVirtualTrace(CXXMethodDecl* overridden) {
+    SourceLocation loc = overridden->getLocStart();
+    SourceManager& manager = instance_.getSourceManager();
+    FullSourceLoc full_loc(loc, manager);
+    diagnostic_.Report(full_loc, diag_overridden_non_virtual_trace_note_)
+        << overridden;
+  }
+
   unsigned diag_class_requires_trace_method_;
   unsigned diag_base_requires_tracing_;
   unsigned diag_fields_require_tracing_;
   unsigned diag_class_contains_invalid_fields_;
   unsigned diag_class_contains_gc_root_;
   unsigned diag_class_requires_finalization_;
   unsigned diag_finalizer_accesses_finalized_field_;
+  unsigned diag_overridden_non_virtual_trace_;
+  unsigned diag_virtual_and_manual_dispatch_;
+  unsigned diag_missing_trace_dispatch_;
+  unsigned diag_missing_finalize_;
+  unsigned diag_missing_finalize_dispatch_;
 
   unsigned diag_field_requires_tracing_note_;
   unsigned diag_raw_ptr_to_gc_managed_class_note_;
   unsigned diag_ref_ptr_to_gc_managed_class_note_;
   unsigned diag_own_ptr_to_gc_managed_class_note_;
   unsigned diag_part_object_contains_gc_root_note_;
   unsigned diag_field_contains_gc_root_note_;
   unsigned diag_finalized_field_note_;
   unsigned diag_user_declared_destructor_note_;
   unsigned diag_base_requires_finalization_note_;
   unsigned diag_field_requires_finalization_note_;
+  unsigned diag_overridden_non_virtual_trace_note_;
 
   CompilerInstance& instance_;
   DiagnosticsEngine& diagnostic_;
   BlinkGCPluginOptions options_;
   RecordCache cache_;
 };
 
 class BlinkGCPluginAction : public PluginASTAction {
  public:
   BlinkGCPluginAction() {}
@@ skipping 23 matching lines @@
 
  private:
   BlinkGCPluginOptions options_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<BlinkGCPluginAction> X(
     "blink-gc-plugin",
     "Check Blink GC invariants");