Fix KT-33824, ignore FreezableAtomicReference building condensation (#3365)

backend.native/tests/runtime/workers/freeze6.kt

@@ -7,6 +7,10 @@ package runtime.workers.freeze6
 
 import kotlin.test.*
 import kotlin.native.concurrent.*
+import kotlin.native.ref.*
+
+data class Hi(val s: String)
+data class Nested(val hi: Hi)
 
 @Test
 fun ensureNeverFrozenNoFreezeChild(){
@@ -30,5 +34,126 @@ fun ensureNeverFrozenFailsTarget(){
     println("OK")
 }
 
-data class Hi(val s:String)
+fun createRef1(): FreezableAtomicReference<Any?> {
-data class Nested(val hi:Hi)
+    val ref = FreezableAtomicReference<Any?>(null)
+    ref.value = ref
+    ref.freeze()
+    ref.value = null
+    return ref
+}
+
+var global = 0
+
+@Test
+fun ensureFreezableHandlesCycles1() {
+    val ref = createRef1()
+    kotlin.native.internal.GC.collect()
+
+    val obj: Any = ref
+    global = obj.hashCode()
+}
+
+class Node(var ref: Any?)
+
+/**
+ *    ref1 -> Node <- ref3
+ *             |        /\
+ *             V        |
+ *             ref2  ---
+ */
+fun createRef2(): Pair<FreezableAtomicReference<Node?>, Any> {
+    val ref1 = FreezableAtomicReference<Node?>(null)
+
+    val node = Node(null)
+    val ref3 = FreezableAtomicReference<Any?>(node)
+    val ref2 = FreezableAtomicReference<Any?>(ref3)
+
+    node.ref = ref2
+    ref1.value = node
+
+    ref1.freeze()
+    ref3.value = null
+
+    assertTrue(node.isFrozen)
+    assertTrue(ref1.isFrozen)
+    assertTrue(ref2.isFrozen)
+    assertTrue(ref3.isFrozen)
+
+    return ref1 to ref2
+}
+
+@Test
+fun ensureFreezableHandlesCycles2() {
+    val (ref, obj) = createRef2()
+    kotlin.native.internal.GC.collect()
+
+    assertTrue(obj.toString().length > 0)
+    global = ref.value!!.ref!!.hashCode()
+}
+
+fun createRef3(): FreezableAtomicReference<Any?> {
+    val ref = FreezableAtomicReference<Any?>(null)
+    val node = Node(ref)
+    ref.value = node
+    ref.freeze()
+
+    assertTrue(node.isFrozen)
+    assertTrue(ref.isFrozen)
+
+    return ref
+}
+
+@Test
+fun ensureFreezableHandlesCycles3() {
+    val ref = createRef3()
+    ref.value = null
+    kotlin.native.internal.GC.collect()
+
+    val obj: Any = ref
+    assertTrue(obj.toString().length > 0)
+    global = obj.hashCode()
+}
+
+lateinit var weakRef: WeakReference<Any>
+
+fun createRef4(): FreezableAtomicReference<Any?> {
+    val ref = FreezableAtomicReference<Any?>(null)
+    val node = Node(ref)
+    weakRef = WeakReference(node)
+    ref.value = node
+    ref.freeze()
+    assertTrue(weakRef.get() != null)
+    return ref
+}
+
+@Test
+fun ensureWeakRefNotLeaks1() {
+    val ref = createRef4()
+    ref.value = null
+    // We cannot check weakRef.get() here, as value read will be stored in the stack slot,
+    // and thus hold weak reference from release.
+    kotlin.native.internal.GC.collect()
+
+    assertTrue(weakRef.get() == null)
+}
+
+lateinit var node1: Node
+lateinit var weakNode2: WeakReference<Node>
+
+fun createRef5() {
+    val ref = FreezableAtomicReference<Any?>(null)
+    node1 = Node(ref)
+    val node2 = Node(node1)
+    weakNode2 = WeakReference(node2)
+    ref.value = node2
+    node1.freeze()
+    assertTrue(weakNode2.get() != null)
+    ref.value = null
+}
+
+@Test
+fun ensureWeakRefNotLeaks2() {
+    createRef5()
+    kotlin.native.internal.GC.collect()
+    assertTrue(weakNode2.get() == null)
+}

runtime/src/main/cpp/Memory.cpp

@@ -742,6 +742,16 @@ inline bool canFreeze(ContainerHeader* container) {
     return container != nullptr && !container->frozen();
 }
 
+inline bool isFreezableAtomic(ObjHeader* obj) {
+  return obj->type_info() == theFreezableAtomicReferenceTypeInfo;
+}
+
+inline bool isFreezableAtomic(ContainerHeader* container) {
+  RuntimeAssert(!isAggregatingFrozenContainer(container), "Must be single object");
+  ObjHeader* obj = reinterpret_cast<ObjHeader*>(container + 1);
+  return isFreezableAtomic(obj);
+}
+
 ContainerHeader* allocContainer(MemoryState* state, size_t size) {
  ContainerHeader* result = nullptr;
 #if USE_GC
@@ -943,7 +953,7 @@ void depthFirstTraversal(ContainerHeader* start, bool* hasCycles,
       continue;
     }
     toVisit.push_front(markAsRemoved(container));
-    traverseContainerReferredObjects(container, [hasCycles, firstBlocker, &order, &toVisit](ObjHeader* obj) {
+    traverseContainerReferredObjects(container, [container, hasCycles, firstBlocker, &order, &toVisit](ObjHeader* obj) {
       if (*firstBlocker != nullptr)
         return;
       if (obj->has_meta_object() && ((obj->meta_object()->flags_ & MF_NEVER_FROZEN) != 0)) {
@@ -959,7 +969,14 @@ void depthFirstTraversal(ContainerHeader* start, bool* hasCycles,
         if (!objContainer->seen() && !objContainer->marked()) {
           // Mark GRAY.
           objContainer->setSeen();
-          toVisit.push_front(objContainer);
+          // Here we do rather interesting trick: when doing DFS we postpone processing references going from
+          // FreezableAtomic, so that in 'order' referred value will be seen as not actually belonging
+          // to the same SCC (unless there are other edges not going through FreezableAtomic reaching the same value).
+          if (isFreezableAtomic(container)) {
+            toVisit.push_back(objContainer);
+          } else {
+            toVisit.push_front(objContainer);
+          }
         }
       }
     });
@@ -1503,6 +1520,7 @@ void decrementStack(MemoryState* state) {
     while (current < end) {
       ObjHeader* obj = *current++;
       if (obj != nullptr) {
+        MEMORY_LOG("decrement stack %p\n", obj)
         auto* container = obj->container();
         if (container != nullptr)
           enqueueDecrementRC</* CanCollect = */ false>(container);
@@ -1566,6 +1584,13 @@ void garbageCollect(MemoryState* state, bool force) {
   }
   GC_LOG("GC: duration=%lld sinceLast=%lld\n", (gcEndTime - gcStartTime), gcStartTime - state->lastGcTimestamp);
   state->lastGcTimestamp = gcEndTime;
+
+#if TRACE_MEMORY
+  for (auto* obj: *state->toRelease) {
+    MEMORY_LOG("toRelease %p\n", obj)
+  }
+#endif
+
   GC_LOG("<<< GC: toFree %d toRelease %d\n", state->toFree->size(), state->toRelease->size())
 }
 
@@ -2262,56 +2287,65 @@ void freezeAcyclic(ContainerHeader* rootContainer, ContainerHeaderSet* newlyFroz
   }
 }
 
-void freezeCyclic(ContainerHeader* rootContainer,
+void freezeCyclic(ObjHeader* root,
                   const KStdVector<ContainerHeader*>& order,
                   ContainerHeaderSet* newlyFrozen) {
   KStdUnorderedMap<ContainerHeader*, KStdVector<ContainerHeader*>> reversedEdges;
-  KStdDeque<ContainerHeader*> queue;
+  KStdDeque<ObjHeader*> queue;
-  queue.push_back(rootContainer);
+  queue.push_back(root);
   while (!queue.empty()) {
-    ContainerHeader* current = queue.front();
+    ObjHeader* current = queue.front();
     queue.pop_front();
-    current->unMark();
+    ContainerHeader* currentContainer = current->container();
-    reversedEdges.emplace(current, KStdVector<ContainerHeader*>(0));
+    currentContainer->unMark();
-    traverseContainerReferredObjects(current, [current, &queue, &reversedEdges](ObjHeader* obj) {
+    reversedEdges.emplace(currentContainer, KStdVector<ContainerHeader*>(0));
+    traverseContainerReferredObjects(currentContainer, [current, currentContainer, &queue, &reversedEdges](ObjHeader* obj) {
           ContainerHeader* objContainer = obj->container();
           if (canFreeze(objContainer)) {
             if (objContainer->marked())
-              queue.push_back(objContainer);
+              queue.push_back(obj);
-            reversedEdges.emplace(objContainer, KStdVector<ContainerHeader*>(0)).first->second.push_back(current);
+            // We ignore references from FreezableAtomicsReference during condensation, to avoid KT-33824.
+            if (!isFreezableAtomic(current))
+              reversedEdges.emplace(objContainer, KStdVector<ContainerHeader*>(0)).
+                first->second.push_back(currentContainer);
           }
       });
-    }
+   }
 
-    KStdVector<KStdVector<ContainerHeader*>> components;
+   KStdVector<KStdVector<ContainerHeader*>> components;
-    MEMORY_LOG("Condensation:\n");
+   MEMORY_LOG("Condensation:\n");
-    // Enumerate in the topological order.
+   // Enumerate in the topological order.
-    for (auto it = order.rbegin(); it != order.rend(); ++it) {
+   for (auto it = order.rbegin(); it != order.rend(); ++it) {
-      auto* container = *it;
+     auto* container = *it;
-      if (container->marked()) continue;
+     if (container->marked()) continue;
-      KStdVector<ContainerHeader*> component;
+     KStdVector<ContainerHeader*> component;
-      traverseStronglyConnectedComponent(container, &reversedEdges, &component);
+     traverseStronglyConnectedComponent(container, &reversedEdges, &component);
-      MEMORY_LOG("SCC:\n");
+     MEMORY_LOG("SCC:\n");
   #if TRACE_MEMORY
-      for (auto c: component)
+     for (auto c: component)
-        konan::consolePrintf("    %p\n", c);
+       konan::consolePrintf("    %p\n", c);
   #endif
-      components.push_back(std::move(component));
+     components.push_back(std::move(component));
-    }
+   }
 
-    // Enumerate strongly connected components in reversed topological order.
+  // Enumerate strongly connected components in reversed topological order.
   for (auto it = components.rbegin(); it != components.rend(); ++it) {
     auto& component = *it;
     int internalRefsCount = 0;
     int totalCount = 0;
     for (auto* container : component) {
+      RuntimeAssert(!isAggregatingFrozenContainer(container), "Must not be called on such containers");
       totalCount += container->refCount();
+      if (isFreezableAtomic(container)) {
+        RuntimeAssert(component.size() == 1, "Must be trivial condensation");
+        continue;
+      }
       traverseContainerReferredObjects(container, [&internalRefsCount](ObjHeader* obj) {
           auto* container = obj->container();
           if (canFreeze(container))
-              ++internalRefsCount;
+            ++internalRefsCount;
         });
-      }
+    }
 
     // Freeze component.
     for (auto* container : component) {
@@ -2327,11 +2361,12 @@ void freezeCyclic(ContainerHeader* rootContainer,
       // meta-object, where aggregating container is stored.
       container->setRefCount(0);
     }
+
     // Create fictitious container for the whole component.
     auto superContainer = component.size() == 1 ? component[0] : allocAggregatingFrozenContainer(component);
     // Don't count internal references.
     MEMORY_LOG("Setting aggregating %p rc to %d (total %d inner %d)\n", \
-        superContainer, totalCount - internalRefsCount, totalCount, internalRefsCount)
+       superContainer, totalCount - internalRefsCount, totalCount, internalRefsCount)
     superContainer->setRefCount(totalCount - internalRefsCount);
     newlyFrozen->insert(superContainer);
   }
@@ -2382,7 +2417,7 @@ void freezeSubgraph(ObjHeader* root) {
   ContainerHeaderSet newlyFrozen;
   // Now unmark all marked objects, and freeze them, if no cycles detected.
   if (hasCycles) {
-    freezeCyclic(rootContainer, order, &newlyFrozen);
+    freezeCyclic(root, order, &newlyFrozen);
   } else {
     freezeAcyclic(rootContainer, &newlyFrozen);
   }

runtime/src/main/cpp/Types.h

@@ -81,20 +81,22 @@ extern "C" {
 
 extern const TypeInfo* theAnyTypeInfo;
 extern const TypeInfo* theArrayTypeInfo;
+extern const TypeInfo* theBooleanArrayTypeInfo;
 extern const TypeInfo* theByteArrayTypeInfo;
 extern const TypeInfo* theCharArrayTypeInfo;
-extern const TypeInfo* theShortArrayTypeInfo;
+extern const TypeInfo* theDoubleArrayTypeInfo;
+extern const TypeInfo* theForeignObjCObjectTypeInfo;
 extern const TypeInfo* theIntArrayTypeInfo;
 extern const TypeInfo* theLongArrayTypeInfo;
+extern const TypeInfo* theNativePtrArrayTypeInfo;
 extern const TypeInfo* theFloatArrayTypeInfo;
-extern const TypeInfo* theDoubleArrayTypeInfo;
+extern const TypeInfo* theForeignObjCObjectTypeInfo;
-extern const TypeInfo* theBooleanArrayTypeInfo;
+extern const TypeInfo* theFreezableAtomicReferenceTypeInfo;
+extern const TypeInfo* theObjCObjectWrapperTypeInfo;
+extern const TypeInfo* theShortArrayTypeInfo;
 extern const TypeInfo* theStringTypeInfo;
 extern const TypeInfo* theThrowableTypeInfo;
 extern const TypeInfo* theUnitTypeInfo;
-extern const TypeInfo* theForeignObjCObjectTypeInfo;
-extern const TypeInfo* theObjCObjectWrapperTypeInfo;
-extern const TypeInfo* theNativePtrArrayTypeInfo;
 
 KBoolean IsInstance(const ObjHeader* obj, const TypeInfo* type_info) RUNTIME_PURE;
 void CheckCast(const ObjHeader* obj, const TypeInfo* type_info);

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

@@ -5,6 +5,7 @@
 
 package kotlin.native.concurrent
 
+import kotlin.native.internal.ExportTypeInfo
 import kotlin.native.internal.Frozen
 import kotlin.native.internal.NoReorderFields
 import kotlin.native.SymbolName
@@ -279,6 +280,7 @@ public class AtomicReference<T>(private var value_: T) {
  * otherwise behaves as regular box for the value.
  */
 @NoReorderFields
+@ExportTypeInfo("theFreezableAtomicReferenceTypeInfo")
 public class FreezableAtomicReference<T>(private var value_: T) {
     // A spinlock to fix potential ARC race.
     private var lock: Int = 0