Revert "Reland "[compiler] Consider IsPendingAllocation in Ref construction""

This reverts commit 4683d6fe.

Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN/36744/overview


Original change's description:
> Reland "[compiler] Consider IsPendingAllocation in Ref construction"
>
> This is a reland of 5f0ac36c
>
> Fixes Ref construction failures in:
> - MapRef::instance_descriptors
> - NativeContext reads (see also crrev.com/c/2891575)
>
> Original change's description:
> > [compiler] Consider IsPendingAllocation in Ref construction
> >
> > The logic in JSHeapBroker::TryGetOrCreateData assumes that parts
> > of the object are safe to read. In particular, the instance type
> > must be readable for the chain of `Is##Name()` type checks.
> >
> > This is guaranteed if
> >
> >  - a global memory fence happened after object initialization and
> >    prior to the read by the compiler; or
> >  - the object was published through a release store and read through
> >    an acquire read.
> >
> > The former is protected by the new call to ObjectMayBeUninitialized
> > (which internally calls IsPendingAllocation) in TryGetOrCreateData.
> >
> > The latter must be marked explicitly by calling the new
> > MakeRefAssumeMemoryFence variant.
> >
> > Note that support in this CL is expected to be incomplete and will
> > have to be extended in the future as more cases show up in which
> > MakeRef calls must be converted to MakeRefAssumeMemoryFence or to
> > TryMakeRef.
> >
> > Bug: v8:7790,v8:11711
> > Change-Id: Ic2f7d9fc46e4bfc3f6bbe42816f73fc5ec174337
> > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2874663
> > Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> > Reviewed-by: Georg Neis <neis@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#74474}
>
> Bug: v8:7790,v8:11711,chromium:1207680,chromium:1207679
> Change-Id: Ib3dbf59909e6982a3230dd6a67c9fb7d6ffb9ab4
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2886861
> Reviewed-by: Georg Neis <neis@chromium.org>
> Commit-Queue: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#74587}

Bug: v8:7790
Bug: v8:11711
Bug: chromium:1207680
Bug: chromium:1207679
Change-Id: I8cd45ac006b7b5f3d668d0df272bcba880c75926
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2901990Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74621}

src/compiler/heap-refs.cc

@@ -70,11 +70,8 @@ enum ObjectDataKind {
 
 namespace {
 
-bool IsReadOnlyHeapObjectForCompiler(HeapObject object) {
+bool IsReadOnlyHeapObject(Object object) {
   DisallowGarbageCollection no_gc;
-  // TODO(jgruber): Remove this compiler-specific predicate and use the plain
-  // heap predicate instead. This would involve removing the special cases for
-  // builtins.
   return (object.IsCode() && Code::cast(object).is_builtin()) ||
          (object.IsHeapObject() &&
           ReadOnlyHeap::Contains(HeapObject::cast(object)));
@@ -116,8 +113,7 @@ class ObjectData : public ZoneObject {
                       kind == kNeverSerializedHeapObject ||
                       kind == kBackgroundSerializedHeapObject);
     CHECK_IMPLIES(kind == kUnserializedReadOnlyHeapObject,
-                  object->IsHeapObject() && IsReadOnlyHeapObjectForCompiler(
-                                                HeapObject::cast(*object)));
+                  IsReadOnlyHeapObject(*object));
   }
 
 #define DECLARE_IS(Name, ...) bool Is##Name() const;
@@ -338,7 +334,7 @@ bool PropertyCellData::Serialize(JSHeapBroker* broker) {
     }
   }
 
-  ObjectData* value_data = broker->TryGetOrCreateData(value);
+  ObjectData* value_data = broker->TryGetOrCreateData(value, false);
   if (value_data == nullptr) {
     DCHECK(!broker->IsMainThread());
     return false;
@@ -2121,7 +2117,7 @@ base::Optional<PropertyCellRef> GetPropertyCellFromHeap(JSHeapBroker* broker,
   it.TryLookupCachedProperty();
   if (it.state() == LookupIterator::DATA &&
       it.GetHolder<JSObject>()->IsJSGlobalObject()) {
-    return TryMakeRef(broker, it.GetPropertyCell());
+    return MakeRef(broker, it.GetPropertyCell());
   }
   return base::nullopt;
 }
@@ -2756,13 +2752,13 @@ void JSHeapBroker::ClearReconstructibleData() {
 }
 
 ObjectData* JSHeapBroker::TryGetOrCreateData(Handle<Object> object,
-                                             GetOrCreateDataFlags flags) {
+                                             bool crash_on_error) {
   RefsMap::Entry* entry = refs_->Lookup(object.address());
   if (entry != nullptr) return entry->value;
 
   if (mode() == JSHeapBroker::kDisabled) {
     entry = refs_->LookupOrInsert(object.address());
-    ObjectData** storage = &entry->value;
+    ObjectData** storage = &(entry->value);
     if (*storage == nullptr) {
       entry->value = zone()->New<ObjectData>(
           this, storage, object,
@@ -2777,37 +2773,23 @@ ObjectData* JSHeapBroker::TryGetOrCreateData(Handle<Object> object,
   ObjectData* object_data;
   if (object->IsSmi()) {
     entry = refs_->LookupOrInsert(object.address());
-    return zone()->New<ObjectData>(this, &entry->value, object, kSmi);
-  }
-
-  DCHECK(!object->IsSmi());
-
-  const bool crash_on_error = (flags & kCrashOnError) != 0;
-  if ((flags & kAssumeMemoryFence) == 0 &&
-      ObjectMayBeUninitialized(HeapObject::cast(*object))) {
-    TRACE_BROKER_MISSING(this, "Object may be uninitialized " << *object);
-    CHECK_WITH_MSG(!crash_on_error, "Ref construction failed");
-    return nullptr;
-  }
-
-  if (IsReadOnlyHeapObjectForCompiler(HeapObject::cast(*object))) {
+    object_data = zone()->New<ObjectData>(this, &(entry->value), object, kSmi);
+  } else if (IsReadOnlyHeapObject(*object)) {
     entry = refs_->LookupOrInsert(object.address());
-    return zone()->New<ObjectData>(this, &entry->value, object,
-                                   kUnserializedReadOnlyHeapObject);
-  }
-
-#define CREATE_DATA(Name, Kind)                                   \
-  if (object->Is##Name()) {                                       \
-    CreateDataFunctor<Kind, Name##Data, Name> f;                  \
-    if (!f(this, refs_, object, &entry, &object_data)) {          \
-      CHECK_WITH_MSG(!crash_on_error, "Ref construction failed"); \
-      return nullptr;                                             \
-    }                                                             \
-    /* NOLINTNEXTLINE(readability/braces) */                      \
-  } else
-  HEAP_BROKER_OBJECT_LIST(CREATE_DATA)
+    object_data = zone()->New<ObjectData>(this, &(entry->value), object,
+                                          kUnserializedReadOnlyHeapObject);
+#define CREATE_DATA(Name, Kind)                          \
+  }                                                      \
+  /* NOLINTNEXTLINE(readability/braces) */               \
+  else if (object->Is##Name()) {                         \
+    CreateDataFunctor<Kind, Name##Data, Name> f;         \
+    if (!f(this, refs_, object, &entry, &object_data)) { \
+      CHECK(!crash_on_error);                            \
+      return nullptr;                                    \
+    }
+    HEAP_BROKER_OBJECT_LIST(CREATE_DATA)
 #undef CREATE_DATA
-  {
+  } else {
     UNREACHABLE();
   }
   // At this point the entry pointer is not guaranteed to be valid as
@@ -3345,7 +3327,7 @@ base::Optional<CallHandlerInfoRef> FunctionTemplateInfoRef::call_code() const {
   if (data_->should_access_heap()) {
     HeapObject call_code = object()->call_code(kAcquireLoad);
     if (call_code.IsUndefined()) return base::nullopt;
-    return TryMakeRef(broker(), CallHandlerInfo::cast(call_code));
+    return MakeRef(broker(), CallHandlerInfo::cast(call_code));
   }
   ObjectData* call_code = data()->AsFunctionTemplateInfo()->call_code();
   if (!call_code) return base::nullopt;
@@ -3517,9 +3499,8 @@ base::Optional<ObjectRef> MapRef::GetStrongValue(
 
 DescriptorArrayRef MapRef::instance_descriptors() const {
   if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
-    return MakeRefAssumeMemoryFence(
-        broker(),
-        object()->instance_descriptors(broker()->isolate(), kAcquireLoad));
+    return MakeRef(broker(), object()->instance_descriptors(broker()->isolate(),
+                                                            kRelaxedLoad));
   }
 
   return DescriptorArrayRef(broker(), data()->AsMap()->instance_descriptors());
@@ -3689,8 +3670,7 @@ bool NativeContextRef::is_unserialized_heap_object() const {
 
 ScopeInfoRef NativeContextRef::scope_info() const {
   if (data_->should_access_heap()) {
-    // The scope_info is immutable after initialization.
-    return MakeRefAssumeMemoryFence(broker(), object()->scope_info());
+    return MakeRef(broker(), object()->scope_info());
   }
   return ScopeInfoRef(broker(), data()->AsNativeContext()->scope_info());
 }
@@ -3708,10 +3688,7 @@ MapRef NativeContextRef::GetFunctionMapFromIndex(int index) const {
   DCHECK_GE(index, Context::FIRST_FUNCTION_MAP_INDEX);
   DCHECK_LE(index, Context::LAST_FUNCTION_MAP_INDEX);
   if (data_->should_access_heap()) {
-    CHECK_LT(index, object()->length());
-    return MakeRefAssumeMemoryFence(broker(),
-                                    object()->get(index, kAcquireLoad))
-        .AsMap();
+    return get(index).value().AsMap();
   }
   return MapRef(broker(), data()->AsNativeContext()->function_maps().at(
                               index - Context::FIRST_FUNCTION_MAP_INDEX));
@@ -3951,7 +3928,7 @@ base::Optional<ObjectRef> JSArrayRef::GetOwnCowElement(
 
 base::Optional<CellRef> SourceTextModuleRef::GetCell(int cell_index) const {
   if (data_->should_access_heap()) {
-    return TryMakeRef(broker(), object()->GetCell(cell_index));
+    return MakeRef(broker(), object()->GetCell(cell_index));
   }
   ObjectData* cell =
       data()->AsSourceTextModule()->GetCell(broker(), cell_index);
@@ -3971,7 +3948,12 @@ ObjectRef::ObjectRef(JSHeapBroker* broker, Handle<Object> object,
                      bool check_type)
     : broker_(broker) {
   CHECK_NE(broker->mode(), JSHeapBroker::kRetired);
+
   data_ = broker->GetOrCreateData(object);
+  if (!data_) {  // TODO(mslekova): Remove once we're on the background thread.
+    object->Print();
+  }
+  CHECK_WITH_MSG(data_ != nullptr, "Object is not known to the heap broker");
 }
 
 namespace {
@@ -4018,10 +4000,9 @@ HeapObjectType HeapObjectRef::GetHeapObjectType() const {
   if (map().is_callable()) flags |= HeapObjectType::kCallable;
   return HeapObjectType(map().instance_type(), flags, map().oddball_type());
 }
-
 base::Optional<JSObjectRef> AllocationSiteRef::boilerplate() const {
   if (data_->should_access_heap()) {
-    return TryMakeRef(broker(), object()->boilerplate(kAcquireLoad));
+    return MakeRef(broker(), object()->boilerplate(kAcquireLoad));
   }
   ObjectData* boilerplate = data()->AsAllocationSite()->boilerplate();
   if (boilerplate) {
@@ -4037,7 +4018,7 @@ ElementsKind JSObjectRef::GetElementsKind() const {
 
 base::Optional<FixedArrayBaseRef> JSObjectRef::elements() const {
   if (data_->should_access_heap()) {
-    return TryMakeRef(broker(), object()->elements());
+    return MakeRef(broker(), object()->elements());
   }
   const JSObjectData* d = data()->AsJSObject();
   if (!d->serialized_elements()) {
@@ -4240,12 +4221,11 @@ void NativeContextData::SerializeOnBackground(JSHeapBroker* broker) {
   TraceScope tracer(broker, this, "NativeContextData::SerializeOnBackground");
   Handle<NativeContext> context = Handle<NativeContext>::cast(object());
 
-#define SERIALIZE_MEMBER(type, name)                             \
-  DCHECK_NULL(name##_);                                          \
-  name##_ = broker->GetOrCreateData(context->name(kAcquireLoad), \
-                                    kAssumeMemoryFence);         \
-  if (!name##_->should_access_heap()) {                          \
-    DCHECK(!name##_->IsJSFunction());                            \
+#define SERIALIZE_MEMBER(type, name)                              \
+  DCHECK_NULL(name##_);                                           \
+  name##_ = broker->GetOrCreateData(context->name(kAcquireLoad)); \
+  if (!name##_->should_access_heap()) {                           \
+    DCHECK(!name##_->IsJSFunction());                             \
   }
   BROKER_COMPULSORY_BACKGROUND_NATIVE_CONTEXT_FIELDS(SERIALIZE_MEMBER)
   if (!broker->is_isolate_bootstrapping()) {
@@ -4258,8 +4238,8 @@ void NativeContextData::SerializeOnBackground(JSHeapBroker* broker) {
   int const last = Context::LAST_FUNCTION_MAP_INDEX;
   function_maps_.reserve(last + 1 - first);
   for (int i = first; i <= last; ++i) {
-    function_maps_.push_back(broker->GetOrCreateData(
-        context->get(i, kAcquireLoad), kAssumeMemoryFence));
+    function_maps_.push_back(
+        broker->GetOrCreateData(context->get(i, kAcquireLoad)));
   }
 }
 
@@ -4296,7 +4276,7 @@ bool JSFunctionRef::serialized_code_and_feedback() const {
 
 CodeRef JSFunctionRef::code() const {
   if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
-    return MakeRefAssumeMemoryFence(broker(), object()->code(kAcquireLoad));
+    return MakeRef(broker(), object()->code(kAcquireLoad));
   }
 
   return CodeRef(broker(), ObjectRef::data()->AsJSFunction()->code());

src/compiler/js-heap-broker.cc

@@ -228,20 +228,18 @@ bool JSHeapBroker::IsArrayOrObjectPrototype(Handle<JSObject> object) const {
 }
 
 ObjectData* JSHeapBroker::TryGetOrCreateData(Object object,
-                                             GetOrCreateDataFlags flags) {
-  return TryGetOrCreateData(CanonicalPersistentHandle(object), flags);
+                                             bool crash_on_error) {
+  return TryGetOrCreateData(CanonicalPersistentHandle(object), crash_on_error);
 }
 
-ObjectData* JSHeapBroker::GetOrCreateData(Handle<Object> object,
-                                          GetOrCreateDataFlags flags) {
-  ObjectData* return_value = TryGetOrCreateData(object, flags | kCrashOnError);
+ObjectData* JSHeapBroker::GetOrCreateData(Handle<Object> object) {
+  ObjectData* return_value = TryGetOrCreateData(object, true);
   DCHECK_NOT_NULL(return_value);
   return return_value;
 }
 
-ObjectData* JSHeapBroker::GetOrCreateData(Object object,
-                                          GetOrCreateDataFlags flags) {
-  return GetOrCreateData(CanonicalPersistentHandle(object), flags);
+ObjectData* JSHeapBroker::GetOrCreateData(Object object) {
+  return GetOrCreateData(CanonicalPersistentHandle(object));
 }
 
 bool JSHeapBroker::StackHasOverflowed() const {
@@ -253,12 +251,8 @@ bool JSHeapBroker::StackHasOverflowed() const {
 }
 
 bool JSHeapBroker::ObjectMayBeUninitialized(Handle<Object> object) const {
-  if (!object->IsHeapObject()) return false;
-  return ObjectMayBeUninitialized(HeapObject::cast(*object));
-}
-
-bool JSHeapBroker::ObjectMayBeUninitialized(HeapObject object) const {
-  return !IsMainThread() && isolate()->heap()->IsPendingAllocation(object);
+  return !IsMainThread() && object->IsHeapObject() &&
+         isolate()->heap()->IsPendingAllocation(HeapObject::cast(*object));
 }
 
 bool CanInlineElementAccess(MapRef const& map) {

src/compiler/js-heap-broker.h

@@ -80,18 +80,6 @@ struct PropertyAccessTarget {
   };
 };
 
-enum GetOrCreateDataFlag {
-  // If set, a failure to create the data object results in a crash.
-  kCrashOnError = 1 << 0,
-  // If set, data construction assumes that the given object is protected by
-  // a memory fence (e.g. acquire-release) and thus fields required for
-  // construction (like Object::map) are safe to read. The protection can
-  // extend to some other situations as well.
-  kAssumeMemoryFence = 1 << 1,
-};
-using GetOrCreateDataFlags = base::Flags<GetOrCreateDataFlag>;
-DEFINE_OPERATORS_FOR_FLAGS(GetOrCreateDataFlags)
-
 class V8_EXPORT_PRIVATE JSHeapBroker {
  public:
   JSHeapBroker(Isolate* isolate, Zone* broker_zone, bool tracing_enabled,
@@ -164,16 +152,14 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   Handle<Object> GetRootHandle(Object object);
 
   // Never returns nullptr.
-  ObjectData* GetOrCreateData(Handle<Object> object,
-                              GetOrCreateDataFlags flags = {});
-  ObjectData* GetOrCreateData(Object object, GetOrCreateDataFlags flags = {});
+  ObjectData* GetOrCreateData(Handle<Object>);
+  // Like the previous but wraps argument in handle first (for convenience).
+  ObjectData* GetOrCreateData(Object);
 
   // Gets data only if we have it. However, thin wrappers will be created for
   // smis, read-only objects and never-serialized objects.
-  ObjectData* TryGetOrCreateData(Handle<Object> object,
-                                 GetOrCreateDataFlags flags = {});
-  ObjectData* TryGetOrCreateData(Object object,
-                                 GetOrCreateDataFlags flags = {});
+  ObjectData* TryGetOrCreateData(Handle<Object>, bool crash_on_error = false);
+  ObjectData* TryGetOrCreateData(Object object, bool crash_on_error = false);
 
   // Check if {object} is any native context's %ArrayPrototype% or
   // %ObjectPrototype%.
@@ -390,7 +376,6 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   // thus safe to read from a memory safety perspective. The converse does not
   // necessarily hold.
   bool ObjectMayBeUninitialized(Handle<Object> object) const;
-  bool ObjectMayBeUninitialized(HeapObject object) const;
 
   bool CanUseFeedback(const FeedbackNexus& nexus) const;
   const ProcessedFeedback& NewInsufficientFeedback(FeedbackSlotKind kind) const;
@@ -579,8 +564,8 @@ class V8_NODISCARD UnparkedScopeIfNeeded {
 template <class T,
           typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
 base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
-    JSHeapBroker* broker, T object, GetOrCreateDataFlags flags = {}) {
-  ObjectData* data = broker->TryGetOrCreateData(object, flags);
+    JSHeapBroker* broker, T object) {
+  ObjectData* data = broker->TryGetOrCreateData(object);
   if (data == nullptr) {
     TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(object));
     return {};
@@ -591,8 +576,8 @@ base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
 template <class T,
           typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
 base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
-    JSHeapBroker* broker, Handle<T> object, GetOrCreateDataFlags flags = {}) {
-  ObjectData* data = broker->TryGetOrCreateData(object, flags);
+    JSHeapBroker* broker, Handle<T> object) {
+  ObjectData* data = broker->TryGetOrCreateData(object);
   if (data == nullptr) {
     TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(*object));
     return {};
@@ -613,20 +598,6 @@ typename ref_traits<T>::ref_type MakeRef(JSHeapBroker* broker,
   return TryMakeRef(broker, object).value();
 }
 
-template <class T,
-          typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
-typename ref_traits<T>::ref_type MakeRefAssumeMemoryFence(JSHeapBroker* broker,
-                                                          T object) {
-  return TryMakeRef(broker, object, kAssumeMemoryFence).value();
-}
-
-template <class T,
-          typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
-typename ref_traits<T>::ref_type MakeRefAssumeMemoryFence(JSHeapBroker* broker,
-                                                          Handle<T> object) {
-  return TryMakeRef(broker, object, kAssumeMemoryFence).value();
-}
-
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8

test/mjsunit/regress/regress-1207679.js

-// Copyright 2021 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-//
-// Flags: --expose-gc --interrupt-budget=1000 --no-lazy-feedback-allocation
-
-var __v_5;
-function __v_1() {
-  var PI = {
-    get() {}
-  };
-  function __v_5() {
-    Object.defineProperty(PI, 'func', {
-    });
-    '𝌆'.match();
-  }
-  __v_5(...[__v_5]);
-  try {
-    __v_1();
-  } catch (PI) {}
-}
-__v_1();
-gc();
-__v_1();