Check some finalization properties.

  - Classes that have a user-declared destructor must inherit from a finalized GC base.
  - Destructors in GC derived class must not access GC managed fields.

BUG=334149
R=ager@chromium.org
NOTRY=true

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@255109 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 319 insertions, 34 deletions

diff --git a/tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp b/tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
index 800b316..479e1ac 100644
--- a/tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
+++ b/tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp
@@ -41,6 +41,12 @@ const char kClassContainsInvalidFields[] =
 const char kClassContainsGCRoot[] =
     "[blink-gc] Class %0 contains GC root in field %1.";
 
+const char kFinalizerInNonFinalizedClass[] =
+    "[blink-gc] Non-finalized class %0 has a user-declared finalizer %1.";
+
+const char kFinalizerAccessesFinalizedField[] =
+    "[blink-gc] Finalizer %0 accesses potentially finalized field %1.";
+
 const char kRawPtrToGCManagedClassNote[] =
     "[blink-gc] Raw pointer field %0 to a GC managed class declared here:";
 
@@ -56,6 +62,9 @@ const char kPartObjectContainsGCRoot[] =
 const char kFieldContainsGCRoot[] =
     "[blink-gc] Field %0 defining a GC root declared here:";
 
+const char kFinalizedFieldNote[] =
+    "[blink-gc] Potentially finalized field %0 declared here:";
+
 struct BlinkGCPluginOptions {
   BlinkGCPluginOptions() : enable_oilpan(false) {}
   bool enable_oilpan;
@@ -117,6 +126,67 @@ class CollectVisitor : public RecursiveASTVisitor<CollectVisitor> {
   MethodVector trace_decls_;
 };
 
+// This visitor checks that a finalizer method does not access fields that are
+// potentially finalized. A potentially finalized field is either a Member, a
+// heap-allocated collection or an off-heap collection that contains Members.
+class CheckFinalizerVisitor
+    : public RecursiveASTVisitor<CheckFinalizerVisitor> {
+ private:
+  // Simple visitor to determine if the content of a field might be collected
+  // during finalization.
+  class MightBeCollectedVisitor : public EdgeVisitor {
+   public:
+    MightBeCollectedVisitor() : might_be_collected_(false) {}
+    bool might_be_collected() { return might_be_collected_; }
+    void VisitMember(Member* edge) { might_be_collected_ = true; }
+    void VisitCollection(Collection* edge) {
+      if (edge->on_heap()) {
+        might_be_collected_ = !edge->is_root();
+      } else {
+        edge->AcceptMembers(this);
+      }
+    }
+
+   private:
+    bool might_be_collected_;
+  };
+
+ public:
+  typedef std::vector<std::pair<MemberExpr*, FieldPoint*> > Errors;
+
+  CheckFinalizerVisitor(RecordCache* cache) : cache_(cache) {}
+
+  Errors& finalized_fields() { return finalized_fields_; }
+
+  bool VisitMemberExpr(MemberExpr* member) {
+    FieldDecl* field = dyn_cast<FieldDecl>(member->getMemberDecl());
+    if (!field)
+      return true;
+
+    RecordInfo* info = cache_->Lookup(field->getParent());
+    if (!info)
+      return true;
+
+    RecordInfo::Fields::iterator it = info->GetFields().find(field);
+    if (it == info->GetFields().end())
+      return true;
+
+    if (MightBeCollected(&it->second))
+      finalized_fields_.push_back(std::make_pair(member, &it->second));
+    return true;
+  }
+
+  bool MightBeCollected(FieldPoint* point) {
+    MightBeCollectedVisitor visitor;
+    point->edge()->Accept(&visitor);
+    return visitor.might_be_collected();
+  }
+
+ private:
+  Errors finalized_fields_;
+  RecordCache* cache_;
+};
+
 // 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.
@@ -339,6 +409,10 @@ class BlinkGCPluginConsumer : public ASTConsumer {
                                     kClassContainsInvalidFields);
     diag_class_contains_gc_root_ =
         diagnostic_.getCustomDiagID(getErrorLevel(), kClassContainsGCRoot);
+    diag_finalizer_in_nonfinalized_class_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kFinalizerInNonFinalizedClass);
+    diag_finalizer_accesses_finalized_field_ = diagnostic_.getCustomDiagID(
+        getErrorLevel(), kFinalizerAccessesFinalizedField);
 
     // Register note messages.
     diag_field_requires_tracing_note_ = diagnostic_.getCustomDiagID(
@@ -353,6 +427,8 @@ class BlinkGCPluginConsumer : public ASTConsumer {
         DiagnosticsEngine::Note, kPartObjectContainsGCRoot);
     diag_field_contains_gc_root_note_ = diagnostic_.getCustomDiagID(
         DiagnosticsEngine::Note, kFieldContainsGCRoot);
+    diag_finalized_field_note_ = diagnostic_.getCustomDiagID(
+        DiagnosticsEngine::Note, kFinalizedFieldNote);
   }
 
   virtual void HandleTranslationUnit(ASTContext& context) {
@@ -414,9 +490,37 @@ class BlinkGCPluginConsumer : public ASTConsumer {
     }
 
     if (info->IsGCDerived()) {
-      CheckGCRootsVisitor visitor;
-      if (visitor.ContainsGCRoots(info))
-        ReportClassContainsGCRoots(info, &visitor.gc_roots());
+      {
+        CheckGCRootsVisitor visitor;
+        if (visitor.ContainsGCRoots(info))
+          ReportClassContainsGCRoots(info, &visitor.gc_roots());
+      }
+
+      // TODO: check for non-user defined and non-trivial destructors too.
+      // TODO: support overridden finalize().
+      if (CXXDestructorDecl* dtor = info->record()->getDestructor()) {
+        if (dtor->isUserProvided() && !info->IsGCFinalized()) {
+          // Don't report if using transition types and the body is empty.
+          if (!options_.enable_oilpan) {
+            ReportFinalizerInNonFinalizedClass(info, dtor);
+          } else {
+            if (dtor->hasBody()) {
+              CompoundStmt* stmt = cast<CompoundStmt>(dtor->getBody());
+              if (stmt && !stmt->body_empty())
+                ReportFinalizerInNonFinalizedClass(info, dtor);
+            }
+          }
+        }
+
+        if (dtor->hasBody()) {
+          CheckFinalizerVisitor visitor(&cache_);
+          visitor.TraverseCXXMethodDecl(dtor);
+          if (!visitor.finalized_fields().empty()) {
+            ReportFinalizerAccessesFinalizedFields(dtor,
+                                                   &visitor.finalized_fields());
+          }
+        }
+      }
     }
   }
 
@@ -603,11 +707,11 @@ class BlinkGCPluginConsumer : public ASTConsumer {
          it != errors->end();
          ++it) {
       if (it->second->IsRawPtr()) {
-        NoteInvalidField(it->first, diag_raw_ptr_to_gc_managed_class_note_);
+        NoteField(it->first, diag_raw_ptr_to_gc_managed_class_note_);
       } else if (it->second->IsRefPtr()) {
-        NoteInvalidField(it->first, diag_ref_ptr_to_gc_managed_class_note_);
+        NoteField(it->first, diag_ref_ptr_to_gc_managed_class_note_);
       } else if (it->second->IsOwnPtr()) {
-        NoteInvalidField(it->first, diag_own_ptr_to_gc_managed_class_note_);
+        NoteField(it->first, diag_own_ptr_to_gc_managed_class_note_);
       }
     }
   }
@@ -632,18 +736,32 @@ class BlinkGCPluginConsumer : public ASTConsumer {
     }
   }
 
-  void NoteFieldRequiresTracing(RecordInfo* holder, FieldDecl* field) {
-    SourceLocation loc = field->getLocStart();
+  void ReportFinalizerInNonFinalizedClass(RecordInfo* info,
+                                          CXXMethodDecl* dtor) {
+    SourceLocation loc = dtor->getLocStart();
     SourceManager& manager = instance_.getSourceManager();
     FullSourceLoc full_loc(loc, manager);
-    diagnostic_.Report(full_loc, diag_field_requires_tracing_note_) << field;
+    diagnostic_.Report(full_loc, diag_finalizer_in_nonfinalized_class_)
+        << info->record() << dtor;
   }
 
-  void NoteInvalidField(FieldPoint* point, unsigned note) {
-    SourceLocation loc = point->field()->getLocStart();
-    SourceManager& manager = instance_.getSourceManager();
-    FullSourceLoc full_loc(loc, manager);
-    diagnostic_.Report(full_loc, note) << point->field();
+  void ReportFinalizerAccessesFinalizedFields(
+      CXXMethodDecl* dtor,
+      CheckFinalizerVisitor::Errors* fields) {
+    for (CheckFinalizerVisitor::Errors::iterator it = fields->begin();
+         it != fields->end();
+         ++it) {
+      SourceLocation loc = it->first->getLocStart();
+      SourceManager& manager = instance_.getSourceManager();
+      FullSourceLoc full_loc(loc, manager);
+      diagnostic_.Report(full_loc, diag_finalizer_accesses_finalized_field_)
+          << dtor << it->second->field();
+      NoteField(it->second, diag_finalized_field_note_);
+    }
+  }
+
+  void NoteFieldRequiresTracing(RecordInfo* holder, FieldDecl* field) {
+    NoteField(field, diag_field_requires_tracing_note_);
   }
 
   void NotePartObjectContainsGCRoot(FieldPoint* point) {
@@ -656,12 +774,18 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   }
 
   void NoteFieldContainsGCRoot(FieldPoint* point) {
-    FieldDecl* field = point->field();
+    NoteField(point, diag_field_contains_gc_root_note_);
+  }
+
+  void NoteField(FieldPoint* point, unsigned note) {
+    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, diag_field_contains_gc_root_note_)
-        << field;
+    diagnostic_.Report(full_loc, note) << field;
   }
 
   unsigned diag_class_requires_trace_method_;
@@ -669,6 +793,8 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   unsigned diag_fields_require_tracing_;
   unsigned diag_class_contains_invalid_fields_;
   unsigned diag_class_contains_gc_root_;
+  unsigned diag_finalizer_in_nonfinalized_class_;
+  unsigned diag_finalizer_accesses_finalized_field_;
 
   unsigned diag_field_requires_tracing_note_;
   unsigned diag_raw_ptr_to_gc_managed_class_note_;
@@ -676,6 +802,7 @@ class BlinkGCPluginConsumer : public ASTConsumer {
   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_;
 
   CompilerInstance& instance_;
   DiagnosticsEngine& diagnostic_;
diff --git a/tools/clang/blink_gc_plugin/Edge.cpp b/tools/clang/blink_gc_plugin/Edge.cpp
index 33850c0..2b5be75 100644
--- a/tools/clang/blink_gc_plugin/Edge.cpp
+++ b/tools/clang/blink_gc_plugin/Edge.cpp
@@ -67,10 +67,6 @@ void RecursiveEdgeVisitor::VisitPersistent(Persistent* e) {
 void RecursiveEdgeVisitor::VisitCollection(Collection* e) {
   AtCollection(e);
   Enter(e);
-  for (Collection::Members::iterator it = e->members().begin();
-       it != e->members().end();
-       ++it) {
-    (*it)->Accept(this);
-  }
+  e->AcceptMembers(this);
   Leave();
 }
diff --git a/tools/clang/blink_gc_plugin/Edge.h b/tools/clang/blink_gc_plugin/Edge.h
index 9150afd..5de4107 100644
--- a/tools/clang/blink_gc_plugin/Edge.h
+++ b/tools/clang/blink_gc_plugin/Edge.h
@@ -190,6 +190,10 @@ class Collection : public Edge {
   bool is_root() { return is_root_; }
   Members& members() { return members_; }
   void Accept(EdgeVisitor* visitor) { visitor->VisitCollection(this); }
+  void AcceptMembers(EdgeVisitor* visitor) {
+    for (Members::iterator it = members_.begin(); it != members_.end(); ++it)
+      (*it)->Accept(visitor);
+  }
   TracingStatus NeedsTracing(NeedsTracingOption) {
     if (is_root_)
       return TracingStatus::Unneeded();
diff --git a/tools/clang/blink_gc_plugin/RecordInfo.cpp b/tools/clang/blink_gc_plugin/RecordInfo.cpp
index e36e50b..ba9fb66 100644
--- a/tools/clang/blink_gc_plugin/RecordInfo.cpp
+++ b/tools/clang/blink_gc_plugin/RecordInfo.cpp
@@ -19,11 +19,14 @@ RecordInfo::RecordInfo(CXXRecordDecl* record, RecordCache* cache)
       fields_(0),
       determined_trace_methods_(false),
       trace_method_(0),
-      trace_dispatch_method_(0) {}
+      trace_dispatch_method_(0),
+      is_gc_derived_(false),
+      base_paths_(0) {}
 
 RecordInfo::~RecordInfo() {
   delete fields_;
   delete bases_;
+  delete base_paths_;
 }
 
 // Get |count| number of template arguments. Returns false if there
@@ -77,7 +80,13 @@ static bool IsGCBaseCallback(const CXXBaseSpecifier* specifier,
 }
 
 // Test if a record is derived from a garbage collected base.
-bool RecordInfo::IsGCDerived(CXXBasePaths* paths) {
+bool RecordInfo::IsGCDerived() {
+  // If already computed, return the known result.
+  if (base_paths_)
+    return is_gc_derived_;
+
+  base_paths_ = new CXXBasePaths(true, true, false);
+
   if (!record_->hasDefinition())
     return false;
 
@@ -86,10 +95,22 @@ bool RecordInfo::IsGCDerived(CXXBasePaths* paths) {
     return false;
 
   // Walk the inheritance tree to find GC base classes.
-  CXXBasePaths localPaths(false, false, false);
-  if (!paths)
-    paths = &localPaths;
-  return record_->lookupInBases(IsGCBaseCallback, 0, *paths);
+  is_gc_derived_ = record_->lookupInBases(IsGCBaseCallback, 0, *base_paths_);
+  return is_gc_derived_;
+}
+
+bool RecordInfo::IsGCFinalized() {
+  if (!IsGCDerived())
+    return false;
+  for (CXXBasePaths::paths_iterator it = base_paths_->begin();
+       it != base_paths_->end();
+       ++it) {
+    const CXXBasePathElement& elem = (*it)[it->size() - 1];
+    CXXRecordDecl* base = elem.Base->getType()->getAsCXXRecordDecl();
+    if (Config::IsGCFinalizedBase(base->getName()))
+      return true;
+  }
+  return false;
 }
 
 // Test if a record is allocated on the managed heap.
diff --git a/tools/clang/blink_gc_plugin/RecordInfo.h b/tools/clang/blink_gc_plugin/RecordInfo.h
index 9fdb228..a5907e8 100644
--- a/tools/clang/blink_gc_plugin/RecordInfo.h
+++ b/tools/clang/blink_gc_plugin/RecordInfo.h
@@ -76,8 +76,9 @@ class RecordInfo {
   bool GetTemplateArgs(size_t count, TemplateArgs* output_args);
 
   bool IsHeapAllocatedCollection();
-  bool IsGCDerived(clang::CXXBasePaths* paths = 0);
+  bool IsGCDerived();
   bool IsGCAllocated();
+  bool IsGCFinalized();
 
   bool IsStackAllocated();
   bool RequiresTraceMethod();
@@ -103,6 +104,9 @@ class RecordInfo {
   clang::CXXMethodDecl* trace_method_;
   clang::CXXMethodDecl* trace_dispatch_method_;
 
+  bool is_gc_derived_;
+  clang::CXXBasePaths* base_paths_;
+
   friend class RecordCache;
 };
 
@@ -122,6 +126,10 @@ class RecordCache {
     return Lookup(const_cast<clang::CXXRecordDecl*>(record));
   }
 
+  RecordInfo* Lookup(clang::Decl* decl) {
+    return Lookup(clang::dyn_cast<clang::CXXRecordDecl>(decl));
+  }
+
   ~RecordCache() {
     for (Cache::iterator it = cache_.begin(); it != cache_.end(); ++it) {
       if (!it->second.fields_)
diff --git a/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.cpp b/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.cpp
new file mode 100644
index 0000000..7c11cf6
--- /dev/null
+++ b/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.cpp
@@ -0,0 +1,21 @@
+// 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.
+
+#include "destructor_access_finalized_field.h"
+
+namespace WebCore {
+
+HeapObject::~HeapObject()
+{
+    if (m_ref && m_obj)
+        (void)m_objs;
+}
+
+void HeapObject::trace(Visitor* visitor)
+{
+    visitor->trace(m_obj);
+    visitor->trace(m_objs);
+}
+
+}
diff --git a/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.h b/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.h
new file mode 100644
index 0000000..37e94ff
--- /dev/null
+++ b/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.h
@@ -0,0 +1,26 @@
+// 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.
+
+#ifndef DESTRUCTOR_ACCESS_FINALIZED_FIELD_H_
+#define DESTRUCTOR_ACCESS_FINALIZED_FIELD_H_
+
+#include "heap/stubs.h"
+
+namespace WebCore {
+
+class Other : public RefCounted<Other> {};
+
+class HeapObject : public GarbageCollectedFinalized<HeapObject> {
+public:
+    ~HeapObject();
+    void trace(Visitor*);
+private:
+    RefPtr<Other> m_ref;
+    Member<HeapObject> m_obj;
+    Vector<Member<HeapObject> > m_objs;
+};
+
+}
+
+#endif
diff --git a/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.txt b/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.txt
new file mode 100644
index 0000000..f79b12a
--- /dev/null
+++ b/tools/clang/blink_gc_plugin/tests/destructor_access_finalized_field.txt
@@ -0,0 +1,13 @@
+destructor_access_finalized_field.cpp:11:18: warning: [blink-gc] Finalizer '~HeapObject' accesses potentially finalized field 'm_obj'.
+    if (m_ref && m_obj)
+                 ^
+./destructor_access_finalized_field.h:20:5: note: [blink-gc] Potentially finalized field 'm_obj' declared here:
+    Member<HeapObject> m_obj;
+    ^
+destructor_access_finalized_field.cpp:12:15: warning: [blink-gc] Finalizer '~HeapObject' accesses potentially finalized field 'm_objs'.
+        (void)m_objs;
+              ^
+./destructor_access_finalized_field.h:21:5: note: [blink-gc] Potentially finalized field 'm_objs' declared here:
+    Vector<Member<HeapObject> > m_objs;
+    ^
+2 warnings generated.
diff --git a/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.cpp b/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.cpp
new file mode 100644
index 0000000..4e0b3f3
--- /dev/null
+++ b/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.cpp
@@ -0,0 +1,20 @@
+// 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.
+
+#include "destructor_in_nonfinalized_class.h"
+
+namespace WebCore {
+
+HeapObject::~HeapObject()
+{
+    // Do something when destructed...
+    (void)this;
+}
+
+void HeapObject::trace(Visitor* visitor)
+{
+    visitor->trace(m_obj);
+}
+
+}
diff --git a/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.h b/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.h
new file mode 100644
index 0000000..0e1c815
--- /dev/null
+++ b/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.h
@@ -0,0 +1,22 @@
+// 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.
+
+#ifndef DESTRUCTOR_IN_NONFINALIZED_CLASS_H_
+#define DESTRUCTOR_IN_NONFINALIZED_CLASS_H_
+
+#include "heap/stubs.h"
+
+namespace WebCore {
+
+class HeapObject : public GarbageCollected<HeapObject> {
+public:
+    ~HeapObject();
+    void trace(Visitor*);
+private:
+    Member<HeapObject> m_obj;
+};
+
+}
+
+#endif
diff --git a/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.txt b/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.txt
new file mode 100644
index 0000000..042f66d
--- /dev/null
+++ b/tools/clang/blink_gc_plugin/tests/destructor_in_nonfinalized_class.txt
@@ -0,0 +1,4 @@
+destructor_in_nonfinalized_class.cpp:9:1: warning: [blink-gc] Non-finalized class 'HeapObject' has a user-declared finalizer '~HeapObject'.
+HeapObject::~HeapObject()
+^
+1 warning generated.
diff --git a/tools/clang/blink_gc_plugin/tests/heap/stubs.h b/tools/clang/blink_gc_plugin/tests/heap/stubs.h
index d4f0ddc..10b777c 100644
--- a/tools/clang/blink_gc_plugin/tests/heap/stubs.h
+++ b/tools/clang/blink_gc_plugin/tests/heap/stubs.h
@@ -10,9 +10,21 @@
 namespace WTF {
 
 template<typename T> class RefCounted { };
-template<typename T> class RawPtr { };
-template<typename T> class RefPtr { };
-template<typename T> class OwnPtr { };
+
+template<typename T> class RawPtr {
+public:
+    operator T*() const { return 0; }
+};
+
+template<typename T> class RefPtr {
+public:
+    operator T*() const { return 0; }
+};
+
+template<typename T> class OwnPtr {
+public:
+    operator T*() const { return 0; }
+};
 
 class DefaultAllocator { };
 
@@ -45,8 +57,19 @@ using namespace WTF;
     virtual bool isAlive(Visitor*) const { return 0; }
 
 template<typename T> class GarbageCollected { };
-template<typename T> class Member { };
-template<typename T> class Persistent { };
+
+template<typename T>
+class GarbageCollectedFinalized : public GarbageCollected<T> { };
+
+template<typename T> class Member {
+public:
+    operator T*() const { return 0; }
+};
+
+template<typename T> class Persistent {
+public:
+    operator T*() const { return 0; }
+};
 
 class HeapAllocator { };