Fix weak-key hash ephemeron semantics

src/hx/Hash.h

@@ -270,6 +270,7 @@ struct HashRoot : public Object
     HX_IS_INSTANCE_OF enum { _hx_ClassId = hx::clsIdHash };
 
    virtual void updateAfterGc() = 0;
+   virtual bool markWeakValues(MarkContext *__inCtx) = 0;
 
    inline int getSize() { return size; }
 };
@@ -352,6 +353,46 @@ struct Hash : public HashBase< typename ELEMENT::Key >
    bool TIsWeakRefValid(Dynamic &key) { return IsWeakRefValid(key.mPtr); }
    bool TIsWeakRefValid(String &key) { return IsWeakRefValid(key.raw_ptr()); }
 
+   template<typename T>
+   bool TMarkWeakValue(T &, MarkContext *) { return false; }
+   bool TMarkWeakValue(Dynamic &inValue, MarkContext *__inCtx)
+   {
+      hx::Object *value = inValue.mPtr;
+      if (!value || (((unsigned int *)value)[-1] & hx::gPrevMarkIdMask))
+         return false;
+
+      HX_MARK_MEMBER(inValue);
+      return true;
+   }
+   bool TMarkWeakValue(String &inValue, MarkContext *__inCtx)
+   {
+      const HX_CHAR *value = inValue.raw_ptr();
+      if (!value || (((unsigned int *)value)[-1] & hx::gPrevMarkIdMask))
+         return false;
+
+      HX_MARK_MEMBER(inValue);
+      return true;
+   }
+
+   bool markWeakValues(MarkContext *__inCtx) HXCPP_OVERRIDE
+   {
+      if (!Element::WeakKeys || !Element::ManageKeys)
+         return false;
+
+      bool marked = false;
+      for(int b=0;b<bucketCount;b++)
+      {
+         Element *element = bucket[b];
+         while(element)
+         {
+            if (TIsWeakRefValid(element->key))
+               marked = TMarkWeakValue(element->value, __inCtx) || marked;
+            element = element->next;
+         }
+      }
+      return marked;
+   }
+
 
    void updateAfterGc() HXCPP_OVERRIDE
    {
@@ -787,8 +828,8 @@ struct Hash : public HashBase< typename ELEMENT::Key >
          if (!Hash::Element::WeakKeys)
          {
             HX_MARK_MEMBER(inElem->key);
+            HX_MARK_MEMBER(inElem->value);
          }
-         HX_MARK_MEMBER(inElem->value);
       }
    };
 

src/hx/gc/Immix.cpp

@@ -4772,6 +4772,32 @@ class GlobalAllocator
 
       hx::FindZombies(mMarker);
 
+      // Weak-key hashes are ephemerons: a value is reachable only when both
+      // the hash and its key are reachable. Values marked here may in turn
+      // make keys in other weak hashes reachable, so iterate to a fixed point.
+      bool markedWeakValue;
+      do
+      {
+         markedWeakValue = false;
+         mMarker.init();
+         for(int i=0;i<hx::sWeakHashList.size();i++)
+         {
+            hx::HashRoot *hash = hx::sWeakHashList[i];
+            if (hx::IsWeakRefValid(hash))
+               markedWeakValue = hash->markWeakValues(&mMarker) || markedWeakValue;
+         }
+         if (markedWeakValue)
+         {
+            #ifdef HX_MULTI_THREAD_MARKING
+            mMarker.releaseJobs();
+            StartThreadJobs(tpjMark, MAX_GC_THREADS, true);
+            #else
+            mMarker.processMarkStack();
+            #endif
+         }
+      }
+      while(markedWeakValue);
+
       hx::RunFinalizers();
 
       #ifdef HXCPP_GC_VERIFY

test/haxe/TestWeakHash.hx

@@ -9,6 +9,12 @@ class WeakObjectData
    public function toString() return "Data " + id;
 }
 
+class WeakValueData
+{
+   public var key:WeakObjectData;
+   public function new(inKey:WeakObjectData) key = inKey;
+}
+
 class TestWeakHash extends Test
 {
    var retained:Array<WeakObjectData>;
@@ -104,4 +110,95 @@ class TestWeakHash extends Test
       Assert.pass();
    }
 
+   public function testDeadKeyReleasesValueCycle()
+   {
+      var result:{
+         map:WeakMap<WeakObjectData,WeakValueData>,
+         key:cpp.vm.WeakRef<WeakObjectData>,
+         value:cpp.vm.WeakRef<WeakValueData>
+      } = null;
+
+      final sema = new sys.thread.Semaphore(0);
+      sys.thread.Thread.create(() -> {
+         var map = new WeakMap<WeakObjectData,WeakValueData>();
+         var key = new WeakObjectData(1);
+         var value = new WeakValueData(key);
+         map.set(key,value);
+         result = {
+            map: map,
+            key: new cpp.vm.WeakRef(key),
+            value: new cpp.vm.WeakRef(value)
+         };
+         sema.release();
+      });
+      sema.acquire();
+
+      // Ensure no conservative reference remains on the terminated thread stack.
+      Sys.sleep(1);
+      cpp.vm.Gc.run(true);
+
+      Assert.isNull(result.key.get());
+      Assert.isNull(result.value.get());
+      Assert.isFalse(result.map.keys().hasNext());
+   }
+
+   public function testLiveKeyRetainsValue()
+   {
+      var map:WeakMap<WeakObjectData,WeakValueData> = null;
+      var value:cpp.vm.WeakRef<WeakValueData> = null;
+
+      final sema = new sys.thread.Semaphore(0);
+      sys.thread.Thread.create(() -> {
+         map = new WeakMap<WeakObjectData,WeakValueData>();
+         var key = new WeakObjectData(2);
+         var mapValue = new WeakValueData(key);
+         retained = [key];
+         map.set(key,mapValue);
+         value = new cpp.vm.WeakRef(mapValue);
+         sema.release();
+      });
+      sema.acquire();
+
+      Sys.sleep(1);
+      cpp.vm.Gc.run(true);
+
+      Assert.notNull(value.get());
+      Assert.notNull(map.get(retained[0]));
+   }
+
+   public function testEphemeronsReachFixedPoint()
+   {
+      var maps:{
+         upstream:WeakMap<WeakObjectData,WeakObjectData>,
+         downstream:WeakMap<WeakObjectData,WeakObjectData>
+      } = null;
+      var value:cpp.vm.WeakRef<WeakObjectData> = null;
+
+      final sema = new sys.thread.Semaphore(0);
+      sys.thread.Thread.create(() -> {
+         // Register downstream first so it is visited before upstream. A
+         // single pass would skip it before upstream makes its key reachable.
+         var downstream = new WeakMap<WeakObjectData,WeakObjectData>();
+         var upstream = new WeakMap<WeakObjectData,WeakObjectData>();
+         var rootKey = new WeakObjectData(3);
+         var linkKey = new WeakObjectData(4);
+         var finalValue = new WeakObjectData(5);
+         downstream.set(linkKey,finalValue);
+         upstream.set(rootKey,linkKey);
+         retained = [rootKey];
+         maps = {upstream: upstream, downstream: downstream};
+         value = new cpp.vm.WeakRef(finalValue);
+         sema.release();
+      });
+      sema.acquire();
+
+      Sys.sleep(1);
+      cpp.vm.Gc.run(true);
+
+      var linkKey = maps.upstream.get(retained[0]);
+      Assert.notNull(linkKey);
+      Assert.notNull(maps.downstream.get(linkKey));
+      Assert.notNull(value.get());
+   }
+
 }