From 49ed2007e58f9b9175cbe196547c795c86223f9c Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Mon, 19 Aug 2024 14:04:47 +0000 Subject: [PATCH] [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 5bc107c29d297ac21fd4cb08192d2b3701ffb645). 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 Commit-Queue: Slava Egorov --- runtime/vm/isolate.cc | 2 +- runtime/vm/lockers.cc | 4 ++++ runtime/vm/lockers.h | 6 +++++- runtime/vm/object.cc | 31 +++++++++++++++++++++++++++++++ runtime/vm/object.h | 1 + runtime/vm/object_store.cc | 10 ++++++++++ runtime/vm/runtime_entry.cc | 16 ++++++++++++++-- 7 files changed, 66 insertions(+), 4 deletions(-) diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 05c642a817c..c75dddb775a 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -370,7 +370,7 @@ IsolateGroup::IsolateGroup(std::shared_ptr 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) diff --git a/runtime/vm/lockers.cc b/runtime/vm/lockers.cc index e94893b29e8..ed816904844 100644 --- a/runtime/vm/lockers.cc +++ b/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); diff --git a/runtime/vm/lockers.h b/runtime/vm/lockers.h index ff1cc1daa8a..90f208c5bec 100644 --- a/runtime/vm/lockers.h +++ b/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 diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index a5bc93ebd35..6a6a3ac7b24 100644 --- a/runtime/vm/object.cc +++ b/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(FieldAddr(field)), + Double::Cast(value).value()); + break; + case kFloat32x4Cid: + StoreNonPointer(reinterpret_cast(FieldAddr(field)), + Float32x4::Cast(value).value()); + break; + case kFloat64x2Cid: + StoreNonPointer(reinterpret_cast(FieldAddr(field)), + Float64x2::Cast(value).value()); + break; + default: + StoreNonPointer(reinterpret_cast(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(); diff --git a/runtime/vm/object.h b/runtime/vm/object.h index 22f82936abe..23999eb4b25 100644 --- a/runtime/vm/object.h +++ b/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; diff --git a/runtime/vm/object_store.cc b/runtime/vm/object_store.cc index 6dd63fbc951..93c62aff0af 100644 --- a/runtime/vm/object_store.cc +++ b/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()); diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc index cbc49178b87..d9f521e31d1 100644 --- a/runtime/vm/runtime_entry.cc +++ b/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); }