GC plugin: improve error reporting when tracing illegal fields.

Add detection of trace() calls over smart pointer types that either do not
wrap up references to heap objects, or are otherwise not meant to be traced
over. In particular, CrossThread(Weak)Persistent<T> fields are now detected
as being illegal to trace over. Also consider OwnPtr<T>, RefPtr<T> and
std::unique_ptr<T> as illegal to trace over & emit a more concise error
messages for these.

R=
BUG=619149

Review-Url: https://codereview.chromium.org/2060553002
Cr-Commit-Position: refs/heads/master@{#399861}

tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp

@@ -68,6 +68,7 @@ BlinkGCPluginConsumer::BlinkGCPluginConsumer(
     : instance_(instance),
       reporter_(instance),
       options_(options),
+      cache_(instance),
       json_(0) {
   // Only check structures in the blink and WebKit namespaces.
   options_.checked_namespaces.insert("blink");
@@ -552,9 +553,10 @@ void BlinkGCPluginConsumer::CheckTraceMethod(
       reporter_.BaseRequiresTracing(parent, trace, base.first);
 
   for (auto& field : parent->GetFields()) {
-    if (!field.second.IsProperlyTraced()) {
-      // Discontinue once an untraced-field error is found.
-      reporter_.FieldsRequireTracing(parent, trace);
+    if (!field.second.IsProperlyTraced() ||
+        field.second.IsInproperlyTraced()) {
+      // Report one or more tracing-related field errors.
+      reporter_.FieldsImproperlyTraced(parent, trace);
       break;
     }
   }
@@ -590,6 +592,7 @@ void BlinkGCPluginConsumer::DumpClass(RecordInfo* info) {
                         "reference" : "raw") :
                    Parent()->IsRefPtr() ? "ref" :
                    Parent()->IsOwnPtr() ? "own" :
+                   Parent()->IsUniquePtr() ? "unique" :
                    (Parent()->IsMember() || Parent()->IsWeakMember()) ? "mem" :
                    "val");
       json_->CloseObject();

tools/clang/blink_gc_plugin/CheckFieldsVisitor.cpp

@@ -86,6 +86,7 @@ void CheckFieldsVisitor::AtValue(Value* edge) {
 
   // Disallow  OwnPtr<T>, RefPtr<T> and T* to stack-allocated types.
   if (Parent()->IsOwnPtr() ||
+      Parent()->IsUniquePtr() ||
       Parent()->IsRefPtr() ||
       (stack_allocated_host_ && Parent()->IsRawPtr())) {
     invalid_fields_.push_back(std::make_pair(
@@ -103,6 +104,8 @@ void CheckFieldsVisitor::AtValue(Value* edge) {
 void CheckFieldsVisitor::AtCollection(Collection* edge) {
   if (edge->on_heap() && Parent() && Parent()->IsOwnPtr())
     invalid_fields_.push_back(std::make_pair(current_, kOwnPtrToGCManaged));
+  if (edge->on_heap() && Parent() && Parent()->IsUniquePtr())
+    invalid_fields_.push_back(std::make_pair(current_, kUniquePtrToGCManaged));
 }
 
 CheckFieldsVisitor::Error CheckFieldsVisitor::InvalidSmartPtr(Edge* ptr) {
@@ -116,5 +119,7 @@ CheckFieldsVisitor::Error CheckFieldsVisitor::InvalidSmartPtr(Edge* ptr) {
     return kRefPtrToGCManaged;
   if (ptr->IsOwnPtr())
     return kOwnPtrToGCManaged;
+  if (ptr->IsUniquePtr())
+    return kUniquePtrToGCManaged;
   assert(false && "Unknown smart pointer kind");
 }

tools/clang/blink_gc_plugin/CheckFieldsVisitor.h

@@ -25,6 +25,7 @@ class CheckFieldsVisitor : public RecursiveEdgeVisitor {
     kRefPtrToGCManaged,
     kReferencePtrToGCManaged,
     kOwnPtrToGCManaged,
+    kUniquePtrToGCManaged,
     kMemberToGCUnmanaged,
     kMemberInUnmanaged,
     kPtrFromHeapToStack,

tools/clang/blink_gc_plugin/Config.h

@@ -49,12 +49,13 @@ class Config {
   }
 
   static bool IsPersistent(const std::string& name) {
-    return name == "Persistent";
+    return name == "Persistent" ||
+           name == "WeakPersistent" ;
   }
 
-  static bool IsPersistentHandle(const std::string& name) {
-    return IsPersistent(name) ||
-           IsPersistentGCCollection(name);
+  static bool IsCrossThreadPersistent(const std::string& name) {
+    return name == "CrossThreadPersistent" ||
+           name == "CrossThreadWeakPersistent" ;
   }
 
   static bool IsRefPtr(const std::string& name) {
@@ -65,6 +66,10 @@ class Config {
     return name == "OwnPtr";
   }
 
+  static bool IsUniquePtr(const std::string& name) {
+    return name == "unique_ptr";
+  }
+
   static bool IsWTFCollection(const std::string& name) {
     return name == "Vector" ||
            name == "Deque" ||

tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp

@@ -23,9 +23,15 @@ const char kBaseRequiresTracingNote[] =
 const char kFieldsRequireTracing[] =
     "[blink-gc] Class %0 has untraced fields that require tracing.";
 
+const char kFieldsImproperlyTraced[] =
+    "[blink-gc] Class %0 has untraced or not traceable fields.";
+
 const char kFieldRequiresTracingNote[] =
     "[blink-gc] Untraced field %0 declared here:";
 
+const char kFieldShouldNotBeTracedNote[] =
+    "[blink-gc] Untraceable field %0 declared here:";
+
 const char kClassContainsInvalidFields[] =
     "[blink-gc] Class %0 contains invalid fields.";
 
@@ -57,6 +63,9 @@ const char kReferencePtrToGCManagedClassNote[] =
 const char kOwnPtrToGCManagedClassNote[] =
     "[blink-gc] OwnPtr field %0 to a GC managed class declared here:";
 
+const char kUniquePtrToGCManagedClassNote[] =
+    "[blink-gc] std::unique_ptr field %0 to a GC managed class declared here:";
+
 const char kMemberToGCUnmanagedClassNote[] =
     "[blink-gc] Member field %0 to non-GC managed class declared here:";
 
@@ -162,6 +171,8 @@ DiagnosticsReporter::DiagnosticsReporter(
       diagnostic_.getCustomDiagID(getErrorLevel(), kBaseRequiresTracing);
   diag_fields_require_tracing_ =
       diagnostic_.getCustomDiagID(getErrorLevel(), kFieldsRequireTracing);
+  diag_fields_improperly_traced_ =
+      diagnostic_.getCustomDiagID(getErrorLevel(), kFieldsImproperlyTraced);
   diag_class_contains_invalid_fields_ = diagnostic_.getCustomDiagID(
       getErrorLevel(), kClassContainsInvalidFields);
   diag_class_contains_gc_root_ =
@@ -202,6 +213,8 @@ DiagnosticsReporter::DiagnosticsReporter(
       DiagnosticsEngine::Note, kBaseRequiresTracingNote);
   diag_field_requires_tracing_note_ = diagnostic_.getCustomDiagID(
       DiagnosticsEngine::Note, kFieldRequiresTracingNote);
+  diag_field_should_not_be_traced_note_ = diagnostic_.getCustomDiagID(
+      DiagnosticsEngine::Note, kFieldShouldNotBeTracedNote);
   diag_raw_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
       DiagnosticsEngine::Note, kRawPtrToGCManagedClassNote);
   diag_ref_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
@@ -210,6 +223,8 @@ DiagnosticsReporter::DiagnosticsReporter(
       DiagnosticsEngine::Note, kReferencePtrToGCManagedClassNote);
   diag_own_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
       DiagnosticsEngine::Note, kOwnPtrToGCManagedClassNote);
+  diag_unique_ptr_to_gc_managed_class_note_ = diagnostic_.getCustomDiagID(
+      DiagnosticsEngine::Note, kUniquePtrToGCManagedClassNote);
   diag_member_to_gc_unmanaged_class_note_ = diagnostic_.getCustomDiagID(
       DiagnosticsEngine::Note, kMemberToGCUnmanagedClassNote);
   diag_stack_allocated_field_note_ = diagnostic_.getCustomDiagID(
@@ -279,14 +294,25 @@ void DiagnosticsReporter::BaseRequiresTracing(
       << base << derived->record();
 }
 
-void DiagnosticsReporter::FieldsRequireTracing(
+void DiagnosticsReporter::FieldsImproperlyTraced(
     RecordInfo* info,
     CXXMethodDecl* trace) {
-  ReportDiagnostic(trace->getLocStart(), diag_fields_require_tracing_)
+  // Only mention untraceable in header diagnostic if they appear.
+  unsigned diag = diag_fields_require_tracing_;
+  for (auto& field : info->GetFields()) {
+    if (field.second.IsInproperlyTraced()) {
+      diag = diag_fields_improperly_traced_;
+      break;
+    }
+  }
+  ReportDiagnostic(trace->getLocStart(), diag)
       << info->record();
-  for (auto& field : info->GetFields())
+  for (auto& field : info->GetFields()) {
     if (!field.second.IsProperlyTraced())
       NoteFieldRequiresTracing(info, field.first);
+    if (field.second.IsInproperlyTraced())
+      NoteFieldShouldNotBeTraced(info, field.first);
+  }
 }
 
 void DiagnosticsReporter::ClassContainsInvalidFields(
@@ -307,6 +333,8 @@ void DiagnosticsReporter::ClassContainsInvalidFields(
       note = diag_reference_ptr_to_gc_managed_class_note_;
     } else if (error.second == CheckFieldsVisitor::kOwnPtrToGCManaged) {
       note = diag_own_ptr_to_gc_managed_class_note_;
+    } else if (error.second == CheckFieldsVisitor::kUniquePtrToGCManaged) {
+      note = diag_unique_ptr_to_gc_managed_class_note_;
     } else if (error.second == CheckFieldsVisitor::kMemberToGCUnmanaged) {
       note = diag_member_to_gc_unmanaged_class_note_;
     } else if (error.second == CheckFieldsVisitor::kMemberInUnmanaged) {
@@ -482,6 +510,12 @@ void DiagnosticsReporter::NoteFieldRequiresTracing(
   NoteField(field, diag_field_requires_tracing_note_);
 }
 
+void DiagnosticsReporter::NoteFieldShouldNotBeTraced(
+    RecordInfo* holder,
+    FieldDecl* field) {
+  NoteField(field, diag_field_should_not_be_traced_note_);
+}
+
 void DiagnosticsReporter::NotePartObjectContainsGCRoot(FieldPoint* point) {
   FieldDecl* field = point->field();
   ReportDiagnostic(field->getLocStart(),

tools/clang/blink_gc_plugin/DiagnosticsReporter.h

@@ -30,8 +30,8 @@ class DiagnosticsReporter {
   void BaseRequiresTracing(RecordInfo* derived,
                            clang::CXXMethodDecl* trace,
                            clang::CXXRecordDecl* base);
-  void FieldsRequireTracing(RecordInfo* info,
-                            clang::CXXMethodDecl* trace);
+  void FieldsImproperlyTraced(RecordInfo* info,
+                              clang::CXXMethodDecl* trace);
   void ClassContainsInvalidFields(
       RecordInfo* info,
       const CheckFieldsVisitor::Errors& errors);
@@ -66,6 +66,7 @@ class DiagnosticsReporter {
   void NoteManualDispatchMethod(clang::CXXMethodDecl* dispatch);
   void NoteBaseRequiresTracing(BasePoint* base);
   void NoteFieldRequiresTracing(RecordInfo* holder, clang::FieldDecl* field);
+  void NoteFieldShouldNotBeTraced(RecordInfo* holder, clang::FieldDecl* field);
   void NotePartObjectContainsGCRoot(FieldPoint* point);
   void NoteFieldContainsGCRoot(FieldPoint* point);
   void NoteUserDeclaredDestructor(clang::CXXMethodDecl* dtor);
@@ -93,6 +94,7 @@ class DiagnosticsReporter {
   unsigned diag_class_requires_trace_method_;
   unsigned diag_base_requires_tracing_;
   unsigned diag_fields_require_tracing_;
+  unsigned diag_fields_improperly_traced_;
   unsigned diag_class_contains_invalid_fields_;
   unsigned diag_class_contains_gc_root_;
   unsigned diag_class_requires_finalization_;
@@ -113,10 +115,12 @@ class DiagnosticsReporter {
 
   unsigned diag_base_requires_tracing_note_;
   unsigned diag_field_requires_tracing_note_;
+  unsigned diag_field_should_not_be_traced_note_;
   unsigned diag_raw_ptr_to_gc_managed_class_note_;
   unsigned diag_ref_ptr_to_gc_managed_class_note_;
   unsigned diag_reference_ptr_to_gc_managed_class_note_;
   unsigned diag_own_ptr_to_gc_managed_class_note_;
+  unsigned diag_unique_ptr_to_gc_managed_class_note_;
   unsigned diag_member_to_gc_unmanaged_class_note_;
   unsigned diag_stack_allocated_field_note_;
   unsigned diag_member_in_unmanaged_class_note_;

tools/clang/blink_gc_plugin/Edge.cpp

@@ -17,9 +17,11 @@ void RecursiveEdgeVisitor::AtValue(Value*) {}
 void RecursiveEdgeVisitor::AtRawPtr(RawPtr*) {}
 void RecursiveEdgeVisitor::AtRefPtr(RefPtr*) {}
 void RecursiveEdgeVisitor::AtOwnPtr(OwnPtr*) {}
+void RecursiveEdgeVisitor::AtUniquePtr(UniquePtr*) {}
 void RecursiveEdgeVisitor::AtMember(Member*) {}
 void RecursiveEdgeVisitor::AtWeakMember(WeakMember*) {}
 void RecursiveEdgeVisitor::AtPersistent(Persistent*) {}
+void RecursiveEdgeVisitor::AtCrossThreadPersistent(CrossThreadPersistent*) {}
 void RecursiveEdgeVisitor::AtCollection(Collection*) {}
 
 void RecursiveEdgeVisitor::VisitValue(Value* e) {
@@ -46,6 +48,13 @@ void RecursiveEdgeVisitor::VisitOwnPtr(OwnPtr* e) {
   Leave();
 }
 
+void RecursiveEdgeVisitor::VisitUniquePtr(UniquePtr* e) {
+  AtUniquePtr(e);
+  Enter(e);
+  e->ptr()->Accept(this);
+  Leave();
+}
+
 void RecursiveEdgeVisitor::VisitMember(Member* e) {
   AtMember(e);
   Enter(e);
@@ -67,6 +76,14 @@ void RecursiveEdgeVisitor::VisitPersistent(Persistent* e) {
   Leave();
 }
 
+void RecursiveEdgeVisitor::VisitCrossThreadPersistent(
+    CrossThreadPersistent* e) {
+  AtCrossThreadPersistent(e);
+  Enter(e);
+  e->ptr()->Accept(this);
+  Leave();
+}
+
 void RecursiveEdgeVisitor::VisitCollection(Collection* e) {
   AtCollection(e);
   Enter(e);