diff --git a/kotlin-native/runtime/src/mm/cpp/SafePoint.cpp b/kotlin-native/runtime/src/mm/cpp/SafePoint.cpp index c6bc281d876..0e1c2277a25 100644 --- a/kotlin-native/runtime/src/mm/cpp/SafePoint.cpp +++ b/kotlin-native/runtime/src/mm/cpp/SafePoint.cpp @@ -81,17 +81,17 @@ mm::SafePointActivator::~SafePointActivator() { } } -ALWAYS_INLINE void mm::safePoint() noexcept { +ALWAYS_INLINE void mm::safePoint(std::memory_order fastPathOrder) noexcept { AssertThreadState(ThreadState::kRunnable); - auto action = safePointAction.load(std::memory_order_relaxed); + auto action = safePointAction.load(fastPathOrder); if (__builtin_expect(action != nullptr, false)) { slowPath(); } } -ALWAYS_INLINE void mm::safePoint(mm::ThreadData& threadData) noexcept { +ALWAYS_INLINE void mm::safePoint(mm::ThreadData& threadData, std::memory_order fastPathOrder) noexcept { AssertThreadState(&threadData, ThreadState::kRunnable); - auto action = safePointAction.load(std::memory_order_relaxed); + auto action = safePointAction.load(fastPathOrder); if (__builtin_expect(action != nullptr, false)) { slowPath(threadData); } diff --git a/kotlin-native/runtime/src/mm/cpp/SafePoint.hpp b/kotlin-native/runtime/src/mm/cpp/SafePoint.hpp index 0eb760eb07a..19c708ce0f8 100644 --- a/kotlin-native/runtime/src/mm/cpp/SafePoint.hpp +++ b/kotlin-native/runtime/src/mm/cpp/SafePoint.hpp @@ -5,6 +5,7 @@ #pragma once +#include #include #include "Utils.hpp" @@ -32,8 +33,8 @@ private: bool active_; }; -void safePoint() noexcept; -void safePoint(ThreadData& threadData) noexcept; +void safePoint(std::memory_order fastPathOrder = std::memory_order_relaxed) noexcept; +void safePoint(ThreadData& threadData, std::memory_order fastPathOrder = std::memory_order_relaxed) noexcept; namespace test_support { diff --git a/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.cpp b/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.cpp index 4a4129cc08d..a13e6919970 100644 --- a/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.cpp +++ b/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.cpp @@ -30,8 +30,15 @@ std::atomic kotlin::mm::internal::gSuspensionRequested = false; kotlin::ThreadState kotlin::mm::ThreadSuspensionData::setState(kotlin::ThreadState newState) noexcept { ThreadState oldState = state_.exchange(newState); if (oldState == ThreadState::kNative && newState == ThreadState::kRunnable) { - // must use already acquired ThreadData because TLS may be in invalid state e.g. during thread detach - safePoint(threadData_); + // Must use already acquired `ThreadData` because TLS may be in invalid state e.g. during thread detach. + // Also, this must load SP in sequentially consistent order, because GC + // may have touched this thread's data, and we must synchronize before + // continuing. + // GC would have either changed stored SP handler (with seq_cst), + // or would have changed `internal::gSuspensionRequested` (with seq_cst), + // so, loading SP here, or checking `internal::gSuspensionRequested` in + // `suspendIfRequested` is enough. + safePoint(threadData_, std::memory_order_seq_cst); } return oldState; } diff --git a/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.hpp b/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.hpp index 63b3842c7c4..bfd2175a869 100644 --- a/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.hpp +++ b/kotlin-native/runtime/src/mm/cpp/ThreadSuspension.hpp @@ -22,7 +22,8 @@ extern std::atomic gSuspensionRequested; } // namespace internal inline bool IsThreadSuspensionRequested() noexcept { - // TODO: Consider using a more relaxed memory order. + // Must use seq_cst ordering for synchronization with GC + // in native->runnable transition. return internal::gSuspensionRequested.load(); }