From 02eb63162f21e1a3be044f0a03a56e22d5cf9780 Mon Sep 17 00:00:00 2001 From: Alexander Shabalin Date: Thu, 12 Mar 2020 17:30:14 +0300 Subject: [PATCH] Migrate StablePtr to KRefSharedHolder (#3966) This PR also fixes tests from #3969 --- backend.native/tests/build.gradle | 4 ++ .../memory/stable_ref_cross_thread_check.kt | 48 +++++++++++++++++++ runtime/src/main/cpp/Interop.cpp | 13 +++-- runtime/src/main/cpp/Memory.cpp | 12 +++-- runtime/src/main/cpp/MemoryPrivate.hpp | 4 ++ runtime/src/main/cpp/MemorySharedRefs.cpp | 10 +++- runtime/src/main/cpp/MemorySharedRefs.hpp | 5 +- 7 files changed, 84 insertions(+), 12 deletions(-) create mode 100644 backend.native/tests/runtime/memory/stable_ref_cross_thread_check.kt diff --git a/backend.native/tests/build.gradle b/backend.native/tests/build.gradle index c44954be029..c2cc7625ce7 100644 --- a/backend.native/tests/build.gradle +++ b/backend.native/tests/build.gradle @@ -2742,6 +2742,10 @@ standaloneTest("memory_only_gc") { source = "runtime/memory/only_gc.kt" } +task memory_stable_ref_cross_thread_check(type: KonanLocalTest) { + source = "runtime/memory/stable_ref_cross_thread_check.kt" +} + standaloneTest("cycle_collector") { disabled = project.globalTestArgs.contains('-opt') || (project.testTarget == 'wasm32') // Needs debug build. flags = ['-g'] diff --git a/backend.native/tests/runtime/memory/stable_ref_cross_thread_check.kt b/backend.native/tests/runtime/memory/stable_ref_cross_thread_check.kt new file mode 100644 index 00000000000..7634a3c6a8d --- /dev/null +++ b/backend.native/tests/runtime/memory/stable_ref_cross_thread_check.kt @@ -0,0 +1,48 @@ +/* + * Copyright 2010-2020 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license + * that can be found in the LICENSE file. + */ + +package runtime.memory.stable_ref_cross_thread_check + +import kotlin.test.* + +import kotlin.native.concurrent.* +import kotlinx.cinterop.* + +@Test +fun runTest1() { + val worker = Worker.start() + + val future = worker.execute(TransferMode.SAFE, { }) { + StableRef.create(Any()) + } + val ref = future.result + assertFailsWith { + val value = ref.get() + println(value.toString()) + } + + worker.requestTermination().result +} + +@Test +fun runTest2() { + val worker = Worker.start() + + val mainThreadRef = StableRef.create(Any()) + // Simulate this going through interop as raw C pointer. + val pointerValue: Long = mainThreadRef.asCPointer().toLong() + val future = worker.execute(TransferMode.SAFE, { pointerValue }) { + val pointer: COpaquePointer = it.toCPointer()!! + assertFailsWith { + // Even attempting to convert a pointer to StableRef should fail. + val otherThreadRef: StableRef = pointer.asStableRef() + println(otherThreadRef.toString()) + } + Unit + } + future.result + + worker.requestTermination().result +} diff --git a/runtime/src/main/cpp/Interop.cpp b/runtime/src/main/cpp/Interop.cpp index eaad8762600..dea5b3415e5 100644 --- a/runtime/src/main/cpp/Interop.cpp +++ b/runtime/src/main/cpp/Interop.cpp @@ -18,21 +18,28 @@ #include #include +#include "Alloc.h" #include "Memory.h" +#include "MemorySharedRefs.hpp" #include "Types.h" extern "C" { KNativePtr Kotlin_Interop_createStablePointer(KRef any) { - return CreateStablePointer(any); + KRefSharedHolder* holder = konanConstructInstance(); + holder->init(any); + return holder; } void Kotlin_Interop_disposeStablePointer(KNativePtr pointer) { - DisposeStablePointer(pointer); + KRefSharedHolder* holder = reinterpret_cast(pointer); + holder->dispose(); + konanDestructInstance(holder); } OBJ_GETTER(Kotlin_Interop_derefStablePointer, KNativePtr pointer) { - RETURN_RESULT_OF(DerefStablePointer, pointer); + KRefSharedHolder* holder = reinterpret_cast(pointer); + RETURN_OBJ(holder->ref()); } } diff --git a/runtime/src/main/cpp/Memory.cpp b/runtime/src/main/cpp/Memory.cpp index 2ac2e7cc1ef..86fc53b1ebd 100644 --- a/runtime/src/main/cpp/Memory.cpp +++ b/runtime/src/main/cpp/Memory.cpp @@ -2279,10 +2279,7 @@ void disposeStablePointer(KNativePtr pointer) { OBJ_GETTER(derefStablePointer, KNativePtr pointer) { KRef ref = reinterpret_cast(pointer); -#if USE_GC - if (IsStrictMemoryModel && ref != nullptr) - rememberNewContainer(ref->container()); -#endif // USE_GC + AdoptReferenceFromSharedVariable(ref); RETURN_OBJ(ref); } @@ -2758,6 +2755,13 @@ bool IsForeignRefAccessible(ObjHeader* object, ForeignRefContext context) { return isForeignRefAccessible(object, context); } +void AdoptReferenceFromSharedVariable(ObjHeader* object) { +#if USE_GC + if (IsStrictMemoryModel && object != nullptr && isShareable(object->container())) + rememberNewContainer(object->container()); +#endif // USE_GC +} + // Public memory interface. MemoryState* InitMemory() { return initMemory(); diff --git a/runtime/src/main/cpp/MemoryPrivate.hpp b/runtime/src/main/cpp/MemoryPrivate.hpp index aa43e61ca97..76187da9ad4 100644 --- a/runtime/src/main/cpp/MemoryPrivate.hpp +++ b/runtime/src/main/cpp/MemoryPrivate.hpp @@ -36,6 +36,10 @@ void DeinitForeignRef(ObjHeader* object, ForeignRefContext context); bool IsForeignRefAccessible(ObjHeader* object, ForeignRefContext context); +// Should be used when reference is read from a possibly shared variable, +// and there's nothing else keeping the object alive. +void AdoptReferenceFromSharedVariable(ObjHeader* object); + } // extern "C" #endif // RUNTIME_MEMORYPRIVATE_HPP diff --git a/runtime/src/main/cpp/MemorySharedRefs.cpp b/runtime/src/main/cpp/MemorySharedRefs.cpp index fab850cf73a..d08a633a098 100644 --- a/runtime/src/main/cpp/MemorySharedRefs.cpp +++ b/runtime/src/main/cpp/MemorySharedRefs.cpp @@ -22,6 +22,7 @@ void KRefSharedHolder::init(ObjHeader* obj) { ObjHeader* KRefSharedHolder::ref() const { ensureRefAccessible(); + AdoptReferenceFromSharedVariable(obj_); return obj_; } @@ -76,7 +77,8 @@ void BackRefFromAssociatedObject::addRef() { bool BackRefFromAssociatedObject::tryAddRef() { // Suboptimal but simple: - ObjHeader* obj = this->ref(); + this->ensureRefAccessible(); + ObjHeader* obj = this->obj_; if (!TryAddHeapRef(obj)) return false; this->addRef(); @@ -97,6 +99,12 @@ void BackRefFromAssociatedObject::releaseRef() { } } +ObjHeader* BackRefFromAssociatedObject::ref() const { + ensureRefAccessible(); + AdoptReferenceFromSharedVariable(obj_); + return obj_; +} + void BackRefFromAssociatedObject::ensureRefAccessible() const { ensureForeignRefAccessible(obj_, context_); } diff --git a/runtime/src/main/cpp/MemorySharedRefs.hpp b/runtime/src/main/cpp/MemorySharedRefs.hpp index d8649e07c2f..bd291263e61 100644 --- a/runtime/src/main/cpp/MemorySharedRefs.hpp +++ b/runtime/src/main/cpp/MemorySharedRefs.hpp @@ -35,10 +35,7 @@ class BackRefFromAssociatedObject { void releaseRef(); - ObjHeader* ref() const { - ensureRefAccessible(); - return obj_; - } + ObjHeader* ref() const; inline bool permanent() const { return obj_->permanent(); // Safe to query from any thread.