[vm/shared] Ensure exclusive execution of shared field initialization.

aam · Commit Queue · commit ef4e39a6ef5a · 2025-06-05T14:12:33.000-07:00
Add a mutex to guard execution of vm:shared field initialization. TEST=isolate_group_shared_init_test BUG=dartbug.com/60699 Change-Id: If544351fc26bfcc7fb9703954efe785989d488bc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/431742 Commit-Queue: Alexander Aprelev <aam@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>
diff --git a/runtime/vm/compiler/asm_intrinsifier_riscv.cc b/runtime/vm/compiler/asm_intrinsifier_riscv.cc
@@ -2035,20 +2035,20 @@ void AsmIntrinsifier::Timeline_getNextTaskId(Assembler* assembler,
   __ LoadImmediate(A0, target::ToRawSmi(0));
   __ ret();
 #elif XLEN == 64
-  __ ld(A0, Address(THR, target::Thread::next_task_id_offset()));
+  __ LoadFromOffset(A0, THR, target::Thread::next_task_id_offset());
   __ addi(A1, A0, 1);
-  __ sd(A1, Address(THR, target::Thread::next_task_id_offset()));
+  __ StoreToOffset(A1, THR, target::Thread::next_task_id_offset());
   __ SmiTag(A0);  // Ignore loss of precision.
   __ ret();
 #else
-  __ lw(T0, Address(THR, target::Thread::next_task_id_offset()));
-  __ lw(T1, Address(THR, target::Thread::next_task_id_offset() + 4));
+  __ LoadFromOffset(T0, THR, target::Thread::next_task_id_offset());
+  __ LoadFromOffset(T1, THR, target::Thread::next_task_id_offset() + 4);
   __ SmiTag(A0, T0);  // Ignore loss of precision.
   __ addi(T2, T0, 1);
   __ sltu(T3, T2, T0);  // Carry.
   __ add(T1, T1, T3);
-  __ sw(T2, Address(THR, target::Thread::next_task_id_offset()));
-  __ sw(T1, Address(THR, target::Thread::next_task_id_offset() + 4));
+  __ StoreToOffset(T2, THR, target::Thread::next_task_id_offset());
+  __ StoreToOffset(T1, THR, target::Thread::next_task_id_offset() + 4);
   __ ret();
 #endif
 }
diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc
@@ -4574,10 +4574,7 @@ void LoadStaticFieldInstr::EmitNativeCode(FlowGraphCompiler* compiler) {
           original_field.EnsureInitializerFunction();
         }
         stub = field().is_shared()
-                   ? (field().is_final()
-                          ? object_store
-                                ->init_shared_late_final_static_field_stub()
-                          : object_store->init_shared_late_static_field_stub())
+                   ? object_store->init_shared_late_static_field_stub()
                    : (field().is_final()
                           ? object_store->init_late_final_static_field_stub()
                           : object_store->init_late_static_field_stub());
diff --git a/runtime/vm/compiler/runtime_offsets_extracted.h b/runtime/vm/compiler/runtime_offsets_extracted.h
diff --git a/runtime/vm/compiler/stub_code_compiler.cc b/runtime/vm/compiler/stub_code_compiler.cc
@@ -91,14 +91,26 @@ void StubCodeCompiler::GenerateInitLateStaticFieldStub(bool is_final,
 
   __ EnterStubFrame();
 
+  if (FLAG_experimental_shared_data && is_shared) {
+    // Since initialization of shared fields has to be guarded by
+    // a mutex, do the initialization in the runtime.
+    __ PushObject(NullObject());  // Make room for the result
+    __ PushRegister(kFieldReg);
+    __ CallRuntime(kInitializeSharedFieldRuntimeEntry, /*argument_count=*/1);
+    __ PopRegister(kFieldReg);
+    __ PopRegister(kResultReg);
+    __ LeaveStubFrame();
+    __ Ret();
+    return;
+  }
+
   Label throw_since_no_isolate_is_present;
   if (FLAG_experimental_shared_data) {
-    if (!is_shared) {
-      // This stub is also called from mutator thread running without an
-      // isolate and attempts to load value from isolate static field.
-      __ LoadIsolate(kScratchReg);
-      __ BranchIfZero(kScratchReg, &throw_since_no_isolate_is_present);
-    }
+    ASSERT(!is_shared);
+    // This stub is also called from mutator thread running without an
+    // isolate and attempts to load value from isolate static field.
+    __ LoadIsolate(kScratchReg);
+    __ BranchIfZero(kScratchReg, &throw_since_no_isolate_is_present);
   }
 
   __ Comment("Calling initializer function");
@@ -143,20 +155,19 @@ void StubCodeCompiler::GenerateInitLateStaticFieldStub(bool is_final,
   }
 
   if (FLAG_experimental_shared_data) {
-    if (!is_shared) {
+    ASSERT(!is_shared);
 #if defined(TARGET_ARCH_ARM) || defined(TARGET_ARCH_ARM64)
-      // We are jumping over LeaveStubFrame so restore LR state to match one
-      // at the jump point.
-      __ set_lr_state(compiler::LRState::OnEntry().EnterFrame());
+    // We are jumping over LeaveStubFrame so restore LR state to match one
+    // at the jump point.
+    __ set_lr_state(compiler::LRState::OnEntry().EnterFrame());
 #endif  // defined(TARGET_ARCH_ARM) || defined(TARGET_ARCH_ARM64)
-      // Throw FieldAccessError
-      __ Bind(&throw_since_no_isolate_is_present);
-      __ PushObject(NullObject());  // Make room for (unused) result.
-      __ PushRegister(kFieldReg);
-      __ CallRuntime(kStaticFieldAccessedWithoutIsolateErrorRuntimeEntry,
-                     /*argument_count=*/1);
-      __ Breakpoint();
-    }
+    // Throw FieldAccessError
+    __ Bind(&throw_since_no_isolate_is_present);
+    __ PushObject(NullObject());  // Make room for (unused) result.
+    __ PushRegister(kFieldReg);
+    __ CallRuntime(kStaticFieldAccessedWithoutIsolateErrorRuntimeEntry,
+                   /*argument_count=*/1);
+    __ Breakpoint();
   }
 }
 
@@ -172,10 +183,6 @@ void StubCodeCompiler::GenerateInitSharedLateStaticFieldStub() {
   GenerateInitLateStaticFieldStub(/*is_final=*/false, /*is_shared=*/true);
 }
 
-void StubCodeCompiler::GenerateInitSharedLateFinalStaticFieldStub() {
-  GenerateInitLateStaticFieldStub(/*is_final=*/true, /*is_shared=*/true);
-}
-
 void StubCodeCompiler::GenerateInitInstanceFieldStub() {
   __ EnterStubFrame();
   __ PushObject(NullObject());  // Make room for result.
diff --git a/runtime/vm/compiler/stub_code_compiler.h b/runtime/vm/compiler/stub_code_compiler.h
@@ -190,7 +190,6 @@ class StubCodeCompiler {
 
   // Common function for generating InitLateStaticField,
   // InitLateFinalStaticField, InitSharedLateStaticField,
-  // InitSharedLateFinalStaticField,
   void GenerateInitLateStaticFieldStub(bool is_final, bool is_shared);
 
   // Common function for generating InitLateInstanceField and
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
@@ -354,6 +354,7 @@ IsolateGroup::IsolateGroup(std::shared_ptr<IsolateGroupSource> source,
       kernel_data_lib_cache_mutex_(),
       kernel_data_class_cache_mutex_(),
       kernel_constants_mutex_(),
+      shared_field_initializer_rwlock_(),
       field_list_mutex_(),
       boxed_field_list_(GrowableObjectArray::null()),
       program_lock_(new SafepointRwLock(SafepointLevel::kGCAndDeopt)),
@@ -890,6 +891,7 @@ void IsolateGroup::FreeStaticField(const Field& field) {
   const intptr_t field_id = field.field_id();
   if (field.is_shared()) {
     shared_field_table()->Free(field_id);
+    shared_initial_field_table()->Free(field_id);
   } else {
     initial_field_table()->Free(field_id);
     sentinel_field_table()->Free(field_id);
diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h
@@ -534,6 +534,10 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   Mutex* initializer_functions_mutex() { return &initializer_functions_mutex_; }
 #endif  // !defined(DART_PRECOMPILED_RUNTIME) || defined(DART_DYNAMIC_MODULES)
 
+  SafepointRwLock* shared_field_initializer_rwlock() {
+    return &shared_field_initializer_rwlock_;
+  }
+
   SafepointRwLock* program_lock() { return program_lock_.get(); }
 
   static inline IsolateGroup* Current() {
@@ -936,6 +940,9 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   Mutex initializer_functions_mutex_;
 #endif  // !defined(DART_PRECOMPILED_RUNTIME) || defined(DART_DYNAMIC_MODULES)
 
+  // Ensure exclusive execution of shared field initializers.
+  SafepointRwLock shared_field_initializer_rwlock_;
+
   // Protect access to boxed_field_list_.
   Mutex field_list_mutex_;
   // List of fields that became boxed and that trigger deoptimization.
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
@@ -13430,7 +13430,6 @@ TypePtr Class::GetInstantiationOf(Zone* zone, const Type& type) const {
 }
 
 void Field::SetStaticValue(const Object& value) const {
-  ASSERT(!is_shared());
   auto thread = Thread::Current();
   ASSERT(thread->IsDartMutatorThread());
   ASSERT(value.IsNull() || value.IsSentinel() || value.IsInstance());
@@ -13440,7 +13439,11 @@ void Field::SetStaticValue(const Object& value) const {
   ASSERT(id >= 0);
 
   SafepointReadRwLocker ml(thread, thread->isolate_group()->program_lock());
-  thread->isolate()->field_table()->SetAt(id, value.ptr());
+  if (is_shared()) {
+    thread->isolate_group()->shared_field_table()->SetAt(id, value.ptr());
+  } else {
+    thread->isolate()->field_table()->SetAt(id, value.ptr());
+  }
 }
 
 static StaticTypeExactnessState TrivialTypeExactnessFor(const Class& cls) {
diff --git a/runtime/vm/object.h b/runtime/vm/object.h
@@ -13399,7 +13399,9 @@ void Field::SetOffset(intptr_t host_offset_in_bytes,
 
 ObjectPtr Field::StaticValue() const {
   ASSERT(is_static());  // Valid only for static dart fields.
-  return Isolate::Current()->field_table()->At(field_id());
+  return is_shared()
+             ? IsolateGroup::Current()->shared_field_table()->At(field_id())
+             : Isolate::Current()->field_table()->At(field_id());
 }
 
 inline intptr_t Field::field_id() const {
diff --git a/runtime/vm/object_store.h b/runtime/vm/object_store.h
@@ -268,7 +268,6 @@ class ObjectPointerVisitor;
   RW(Code, init_late_instance_field_stub)                                      \
   RW(Code, init_late_final_instance_field_stub)                                \
   RW(Code, init_shared_late_static_field_stub)                                 \
-  RW(Code, init_shared_late_final_static_field_stub)                           \
   RW(Code, call_closure_no_such_method_stub)                                   \
   RW(Code, default_tts_stub)                                                   \
   RW(Code, default_nullable_tts_stub)                                          \
@@ -383,7 +382,6 @@ class ObjectPointerVisitor;
   DO(init_late_instance_field_stub, InitLateInstanceField)                     \
   DO(init_late_final_instance_field_stub, InitLateFinalInstanceField)          \
   DO(init_shared_late_static_field_stub, InitSharedLateStaticField)            \
-  DO(init_shared_late_final_static_field_stub, InitSharedLateFinalStaticField) \
   DO(await_stub, Await)                                                        \
   DO(await_with_type_check_stub, AwaitWithTypeCheck)                           \
   DO(clone_suspend_state_stub, CloneSuspendState)                              \
diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc
@@ -4724,6 +4724,21 @@ DEFINE_LEAF_RUNTIME_ENTRY(PropagateError,
                           /*argument_count=*/1,
                           DLRT_PropagateError);
 
+DEFINE_RUNTIME_ENTRY(InitializeSharedField, 1) {
+  SafepointWriteRwLocker locker(
+      thread, thread->isolate_group()->shared_field_initializer_rwlock());
+  const Field& field = Field::CheckedHandle(zone, arguments.ArgAt(0));
+  Object& result = Object::Handle(zone, field.StaticValue());
+  if (result.ptr() == Object::sentinel().ptr()) {
+    // Haven't lost a race to set the initial value.
+    result = field.InitializeStatic();
+    ThrowIfError(result);
+    result = field.StaticValue();
+    ASSERT(result.ptr() != Object::sentinel().ptr());
+  }
+  arguments.SetReturn(result);
+}
+
 #if !defined(USING_MEMORY_SANITIZER)
 extern "C" void __msan_unpoison(const volatile void*, size_t) {
   UNREACHABLE();
diff --git a/runtime/vm/runtime_entry_list.h b/runtime/vm/runtime_entry_list.h
@@ -81,7 +81,8 @@ namespace dart {
   V(ResolveCallFunction)                                                       \
   V(InterpretedInstanceCallMissHandler)                                        \
   V(InvokeNoSuchMethod)                                                        \
-  V(ResumeInterpreter)
+  V(ResumeInterpreter)                                                         \
+  V(InitializeSharedField)
 
 // Note: Leaf runtime function have C linkage, so they cannot pass C++ struct
 // values like ObjectPtr.
diff --git a/runtime/vm/stub_code_list.h b/runtime/vm/stub_code_list.h
@@ -163,7 +163,6 @@ namespace dart {
   V(InitLateInstanceField)                                                     \
   V(InitLateFinalInstanceField)                                                \
   V(InitSharedLateStaticField)                                                 \
-  V(InitSharedLateFinalStaticField)                                            \
   V(Throw)                                                                     \
   V(ReThrow)                                                                   \
   V(InstanceOf)                                                                \
diff --git a/sdk/lib/_internal/vm/lib/ffi_patch.dart b/sdk/lib/_internal/vm/lib/ffi_patch.dart
@@ -1926,22 +1926,15 @@ class Native<T> {
   _get_ffi_native_resolver<T extends NativeFunction>();
 
   // Resolver for FFI Native C function pointers.
-  //
-  // TODO(dartbug.com/60699): Once the referenced issue is fixed, this can be
-  // restored back to use of a field with initializer.
   @pragma('vm:entry-point')
   @pragma('vm:shared')
-  static int Function(Object, Object, int)? _ffi_resolver = null;
+  static final _ffi_resolver =
+      _get_ffi_native_resolver<
+            NativeFunction<IntPtr Function(Handle, Handle, IntPtr)>
+          >()
+          .asFunction<int Function(Object, Object, int)>();
 
   @pragma('vm:entry-point')
-  static int _ffi_resolver_function(Object a, Object s, int n) {
-    if (_ffi_resolver == null) {
-      _ffi_resolver =
-          _get_ffi_native_resolver<
-                NativeFunction<IntPtr Function(Handle, Handle, IntPtr)>
-              >()
-              .asFunction<int Function(Object, Object, int)>();
-    }
-    return _ffi_resolver!(a, s, n);
-  }
+  static int _ffi_resolver_function(Object a, Object s, int n) =>
+      _ffi_resolver(a, s, n);
 }
diff --git a/tests/ffi/ffi.status b/tests/ffi/ffi.status
@@ -57,6 +57,7 @@ function_callbacks_structs_by_value_generated_test/*: Skip # Test harness doesn'
 function_callbacks_structs_by_value_native_callable_generated_test/*: Skip # Test harness doesn't support multitest with Fuchsia
 function_callbacks_subtype_test/*: Skip # Test harness doesn't support multitest with Fuchsia
 isolate_group_shared_callback_test: Skip # gen_snapshot requires experimental-shared-data flag
+isolate_group_shared_init_test: Skip # gen_snapshot requires experimental-shared-data flag
 isolate_group_shared_send_test: Skip # gen_snapshot requires experimental-shared-data flag
 native_assets/*: Skip # Source not available in the emulator
 run_isolate_group_run_test: Skip # gen_snapshot requires experimental-shared-data flag
diff --git a/tests/ffi/isolate_group_shared_init_test.dart b/tests/ffi/isolate_group_shared_init_test.dart

Original file line number	Diff line number	Diff line change
`@@ -13399,7 +13399,9 @@ void Field::SetOffset(intptr_t host_offset_in_bytes,`
`13399`	`13399`
`13400`	`13400`	`ObjectPtr Field::StaticValue() const {`
`13401`	`13401`	`ASSERT(is_static()); // Valid only for static dart fields.`
`13402`		`- return Isolate::Current()->field_table()->At(field_id());`
	`13402`	`+ return is_shared()`
	`13403`	`+ ? IsolateGroup::Current()->shared_field_table()->At(field_id())`
	`13404`	`+ : Isolate::Current()->field_table()->At(field_id());`
`13403`	`13405`	`}`
`13404`	`13406`
`13405`	`13407`	`inline intptr_t Field::field_id() const {`