Augment the blink gc plugin to detect missing USING_GARBAGE_COLLECTED_MIXIN.

It's a common and difficult to debug pitfall when you derive a GC mixin
but fail to use this macro. It leads to confusing crashes during tracing.

With this CL, the compiler will emit a warning like this:

  In file included from ../../third_party/blink/renderer/core/html/portal/document_portals.cc:5:
  ../../third_party/blink/renderer/core/html/portal/document_portals.h:17:25: error: [blink-gc] Garbage-collected class 'DocumentPortals' derives mixin class 'Supplement<blink::Document>'. You must add USING_GARBAGE_COLLECTED_MIXIN(DocumentPortals).
                          public Supplement<Document> {
                          ~~~~~~~^~~~~~~~~~~~~~~~~~~~
  ../../third_party/blink/renderer/platform/supplementable.h:119:27: note: [blink-gc] Mixin base class derived here:
  class Supplement : public GarbageCollectedMixin {
                     ~~~~~~~^~~~~~~~~~~~~~~~~~~~~
  1 error generated.

Bug: 869498
Change-Id: I93454c418d53ebe0fcd411e0a829597dd9aaf925
Reviewed-on: https://chromium-review.googlesource.com/c/1340784
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609280}

tools/clang/blink_gc_plugin/BadPatternFinder.cpp

@@ -5,6 +5,7 @@
 #include "BadPatternFinder.h"
 #include "DiagnosticsReporter.h"
 
+#include <algorithm>
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
@@ -82,6 +83,62 @@ class OptionalGarbageCollectedMatcher : public MatchFinder::MatchCallback {
   DiagnosticsReporter& diagnostics_;
 };
 
+class MissingMixinMarker : public MatchFinder::MatchCallback {
+ public:
+  explicit MissingMixinMarker(clang::ASTContext& ast_context,
+                              DiagnosticsReporter& diagnostics)
+      : ast_context_(ast_context), diagnostics_(diagnostics) {}
+
+  void Register(MatchFinder& match_finder) {
+    auto class_missing_mixin_marker = cxxRecordDecl(
+        decl().bind("bad_class"),
+        // Definition of a garbage-collected class
+        isDefinition(),
+        isDerivedFrom(cxxRecordDecl(decl().bind("gc_base_class"),
+                                    hasName("::blink::GarbageCollected"))),
+        // ...which derives some mixin...
+        isDerivedFrom(cxxRecordDecl(decl().bind("mixin_base_class"),
+                                    hasName("::blink::GarbageCollectedMixin"))),
+        // ...and doesn't use USING_GARBAGE_COLLECTED_MIXIN
+        unless(isSameOrDerivedFrom(
+            has(fieldDecl(hasName("mixin_constructor_marker_"))))),
+        // ...and might end up actually being constructed
+        unless(hasMethod(isPure())), unless(matchesName("::SameSizeAs")));
+    match_finder.addDynamicMatcher(class_missing_mixin_marker, this);
+  }
+
+  void run(const MatchFinder::MatchResult& result) {
+    auto* bad_class = result.Nodes.getNodeAs<clang::CXXRecordDecl>("bad_class");
+    auto* gc_base_class =
+        result.Nodes.getNodeAs<clang::CXXRecordDecl>("gc_base_class");
+    auto* mixin_base_class =
+        result.Nodes.getNodeAs<clang::CXXRecordDecl>("mixin_base_class");
+
+    clang::CXXBasePaths paths;
+    if (!bad_class->isDerivedFrom(mixin_base_class, paths))
+      return;
+    const auto& path = paths.front();
+
+    // It's most useful to describe the most derived "mixin" class (i.e. which
+    // does not derive the concrete GarbageCollected base).
+    auto mixin_it = std::find_if(
+        path.begin(), path.end(),
+        [gc_base_class](const clang::CXXBasePathElement& path_element) {
+          return !path_element.Class->isDerivedFrom(gc_base_class);
+        });
+    const clang::CXXRecordDecl* mixin_class = mixin_it->Class;
+    diagnostics_.MissingMixinMarker(bad_class, mixin_class, path.begin()->Base);
+
+    ++mixin_it;
+    for (auto it = path.begin() + 1; it != mixin_it; ++it)
+      diagnostics_.MissingMixinMarkerNote(it->Base);
+  }
+
+ private:
+  clang::ASTContext& ast_context_;
+  DiagnosticsReporter& diagnostics_;
+};
+
 }  // namespace
 
 void FindBadPatterns(clang::ASTContext& ast_context,
@@ -94,5 +151,8 @@ void FindBadPatterns(clang::ASTContext& ast_context,
   OptionalGarbageCollectedMatcher optional_gc(diagnostics);
   optional_gc.Register(match_finder);
 
+  MissingMixinMarker missing_mixin_marker(ast_context, diagnostics);
+  missing_mixin_marker.Register(match_finder);
+
   match_finder.matchAST(ast_context);
 }

tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp

@@ -162,6 +162,13 @@ const char kOptionalUsedWithGC[] =
     "[blink-gc] Disallowed construction of %0 found; %1 is a garbage-collected "
     "type. optional cannot hold garbage-collected objects.";
 
+const char kMissingMixinMarker[] =
+    "[blink-gc] Garbage-collected class %0 derives mixin class %1. "
+    "You must add USING_GARBAGE_COLLECTED_MIXIN(%2).";
+
+const char kMissingMixinMarkerNote[] =
+    "[blink-gc] Mixin base class derived here:";
+
 } // namespace
 
 DiagnosticBuilder DiagnosticsReporter::ReportDiagnostic(
@@ -275,6 +282,10 @@ DiagnosticsReporter::DiagnosticsReporter(
       diagnostic_.getCustomDiagID(getErrorLevel(), kUniquePtrUsedWithGC);
   diag_optional_used_with_gc_ =
       diagnostic_.getCustomDiagID(getErrorLevel(), kOptionalUsedWithGC);
+  diag_missing_mixin_marker_ =
+      diagnostic_.getCustomDiagID(getErrorLevel(), kMissingMixinMarker);
+  diag_missing_mixin_marker_note_ = diagnostic_.getCustomDiagID(
+      DiagnosticsEngine::Note, kMissingMixinMarkerNote);
 }
 
 bool DiagnosticsReporter::hasErrorOccurred() const
@@ -600,3 +611,18 @@ void DiagnosticsReporter::OptionalUsedWithGC(
   ReportDiagnostic(expr->getBeginLoc(), diag_optional_used_with_gc_)
       << optional << gc_type << expr->getSourceRange();
 }
+
+void DiagnosticsReporter::MissingMixinMarker(
+    const clang::CXXRecordDecl* bad_class,
+    const clang::CXXRecordDecl* mixin_class,
+    const clang::CXXBaseSpecifier* first_base) {
+  ReportDiagnostic(first_base->getBaseTypeLoc(), diag_missing_mixin_marker_)
+      << bad_class << mixin_class << bad_class->getName()
+      << first_base->getSourceRange();
+}
+
+void DiagnosticsReporter::MissingMixinMarkerNote(
+    const clang::CXXBaseSpecifier* base) {
+  ReportDiagnostic(base->getBaseTypeLoc(), diag_missing_mixin_marker_note_)
+      << base->getSourceRange();
+}

tools/clang/blink_gc_plugin/DiagnosticsReporter.h

@@ -86,6 +86,10 @@ class DiagnosticsReporter {
   void OptionalUsedWithGC(const clang::Expr* expr,
                           const clang::CXXRecordDecl* optional,
                           const clang::CXXRecordDecl* gc_type);
+  void MissingMixinMarker(const clang::CXXRecordDecl* bad_class,
+                          const clang::CXXRecordDecl* mixin_class,
+                          const clang::CXXBaseSpecifier* first_base);
+  void MissingMixinMarkerNote(const clang::CXXBaseSpecifier* base);
 
  private:
   clang::DiagnosticBuilder ReportDiagnostic(
@@ -150,6 +154,8 @@ class DiagnosticsReporter {
 
   unsigned diag_unique_ptr_used_with_gc_;
   unsigned diag_optional_used_with_gc_;
+  unsigned diag_missing_mixin_marker_;
+  unsigned diag_missing_mixin_marker_note_;
 };
 
 #endif // TOOLS_BLINK_GC_PLUGIN_DIAGNOSTICS_REPORTER_H_

tools/clang/blink_gc_plugin/tests/heap/stubs.h

@@ -192,10 +192,11 @@ using namespace WTF;
 #define GC_PLUGIN_IGNORE(bug)                           \
     __attribute__((annotate("blink_gc_plugin_ignore")))
 
-#define USING_GARBAGE_COLLECTED_MIXIN(type)                     \
-public:                                                         \
-    virtual void AdjustAndMark(Visitor*) const override { }     \
-    virtual bool IsHeapObjectAlive(Visitor*) const override { return 0; }
+#define USING_GARBAGE_COLLECTED_MIXIN(type)                             \
+ public:                                                                \
+  virtual void AdjustAndMark(Visitor*) const override {}                \
+  virtual bool IsHeapObjectAlive(Visitor*) const override { return 0; } \
+  void* mixin_constructor_marker_;
 
 #define EAGERLY_FINALIZED() typedef int IsEagerlyFinalizedMarker
 

tools/clang/blink_gc_plugin/tests/missing_mixin_marker.cpp

+// Copyright 2018 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 "heap/stubs.h"
+
+namespace blink {
+
+class Mixin : public GarbageCollectedMixin {
+ public:
+  virtual void Trace(Visitor*) {}
+};
+
+// Class derived from a mixin needs USING_GARBAGE_COLLECTED_MIXIN.
+class Derived : public GarbageCollected<Derived>, public Mixin {
+  virtual void Trace(Visitor* visitor) override { Mixin::Trace(visitor); }
+};
+
+template <typename T>
+class Supplement : public GarbageCollectedMixin {
+ public:
+  virtual void Trace(Visitor*) {}
+};
+
+// Class derived from a mixin template needs USING_GARBAGE_COLLECTED_MIXIN.
+class MySupplement : public GarbageCollectedFinalized<MySupplement>,
+                     public Supplement<Derived> {
+  virtual void Trace(Visitor* visitor) override { Supplement::Trace(visitor); }
+};
+
+// This is the right way to do it.
+// We should get no warning either here or at a forward declaration.
+class GoodDerived : public GarbageCollected<GoodDerived>, public Mixin {
+  USING_GARBAGE_COLLECTED_MIXIN(GoodDerived);
+};
+class GoodDerived;
+
+// Abstract classes (i.e. ones with pure virtual methods) can't be constructed
+// and so it's assumed their derived classes will have
+// USING_GARBAGE_COLLECTED_MIXIN.
+class PureVirtual : public GarbageCollected<PureVirtual>, public Mixin {
+ public:
+  virtual void Foo() = 0;
+};
+// ...but failure to do so is still bad. Warn here.
+class PureVirtualDerived : public PureVirtual {
+ public:
+  void Foo() override {}
+};
+
+// And there's an exception for "same size" classes, which are just used for
+// assertions. This should not warn.
+class SameSizeAsGoodDerived : public GarbageCollected<SameSizeAsGoodDerived>,
+                              public Mixin {
+  char same_size_as_mixin_marker_[1];
+};
+
+}  // namespace blink

tools/clang/blink_gc_plugin/tests/missing_mixin_marker.txt

+missing_mixin_marker.cpp:15:58: warning: [blink-gc] Garbage-collected class 'Derived' derives mixin class 'Mixin'. You must add USING_GARBAGE_COLLECTED_MIXIN(Derived).
+class Derived : public GarbageCollected<Derived>, public Mixin {
+                                                  ~~~~~~~^~~~~
+missing_mixin_marker.cpp:9:22: note: [blink-gc] Mixin base class derived here:
+class Mixin : public GarbageCollectedMixin {
+              ~~~~~~~^~~~~~~~~~~~~~~~~~~~~
+missing_mixin_marker.cpp:27:29: warning: [blink-gc] Garbage-collected class 'MySupplement' derives mixin class 'Supplement<blink::Derived>'. You must add USING_GARBAGE_COLLECTED_MIXIN(MySupplement).
+                     public Supplement<Derived> {
+                     ~~~~~~~^~~~~~~~~~~~~~~~~~~
+missing_mixin_marker.cpp:20:27: note: [blink-gc] Mixin base class derived here:
+class Supplement : public GarbageCollectedMixin {
+                   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~
+missing_mixin_marker.cpp:46:35: warning: [blink-gc] Garbage-collected class 'PureVirtualDerived' derives mixin class 'Mixin'. You must add USING_GARBAGE_COLLECTED_MIXIN(PureVirtualDerived).
+class PureVirtualDerived : public PureVirtual {
+                           ~~~~~~~^~~~~~~~~~~
+missing_mixin_marker.cpp:41:66: note: [blink-gc] Mixin base class derived here:
+class PureVirtual : public GarbageCollected<PureVirtual>, public Mixin {
+                                                          ~~~~~~~^~~~~
+missing_mixin_marker.cpp:9:22: note: [blink-gc] Mixin base class derived here:
+class Mixin : public GarbageCollectedMixin {
+              ~~~~~~~^~~~~~~~~~~~~~~~~~~~~
+3 warnings generated.