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 <rmacnak@google.com>
> Reviewed-by: Ben Konyi <bkonyi@google.com>
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 <dacoharkes@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
diff --git a/runtime/platform/atomic.h b/runtime/platform/atomic.h
index ffb5bdc..98bc900 100644
--- a/runtime/platform/atomic.h
+++ b/runtime/platform/atomic.h
@@ -21,16 +21,9 @@
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 @@
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 52da48b..4a7e5de 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->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 8993e5f..cb00d29 100644
--- a/runtime/vm/isolate.cc
+++ b/runtime/vm/isolate.cc
@@ -867,6 +867,19 @@
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 4c2d9f5..c17a81d 100644
--- a/runtime/vm/isolate.h
+++ b/runtime/vm/isolate.h
@@ -134,6 +134,16 @@
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_test.cc b/runtime/vm/isolate_test.cc
index 2003591..9402106 100644
--- a/runtime/vm/isolate_test.cc
+++ b/runtime/vm/isolate_test.cc
@@ -145,4 +145,106 @@
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 8c08ad9..7c9214a 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -11162,6 +11162,7 @@
}
#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 45d99f9..16a1f8f 100644
--- a/runtime/vm/thread.cc
+++ b/runtime/vm/thread.cc
@@ -91,6 +91,9 @@
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 @@
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::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 6e3ff67..2d80830 100644
--- a/runtime/vm/thread.h
+++ b/runtime/vm/thread.h
@@ -417,10 +417,11 @@
};
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 @@
// 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<uword> stack_limit_;
+ RelaxedAtomic<uword> stack_limit_;
uword write_barrier_mask_;
uword heap_base_;
Isolate* isolate_;
@@ -1106,6 +1107,9 @@
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 @@
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 @@
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 321fbda4..0000000
--- 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 af8650e..0000000
--- 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);
- });
-}
