[vm] Fix deadlock in AllocateSuspendState

AllocateSuspendState was calling `Instance::SetField` but did not allow lazy deopt to occur. Possibility of lazy deopt from those
field stores is only theoretical, but it manifested as a
deadlock between background compiler and main thread: if main
thread calls `SetField` which tries to acquire write access to
the program lock, while background compiler has already
acquired write access to the program lock and is trying to
stop all mutators at a GC+Deopt safepoint then we will
deadlock as `AllocateSuspendState` does not allow Deopts
(safepoint level was lowered by 5bc107c29d).

We fix this problem by bypassing field guard and simply
writing affected fields directly in AllocateSuspendState.
We make sure to initialize guarded state for these fields
eagerly, so it never needs to change.

TEST=added assertion which validates that attempting to acquire program lock for write can only occur where GC+Deopt are permitted.

Change-Id: I6ee6b82f3296f49f799c0069e42850711c9320ac
Bug: b/355226004
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/381240
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>

runtime/vm/isolate.cc

@@ -370,7 +370,7 @@ IsolateGroup::IsolateGroup(std::shared_ptr<IsolateGroupSource> source,
       kernel_constants_mutex_(),
       field_list_mutex_(),
       boxed_field_list_(GrowableObjectArray::null()),
-      program_lock_(new SafepointRwLock()),
+      program_lock_(new SafepointRwLock(SafepointLevel::kGCAndDeopt)),
       active_mutators_monitor_(new Monitor()),
       max_active_mutators_(Scavenger::MaxMutatorThreadCount())
 #if !defined(PRODUCT)

runtime/vm/lockers.cc

@@ -183,6 +183,10 @@ void SafepointRwLock::EnterWrite() {
 
   const bool can_block_without_safepoint = thread == nullptr;
 
+  RELEASE_ASSERT(can_block_without_safepoint ||
+                 thread->current_safepoint_level() >=
+                     expected_safepoint_level_);
+
   if (!TryEnterWrite(can_block_without_safepoint)) {
     // Important: must never hold monitor_ when blocking for safepoint.
     TransitionVMToBlocked transition(thread);

runtime/vm/lockers.h

@@ -362,7 +362,9 @@ class RwLock {
 
 class SafepointRwLock {
  public:
-  SafepointRwLock() {}
+  explicit SafepointRwLock(
+      SafepointLevel expected_safepoint_level = SafepointLevel::kGC)
+      : expected_safepoint_level_(expected_safepoint_level) {}
   ~SafepointRwLock() {}
 
   DEBUG_ONLY(bool IsCurrentThreadReader());
@@ -386,6 +388,8 @@ class SafepointRwLock {
   bool TryEnterWrite(bool can_block);
   void LeaveWrite();
 
+  const SafepointLevel expected_safepoint_level_;
+
   // We maintain an invariant that this monitor is never locked for long periods
   // of time: Any thread that acquired this monitor must always be able to do
   // it's work and release it (or wait on the monitor which will also release

runtime/vm/object.cc

@@ -21036,6 +21036,37 @@ void Instance::SetField(const Field& field, const Object& value) const {
   }
 }
 
+void Instance::SetFieldWithoutFieldGuard(const Field& field,
+                                         const Object& value) const {
+  if (field.is_unboxed()) {
+    switch (field.guarded_cid()) {
+      case kDoubleCid:
+        StoreNonPointer(reinterpret_cast<double_t*>(FieldAddr(field)),
+                        Double::Cast(value).value());
+        break;
+      case kFloat32x4Cid:
+        StoreNonPointer(reinterpret_cast<simd128_value_t*>(FieldAddr(field)),
+                        Float32x4::Cast(value).value());
+        break;
+      case kFloat64x2Cid:
+        StoreNonPointer(reinterpret_cast<simd128_value_t*>(FieldAddr(field)),
+                        Float64x2::Cast(value).value());
+        break;
+      default:
+        StoreNonPointer(reinterpret_cast<int64_t*>(FieldAddr(field)),
+                        Integer::Cast(value).AsInt64Value());
+        break;
+    }
+  } else {
+    // Some basic validation that we are not violating guarded cid.
+    RELEASE_ASSERT(!Thread::Current()->isolate_group()->use_field_guards() ||
+                   field.guarded_cid() == kDynamicCid ||
+                   field.guarded_cid() == value.GetClassId() ||
+                   (field.is_nullable() && value.IsNull()));
+    StoreCompressedPointer(FieldAddr(field), value.ptr());
+  }
+}
+
 AbstractTypePtr Instance::GetType(Heap::Space space) const {
   if (IsNull()) {
     return Type::NullType();

runtime/vm/object.h

@@ -8414,6 +8414,7 @@ class Instance : public Object {
   ObjectPtr GetField(const Field& field) const;
 
   void SetField(const Field& field, const Object& value) const;
+  void SetFieldWithoutFieldGuard(const Field& field, const Object& value) const;
 
   AbstractTypePtr GetType(Heap::Space space) const;
 

runtime/vm/object_store.cc

@@ -255,6 +255,11 @@ void ObjectStore::InitKnownObjects() {
 
   field = cls.LookupFieldAllowPrivate(Symbols::asyncStarBody());
   ASSERT(!field.IsNull());
+  // Force the state of guarded cid to be nullable closure so that
+  // AllocateSuspendState could write to the field directly without
+  // updating the guard state.
+  field.set_guarded_cid(kClosureCid);
+  field.set_is_nullable(true);
   set_async_star_stream_controller_async_star_body(field);
 
 #if !defined(PRODUCT)
@@ -335,6 +340,11 @@ void ObjectStore::InitKnownObjects() {
 
   field = cls.LookupFieldAllowPrivate(Symbols::_state());
   ASSERT(!field.IsNull());
+  // Force the state of guarded cid to be nullable SuspendState so that
+  // AllocateSuspendState could write to the field directly without
+  // updating the guard state.
+  field.set_guarded_cid(kSuspendStateCid);
+  field.set_is_nullable(true);
   set_sync_star_iterator_state(field);
 
   field = cls.LookupFieldAllowPrivate(Symbols::_yieldStarIterable());

runtime/vm/runtime_entry.cc

@@ -824,7 +824,13 @@ DEFINE_RUNTIME_ENTRY_NO_LAZY_DEOPT(AllocateSuspendState, 2) {
       // Reset _AsyncStarStreamController.asyncStarBody to null in order
       // to create a new callback closure during next yield.
       // The new callback closure will capture the reallocated SuspendState.
-      function_data.SetField(
+      //
+      // Caveat: can't use [SetField] here because it will try to take program
+      // lock (to update the state of guarded cid) and that requires us to
+      // be at safepoint which permits lazy deopt. Instead bypass
+      // field guard by making sure that guarded_cid allows our store here.
+      // (See ObjectStore::InitKnownObjects which initializes it).
+      function_data.SetFieldWithoutFieldGuard(
           Field::Handle(
               zone,
               object_store->async_star_stream_controller_async_star_body()),
@@ -835,7 +841,13 @@ DEFINE_RUNTIME_ENTRY_NO_LAZY_DEOPT(AllocateSuspendState, 2) {
     if (function_data.GetClassId() ==
         Class::Handle(zone, object_store->sync_star_iterator_class()).id()) {
       // Refresh _SyncStarIterator._state with the new SuspendState object.
-      function_data.SetField(
+      //
+      // Caveat: can't use [SetField] here because it will try to take program
+      // lock (to update the state of guarded cid) and that requires us to
+      // be at safepoint which permits lazy deopt. Instead bypass
+      // field guard by making sure that guarded_cid allows our store here.
+      // (See ObjectStore::InitKnownObjects which initializes it).
+      function_data.SetFieldWithoutFieldGuard(
           Field::Handle(zone, object_store->sync_star_iterator_state()),
           result);
     }