Support ignorance of base class finalizers.

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 118 matching lines @@
     "[blink-gc] Garbage collected class %0"
     " is not permitted to declare a pure-virtual trace method.";
 
 struct BlinkGCPluginOptions {
   BlinkGCPluginOptions() : enable_oilpan(false), dump_graph(false) {}
   bool enable_oilpan;
   bool dump_graph;
   std::set<std::string> ignored_classes;
   std::set<std::string> checked_namespaces;
   std::vector<std::string> ignored_directories;
+  std::set<std::string> ignored_base_class_finalizers;
 };
 
 typedef std::vector<CXXRecordDecl*> RecordVector;
 typedef std::vector<CXXMethodDecl*> MethodVector;
 
 // Test if a template specialization is an instantiation.
 static bool IsTemplateInstantiation(CXXRecordDecl* record) {
   ClassTemplateSpecializationDecl* spec =
       dyn_cast<ClassTemplateSpecializationDecl>(record);
   if (!spec)
@@ skipping 426 matching lines @@
         json_(0) {
 
     // Only check structures in the blink, WebCore and WebKit namespaces.
     options_.checked_namespaces.insert("blink");
     options_.checked_namespaces.insert("WebCore");
     options_.checked_namespaces.insert("WebKit");
 
     // Ignore GC implementation files.
     options_.ignored_directories.push_back("/heap/");
 
+    // Following http://crrev.com/369633033 (Blink r177436),
+    // ignore WebCore::ScriptWrappable.
+    options_.ignored_base_class_finalizers.insert("ScriptWrappable::WebCore");

[zerny-chromium, 2014/07/07 09:04:33]

Lets swap this so it is ns::record

+
     // Register warning/error messages.
     diag_class_must_left_mostly_derive_gc_ = diagnostic_.getCustomDiagID(
         getErrorLevel(), kClassMustLeftMostlyDeriveGC);
     diag_class_requires_trace_method_ =
         diagnostic_.getCustomDiagID(getErrorLevel(), kClassRequiresTraceMethod);
     diag_base_requires_tracing_ =
         diagnostic_.getCustomDiagID(getErrorLevel(), kBaseRequiresTracing);
     diag_fields_require_tracing_ =
         diagnostic_.getCustomDiagID(getErrorLevel(), kFieldsRequireTracing);
     diag_class_contains_invalid_fields_ = diagnostic_.getCustomDiagID(
@@ skipping 258 matching lines @@
               dtor, &visitor.finalized_fields());
         }
       }
       return;
     }
 
     // Don't require finalization of a mixin that has not yet been "mixed in".
     if (info->IsGCMixin())
       return;
 
+    // If class has no destructor and all base class destructors can
+    // safely be ignored, do not require finalization.
+    bool has_relevant_dtors = dtor && dtor->isUserProvided();
+    for (RecordInfo::Bases::iterator it = info->GetBases().begin();
+         !has_relevant_dtors && it != info->GetBases().end();
+         ++it) {
+      if (it->second.info()->NeedsFinalization())
+        has_relevant_dtors = !IsIgnoredFinalizableBaseClass(it->second.info());
+    }
+
+    if (!has_relevant_dtors)
+      return;

[zerny-chromium, 2014/07/07 09:04:33]

This will allow erroneous code to pass the plugin, since a class declaring
fields that need finalization will not necessarily cause the class to need
finalization anymore.

class Foo : public GarbageCollected<Foo>, public ScriptWrappable {
  // ... and no user declared dtor.
private:
  String m_str;
};

I think this definition of Foo will not give a compile error despite the String
requiring Foo to have finalization support. Also, say Foo did not have the
string, then still a subclass would still require finalization since we only
ignore on the direct bases:

class Foo : public GarbageCollected<Foo>, public ScriptWrappable { };
class Bar : public Foo { };

Ie, this definition of Bar will give an error I think.

+
     // Report the finalization error, and proceed to print possible causes for
     // the finalization requirement.
     ReportClassRequiresFinalization(info);
 
     if (dtor && dtor->isUserProvided())
       NoteUserDeclaredDestructor(dtor);
 
     for (RecordInfo::Bases::iterator it = info->GetBases().begin();
          it != info->GetBases().end();
          ++it) {
       if (it->second.info()->NeedsFinalization())
-        NoteBaseRequiresFinalization(&it->second);
+        if (!IsIgnoredFinalizableBaseClass(it->second.info()))
+          NoteBaseRequiresFinalization(&it->second);
     }
 
     for (RecordInfo::Fields::iterator it = info->GetFields().begin();
          it != info->GetFields().end();
          ++it) {
       if (it->second.edge()->NeedsFinalization())
         NoteField(&it->second, diag_field_requires_finalization_note_);
     }
   }
 
@@ skipping 192 matching lines @@
   bool IsIgnoredClass(RecordInfo* info) {
     // Ignore any class prefixed by SameSizeAs. These are used in
     // Blink to verify class sizes and don't need checking.
     const string SameSizeAs = "SameSizeAs";
     if (info->name().compare(0, SameSizeAs.size(), SameSizeAs) == 0)
       return true;
     return options_.ignored_classes.find(info->name()) !=
            options_.ignored_classes.end();
   }
 
+  bool IsIgnoredFinalizableBaseClass(RecordInfo* info) {
+    NamespaceDecl* ns =
+        dyn_cast<NamespaceDecl>(info->record()->getDeclContext());
+    if (!ns)
+      return false;
+
+    string qualified_name =
+        string(info->name()).append("::").append(ns->getName());
+    return options_.ignored_base_class_finalizers.find(qualified_name) !=
+           options_.ignored_base_class_finalizers.end();
+  }
+
   bool InIgnoredDirectory(RecordInfo* info) {
     string filename;
     if (!GetFilename(info->record()->getLocStart(), &filename))
       return false;  // TODO: should we ignore non-existing file locations?
     std::vector<string>::iterator it = options_.ignored_directories.begin();
     for (; it != options_.ignored_directories.end(); ++it)
       if (filename.find(*it) != string::npos)
         return true;
     return false;
   }
@@ skipping 380 matching lines @@
 
  private:
   BlinkGCPluginOptions options_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<BlinkGCPluginAction> X(
     "blink-gc-plugin",
     "Check Blink GC invariants");