Revert "blink_gc_plugin: Disallow Members for stack-allocated types"

This reverts commit 71932bd3.

Reason for revert:
This broke the stack_allocated.cpp test, see https://ci.chromium.org/p/chromium/builders/ci/ToTLinux/9408

Original change's description:
> 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}

TBR=haraken@chromium.org,mlippautz@chromium.org,bikineev@chromium.org

Change-Id: I3ae05fc50c49a3d668a3cb4d49c0fb010225861f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1044611
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2015342Reviewed-by: Hans Wennborg <hans@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734121}

tools/clang/blink_gc_plugin/BlinkGCPlugin.cpp

@@ -33,8 +33,6 @@ 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,9 +12,6 @@
 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,19 +78,6 @@ 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>.
@@ -101,23 +88,25 @@ 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() || Parent()->IsWeakMember()) &&
+  if (stack_allocated_host_ && Parent() && Parent()->IsMember() &&
       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>.
-  if (Parent()->IsUniquePtr() || Parent()->IsRefPtr()) {
+  // Disallow  unique_ptr<T>, RefPtr<T> and T* to stack-allocated types.
+  if (Parent()->IsUniquePtr() ||
+      Parent()->IsRefPtr() ||
+      (stack_allocated_host_ && Parent()->IsRawPtr())) {
     invalid_fields_.push_back(std::make_pair(
         current_, InvalidSmartPtr(Parent())));
     return;
   }
-  if (Parent()->IsRawPtr() && !stack_allocated_host_) {
+  if (Parent()->IsRawPtr()) {
     RawPtr* rawPtr = static_cast<RawPtr*>(Parent());
     Error error = rawPtr->HasReferenceType() ?
         kReferencePtrToGCManaged : kRawPtrToGCManaged;
@@ -131,6 +120,12 @@ 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,7 +30,6 @@ class CheckFieldsVisitor : public RecursiveEdgeVisitor {
     kPtrFromHeapToStack,
     kGCDerivedPartObject,
     kIteratorToGCManaged,
-    kMemberInStackAllocated,
   };
 
   using Errors = std::vector<std::pair<FieldPoint*, Error>>;

tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp

@@ -130,10 +130,6 @@ 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.";
@@ -207,8 +203,6 @@ 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(
@@ -340,8 +334,6 @@ 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,7 +139,6 @@ 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,9 +63,6 @@ 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,7 +23,8 @@ private:
 class StackObject {
     STACK_ALLOCATED();
 private:
-    HeapObject* m_obj; // OK
+    Member<HeapObject> m_obj; // OK
+    Member<OffHeapObject> m_memberOff; // NOT OK
     HeapVector<Member<OffHeapObject>> m_heapVectorMemberOff; // NOT OK
 };
 

tools/clang/blink_gc_plugin/tests/member_in_offheap_class.txt

@@ -11,7 +11,10 @@ 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_heapVectorMemberOff' to non-GC managed class declared here:
+./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:
     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; // Can't be Member.
+    Member<HeapObject> m_obj; // Does not need tracing.
 };
 
 class HeapObject : public GarbageCollected<HeapObject> {
@@ -49,14 +49,6 @@ 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,12 +8,6 @@ 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> {
 ^
@@ -32,4 +26,10 @@ class DerivedHeapObject2 : public HeapObject {
 ./heap/stubs.h:182:5: note: expanded from macro 'STACK_ALLOCATED'
     __attribute__((annotate("blink_stack_allocated")))      \
     ^
-8 warnings generated.
+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.