Reland "heap: Fix clang plugin handling of cppgc library types"

This is a reland of 3d3cb2f878

Original change's description:
> heap: Fix clang plugin handling of cppgc library types
>
> Bug: 1056170
> Change-Id: I8a056a9a44e6b482e6d96df8c486b6d134fed46e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2976400
> Commit-Queue: Omer Katz <omerkatz@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#894876}

Bug: 1056170
Change-Id: Ibcfe9064edaa739badebffc3701b863f5cc359d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2982560
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#896116}

tools/clang/blink_gc_plugin/Config.h

@@ -14,6 +14,7 @@
 
 #include <cassert>
 
+#include "RecordInfo.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/Attr.h"
 
@@ -35,28 +36,78 @@ extern const char kConstReverseIteratorName[];
 extern const char kReverseIteratorName[];
 
 class Config {
+ private:
+  // Checks that the namespace matches the expected namespace and that the type
+  // takes at least |expected_minimum_arg_count| template arguments. If both
+  // requirements are fulfilled, populates |args| with the first
+  // |expected_minimum_arg_count| template arguments. Verifying only the minimum
+  // expected argument keeps the plugin resistant to changes in the type
+  // definitions (to some extent)
+  static bool VerifyNamespaceAndArgCount(std::string expected_ns_name,
+                                         int expected_minimum_arg_count,
+                                         llvm::StringRef ns_name,
+                                         RecordInfo* info,
+                                         RecordInfo::TemplateArgs* args) {
+    return (ns_name == expected_ns_name) &&
+           info->GetTemplateArgs(expected_minimum_arg_count, args);
+  }
+
  public:
-  static bool IsMember(llvm::StringRef name) {
-    return name == "Member";
+  static bool IsMember(llvm::StringRef name,
+                       llvm::StringRef ns_name,
+                       RecordInfo* info,
+                       RecordInfo::TemplateArgs* args) {
+    if (name == "Member") {
+      return VerifyNamespaceAndArgCount("blink", 1, ns_name, info, args);
+    }
+    if (name == "BasicMember") {
+      if (!VerifyNamespaceAndArgCount("cppgc", 2, ns_name, info, args))
+        return false;
+      return (*args)[1]->getAsRecordDecl()->getName() == "StrongMemberTag";
+    }
+    return false;
   }
 
-  static bool IsWeakMember(llvm::StringRef name) {
-    return name == "WeakMember";
+  static bool IsWeakMember(llvm::StringRef name,
+                           llvm::StringRef ns_name,
+                           RecordInfo* info,
+                           RecordInfo::TemplateArgs* args) {
+    if (name == "WeakMember") {
+      return VerifyNamespaceAndArgCount("blink", 1, ns_name, info, args);
+    }
+    if (name == "BasicMember") {
+      if (!VerifyNamespaceAndArgCount("cppgc", 2, ns_name, info, args))
+        return false;
+      return (*args)[1]->getAsRecordDecl()->getName() == "WeakMemberTag";
+    }
+    return false;
   }
 
-  static bool IsMemberHandle(llvm::StringRef name) {
-    return IsMember(name) ||
-           IsWeakMember(name);
+  static bool IsPersistent(llvm::StringRef name,
+                           llvm::StringRef ns_name,
+                           RecordInfo* info,
+                           RecordInfo::TemplateArgs* args) {
+    if ((name == "Persistent") || (name == "WeakPersistent")) {
+      return VerifyNamespaceAndArgCount("blink", 1, ns_name, info, args);
+    }
+    if (name == "BasicPersistent") {
+      return VerifyNamespaceAndArgCount("cppgc", 1, ns_name, info, args);
+    }
+    return false;
   }
 
-  static bool IsPersistent(llvm::StringRef name) {
-    return name == "Persistent" ||
-           name == "WeakPersistent" ;
-  }
-
-  static bool IsCrossThreadPersistent(llvm::StringRef name) {
-    return name == "CrossThreadPersistent" ||
-           name == "CrossThreadWeakPersistent" ;
+  static bool IsCrossThreadPersistent(llvm::StringRef name,
+                                      llvm::StringRef ns_name,
+                                      RecordInfo* info,
+                                      RecordInfo::TemplateArgs* args) {
+    if ((name == "CrossThreadPersistent") ||
+        (name == "CrossThreadWeakPersistent")) {
+      return VerifyNamespaceAndArgCount("blink", 1, ns_name, info, args);
+    }
+    if (name == "BasicCrossThreadPersistent") {
+      return VerifyNamespaceAndArgCount("cppgc", 1, ns_name, info, args);
+    }
+    return false;
   }
 
   static bool IsRefPtr(llvm::StringRef name) { return name == "scoped_refptr"; }
@@ -71,8 +122,12 @@ class Config {
     return name == "unique_ptr";
   }
 
-  static bool IsTraceWrapperV8Reference(llvm::StringRef name) {
-    return name == "TraceWrapperV8Reference";
+  static bool IsTraceWrapperV8Reference(llvm::StringRef name,
+                                        llvm::StringRef ns_name,
+                                        RecordInfo* info,
+                                        RecordInfo::TemplateArgs* args) {
+    return name == "TraceWrapperV8Reference" &&
+           VerifyNamespaceAndArgCount("blink", 1, ns_name, info, args);
   }
 
   static bool IsWTFCollection(llvm::StringRef name) {

tools/clang/blink_gc_plugin/RecordInfo.cpp

@@ -686,33 +686,32 @@ Edge* RecordInfo::CreateEdge(const Type* type) {
     return 0;
   }
 
-  if (Config::IsMember(info->name()) && info->GetTemplateArgs(1, &args)) {
-    if (Edge* ptr = CreateEdge(args[0]))
+  // Find top-level namespace.
+  NamespaceDecl* ns = dyn_cast<NamespaceDecl>(info->record()->getDeclContext());
+  if (ns) {
+    while (NamespaceDecl* outer_ns =
+               dyn_cast<NamespaceDecl>(ns->getDeclContext())) {
+      ns = outer_ns;
+    }
+  }
+  auto ns_name = ns ? ns->getName() : "";
+
+  if (Config::IsMember(info->name(), ns_name, info, &args)) {
+    if (Edge* ptr = CreateEdge(args[0])) {
       return new Member(ptr);
+    }
     return 0;
   }
 
-  if (Config::IsWeakMember(info->name()) && info->GetTemplateArgs(1, &args)) {
+  if (Config::IsWeakMember(info->name(), ns_name, info, &args)) {
     if (Edge* ptr = CreateEdge(args[0]))
       return new WeakMember(ptr);
     return 0;
   }
 
-  bool is_persistent = Config::IsPersistent(info->name());
-  if (is_persistent || Config::IsCrossThreadPersistent(info->name())) {
-    // Persistent might refer to v8::Persistent, so check the name space.
-    // TODO: Consider using a more canonical identification than names.
-    NamespaceDecl* ns =
-        dyn_cast<NamespaceDecl>(info->record()->getDeclContext());
-    // Find outer-most namespace.
-    while (NamespaceDecl* outer_ns =
-               dyn_cast<NamespaceDecl>(ns->getDeclContext())) {
-      ns = outer_ns;
-    }
-    if (!ns || (ns->getName() != "blink") && (ns->getName() != "cppgc"))
-      return 0;
-    if (!info->GetTemplateArgs(1, &args))
-      return 0;
+  bool is_persistent = Config::IsPersistent(info->name(), ns_name, info, &args);
+  if (is_persistent ||
+      Config::IsCrossThreadPersistent(info->name(), ns_name, info, &args)) {
     if (Edge* ptr = CreateEdge(args[0])) {
       if (is_persistent)
         return new Persistent(ptr);
@@ -739,8 +738,7 @@ Edge* RecordInfo::CreateEdge(const Type* type) {
     return edge;
   }
 
-  if (Config::IsTraceWrapperV8Reference(info->name()) &&
-      info->GetTemplateArgs(1, &args)) {
+  if (Config::IsTraceWrapperV8Reference(info->name(), ns_name, info, &args)) {
     if (Edge* ptr = CreateEdge(args[0]))
       return new TraceWrapperV8Reference(ptr);
     return 0;

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

@@ -196,6 +196,44 @@ class Visitor {
 
 namespace internal {
 class GarbageCollectedBase {};
+
+class StrongMemberTag;
+class WeakMemberTag;
+
+class MemberBase {};
+
+template <typename T, typename Tag>
+class BasicMember : public MemberBase {
+ public:
+  operator T*() const { return 0; }
+  T* operator->() const { return 0; }
+  bool operator!() const { return false; }
+};
+
+class StrongPersistentPolicy;
+class WeakPersistentPolicy;
+
+class PersistentBase {};
+
+template <typename T, typename Tag>
+class BasicPersistent : public PersistentBase {
+ public:
+  operator T*() const { return 0; }
+  T* operator->() const { return 0; }
+  bool operator!() const { return false; }
+};
+
+class StrongCrossThreadPersistentPolicy;
+class WeakCrossThreadPersistentPolicy;
+
+template <typename T, typename Tag>
+class BasicCrossThreadPersistent : public PersistentBase {
+ public:
+  operator T*() const { return 0; }
+  T* operator->() const { return 0; }
+  bool operator!() const { return false; }
+};
+
 }  // namespace internal
 
 template <typename T>
@@ -209,54 +247,25 @@ class GarbageCollectedMixin : public internal::GarbageCollectedBase {
 };
 
 template <typename T>
-class Member {
- public:
-  operator T*() const { return 0; }
-  T* operator->() const { return 0; }
-  bool operator!() const { return false; }
-};
+using Member = internal::BasicMember<T, internal::StrongMemberTag>;
+template <typename T>
+using WeakMember = internal::BasicMember<T, internal::WeakMemberTag>;
 
 template <typename T>
-class WeakMember {
- public:
-  operator T*() const { return 0; }
-  T* operator->() const { return 0; }
-  bool operator!() const { return false; }
-};
-
+using Persistent =
+    internal::BasicPersistent<T, internal::StrongPersistentPolicy>;
 template <typename T>
-class Persistent {
- public:
-  operator T*() const { return 0; }
-  T* operator->() const { return 0; }
-  bool operator!() const { return false; }
-};
-
-template <typename T>
-class WeakPersistent {
- public:
-  operator T*() const { return 0; }
-  T* operator->() const { return 0; }
-  bool operator!() const { return false; }
-};
+using WeakPersistent =
+    internal::BasicPersistent<T, internal::WeakPersistentPolicy>;
 
 namespace subtle {
 
 template <typename T>
-class CrossThreadPersistent {
- public:
-  operator T*() const { return 0; }
-  T* operator->() const { return 0; }
-  bool operator!() const { return false; }
-};
-
+using CrossThreadPersistent = internal::
+    BasicCrossThreadPersistent<T, internal::StrongCrossThreadPersistentPolicy>;
 template <typename T>
-class CrossThreadWeakPersistent {
- public:
-  operator T*() const { return 0; }
-  T* operator->() const { return 0; }
-  bool operator!() const { return false; }
-};
+using CrossThreadWeakPersistent = internal::
+    BasicCrossThreadPersistent<T, internal::WeakCrossThreadPersistentPolicy>;
 
 }  // namespace subtle
 

tools/clang/blink_gc_plugin/tests/stack_allocated.txt

@@ -23,7 +23,7 @@ class DerivedHeapObject2 : public HeapObject {
 ./stack_allocated.h:44:3: warning: [blink-gc] Garbage collected class 'DerivedHeapObject2' is not permitted to override its new operator.
   STACK_ALLOCATED();
   ^
-./heap/stubs.h:362:3: note: expanded from macro 'STACK_ALLOCATED'
+./heap/stubs.h:371:3: note: expanded from macro 'STACK_ALLOCATED'
   __attribute__((annotate("blink_stack_allocated"))) \
   ^
 In file included from stack_allocated.cpp:5:

tools/clang/blink_gc_plugin/tests/trace_if_needed.cppgc.txt

@@ -1,4 +1,4 @@
-trace_if_needed.cpp:9:1: warning: [blink-gc] Class 'TemplatedObject<cppgc::Member<blink::HeapObject>>' has untraced fields that require tracing.
+trace_if_needed.cpp:9:1: warning: [blink-gc] Class 'TemplatedObject<cppgc::internal::BasicMember<blink::HeapObject, cppgc::internal::StrongMemberTag>>' has untraced fields that require tracing.
 template <typename T>
 ^
 ./trace_if_needed.h:21:5: note: [blink-gc] Untraced field 'm_two' declared here: