[vm] Don't block OOB messages or reload during field initialization; make interrupts lock-free.
- Account for initialization-in-progress sentinel when checking static field types for reload.
- Don't read the true stack limit when setting or clearing interrupts.
TEST=ci
Bug: https://github.com/dart-lang/sdk/issues/46596
Change-Id: I80adb4d7d69f01125b7eae8215b5da4d2e467bda
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/206662
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
diff --git a/runtime/platform/atomic.h b/runtime/platform/atomic.h
index 98bc900..ffb5bdc 100644
--- a/runtime/platform/atomic.h
+++ b/runtime/platform/atomic.h
@@ -21,9 +21,16 @@
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);
@@ -44,6 +51,12 @@
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 4a7e5de..52da48b 100644
--- a/runtime/vm/heap/safepoint.cc
+++ b/runtime/vm/heap/safepoint.cc
@@ -155,7 +155,7 @@
if (!Thread::IsAtSafepoint(level_, state)) {
// Send OOB message to get it to safepoint.
if (current->IsMutatorThread()) {
- current->ScheduleInterruptsLocked(Thread::kVMInterrupt);
+ current->ScheduleInterrupts(Thread::kVMInterrupt);
}
MonitorLocker sl(&parked_lock_);
num_threads_not_parked_++;
diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc
index cb00d29..8993e5f 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -867,19 +867,6 @@
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 c17a81d..4c2d9f5 100644
--- a/runtime/vm/isolate.h
+++ b/runtime/vm/isolate.h
@@ -134,16 +134,6 @@
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<intptr_t, ExceptionHandlerInfo, 16> HandlerInfoCache;
// Fixed cache for catch entry state lookup.
diff --git a/runtime/vm/isolate_reload.cc b/runtime/vm/isolate_reload.cc
index 4bbf74e..7efed86 100644
--- a/runtime/vm/isolate_reload.cc
+++ b/runtime/vm/isolate_reload.cc
@@ -2147,7 +2147,8 @@
// At that point it doesn't have the field table setup yet.
if (field_table->IsReadyToUse()) {
value_ = field_table->At(field_id);
- if (value_.ptr() != Object::sentinel().ptr()) {
+ if ((value_.ptr() != Object::sentinel().ptr()) &&
+ (value_.ptr() != Object::transition_sentinel().ptr())) {
CheckValueType(null_safety, value_, field);
}
}
diff --git a/runtime/vm/isolate_test.cc b/runtime/vm/isolate_test.cc
index 9402106..2003591 100644
--- a/runtime/vm/isolate_test.cc
+++ b/runtime/vm/isolate_test.cc
@@ -145,106 +145,4 @@
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 77f8e4d..3f6d0f7 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -11156,8 +11156,7 @@
}
ObjectPtr Field::EvaluateInitializer() const {
- Thread* const thread = Thread::Current();
- ASSERT(thread->IsMutatorThread());
+ ASSERT(Thread::Current()->IsMutatorThread());
#if !defined(DART_PRECOMPILED_RUNTIME)
if (is_static() && is_const()) {
@@ -11165,8 +11164,6 @@
}
#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 16a1f8f..d03ff5a 100644
--- a/runtime/vm/thread.cc
+++ b/runtime/vm/thread.cc
@@ -91,9 +91,6 @@
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),
@@ -398,7 +395,7 @@
MonitorLocker ml(&thread_lock_);
if (!HasScheduledInterrupts()) {
// No interrupt pending, set stack_limit_ too.
- stack_limit_ = limit;
+ stack_limit_.store(limit);
}
saved_stack_limit_ = limit;
}
@@ -407,96 +404,40 @@
SetStackLimit(~static_cast<uword>(0));
}
-void Thread::ScheduleInterrupts(uword interrupt_bits) {
- MonitorLocker ml(&thread_lock_);
- ScheduleInterruptsLocked(interrupt_bits);
+static bool IsInterruptLimit(uword limit) {
+ return (limit & ~Thread::kInterruptsMask) ==
+ (kInterruptStackLimit & ~Thread::kInterruptsMask);
}
-void Thread::ScheduleInterruptsLocked(uword interrupt_bits) {
- ASSERT(thread_lock_.IsOwnedByCurrentThread());
+void Thread::ScheduleInterrupts(uword interrupt_bits) {
ASSERT((interrupt_bits & ~kInterruptsMask) == 0); // Must fit in mask.
- // 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;
+ uword old_limit = stack_limit_.load();
+ uword new_limit;
+ do {
+ if (IsInterruptLimit(old_limit)) {
+ new_limit = old_limit | interrupt_bits;
+ } else {
+ new_limit = (kInterruptStackLimit & ~kInterruptsMask) | interrupt_bits;
}
- }
-
- if (stack_limit_ == saved_stack_limit_) {
- stack_limit_ = (kInterruptStackLimit & ~kInterruptsMask) | interrupt_bits;
- } else {
- stack_limit_ = stack_limit_ | interrupt_bits;
- }
+ } while (!stack_limit_.compare_exchange_weak(old_limit, new_limit));
}
uword Thread::GetAndClearInterrupts() {
- 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_;
+ uword interrupt_bits = 0;
+ uword old_limit = stack_limit_.load();
+ uword new_limit = saved_stack_limit_;
+ do {
+ if (IsInterruptLimit(old_limit)) {
+ interrupt_bits = interrupt_bits | (old_limit & kInterruptsMask);
+ } else {
+ return interrupt_bits;
+ }
+ } while (!stack_limit_.compare_exchange_weak(old_limit, new_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 2d80830..6e3ff67 100644
--- a/runtime/vm/thread.h
+++ b/runtime/vm/thread.h
@@ -417,11 +417,10 @@
};
void ScheduleInterrupts(uword interrupt_bits);
- void ScheduleInterruptsLocked(uword interrupt_bits);
ErrorPtr HandleInterrupts();
uword GetAndClearInterrupts();
bool HasScheduledInterrupts() const {
- return (stack_limit_ & kInterruptsMask) != 0;
+ return (stack_limit_.load() & kInterruptsMask) != 0;
}
// Monitor corresponding to this thread.
@@ -1031,7 +1030,7 @@
// 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.
- RelaxedAtomic<uword> stack_limit_;
+ volatile RelaxedAtomic<uword> stack_limit_;
uword write_barrier_mask_;
uword heap_base_;
Isolate* isolate_;
@@ -1107,9 +1106,6 @@
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;
@@ -1205,9 +1201,6 @@
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)
@@ -1218,9 +1211,7 @@
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
new file mode 100644
index 0000000..321fbda4
--- /dev/null
+++ b/tests/lib/isolate/kill_infinite_loop_in_initializer_test.dart
@@ -0,0 +1,44 @@
+// 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
new file mode 100644
index 0000000..19cf26b
--- /dev/null
+++ b/tests/lib_2/isolate/kill_infinite_loop_in_initializer_test.dart
@@ -0,0 +1,46 @@
+// 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.
+
+// @dart = 2.9
+
+// 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);
+ });
+}