blink_gc_plugin: Disallow Members for stack-allocated types

Members in stack allocated types issue redundant write-barriers. This CL
provides an option --no-members-in-gc-allocated, which prohibits use of
them.

Bug: 1044611
Change-Id: Icc83756d9a1d6e414a09b705c0f008f9e79c4ae9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2010986
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734022}

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -33,6 +33,8 @@ class BlinkGCPluginAction : public PluginASTAction {
     for (const auto& arg : args) {
       if (arg == "dump-graph") {
         options_.dump_graph = true;
+      } else if (arg == "no-members-in-stack-allocated") {
+        options_.no_members_in_stack_allocated = true;
       } else if (arg == "enable-weak-members-in-unmanaged-classes") {
         options_.enable_weak_members_in_unmanaged_classes = true;
       } else if (arg == "no-gc-finalized" || arg == "warn-unneeded-finalizer") {

tools/clang/blink_gc_plugin/BlinkGCPluginOptions.h

@@ -12,6 +12,9 @@
 struct BlinkGCPluginOptions {
   bool dump_graph = false;
 
+  // If |true|, disallow Member types in stack allocated classes.
+  bool no_members_in_stack_allocated = false;
+
   // Member<T> fields are only permitted in managed classes,
   // something CheckFieldsVisitor verifies, issuing errors if
   // found in unmanaged classes. WeakMember<T> should be treated

tools/clang/blink_gc_plugin/CheckFieldsVisitor.cpp

@@ -78,6 +78,19 @@ void CheckFieldsVisitor::AtValue(Value* edge) {
     return;
   }
 
+  if (options_.no_members_in_stack_allocated) {
+    // Members/WeakMembers are prohibited if the host is stack allocated, but
+    // heap collections with Members are okay.
+    if (stack_allocated_host_ && Parent() &&
+        (Parent()->IsMember() || Parent()->IsWeakMember())) {
+      if (!GrandParent() || !GrandParent()->IsCollection()) {
+        invalid_fields_.push_back(
+            std::make_pair(current_, kMemberInStackAllocated));
+        return;
+      }
+    }
+  }
+
   // If in a stack allocated context, be fairly insistent that T in Member<T>
   // is GC allocated, as stack allocated objects do not have a trace()
   // that separately verifies the validity of Member<T>.
@@ -88,25 +101,23 @@ void CheckFieldsVisitor::AtValue(Value* edge) {
   //
   // (Note: Member<>'s constructor will at run-time verify that the
   // pointer it wraps is indeed heap allocated.)
-  if (stack_allocated_host_ && Parent() && Parent()->IsMember() &&
+  if (stack_allocated_host_ && Parent() &&
+      (Parent()->IsMember() || Parent()->IsWeakMember()) &&
       edge->value()->HasDefinition() && !edge->value()->IsGCAllocated()) {
-    invalid_fields_.push_back(std::make_pair(current_,
-                                             kMemberToGCUnmanaged));
+    invalid_fields_.push_back(std::make_pair(current_, kMemberToGCUnmanaged));
     return;
   }
 
   if (!Parent() || !edge->value()->IsGCAllocated())
     return;
 
-  // Disallow  unique_ptr<T>, RefPtr<T> and T* to stack-allocated types.
-  if (Parent()->IsUniquePtr() ||
-      Parent()->IsRefPtr() ||
-      (stack_allocated_host_ && Parent()->IsRawPtr())) {
+  // Disallow unique_ptr<T>, RefPtr<T>.
+  if (Parent()->IsUniquePtr() || Parent()->IsRefPtr()) {
     invalid_fields_.push_back(std::make_pair(
         current_, InvalidSmartPtr(Parent())));
     return;
   }
-  if (Parent()->IsRawPtr()) {
+  if (Parent()->IsRawPtr() && !stack_allocated_host_) {
     RawPtr* rawPtr = static_cast<RawPtr*>(Parent());
     Error error = rawPtr->HasReferenceType() ?
         kReferencePtrToGCManaged : kRawPtrToGCManaged;
@@ -120,12 +131,6 @@ void CheckFieldsVisitor::AtCollection(Collection* edge) {
 }
 
 CheckFieldsVisitor::Error CheckFieldsVisitor::InvalidSmartPtr(Edge* ptr) {
-  if (ptr->IsRawPtr()) {
-    if (static_cast<RawPtr*>(ptr)->HasReferenceType())
-      return kReferencePtrToGCManaged;
-    else
-      return kRawPtrToGCManaged;
-  }
   if (ptr->IsRefPtr())
     return kRefPtrToGCManaged;
   if (ptr->IsUniquePtr())

tools/clang/blink_gc_plugin/CheckFieldsVisitor.h

@@ -30,6 +30,7 @@ class CheckFieldsVisitor : public RecursiveEdgeVisitor {
     kPtrFromHeapToStack,
     kGCDerivedPartObject,
     kIteratorToGCManaged,
+    kMemberInStackAllocated,
   };
 
   using Errors = std::vector<std::pair<FieldPoint*, Error>>;

tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp

@@ -130,6 +130,10 @@ const char kTraceMethodOfStackAllocatedParentNote[] =
     "[blink-gc] The stack allocated class %0 provides an unnecessary "
     "trace method:";
 
+const char kMemberInStackAllocated[] =
+    "[blink-gc] Member field %0 in stack allocated class declared here (use "
+    "raw pointer or reference instead):";
+
 const char kUniquePtrUsedWithGC[] =
     "[blink-gc] Disallowed use of %0 found; %1 is a garbage-collected type. "
     "std::unique_ptr cannot hold garbage-collected objects.";
@@ -203,6 +207,8 @@ DiagnosticsReporter::DiagnosticsReporter(
       getErrorLevel(), kIteratorToGCManagedCollectionNote);
   diag_trace_method_of_stack_allocated_parent_ = diagnostic_.getCustomDiagID(
       getErrorLevel(), kTraceMethodOfStackAllocatedParentNote);
+  diag_member_in_stack_allocated_class_ =
+      diagnostic_.getCustomDiagID(getErrorLevel(), kMemberInStackAllocated);
 
   // Register note messages.
   diag_base_requires_tracing_note_ = diagnostic_.getCustomDiagID(
@@ -334,6 +340,8 @@ void DiagnosticsReporter::ClassContainsInvalidFields(
       note = diag_part_object_to_gc_derived_class_note_;
     } else if (error.second == CheckFieldsVisitor::kIteratorToGCManaged) {
       note = diag_iterator_to_gc_managed_collection_note_;
+    } else if (error.second == CheckFieldsVisitor::kMemberInStackAllocated) {
+      note = diag_member_in_stack_allocated_class_;
     } else {
       llvm_unreachable("Unknown field error.");
     }

tools/clang/blink_gc_plugin/DiagnosticsReporter.h

@@ -139,6 +139,7 @@ class DiagnosticsReporter {
   unsigned diag_manual_dispatch_method_note_;
   unsigned diag_iterator_to_gc_managed_collection_note_;
   unsigned diag_trace_method_of_stack_allocated_parent_;
+  unsigned diag_member_in_stack_allocated_class_;
 
   unsigned diag_unique_ptr_used_with_gc_;
   unsigned diag_optional_used_with_gc_;

tools/clang/blink_gc_plugin/Edge.h

@@ -63,6 +63,9 @@ class RecursiveEdgeVisitor : public EdgeVisitor {
   typedef std::deque<Edge*> Context;
   Context& context() { return context_; }
   Edge* Parent() { return context_.empty() ? 0 : context_.front(); }
+  Edge* GrandParent() {
+    return Parent() ? (context_.size() > 1 ? context_[1] : nullptr) : nullptr;
+  }
   void Enter(Edge* e) { return context_.push_front(e); }
   void Leave() { context_.pop_front(); }
 

tools/clang/blink_gc_plugin/tests/member_in_offheap_class.h

@@ -23,8 +23,7 @@ private:
 class StackObject {
     STACK_ALLOCATED();
 private:
-    Member<HeapObject> m_obj; // OK
-    Member<OffHeapObject> m_memberOff; // NOT OK
+    HeapObject* m_obj; // OK
     HeapVector<Member<OffHeapObject>> m_heapVectorMemberOff; // NOT OK
 };
 

tools/clang/blink_gc_plugin/tests/member_in_offheap_class.txt

@@ -11,10 +11,7 @@ class OffHeapObject {
 ./member_in_offheap_class.h:23:1: warning: [blink-gc] Class 'StackObject' contains invalid fields.
 class StackObject {
 ^
-./member_in_offheap_class.h:27:5: note: [blink-gc] Member field 'm_memberOff' to non-GC managed class declared here:
-    Member<OffHeapObject> m_memberOff; // NOT OK
-    ^
-./member_in_offheap_class.h:28:5: note: [blink-gc] Member field 'm_heapVectorMemberOff' to non-GC managed class declared here:
+./member_in_offheap_class.h:27:5: note: [blink-gc] Member field 'm_heapVectorMemberOff' to non-GC managed class declared here:
     HeapVector<Member<OffHeapObject>> m_heapVectorMemberOff; // NOT OK
     ^
 2 warnings generated.

tools/clang/blink_gc_plugin/tests/stack_allocated.h

@@ -24,7 +24,7 @@ class StackObject {
     void Trace(Visitor* visitor) { visitor->Trace(m_obj); }
 
 private:
-    Member<HeapObject> m_obj; // Does not need tracing.
+    Member<HeapObject> m_obj; // Can't be Member.
 };
 
 class HeapObject : public GarbageCollected<HeapObject> {
@@ -49,6 +49,14 @@ private:
     StackObject m_anotherPart; // Also fine.
 };
 
+class GoodStackObject {
+    STACK_ALLOCATED();
+
+public:
+    HeapObject* m_obj;
+    HeapObject& heap_ref;
+};
+
 }
 
 #endif

tools/clang/blink_gc_plugin/tests/stack_allocated.txt

@@ -8,6 +8,12 @@ class PartObject {
 ./stack_allocated.h:24:5: warning: [blink-gc] The stack allocated class 'StackObject' provides an unnecessary trace method:
     void Trace(Visitor* visitor) { visitor->Trace(m_obj); }
     ^
+./stack_allocated.h:20:1: warning: [blink-gc] Class 'StackObject' contains invalid fields.
+class StackObject {
+^
+./stack_allocated.h:27:5: warning: [blink-gc] Member field 'm_obj' in stack allocated class declared here (use raw pointer or reference instead):
+    Member<HeapObject> m_obj; // Can't be Member.
+    ^
 ./stack_allocated.h:30:1: warning: [blink-gc] Class 'HeapObject' contains invalid fields.
 class HeapObject : public GarbageCollected<HeapObject> {
 ^
@@ -26,10 +32,4 @@ class DerivedHeapObject2 : public HeapObject {
 ./heap/stubs.h:182:5: note: expanded from macro 'STACK_ALLOCATED'
     __attribute__((annotate("blink_stack_allocated")))      \
     ^
-stack_allocated.cpp:12:1: warning: [blink-gc] Class 'AnonStackObject' contains invalid fields.
-class AnonStackObject : public StackObject {
-^
-stack_allocated.cpp:14:5: note: [blink-gc] Raw pointer field 'm_obj' to a GC managed class declared here:
-    HeapObject* m_obj;
-    ^
-7 warnings generated.
+8 warnings generated.