[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}

src/compiler/heap-refs.cc

@@ -71,13 +71,15 @@ enum ObjectDataKind {
 
 namespace {
 
-bool IsReadOnlyHeapObject(Object object) {
+bool IsReadOnlyHeapObjectForCompiler(HeapObject object) {
   DisallowGarbageCollection no_gc;
   // HeapNumber is excluded because we have a uniform background
   // serialization treatment for it.
+  // TODO(jgruber): Remove this compiler-specific predicate and use the plain
+  // heap predicate instead. This would involve removing the special cases for
+  // builtins and heap numbers.
   return (object.IsCode() && Code::cast(object).is_builtin()) ||
-         (object.IsHeapObject() && !object.IsHeapNumber() &&
-          ReadOnlyHeap::Contains(HeapObject::cast(object)));
+         (!object.IsHeapNumber() && ReadOnlyHeap::Contains(object));
 }
 
 }  // namespace
@@ -116,7 +118,8 @@ class ObjectData : public ZoneObject {
                       kind == kNeverSerializedHeapObject ||
                       kind == kBackgroundSerializedHeapObject);
     CHECK_IMPLIES(kind == kUnserializedReadOnlyHeapObject,
-                  IsReadOnlyHeapObject(*object));
+                  object->IsHeapObject() && IsReadOnlyHeapObjectForCompiler(
+                                                HeapObject::cast(*object)));
   }
 
 #define DECLARE_IS(Name, ...) bool Is##Name() const;
@@ -337,7 +340,7 @@ bool PropertyCellData::Serialize(JSHeapBroker* broker) {
     }
   }
 
-  ObjectData* value_data = broker->TryGetOrCreateData(value, false);
+  ObjectData* value_data = broker->TryGetOrCreateData(value);
   if (value_data == nullptr) {
     DCHECK(!broker->IsMainThread());
     return false;
@@ -2132,7 +2135,7 @@ base::Optional<PropertyCellRef> GetPropertyCellFromHeap(JSHeapBroker* broker,
   it.TryLookupCachedProperty();
   if (it.state() == LookupIterator::DATA &&
       it.GetHolder<JSObject>()->IsJSGlobalObject()) {
-    return MakeRef(broker, it.GetPropertyCell());
+    return TryMakeRef(broker, it.GetPropertyCell());
   }
   return base::nullopt;
 }
@@ -2804,15 +2807,14 @@ void JSHeapBroker::ClearReconstructibleData() {
   }
 }
 
-ObjectData* JSHeapBroker::TryGetOrCreateData(
-    Handle<Object> object, bool crash_on_error,
-    ObjectRef::BackgroundSerialization background_serialization) {
+ObjectData* JSHeapBroker::TryGetOrCreateData(Handle<Object> object,
+                                             GetOrCreateDataFlags flags) {
   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,
@@ -2827,24 +2829,43 @@ ObjectData* JSHeapBroker::TryGetOrCreateData(
   ObjectData* object_data;
   if (object->IsSmi()) {
     entry = refs_->LookupOrInsert(object.address());
-    object_data = zone()->New<ObjectData>(this, &(entry->value), object, kSmi);
-  } else if (IsReadOnlyHeapObject(*object)) {
+    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))) {
     entry = refs_->LookupOrInsert(object.address());
-    object_data = zone()->New<ObjectData>(this, &(entry->value), object,
-                                          kUnserializedReadOnlyHeapObject);
+    return zone()->New<ObjectData>(this, &entry->value, object,
+                                   kUnserializedReadOnlyHeapObject);
+  }
+
+  const ObjectRef::BackgroundSerialization background_serialization =
+      (flags & kAllowBackgroundSerialization) != 0
+          ? ObjectRef::BackgroundSerialization::kAllowed
+          : ObjectRef::BackgroundSerialization::kDisallowed;
+
 #define CREATE_DATA(Name, Kind)                                   \
-  }                                                               \
-  /* NOLINTNEXTLINE(readability/braces) */                        \
-  else if (object->Is##Name()) {                                  \
+  if (object->Is##Name()) {                                       \
     CreateDataFunctor<Kind, Name##Data, Name> f;                  \
     if (!f(this, refs_, background_serialization, object, &entry, \
            &object_data)) {                                       \
-      CHECK(!crash_on_error);                                     \
+      CHECK_WITH_MSG(!crash_on_error, "Ref construction failed"); \
       return nullptr;                                             \
-    }
-    HEAP_BROKER_OBJECT_LIST(CREATE_DATA)
+    }                                                             \
+    /* NOLINTNEXTLINE(readability/braces) */                      \
+  } else
+  HEAP_BROKER_OBJECT_LIST(CREATE_DATA)
 #undef CREATE_DATA
-  } else {
+  {
     UNREACHABLE();
   }
   // At this point the entry pointer is not guaranteed to be valid as
@@ -3390,7 +3411,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 MakeRef(broker(), CallHandlerInfo::cast(call_code));
+    return TryMakeRef(broker(), CallHandlerInfo::cast(call_code));
   }
   ObjectData* call_code = data()->AsFunctionTemplateInfo()->call_code();
   if (!call_code) return base::nullopt;
@@ -3991,7 +4012,7 @@ base::Optional<ObjectRef> JSArrayRef::GetOwnCowElement(
 
 base::Optional<CellRef> SourceTextModuleRef::GetCell(int cell_index) const {
   if (data_->should_access_heap()) {
-    return MakeRef(broker(), object()->GetCell(cell_index));
+    return TryMakeRef(broker(), object()->GetCell(cell_index));
   }
   ObjectData* cell =
       data()->AsSourceTextModule()->GetCell(broker(), cell_index);
@@ -4007,17 +4028,26 @@ base::Optional<ObjectRef> SourceTextModuleRef::import_meta() const {
                    data()->AsSourceTextModule()->GetImportMeta(broker()));
 }
 
+namespace {
+
+GetOrCreateDataFlags FlagsForBackgroundSerialization(
+    ObjectRef::BackgroundSerialization v) {
+  if (v == ObjectRef::BackgroundSerialization::kAllowed) {
+    return kAllowBackgroundSerialization;
+  }
+  return {};
+}
+
+}  // namespace
+
 ObjectRef::ObjectRef(JSHeapBroker* broker, Handle<Object> object,
                      BackgroundSerialization background_serialization,
                      bool check_type)
     : broker_(broker) {
   CHECK_NE(broker->mode(), JSHeapBroker::kRetired);
 
-  data_ = broker->GetOrCreateData(object, background_serialization);
-  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");
+  data_ = broker->GetOrCreateData(
+      object, FlagsForBackgroundSerialization(background_serialization));
 }
 
 namespace {
@@ -4064,9 +4094,10 @@ 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 MakeRef(broker(), object()->boilerplate(kAcquireLoad));
+    return TryMakeRef(broker(), object()->boilerplate(kAcquireLoad));
   }
   ObjectData* boilerplate = data()->AsAllocationSite()->boilerplate();
   if (boilerplate) {
@@ -4082,7 +4113,7 @@ ElementsKind JSObjectRef::GetElementsKind() const {
 
 base::Optional<FixedArrayBaseRef> JSObjectRef::elements() const {
   if (data_->should_access_heap()) {
-    return MakeRef(broker(), object()->elements());
+    return TryMakeRef(broker(), object()->elements());
   }
   const JSObjectData* d = data()->AsJSObject();
   if (!d->serialized_elements()) {
@@ -4286,11 +4317,12 @@ void NativeContextData::SerializeOnBackground(JSHeapBroker* broker) {
   Handle<NativeContext> context = Handle<NativeContext>::cast(object());
 
   constexpr auto kAllowed = ObjectRef::BackgroundSerialization::kAllowed;
-#define SERIALIZE_MEMBER(type, name)                            \
-  DCHECK_NULL(name##_);                                         \
-  name##_ = broker->GetOrCreateData(context->name(), kAllowed); \
-  if (!name##_->should_access_heap()) {                         \
-    DCHECK(!name##_->IsJSFunction());                           \
+#define SERIALIZE_MEMBER(type, name)                               \
+  DCHECK_NULL(name##_);                                            \
+  name##_ = broker->GetOrCreateData(                               \
+      context->name(), FlagsForBackgroundSerialization(kAllowed)); \
+  if (!name##_->should_access_heap()) {                            \
+    DCHECK(!name##_->IsJSFunction());                              \
   }
   BROKER_COMPULSORY_BACKGROUND_NATIVE_CONTEXT_FIELDS(SERIALIZE_MEMBER)
   if (!broker->is_isolate_bootstrapping()) {
@@ -4303,8 +4335,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), kAllowed));
+    function_maps_.push_back(broker->GetOrCreateData(
+        context->get(i), FlagsForBackgroundSerialization(kAllowed)));
   }
 }
 
@@ -4341,7 +4373,7 @@ bool JSFunctionRef::serialized_code_and_feedback() const {
 
 CodeRef JSFunctionRef::code() const {
   if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
-    return MakeRef(broker(), object()->code(kAcquireLoad));
+    return MakeRefAssumeMemoryFence(broker(), object()->code(kAcquireLoad));
   }
 
   return CodeRef(broker(), ObjectRef::data()->AsJSFunction()->code());

src/compiler/js-heap-broker.cc

@@ -227,27 +227,21 @@ bool JSHeapBroker::IsArrayOrObjectPrototype(Handle<JSObject> object) const {
          array_and_object_prototypes_.end();
 }
 
-ObjectData* JSHeapBroker::TryGetOrCreateData(
-    Object object, bool crash_on_error,
-    ObjectRef::BackgroundSerialization background_serialization) {
-  return TryGetOrCreateData(CanonicalPersistentHandle(object), crash_on_error,
-                            background_serialization);
-}
-
-ObjectData* JSHeapBroker::GetOrCreateData(
-    Handle<Object> object,
-    ObjectRef::BackgroundSerialization background_serialization) {
-  ObjectData* return_value =
-      TryGetOrCreateData(object, true, background_serialization);
+ObjectData* JSHeapBroker::TryGetOrCreateData(Object object,
+                                             GetOrCreateDataFlags flags) {
+  return TryGetOrCreateData(CanonicalPersistentHandle(object), flags);
+}
+
+ObjectData* JSHeapBroker::GetOrCreateData(Handle<Object> object,
+                                          GetOrCreateDataFlags flags) {
+  ObjectData* return_value = TryGetOrCreateData(object, flags | kCrashOnError);
   DCHECK_NOT_NULL(return_value);
   return return_value;
 }
 
-ObjectData* JSHeapBroker::GetOrCreateData(
-    Object object,
-    ObjectRef::BackgroundSerialization background_serialization) {
-  return GetOrCreateData(CanonicalPersistentHandle(object),
-                         background_serialization);
+ObjectData* JSHeapBroker::GetOrCreateData(Object object,
+                                          GetOrCreateDataFlags flags) {
+  return GetOrCreateData(CanonicalPersistentHandle(object), flags);
 }
 
 bool JSHeapBroker::StackHasOverflowed() const {
@@ -259,8 +253,12 @@ bool JSHeapBroker::StackHasOverflowed() const {
 }
 
 bool JSHeapBroker::ObjectMayBeUninitialized(Handle<Object> object) const {
-  return !IsMainThread() && object->IsHeapObject() &&
-         isolate()->heap()->IsPendingAllocation(HeapObject::cast(*object));
+  if (!object->IsHeapObject()) return false;
+  return ObjectMayBeUninitialized(HeapObject::cast(*object));
+}
+
+bool JSHeapBroker::ObjectMayBeUninitialized(HeapObject object) const {
+  return !IsMainThread() && isolate()->heap()->IsPendingAllocation(object);
 }
 
 bool CanInlineElementAccess(MapRef const& map) {

src/compiler/js-heap-broker.h

@@ -79,6 +79,21 @@ 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,
+  // Whether background serialization is allowed (only used for
+  // kPossiblyBackgroundSerialized refs).
+  kAllowBackgroundSerialization = 1 << 2,
+};
+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,
@@ -151,25 +166,16 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
   Handle<Object> GetRootHandle(Object object);
 
   // Never returns nullptr.
-  ObjectData* GetOrCreateData(
-      Handle<Object>,
-      ObjectRef::BackgroundSerialization background_serialization =
-          ObjectRef::BackgroundSerialization::kDisallowed);
-  // Like the previous but wraps argument in handle first (for convenience).
-  ObjectData* GetOrCreateData(
-      Object, ObjectRef::BackgroundSerialization background_serialization =
-                  ObjectRef::BackgroundSerialization::kDisallowed);
+  ObjectData* GetOrCreateData(Handle<Object> object,
+                              GetOrCreateDataFlags flags = {});
+  ObjectData* GetOrCreateData(Object object, GetOrCreateDataFlags flags = {});
 
   // 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>, bool crash_on_error = false,
-      ObjectRef::BackgroundSerialization background_serialization =
-          ObjectRef::BackgroundSerialization::kDisallowed);
-  ObjectData* TryGetOrCreateData(
-      Object object, bool crash_on_error = false,
-      ObjectRef::BackgroundSerialization background_serialization =
-          ObjectRef::BackgroundSerialization::kDisallowed);
+  ObjectData* TryGetOrCreateData(Handle<Object> object,
+                                 GetOrCreateDataFlags flags = {});
+  ObjectData* TryGetOrCreateData(Object object,
+                                 GetOrCreateDataFlags flags = {});
 
   // Check if {object} is any native context's %ArrayPrototype% or
   // %ObjectPrototype%.
@@ -391,6 +397,7 @@ 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;
@@ -580,8 +587,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) {
-  ObjectData* data = broker->TryGetOrCreateData(object);
+    JSHeapBroker* broker, T object, GetOrCreateDataFlags flags = {}) {
+  ObjectData* data = broker->TryGetOrCreateData(object, flags);
   if (data == nullptr) {
     TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(object));
     return {};
@@ -592,8 +599,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) {
-  ObjectData* data = broker->TryGetOrCreateData(object);
+    JSHeapBroker* broker, Handle<T> object, GetOrCreateDataFlags flags = {}) {
+  ObjectData* data = broker->TryGetOrCreateData(object, flags);
   if (data == nullptr) {
     TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(*object));
     return {};
@@ -614,6 +621,20 @@ 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