[compiler] Use acquire-release semantics for NativeContext fields

Reads from the compiler thread require either 1. the last write to
happen before the compiler thread starts, or 2. acquire-release
semantics. For simplicity, this CL converts all NativeContext field
writes to be acq-rel. With the usual exception of writes from
generated code (these are limited for NativeContexts though).

The situation of context sets/gets is still somewhat complex:

- Context::get/set are relaxed (but don't use the corresponding tag)
- Context::get(.., kAcquireLoad) and Context::set(.., kReleaseStore)
  are acquire-release.
- Context::set_foo (defined for all native context fields) uses
  kReleaseStore underneath.
- Context::get_foo (defined for all native context fields) uses
  the default relaxed getter. The get_foo(kAcquireLoad) variant uses
  the acquire getter.
- NativeContext hides the default relaxed setter since all
  NativeContext sets should be acq-rel.

Ideally (future work), this should be simplified and made more explicit.
For example, get/set_foo could move to the NativeContext class, and we
could reevaluate whether we really need both relaxed and acq-rel
semantics (the pairing non-atomic/acq-rel feels more natural lets
tsan find concurrency issues).

Bug: v8:7790
Change-Id: I25efd37ece758da5a11dc11c6ae913e4975f4d20
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2891575Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74524}

src/compiler/heap-refs.cc

@@ -4277,11 +4277,11 @@ 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(kAcquireLoad), kAllowed); \
+  if (!name##_->should_access_heap()) {                                     \
+    DCHECK(!name##_->IsJSFunction());                                       \
   }
   BROKER_COMPULSORY_BACKGROUND_NATIVE_CONTEXT_FIELDS(SERIALIZE_MEMBER)
   if (!broker->is_isolate_bootstrapping()) {
@@ -4295,7 +4295,7 @@ void NativeContextData::SerializeOnBackground(JSHeapBroker* broker) {
   function_maps_.reserve(last + 1 - first);
   for (int i = first; i <= last; ++i) {
     function_maps_.push_back(
-        broker->GetOrCreateData(context->get(i), kAllowed));
+        broker->GetOrCreateData(context->get(i, kAcquireLoad), kAllowed));
   }
 }
 

src/compiler/heap-refs.h

@@ -460,6 +460,9 @@ class ContextRef : public HeapObjectRef {
   SourceTextModuleRef GetModule(SerializationPolicy policy) const;
 };
 
+// TODO(jgruber): Don't serialize NativeContext fields once all refs can be
+// created concurrently.
+
 #define BROKER_COMPULSORY_NATIVE_CONTEXT_FIELDS(V) \
   V(JSFunction, array_function)                    \
   V(JSFunction, function_prototype_apply)          \

src/init/bootstrapper.cc

@@ -1400,7 +1400,8 @@ static void InstallWithIntrinsicDefaultProto(Isolate* isolate,
   JSObject::AddProperty(isolate, function,
                         isolate->factory()->native_context_index_symbol(),
                         index, NONE);
-  isolate->native_context()->set(context_index, *function);
+  isolate->native_context()->set(context_index, *function, UPDATE_WRITE_BARRIER,
+                                 kReleaseStore);
 }
 
 static void InstallError(
@@ -4121,7 +4122,8 @@ Handle<JSFunction> Genesis::InstallTypedArray(const char* name,
   Handle<Map> rab_gsab_initial_map = factory()->NewMap(
       JS_TYPED_ARRAY_TYPE, JSTypedArray::kSizeWithEmbedderFields,
       GetCorrespondingRabGsabElementsKind(elements_kind), 0);
-  native_context()->set(rab_gsab_initial_map_index, *rab_gsab_initial_map);
+  native_context()->set(rab_gsab_initial_map_index, *rab_gsab_initial_map,
+                        UPDATE_WRITE_BARRIER, kReleaseStore);
   Map::SetPrototype(isolate(), rab_gsab_initial_map, prototype);
 
   return result;

src/objects/contexts-inl.h

@@ -55,38 +55,54 @@ NEVER_READ_ONLY_SPACE_IMPL(Context)
 
 CAST_ACCESSOR(NativeContext)
 
-V8_INLINE Object Context::get(int index) const {
-  return elements(index, kRelaxedLoad);
-}
-V8_INLINE Object Context::get(PtrComprCageBase cage_base, int index) const {
-  return elements(cage_base, index, kRelaxedLoad);
+Object Context::get(int index) const {
+  PtrComprCageBase cage_base = GetPtrComprCageBase(*this);
+  return get(cage_base, index);
 }
-V8_INLINE void Context::set(int index, Object value, WriteBarrierMode mode) {
-  DCHECK_NE(index, PREVIOUS_INDEX);
-  set_elements(index, value, kRelaxedStore, mode);
+
+Object Context::get(PtrComprCageBase cage_base, int index) const {
+  DCHECK_LT(static_cast<unsigned int>(index),
+            static_cast<unsigned int>(length()));
+  return TaggedField<Object>::Relaxed_Load(cage_base, *this,
+                                           OffsetOfElementAt(index));
 }
 
-void Context::set_scope_info(ScopeInfo scope_info, WriteBarrierMode mode) {
-  set(SCOPE_INFO_INDEX, scope_info, mode);
+void Context::set(int index, Object value, WriteBarrierMode mode) {
+  DCHECK_LT(static_cast<unsigned int>(index),
+            static_cast<unsigned int>(length()));
+  const int offset = OffsetOfElementAt(index);
+  RELAXED_WRITE_FIELD(*this, offset, value);
+  CONDITIONAL_WRITE_BARRIER(*this, offset, value, mode);
 }
 
-Object Context::synchronized_get(int index) const {
+Object Context::get(int index, AcquireLoadTag tag) const {
   PtrComprCageBase cage_base = GetPtrComprCageBase(*this);
-  return synchronized_get(cage_base, index);
+  return get(cage_base, index, tag);
 }
 
-Object Context::synchronized_get(PtrComprCageBase cage_base, int index) const {
+Object Context::get(PtrComprCageBase cage_base, int index,
+                    AcquireLoadTag) const {
   DCHECK_LT(static_cast<unsigned int>(index),
-            static_cast<unsigned int>(this->length()));
+            static_cast<unsigned int>(length()));
   return ACQUIRE_READ_FIELD(*this, OffsetOfElementAt(index));
 }
 
-void Context::synchronized_set(int index, Object value) {
+void Context::set(int index, Object value, WriteBarrierMode mode,
+                  ReleaseStoreTag) {
   DCHECK_LT(static_cast<unsigned int>(index),
-            static_cast<unsigned int>(this->length()));
+            static_cast<unsigned int>(length()));
   const int offset = OffsetOfElementAt(index);
   RELEASE_WRITE_FIELD(*this, offset, value);
-  WRITE_BARRIER(*this, offset, value);
+  CONDITIONAL_WRITE_BARRIER(*this, offset, value, mode);
+}
+
+void NativeContext::set(int index, Object value, WriteBarrierMode mode,
+                        ReleaseStoreTag tag) {
+  Context::set(index, value, mode, tag);
+}
+
+void Context::set_scope_info(ScopeInfo scope_info, WriteBarrierMode mode) {
+  set(SCOPE_INFO_INDEX, scope_info, mode);
 }
 
 Object Context::unchecked_previous() { return get(PREVIOUS_INDEX); }
@@ -97,7 +113,7 @@ Context Context::previous() {
   return Context::unchecked_cast(result);
 }
 void Context::set_previous(Context context, WriteBarrierMode mode) {
-  set_elements(PREVIOUS_INDEX, context, kRelaxedStore, mode);
+  set(PREVIOUS_INDEX, context, mode);
 }
 
 Object Context::next_context_link() { return get(Context::NEXT_CONTEXT_LINK); }
@@ -161,18 +177,22 @@ bool Context::HasSameSecurityTokenAs(Context that) const {
          that.native_context().security_token();
 }
 
-#define NATIVE_CONTEXT_FIELD_ACCESSORS(index, type, name) \
-  void Context::set_##name(type value) {                  \
-    DCHECK(IsNativeContext());                            \
-    set(index, value);                                    \
-  }                                                       \
-  bool Context::is_##name(type value) const {             \
-    DCHECK(IsNativeContext());                            \
-    return type::cast(get(index)) == value;               \
-  }                                                       \
-  type Context::name() const {                            \
-    DCHECK(IsNativeContext());                            \
-    return type::cast(get(index));                        \
+#define NATIVE_CONTEXT_FIELD_ACCESSORS(index, type, name)   \
+  void Context::set_##name(type value) {                    \
+    DCHECK(IsNativeContext());                              \
+    set(index, value, UPDATE_WRITE_BARRIER, kReleaseStore); \
+  }                                                         \
+  bool Context::is_##name(type value) const {               \
+    DCHECK(IsNativeContext());                              \
+    return type::cast(get(index)) == value;                 \
+  }                                                         \
+  type Context::name() const {                              \
+    DCHECK(IsNativeContext());                              \
+    return type::cast(get(index));                          \
+  }                                                         \
+  type Context::name(AcquireLoadTag tag) const {            \
+    DCHECK(IsNativeContext());                              \
+    return type::cast(get(index, tag));                     \
   }
 NATIVE_CONTEXT_FIELDS(NATIVE_CONTEXT_FIELD_ACCESSORS)
 #undef NATIVE_CONTEXT_FIELD_ACCESSORS
@@ -260,11 +280,13 @@ void NativeContext::set_microtask_queue(Isolate* isolate,
 
 void NativeContext::synchronized_set_script_context_table(
     ScriptContextTable script_context_table) {
-  synchronized_set(SCRIPT_CONTEXT_TABLE_INDEX, script_context_table);
+  set(SCRIPT_CONTEXT_TABLE_INDEX, script_context_table, UPDATE_WRITE_BARRIER,
+      kReleaseStore);
 }
 
 ScriptContextTable NativeContext::synchronized_script_context_table() const {
-  return ScriptContextTable::cast(synchronized_get(SCRIPT_CONTEXT_TABLE_INDEX));
+  return ScriptContextTable::cast(
+      get(SCRIPT_CONTEXT_TABLE_INDEX, kAcquireLoad));
 }
 
 OSROptimizedCodeCache NativeContext::GetOSROptimizedCodeCache() {

src/objects/contexts.cc

@@ -416,11 +416,11 @@ void NativeContext::AddOptimizedCode(Code code) {
   DCHECK(CodeKindCanDeoptimize(code.kind()));
   DCHECK(code.next_code_link().IsUndefined());
   code.set_next_code_link(get(OPTIMIZED_CODE_LIST));
-  set(OPTIMIZED_CODE_LIST, code, UPDATE_WEAK_WRITE_BARRIER);
+  set(OPTIMIZED_CODE_LIST, code, UPDATE_WEAK_WRITE_BARRIER, kReleaseStore);
 }
 
 void NativeContext::SetOptimizedCodeListHead(Object head) {
-  set(OPTIMIZED_CODE_LIST, head, UPDATE_WEAK_WRITE_BARRIER);
+  set(OPTIMIZED_CODE_LIST, head, UPDATE_WEAK_WRITE_BARRIER, kReleaseStore);
 }
 
 Object NativeContext::OptimizedCodeListHead() {
@@ -428,7 +428,7 @@ Object NativeContext::OptimizedCodeListHead() {
 }
 
 void NativeContext::SetDeoptimizedCodeListHead(Object head) {
-  set(DEOPTIMIZED_CODE_LIST, head, UPDATE_WEAK_WRITE_BARRIER);
+  set(DEOPTIMIZED_CODE_LIST, head, UPDATE_WEAK_WRITE_BARRIER, kReleaseStore);
 }
 
 Object NativeContext::DeoptimizedCodeListHead() {

src/objects/contexts.h

@@ -445,15 +445,18 @@ class Context : public TorqueGeneratedContext<Context, HeapObject> {
   NEVER_READ_ONLY_SPACE
 
   // Setter and getter for elements.
+  // Note the plain accessors use relaxed semantics.
+  // TODO(jgruber): Make that explicit through tags.
   V8_INLINE Object get(int index) const;
   V8_INLINE Object get(PtrComprCageBase cage_base, int index) const;
   V8_INLINE void set(int index, Object value,
                      WriteBarrierMode mode = UPDATE_WRITE_BARRIER);
-  // Setter and getter with synchronization semantics.
-  V8_INLINE Object synchronized_get(int index) const;
-  V8_INLINE Object synchronized_get(PtrComprCageBase cage_base,
-                                    int index) const;
-  V8_INLINE void synchronized_set(int index, Object value);
+  // Accessors with acquire-release semantics.
+  V8_INLINE Object get(int index, AcquireLoadTag) const;
+  V8_INLINE Object get(PtrComprCageBase cage_base, int index,
+                       AcquireLoadTag) const;
+  V8_INLINE void set(int index, Object value, WriteBarrierMode mode,
+                     ReleaseStoreTag);
 
   static const int kScopeInfoOffset = kElementsOffset;
   static const int kPreviousOffset = kScopeInfoOffset + kTaggedSize;
@@ -603,7 +606,8 @@ class Context : public TorqueGeneratedContext<Context, HeapObject> {
 #define NATIVE_CONTEXT_FIELD_ACCESSORS(index, type, name) \
   inline void set_##name(type value);                     \
   inline bool is_##name(type value) const;                \
-  inline type name() const;
+  inline type name() const;                               \
+  inline type name(AcquireLoadTag) const;
   NATIVE_CONTEXT_FIELDS(NATIVE_CONTEXT_FIELD_ACCESSORS)
 #undef NATIVE_CONTEXT_FIELD_ACCESSORS
 
@@ -673,6 +677,15 @@ class NativeContext : public Context {
 
   inline void AllocateExternalPointerEntries(Isolate* isolate);
 
+  // NativeContext fields are read concurrently from background threads; any
+  // concurrent writes of affected fields must have acquire-release semantics,
+  // thus we hide the non-atomic setter. Note this doesn't protect fully since
+  // one could still use Context::set and/or write directly using offsets (e.g.
+  // from CSA/Torque).
+  void set(int index, Object value, WriteBarrierMode mode) = delete;
+  V8_INLINE void set(int index, Object value, WriteBarrierMode mode,
+                     ReleaseStoreTag);
+
   // [microtask_queue]: pointer to the MicrotaskQueue object.
   DECL_GETTER(microtask_queue, MicrotaskQueue*)
   inline void set_microtask_queue(Isolate* isolate, MicrotaskQueue* queue);

src/objects/contexts.tq

@@ -13,7 +13,7 @@ class Context extends HeapObject {
     return *ContextSlot(this, ContextSlot::SCOPE_INFO_INDEX);
   }
   const length: Smi;
-  @cppRelaxedLoad @cppRelaxedStore elements[length]: Object;
+  elements[length]: Object;
 }
 
 extern class AwaitContext extends Context generates 'TNode<Context>';

src/objects/objects.cc

@@ -4712,7 +4712,8 @@ Handle<Object> CacheInitialJSArrayMaps(Isolate* isolate,
   Handle<Map> current_map = initial_map;
   ElementsKind kind = current_map->elements_kind();
   DCHECK_EQ(GetInitialFastElementsKind(), kind);
-  native_context->set(Context::ArrayMapIndex(kind), *current_map);
+  native_context->set(Context::ArrayMapIndex(kind), *current_map,
+                      UPDATE_WRITE_BARRIER, kReleaseStore);
   for (int i = GetSequenceIndexFromFastElementsKind(kind) + 1;
        i < kFastElementsKindCount; ++i) {
     Handle<Map> new_map;
@@ -4725,7 +4726,8 @@ Handle<Object> CacheInitialJSArrayMaps(Isolate* isolate,
                                         INSERT_TRANSITION);
     }
     DCHECK_EQ(next_kind, new_map->elements_kind());
-    native_context->set(Context::ArrayMapIndex(next_kind), *new_map);
+    native_context->set(Context::ArrayMapIndex(next_kind), *new_map,
+                        UPDATE_WRITE_BARRIER, kReleaseStore);
     current_map = new_map;
   }
   return initial_map;