Restore cycle detector (#4286)

Reimagines cycle detector removed in "Remove obsolete leak detector. (#3819)" at aae8441b6e

backend.native/tests/build.gradle

@@ -2824,6 +2824,12 @@ task memory_stable_ref_cross_thread_check(type: KonanLocalTest) {
     source = "runtime/memory/stable_ref_cross_thread_check.kt"
 }
 
+standaloneTest("cycle_detector") {
+    disabled = project.globalTestArgs.contains('-opt') || (project.testTarget == 'wasm32') // Needs debug build.
+    flags = ['-tr', '-g']
+    source = "runtime/memory/cycle_detector.kt"
+}
+
 standaloneTest("cycle_collector") {
     disabled = true // Needs USE_CYCLIC_GC, which is disabled.
     flags = ['-g']

backend.native/tests/runtime/memory/cycle_detector.kt

@@ -0,0 +1,193 @@
+import kotlin.native.concurrent.*
+import kotlin.native.internal.GC
+import kotlin.test.*
+
+class Holder(var other: Any?)
+
+class Holder2(var field1: Any?, var field2: Any?)
+
+val <T> Array<T>.description: String
+    get() {
+        val result = StringBuilder()
+        result.append('[')
+        for (elem in this) {
+            result.append(elem.toString())
+            result.append(',')
+        }
+        result.append(']')
+        return result.toString()
+    }
+
+fun assertArrayEquals(
+    expected: Array<Any>,
+    actual: Array<Any>
+): Unit {
+    val lazyMessage: () -> String? = {
+        "Expected <${expected.description}>, actual <${actual.description}>."
+    }
+
+    asserter.assertTrue(lazyMessage, expected.size == actual.size)
+    for (i in expected.indices) {
+        asserter.assertTrue(lazyMessage, expected[i] == actual[i])
+    }
+}
+
+@Test
+fun noCycles() {
+    val atomic1 = AtomicReference<Any?>(null)
+    val atomic2 = AtomicReference<Any?>(null)
+    try {
+        atomic1.value = atomic2
+        val cycles = GC.detectCycles()!!
+        assertEquals(0, cycles.size)
+        assertNull(GC.findCycle(atomic1));
+        assertNull(GC.findCycle(atomic2));
+    } finally {
+        atomic1.value = null
+        atomic2.value = null
+    }
+}
+
+@Test
+fun oneCycle() {
+    val atomic = AtomicReference<Any?>(null)
+    try {
+        atomic.value = atomic
+        val cycles = GC.detectCycles()!!
+        assertEquals(1, cycles.size)
+        assertArrayEquals(arrayOf(atomic, atomic), GC.findCycle(cycles[0])!!)
+    } finally {
+        atomic.value = null
+    }
+}
+
+@Test
+fun oneCycleWithHolder() {
+    val atomic = AtomicReference<Any?>(null)
+    try {
+        atomic.value = Holder(atomic).freeze()
+        val cycles = GC.detectCycles()!!
+        assertEquals(1, cycles.size)
+        assertArrayEquals(arrayOf(atomic, atomic.value!!, atomic), GC.findCycle(cycles[0])!!)
+        assertArrayEquals(arrayOf(atomic.value!!, atomic, atomic.value!!), GC.findCycle(atomic.value!!)!!)
+    } finally {
+        atomic.value = null
+    }
+}
+
+@Test
+fun oneCycleWithArray() {
+    val array = arrayOf(AtomicReference<Any?>(null), AtomicReference<Any?>(null))
+    try {
+        array[0].value = Holder(array).freeze()
+        val cycles = GC.detectCycles()!!
+        assertEquals(1, cycles.size)
+        assertArrayEquals(arrayOf(array[0], array[0].value!!, array, array[0]), GC.findCycle(cycles[0])!!)
+    } finally {
+        array[0].value = null
+        array[1].value = null
+    }
+}
+
+@Test
+fun oneCycleWithLongChain() {
+    val atomic = AtomicReference<Any?>(null)
+    try {
+        val head = Holder(null)
+        var current = head
+        repeat(30) {
+            val next = Holder(null)
+            current.other = next
+            current = next
+        }
+        current.other = atomic
+        atomic.value = head.freeze()
+        val cycles = GC.detectCycles()!!
+        assertEquals(1, cycles.size)
+        val cycle = GC.findCycle(cycles[0])!!
+        assertEquals(33, cycle.size)
+    } finally {
+        atomic.value = null
+    }
+}
+
+@Test
+fun twoCycles() {
+    val atomic1 = AtomicReference<Any?>(null)
+    val atomic2 = AtomicReference<Any?>(null)
+    try {
+        atomic1.value = atomic2
+        atomic2.value = atomic1
+        val cycles = GC.detectCycles()!!
+        assertEquals(2, cycles.size)
+        assertArrayEquals(arrayOf(atomic2, atomic1, atomic2), GC.findCycle(cycles[0])!!)
+        assertArrayEquals(arrayOf(atomic1, atomic2, atomic1), GC.findCycle(cycles[1])!!)
+    } finally {
+        atomic1.value = null
+        atomic2.value = null
+    }
+}
+
+@Test
+fun twoCyclesWithHolder() {
+    val atomic1 = AtomicReference<Any?>(null)
+    val atomic2 = AtomicReference<Any?>(null)
+    try {
+        atomic1.value = atomic2
+        atomic2.value = Holder(atomic1).freeze()
+        val cycles = GC.detectCycles()!!
+        assertEquals(2, cycles.size)
+        assertArrayEquals(arrayOf(atomic2, atomic2.value!!, atomic1, atomic2), GC.findCycle(cycles[0])!!)
+        assertArrayEquals(arrayOf(atomic1, atomic2, atomic2.value!!, atomic1), GC.findCycle(cycles[1])!!)
+    } finally {
+        atomic1.value = null
+        atomic2.value = null
+    }
+}
+
+@Test
+fun threeSeparateCycles() {
+    val atomic1 = AtomicReference<Any?>(null)
+    val atomic2 = AtomicReference<Any?>(null)
+    val atomic3 = AtomicReference<Any?>(null)
+    try {
+        atomic1.value = atomic1
+        atomic2.value = Holder2(atomic1, atomic2).freeze()
+        atomic3.value = Holder2(atomic3, atomic1).freeze()
+        val cycles = GC.detectCycles()!!
+        assertEquals(3, cycles.size)
+        assertArrayEquals(arrayOf(atomic3, atomic3.value!!, atomic3), GC.findCycle(cycles[0])!!)
+        assertArrayEquals(arrayOf(atomic2, atomic2.value!!, atomic2), GC.findCycle(cycles[1])!!)
+        assertArrayEquals(arrayOf(atomic1, atomic1), GC.findCycle(cycles[2])!!)
+    } finally {
+        atomic1.value = null
+        atomic2.value = null
+        atomic3.value = null
+    }
+}
+
+@Test
+fun noCyclesWithFreezableAtomicReference() {
+    val atomic = FreezableAtomicReference<Any?>(null)
+    try {
+        atomic.value = atomic
+        val cycles = GC.detectCycles()!!
+        assertEquals(0, cycles.size)
+    } finally {
+        atomic.value = null
+    }
+}
+
+@Test
+fun oneCycleWithFrozenFreezableAtomicReference() {
+    val atomic = FreezableAtomicReference<Any?>(null)
+    try {
+        atomic.value = atomic
+        atomic.freeze()
+        val cycles = GC.detectCycles()!!
+        assertEquals(1, cycles.size)
+        assertArrayEquals(arrayOf(atomic, atomic), GC.findCycle(cycles[0])!!)
+    } finally {
+        atomic.value = null
+    }
+}

runtime/src/main/cpp/Memory.cpp

@@ -35,6 +35,7 @@
 #include "Natives.h"
 #include "Porting.h"
 #include "Runtime.h"
+#include "Utils.h"
 #include "WorkerBoundReference.h"
 
 // If garbage collection algorithm for cyclic garbage to be used.
@@ -132,6 +133,103 @@ volatile int aliveMemoryStatesCount = 0;
 KBoolean g_hasCyclicCollector = true;
 #endif  // USE_CYCLIC_GC
 
+// TODO: Consider using ObjHolder.
+class ScopedRefHolder {
+ public:
+  ScopedRefHolder() = default;
+
+  explicit ScopedRefHolder(KRef obj);
+
+  ScopedRefHolder(const ScopedRefHolder&) = delete;
+
+  ScopedRefHolder(ScopedRefHolder&& other) noexcept: obj_(other.obj_) {
+    other.obj_ = nullptr;
+  }
+
+  ScopedRefHolder& operator=(const ScopedRefHolder&) = delete;
+
+  ScopedRefHolder& operator=(ScopedRefHolder&& other) noexcept {
+    ScopedRefHolder tmp(std::move(other));
+    swap(tmp);
+    return *this;
+  }
+
+  ~ScopedRefHolder();
+
+  void swap(ScopedRefHolder& other) noexcept {
+    std::swap(obj_, other.obj_);
+  }
+
+ private:
+  KRef obj_ = nullptr;
+};
+
+struct CycleDetectorRootset {
+  // Orders roots.
+  KStdVector<KRef> roots;
+  // Pins a state of each root.
+  KStdUnorderedMap<KRef, KStdVector<KRef>> rootToFields;
+  // Holding roots and their fields to avoid GC-ing them.
+  KStdVector<ScopedRefHolder> heldRefs;
+};
+
+class CycleDetector {
+ public:
+  static void insertCandidateIfNeeded(KRef object) {
+    if (canBeACandidate(object))
+      instance().insertCandidate(object);
+  }
+
+  static void removeCandidateIfNeeded(KRef object) {
+    if (canBeACandidate(object))
+      instance().removeCandidate(object);
+  }
+
+  static CycleDetectorRootset collectRootset();
+
+ private:
+  CycleDetector() = default;
+  ~CycleDetector() = default;
+  CycleDetector(const CycleDetector&) = delete;
+  CycleDetector(CycleDetector&&) = delete;
+  CycleDetector& operator=(const CycleDetector&) = delete;
+  CycleDetector& operator=(CycleDetector&&) = delete;
+
+  static CycleDetector& instance() {
+    // Only store a pointer to CycleDetector in .bss
+    static CycleDetector* result = new CycleDetector();
+    return *result;
+  }
+
+  static bool canBeACandidate(KRef object) {
+    return KonanNeedDebugInfo &&
+        Kotlin_memoryLeakCheckerEnabled() &&
+        (object->type_info()->flags_ & TF_LEAK_DETECTOR_CANDIDATE) != 0;
+  }
+
+  void insertCandidate(KRef candidate) {
+    LockGuard<SimpleMutex> guard(lock_);
+
+    auto it = candidateList_.insert(candidateList_.begin(), candidate);
+    candidateInList_.emplace(candidate, it);
+  }
+
+  void removeCandidate(KRef candidate) {
+    LockGuard<SimpleMutex> guard(lock_);
+
+    auto it = candidateInList_.find(candidate);
+    if (it == candidateInList_.end())
+      return;
+    candidateList_.erase(it->second);
+    candidateInList_.erase(it);
+  }
+
+  SimpleMutex lock_;
+  using CandidateList = KStdList<KRef>;
+  CandidateList candidateList_;
+  KStdUnorderedMap<KRef, CandidateList::iterator> candidateInList_;
+};
+
 // TODO: can we pass this variable as an explicit argument?
 THREAD_LOCAL_VARIABLE MemoryState* memoryState = nullptr;
 THREAD_LOCAL_VARIABLE FrameOverlay* currentFrame = nullptr;
@@ -967,6 +1065,7 @@ ALWAYS_INLINE void runDeallocationHooks(ContainerHeader* container) {
       cyclicRemoveAtomicRoot(obj);
     }
 #endif  // USE_CYCLIC_GC
+    CycleDetector::removeCandidateIfNeeded(obj);
     if (obj->has_meta_object()) {
       ObjHeader::destroyMetaObject(&obj->typeInfoOrMeta_);
     }
@@ -2036,6 +2135,7 @@ OBJ_GETTER(allocInstance, const TypeInfo* type_info) {
     makeShareable(container.header());
   }
 #endif  // USE_GC
+  CycleDetector::insertCandidateIfNeeded(obj);
 #if USE_CYCLIC_GC
   if ((obj->type_info()->flags_ & TF_LEAK_DETECTOR_CANDIDATE) != 0) {
     // Note: this should be performed after [rememberNewContainer] (above).
@@ -2715,6 +2815,125 @@ void shareAny(ObjHeader* obj) {
   container->makeShared();
 }
 
+ScopedRefHolder::ScopedRefHolder(KRef obj): obj_(obj) {
+  if (obj_) {
+    addHeapRef(obj_);
+  }
+}
+
+ScopedRefHolder::~ScopedRefHolder() {
+  if (obj_) {
+    ReleaseHeapRef(obj_);
+  }
+}
+
+// static
+CycleDetectorRootset CycleDetector::collectRootset() {
+  auto& detector = instance();
+  CycleDetectorRootset rootset;
+  LockGuard<SimpleMutex> guard(detector.lock_);
+  for (auto* candidate: detector.candidateList_) {
+    // Only frozen candidates are to be analyzed.
+    if (!isPermanentOrFrozen(candidate))
+      continue;
+    rootset.roots.push_back(candidate);
+    rootset.heldRefs.emplace_back(candidate);
+    traverseReferredObjects(candidate, [&rootset, candidate](KRef field) {
+      rootset.rootToFields[candidate].push_back(field);
+      // TODO: There's currently a race here:
+      // some other thread might null this field and destroy it in GC before
+      // we put it in ScopedRefHolder.
+      rootset.heldRefs.emplace_back(field);
+    });
+  }
+  return rootset;
+}
+
+KStdVector<KRef> findCycleWithDFS(KRef root, const CycleDetectorRootset& rootset) {
+  auto traverseFields = [&rootset](KRef obj, auto process) {
+    auto it = rootset.rootToFields.find(obj);
+    // If obj is in the rootset, use it's pinned state.
+    if (it != rootset.rootToFields.end()) {
+      const auto& fields = it->second;
+      for (KRef field: fields) {
+        if (field != nullptr) {
+          process(field);
+        }
+      }
+      return;
+    }
+
+    traverseReferredObjects(obj, process);
+  };
+
+  KStdVector<KStdVector<KRef>> toVisit;
+  auto appendFieldsToVisit = [&toVisit, &traverseFields](KRef obj, const KStdVector<KRef>& currentPath) {
+    traverseFields(obj, [&toVisit, &currentPath](KRef field) {
+      auto path = currentPath;
+      path.push_back(field);
+      toVisit.emplace_back(std::move(path));
+    });
+  };
+
+  appendFieldsToVisit(root, KRefList(1, root));
+
+  KStdUnorderedSet<KRef> seen;
+  seen.insert(root);
+  while (!toVisit.empty()) {
+    KStdVector<KRef> currentPath = std::move(toVisit.back());
+    toVisit.pop_back();
+    KRef node = currentPath[currentPath.size() - 1];
+
+    if (node == root) {
+      // Found a cycle.
+      return currentPath;
+    }
+
+    // Already traversed this node.
+    if (seen.count(node) != 0)
+      continue;
+    seen.insert(node);
+
+    appendFieldsToVisit(node, currentPath);
+  }
+
+  return {};
+}
+
+template <typename C>
+OBJ_GETTER(createAndFillArray, const C& container) {
+  auto* result = AllocArrayInstance(theArrayTypeInfo, container.size(), OBJ_RESULT)->array();
+  KRef* place = ArrayAddressOfElementAt(result, 0);
+  for (KRef it: container) {
+    UpdateHeapRef(place++, it);
+  }
+  RETURN_OBJ(result->obj());
+}
+
+OBJ_GETTER0(detectCyclicReferences) {
+  auto rootset = CycleDetector::collectRootset();
+
+  KStdVector<KRef> cyclic;
+
+  for (KRef root: rootset.roots) {
+    if (!findCycleWithDFS(root, rootset).empty()) {
+      cyclic.push_back(root);
+    }
+  }
+
+  RETURN_RESULT_OF(createAndFillArray, cyclic);
+}
+
+OBJ_GETTER(findCycle, KRef root) {
+  auto rootset = CycleDetector::collectRootset();
+
+  auto cycle = findCycleWithDFS(root, rootset);
+  if (cycle.empty()) {
+    RETURN_OBJ(nullptr);
+  }
+  RETURN_RESULT_OF(createAndFillArray, cycle);
+}
+
 }  // namespace
 
 MetaObjHeader* ObjHeader::createMetaObject(TypeInfo** location) {
@@ -3142,6 +3361,15 @@ KBoolean Kotlin_native_internal_GC_getTuneThreshold(KRef) {
 #endif
 }
 
+OBJ_GETTER(Kotlin_native_internal_GC_detectCycles, KRef) {
+  if (!KonanNeedDebugInfo || !Kotlin_memoryLeakCheckerEnabled()) RETURN_OBJ(nullptr);
+  RETURN_RESULT_OF0(detectCyclicReferences);
+}
+
+OBJ_GETTER(Kotlin_native_internal_GC_findCycle, KRef, KRef root) {
+  RETURN_RESULT_OF(findCycle, root);
+}
+
 KNativePtr CreateStablePointer(KRef any) {
   return createStablePointer(any);
 }

runtime/src/main/cpp/Types.h

@@ -25,6 +25,7 @@
 #endif
 
 #include <deque>
+#include <list>
 #include <map>
 #include <string>
 #include <set>
@@ -78,6 +79,8 @@ template<class Key, class Value, class Compare = std::less<Key>>
 using KStdOrderedMap = std::map<Key, Value, Compare, KonanAllocator<std::pair<const Key, Value>>>;
 template<class Value>
 using KStdVector = std::vector<Value, KonanAllocator<Value>>;
+template<class Value>
+using KStdList = std::list<Value, KonanAllocator<Value>>;
 
 #ifdef __cplusplus
 extern "C" {

runtime/src/main/kotlin/kotlin/native/concurrent/Atomics.kt

@@ -216,7 +216,8 @@ private fun debugString(value: Any?): String {
 /**
  * An atomic reference to a frozen Kotlin object. Can be used in concurrent scenarious
  * but frequently shall be of nullable type and be zeroed out once no longer needed.
- * Otherwise memory leak could happen if the atomic reference is a part of a reference cycle.
+ * Otherwise memory leak could happen. To detect such leaks [kotlin.native.internal.GC.detectCycles]
+ * in debug mode could be helpful.
  */
 @Frozen
 @LeakDetectorCandidate
@@ -293,7 +294,8 @@ public class AtomicReference<T> {
 /**
  * An atomic reference to a Kotlin object. Can be used in concurrent scenarious, but must be frozen first,
  * otherwise behaves as regular box for the value. If frozen, shall be zeroed out once no longer needed.
- * Otherwise memory leak could happen if atomic reference is a part of a reference cycle.
+ * Otherwise memory leak could happen. To detect such leaks [kotlin.native.internal.GC.detectCycles]
+ * in debug mode could be helpful.
  */
 @NoReorderFields
 @LeakDetectorCandidate

runtime/src/main/kotlin/kotlin/native/internal/GC.kt

@@ -100,6 +100,20 @@ object GC {
         get() = getCyclicCollectorEnabled()
         set(value) = setCyclicCollectorEnabled(value)
 
+    /**
+     * Detect cyclic references going via atomic references and return list of cycle-inducing objects
+     * or `null` if the leak detector is not available. Use [Platform.isMemoryLeakCheckerActive] to check
+     * leak detector availability.
+     */
+    @SymbolName("Kotlin_native_internal_GC_detectCycles")
+    external fun detectCycles(): Array<Any>?
+
+    /**
+     * Find a reference cycle including from the given object, `null` if no cycles detected.
+     */
+    @SymbolName("Kotlin_native_internal_GC_findCycle")
+    external fun findCycle(root: Any): Array<Any>?
+
     @SymbolName("Kotlin_native_internal_GC_getThreshold")
     private external fun getThreshold(): Int
 
@@ -129,4 +143,4 @@ object GC {
 
     @SymbolName("Kotlin_native_internal_GC_setCyclicCollector")
     private external fun setCyclicCollectorEnabled(value: Boolean)
-}
+}