From 21af69a7a56f0e74bc8d5cba873d2ee3bd30579f Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Tue, 13 Jul 2021 11:30:08 +0000 Subject: [PATCH] Revert "[vm] Don't block OOB messages during field initialization; make interrupts lock-free." This reverts commit 4d789f60d1355d4dc772dd6ba9a5eeecc901a022. Reason for revert: TSAN races https://github.com/dart-lang/sdk/issues/46596 Original change's description: > [vm] Don't block OOB messages during field initialization; make interrupts lock-free. > > TEST=ci > Change-Id: I51d0f51995e197ab71c059f17d22259eea19c286 > Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206566 > Commit-Queue: Ryan Macnak > Reviewed-by: Ben Konyi TBR=bkonyi@google.com,rmacnak@google.com Change-Id: I777f5a0bbeb3cd60e03aff46183b19ab4e286664 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206545 Reviewed-by: Daco Harkes Commit-Queue: Daco Harkes --- runtime/platform/atomic.h | 13 --- runtime/vm/heap/safepoint.cc | 2 +- runtime/vm/isolate.cc | 13 +++ runtime/vm/isolate.h | 10 ++ runtime/vm/isolate_test.cc | 102 +++++++++++++++++ runtime/vm/object.cc | 1 + runtime/vm/thread.cc | 103 ++++++++++++++---- runtime/vm/thread.h | 13 ++- ...ill_infinite_loop_in_initializer_test.dart | 44 -------- ...ill_infinite_loop_in_initializer_test.dart | 44 -------- 10 files changed, 222 insertions(+), 123 deletions(-) delete mode 100644 tests/lib/isolate/kill_infinite_loop_in_initializer_test.dart delete mode 100644 tests/lib_2/isolate/kill_infinite_loop_in_initializer_test.dart diff --git a/runtime/platform/atomic.h b/runtime/platform/atomic.h index ffb5bdcd3d7..98bc900a555 100644 --- a/runtime/platform/atomic.h +++ b/runtime/platform/atomic.h @@ -21,16 +21,9 @@ class RelaxedAtomic { T load(std::memory_order order = std::memory_order_relaxed) const { return value_.load(order); } - T load(std::memory_order order = std::memory_order_relaxed) const volatile { - return value_.load(order); - } void store(T arg, std::memory_order order = std::memory_order_relaxed) { value_.store(arg, order); } - void store(T arg, - std::memory_order order = std::memory_order_relaxed) volatile { - value_.store(arg, order); - } T fetch_add(T arg, std::memory_order order = std::memory_order_relaxed) { return value_.fetch_add(arg, order); @@ -51,12 +44,6 @@ class RelaxedAtomic { std::memory_order order = std::memory_order_relaxed) { return value_.compare_exchange_weak(expected, desired, order, order); } - bool compare_exchange_weak( - T& expected, // NOLINT - T desired, - std::memory_order order = std::memory_order_relaxed) volatile { - return value_.compare_exchange_weak(expected, desired, order, order); - } bool compare_exchange_strong( T& expected, // NOLINT T desired, diff --git a/runtime/vm/heap/safepoint.cc b/runtime/vm/heap/safepoint.cc index 52da48b633f..4a7e5dea9fc 100644 --- a/runtime/vm/heap/safepoint.cc +++ b/runtime/vm/heap/safepoint.cc @@ -155,7 +155,7 @@ void SafepointHandler::LevelHandler::NotifyThreadsToGetToSafepointLevel( if (!Thread::IsAtSafepoint(level_, state)) { // Send OOB message to get it to safepoint. if (current->IsMutatorThread()) { - current->ScheduleInterrupts(Thread::kVMInterrupt); + current->ScheduleInterruptsLocked(Thread::kVMInterrupt); } MonitorLocker sl(&parked_lock_); num_threads_not_parked_++; diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 8993e5f3855..cb00d297130 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -867,6 +867,19 @@ bool IsolateVisitor::IsSystemIsolate(Isolate* isolate) const { return Isolate::IsSystemIsolate(isolate); } +NoOOBMessageScope::NoOOBMessageScope(Thread* thread) + : ThreadStackResource(thread) { + if (thread->isolate() != nullptr) { + thread->DeferOOBMessageInterrupts(); + } +} + +NoOOBMessageScope::~NoOOBMessageScope() { + if (thread()->isolate() != nullptr) { + thread()->RestoreOOBMessageInterrupts(); + } +} + Bequest::~Bequest() { if (handle_ == nullptr) { return; diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index 4c2d9f5f6b0..c17a81d9a9e 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -134,6 +134,16 @@ class LambdaCallable : public Callable { DISALLOW_COPY_AND_ASSIGN(LambdaCallable); }; +// Disallow OOB message handling within this scope. +class NoOOBMessageScope : public ThreadStackResource { + public: + explicit NoOOBMessageScope(Thread* thread); + ~NoOOBMessageScope(); + + private: + DISALLOW_COPY_AND_ASSIGN(NoOOBMessageScope); +}; + // Fixed cache for exception handler lookup. typedef FixedCache HandlerInfoCache; // Fixed cache for catch entry state lookup. diff --git a/runtime/vm/isolate_test.cc b/runtime/vm/isolate_test.cc index 200359182fc..9402106763f 100644 --- a/runtime/vm/isolate_test.cc +++ b/runtime/vm/isolate_test.cc @@ -145,4 +145,106 @@ TEST_CASE(StackLimitInterrupts) { barrier.Exit(); } +class IsolateTestHelper { + public: + static uword GetStackLimit(Thread* thread) { return thread->stack_limit_; } + static uword GetSavedStackLimit(Thread* thread) { + return thread->saved_stack_limit_; + } + static uword GetDeferredInterruptsMask(Thread* thread) { + return thread->deferred_interrupts_mask_; + } + static uword GetDeferredInterrupts(Thread* thread) { + return thread->deferred_interrupts_; + } +}; + +TEST_CASE(NoOOBMessageScope) { + // Finish any GC in progress so that no kVMInterrupt is added for GC reasons. + { + TransitionNativeToVM transition(thread); + GCTestHelper::CollectAllGarbage(); + const Error& error = Error::Handle(thread->HandleInterrupts()); + RELEASE_ASSERT(error.IsNull()); + } + + // EXPECT_EQ is picky about type agreement for its arguments. + const uword kZero = 0; + const uword kMessageInterrupt = Thread::kMessageInterrupt; + const uword kVMInterrupt = Thread::kVMInterrupt; + uword stack_limit; + uword interrupt_bits; + + // Initially no interrupts are scheduled or deferred. + EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread), + IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread)); + + { + // Defer message interrupts. + NoOOBMessageScope no_msg_scope(thread); + EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread), + IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread)); + + // Schedule a message, it is deferred. + thread->ScheduleInterrupts(Thread::kMessageInterrupt); + EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread), + IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterrupts(thread)); + + // Schedule a vm interrupt, it is not deferred. + thread->ScheduleInterrupts(Thread::kVMInterrupt); + stack_limit = IsolateTestHelper::GetStackLimit(thread); + EXPECT_NE(stack_limit, IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT((stack_limit & Thread::kVMInterrupt) != 0); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterrupts(thread)); + + // Clear the vm interrupt. Message is still deferred. + interrupt_bits = thread->GetAndClearInterrupts(); + EXPECT_EQ(kVMInterrupt, interrupt_bits); + EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread), + IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterrupts(thread)); + } + + // Restore message interrupts. Message is now pending. + stack_limit = IsolateTestHelper::GetStackLimit(thread); + EXPECT_NE(stack_limit, IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT((stack_limit & Thread::kMessageInterrupt) != 0); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread)); + + { + // Defer message interrupts, again. The pending interrupt is deferred. + NoOOBMessageScope no_msg_scope(thread); + EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread), + IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kMessageInterrupt, + IsolateTestHelper::GetDeferredInterrupts(thread)); + } + + // Restore, then clear interrupts. The world is as it was. + interrupt_bits = thread->GetAndClearInterrupts(); + EXPECT_EQ(kMessageInterrupt, interrupt_bits); + EXPECT_EQ(IsolateTestHelper::GetStackLimit(thread), + IsolateTestHelper::GetSavedStackLimit(thread)); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterruptsMask(thread)); + EXPECT_EQ(kZero, IsolateTestHelper::GetDeferredInterrupts(thread)); +} + } // namespace dart diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 8c08ad9b174..7c9214acca8 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -11162,6 +11162,7 @@ ObjectPtr Field::EvaluateInitializer() const { } #endif // !defined(DART_PRECOMPILED_RUNTIME) + NoOOBMessageScope no_msg_scope(thread); NoReloadScope no_reload_scope(thread); const Function& initializer = Function::Handle(EnsureInitializerFunction()); return DartEntry::InvokeFunction(initializer, Object::empty_array()); diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc index 45d99f9876c..16a1f8fc229 100644 --- a/runtime/vm/thread.cc +++ b/runtime/vm/thread.cc @@ -91,6 +91,9 @@ Thread::Thread(bool is_vm_isolate) no_safepoint_scope_depth_(0), #endif reusable_handles_(), + defer_oob_messages_count_(0), + deferred_interrupts_mask_(0), + deferred_interrupts_(0), stack_overflow_count_(0), hierarchy_info_(NULL), type_usage_info_(NULL), @@ -395,7 +398,7 @@ void Thread::SetStackLimit(uword limit) { MonitorLocker ml(&thread_lock_); if (!HasScheduledInterrupts()) { // No interrupt pending, set stack_limit_ too. - stack_limit_.store(limit); + stack_limit_ = limit; } saved_stack_limit_ = limit; } @@ -405,33 +408,95 @@ void Thread::ClearStackLimit() { } void Thread::ScheduleInterrupts(uword interrupt_bits) { + MonitorLocker ml(&thread_lock_); + ScheduleInterruptsLocked(interrupt_bits); +} + +void Thread::ScheduleInterruptsLocked(uword interrupt_bits) { + ASSERT(thread_lock_.IsOwnedByCurrentThread()); ASSERT((interrupt_bits & ~kInterruptsMask) == 0); // Must fit in mask. - uword old_limit = stack_limit_.load(); - uword new_limit; - do { - if (old_limit == saved_stack_limit_) { - new_limit = (kInterruptStackLimit & ~kInterruptsMask) | interrupt_bits; - } else { - new_limit = old_limit | interrupt_bits; + // Check to see if any of the requested interrupts should be deferred. + uword defer_bits = interrupt_bits & deferred_interrupts_mask_; + if (defer_bits != 0) { + deferred_interrupts_ |= defer_bits; + interrupt_bits &= ~deferred_interrupts_mask_; + if (interrupt_bits == 0) { + return; } - } while (!stack_limit_.compare_exchange_weak(old_limit, new_limit)); + } + + if (stack_limit_ == saved_stack_limit_) { + stack_limit_ = (kInterruptStackLimit & ~kInterruptsMask) | interrupt_bits; + } else { + stack_limit_ = stack_limit_ | interrupt_bits; + } } uword Thread::GetAndClearInterrupts() { - uword interrupt_bits = 0; - uword old_limit = stack_limit_.load(); - uword new_limit = saved_stack_limit_; - do { - if (old_limit == saved_stack_limit_) { - return interrupt_bits; - } - interrupt_bits = interrupt_bits | (old_limit & kInterruptsMask); - } while (!stack_limit_.compare_exchange_weak(old_limit, new_limit)); - + MonitorLocker ml(&thread_lock_); + if (stack_limit_ == saved_stack_limit_) { + return 0; // No interrupt was requested. + } + uword interrupt_bits = stack_limit_ & kInterruptsMask; + stack_limit_ = saved_stack_limit_; return interrupt_bits; } +void Thread::DeferOOBMessageInterrupts() { + MonitorLocker ml(&thread_lock_); + defer_oob_messages_count_++; + if (defer_oob_messages_count_ > 1) { + // OOB message interrupts are already deferred. + return; + } + ASSERT(deferred_interrupts_mask_ == 0); + deferred_interrupts_mask_ = kMessageInterrupt; + + if (stack_limit_ != saved_stack_limit_) { + // Defer any interrupts which are currently pending. + deferred_interrupts_ = stack_limit_ & deferred_interrupts_mask_; + + // Clear deferrable interrupts, if present. + stack_limit_ = stack_limit_ & ~deferred_interrupts_mask_; + + if ((stack_limit_ & kInterruptsMask) == 0) { + // No other pending interrupts. Restore normal stack limit. + stack_limit_ = saved_stack_limit_; + } + } +#if !defined(PRODUCT) + if (FLAG_trace_service && FLAG_trace_service_verbose) { + OS::PrintErr("[+%" Pd64 "ms] Isolate %s deferring OOB interrupts\n", + Dart::UptimeMillis(), isolate()->name()); + } +#endif // !defined(PRODUCT) +} + +void Thread::RestoreOOBMessageInterrupts() { + MonitorLocker ml(&thread_lock_); + defer_oob_messages_count_--; + if (defer_oob_messages_count_ > 0) { + return; + } + ASSERT(defer_oob_messages_count_ == 0); + ASSERT(deferred_interrupts_mask_ == kMessageInterrupt); + deferred_interrupts_mask_ = 0; + if (deferred_interrupts_ != 0) { + if (stack_limit_ == saved_stack_limit_) { + stack_limit_ = kInterruptStackLimit & ~kInterruptsMask; + } + stack_limit_ = stack_limit_ | deferred_interrupts_; + deferred_interrupts_ = 0; + } +#if !defined(PRODUCT) + if (FLAG_trace_service && FLAG_trace_service_verbose) { + OS::PrintErr("[+%" Pd64 "ms] Isolate %s restoring OOB interrupts\n", + Dart::UptimeMillis(), isolate()->name()); + } +#endif // !defined(PRODUCT) +} + ErrorPtr Thread::HandleInterrupts() { uword interrupt_bits = GetAndClearInterrupts(); if ((interrupt_bits & kVMInterrupt) != 0) { diff --git a/runtime/vm/thread.h b/runtime/vm/thread.h index 6e3ff674a3b..2d80830add6 100644 --- a/runtime/vm/thread.h +++ b/runtime/vm/thread.h @@ -417,10 +417,11 @@ class Thread : public ThreadState { }; void ScheduleInterrupts(uword interrupt_bits); + void ScheduleInterruptsLocked(uword interrupt_bits); ErrorPtr HandleInterrupts(); uword GetAndClearInterrupts(); bool HasScheduledInterrupts() const { - return (stack_limit_.load() & kInterruptsMask) != 0; + return (stack_limit_ & kInterruptsMask) != 0; } // Monitor corresponding to this thread. @@ -1030,7 +1031,7 @@ class Thread : public ThreadState { // in SIMARM(IA32) and ARM, and the same offsets in SIMARM64(X64) and ARM64. // We use only word-sized fields to avoid differences in struct packing on the // different architectures. See also CheckOffsets in dart.cc. - volatile RelaxedAtomic stack_limit_; + RelaxedAtomic stack_limit_; uword write_barrier_mask_; uword heap_base_; Isolate* isolate_; @@ -1106,6 +1107,9 @@ class Thread : public ThreadState { int32_t no_safepoint_scope_depth_; #endif VMHandles reusable_handles_; + intptr_t defer_oob_messages_count_; + uint16_t deferred_interrupts_mask_; + uint16_t deferred_interrupts_; int32_t stack_overflow_count_; uint32_t runtime_call_count_ = 0; @@ -1201,6 +1205,9 @@ class Thread : public ThreadState { static void SetCurrent(Thread* current) { OSThread::SetCurrentTLS(current); } + void DeferOOBMessageInterrupts(); + void RestoreOOBMessageInterrupts(); + #define REUSABLE_FRIEND_DECLARATION(name) \ friend class Reusable##name##HandleScope; REUSABLE_HANDLE_LIST(REUSABLE_FRIEND_DECLARATION) @@ -1211,7 +1218,9 @@ class Thread : public ThreadState { friend class InterruptChecker; friend class Isolate; friend class IsolateGroup; + friend class IsolateTestHelper; friend class NoActiveIsolateScope; + friend class NoOOBMessageScope; friend class NoReloadScope; friend class Simulator; friend class StackZone; diff --git a/tests/lib/isolate/kill_infinite_loop_in_initializer_test.dart b/tests/lib/isolate/kill_infinite_loop_in_initializer_test.dart deleted file mode 100644 index 321fbda4a49..00000000000 --- a/tests/lib/isolate/kill_infinite_loop_in_initializer_test.dart +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file -// for details. All rights reserved. Use of this source code is governed by a -// BSD-style license that can be found in the LICENSE file. - -// VMOptions=--enable-isolate-groups --experimental-enable-isolate-groups-jit -// VMOptions=--no-enable-isolate-groups - -// Regression test against out-of-band messages being blocked during lazy -// static field initialization. - -import "dart:isolate"; -import "dart:async"; -import "package:expect/expect.dart"; -import "package:async_helper/async_helper.dart"; - -dynamic staticFieldWithBadInitializer = badInitializer(); - -badInitializer() { - print("badInitializer"); - for (;;) {} - return 42; // Unreachable. -} - -child(message) { - print("child"); - RawReceivePort port = new RawReceivePort(); - print(staticFieldWithBadInitializer); - port.close(); // Unreachable. -} - -void main() { - asyncStart(); - Isolate.spawn(child, null).then((Isolate isolate) { - print("spawned"); - late RawReceivePort exitSignal; - exitSignal = new RawReceivePort((_) { - print("onExit"); - exitSignal.close(); - asyncEnd(); - }); - isolate.addOnExitListener(exitSignal.sendPort); - isolate.kill(priority: Isolate.immediate); - }); -} diff --git a/tests/lib_2/isolate/kill_infinite_loop_in_initializer_test.dart b/tests/lib_2/isolate/kill_infinite_loop_in_initializer_test.dart deleted file mode 100644 index af8650e7358..00000000000 --- a/tests/lib_2/isolate/kill_infinite_loop_in_initializer_test.dart +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file -// for details. All rights reserved. Use of this source code is governed by a -// BSD-style license that can be found in the LICENSE file. - -// VMOptions=--enable-isolate-groups --experimental-enable-isolate-groups-jit -// VMOptions=--no-enable-isolate-groups - -// Regression test against out-of-band messages being blocked during lazy -// static field initialization. - -import "dart:isolate"; -import "dart:async"; -import "package:expect/expect.dart"; -import "package:async_helper/async_helper.dart"; - -dynamic staticFieldWithBadInitializer = badInitializer(); - -badInitializer() { - print("badInitializer"); - for (;;) {} - return 42; // Unreachable. -} - -child(message) { - print("child"); - RawReceivePort port = new RawReceivePort(); - print(staticFieldWithBadInitializer); - port.close(); // Unreachable. -} - -void main() { - asyncStart(); - Isolate.spawn(child, null).then((Isolate isolate) { - print("spawned"); - RawReceivePort exitSignal; - exitSignal = new RawReceivePort((_) { - print("onExit"); - exitSignal.close(); - asyncEnd(); - }); - isolate.addOnExitListener(exitSignal.sendPort); - isolate.kill(priority: Isolate.immediate); - }); -}