Fix two node-copying bugs in update

Facade.h

@@ -136,7 +136,6 @@ struct Facade {
   int64_t getVersion() const { return versioned.getVersion(); }
   int64_t getOldestVersion() const { return versioned.getOldestVersion(); }
 
-private:
   std::vector<std::pair<String, String>> unversionedRead(const String &begin,
                                                          const String &end,
                                                          int &limit,

FacadeTest.cpp

@@ -1,4 +1,5 @@
 #include "Facade.h"
+#include "PrintMutation.h"
 #include "VersionedMap.h"
 #include <inttypes.h>
 
@@ -24,6 +25,8 @@ weaselab::VersionedMap::Mutation clear(weaselab::VersionedMap::Key begin,
   return {begin.p, end.p, begin.len, end.len, weaselab::VersionedMap::Clear};
 }
 
+void breakpoint_me() {}
+
 int main() {
   Facade f(0);
   int64_t version = 1;
@@ -42,13 +45,14 @@ int main() {
   }
   for (int64_t i = 0; i < version; ++i) {
     printf("--- version %" PRId64 " ---\n", i);
-    auto result = f.viewAt(i).rangeRead("a"_s, "d"_s, 10, false);
+    printf("merged:\n");
+    auto result = f.viewAt(i).rangeRead("a"_s, "z"_s, 100, false);
     for (const auto &[k, v] : result) {
       for (auto c : k) {
         if (isprint(c)) {
           printf("%c", c);
         } else {
-          printf("0x%02x", c);
+          printf("x%02x", c);
         }
       }
       printf(" -> ");
@@ -56,10 +60,35 @@ int main() {
         if (isprint(c)) {
           printf("%c", c);
         } else {
-          printf("0x%02x", c);
+          printf("x%02x", c);
         }
       }
       printf("\n");
     }
+    printf("unversioned:\n");
+    for (const auto &[k, v] : f.unversioned) {
+      for (auto c : k) {
+        if (isprint(c)) {
+          printf("%c", c);
+        } else {
+          printf("x%02x", c);
+        }
+      }
+      printf(" -> ");
+      for (auto c : v) {
+        if (isprint(c)) {
+          printf("%c", c);
+        } else {
+          printf("x%02x", c);
+        }
+      }
+      printf("\n");
+    }
+    breakpoint_me();
+    printf("versioned:\n");
+    for (auto iter = f.versioned.begin(i), end = f.versioned.end(i);
+         iter != end; ++iter) {
+      printMutation(*iter);
+    }
   }
 }

PrintMutation.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include "VersionedMap.h"
+#include <ctype.h>
 #include <inttypes.h>
 #include <stdio.h>
 
@@ -10,22 +11,42 @@ printMutation(const weaselab::VersionedMap::Iterator::VersionedMutation &m) {
   case weaselab::VersionedMap::Set:
     printf("set ");
     for (int i = 0; i < m.param1Len; ++i) {
-      printf("x%02x", m.param1[i]);
+      auto c = m.param1[i];
+      if (isprint(c)) {
+        printf("%c", c);
+      } else {
+        printf("x%02x", c);
+      }
     }
     printf(" -> '");
     for (int i = 0; i < m.param2Len; ++i) {
-      printf("x%02x", m.param2[i]);
+      auto c = m.param2[i];
+      if (isprint(c)) {
+        printf("%c", c);
+      } else {
+        printf("x%02x", c);
+      }
     }
     printf("' @ %" PRId64 "\n", m.version);
     break;
   case weaselab::VersionedMap::Clear:
     printf("clear [");
     for (int i = 0; i < m.param1Len; ++i) {
-      printf("x%02x", m.param1[i]);
+      auto c = m.param1[i];
+      if (isprint(c)) {
+        printf("%c", c);
+      } else {
+        printf("x%02x", c);
+      }
     }
     printf(", ");
     for (int i = 0; i < m.param2Len; ++i) {
-      printf("x%02x", m.param2[i]);
+      auto c = m.param2[i];
+      if (isprint(c)) {
+        printf("%c", c);
+      } else {
+        printf("x%02x", c);
+      }
     }
     printf(") @ %" PRId64 "\n", m.version);
     break;

VersionedMap.cpp

@@ -638,7 +638,8 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
       auto &c = mm.base[copy];
       c.entry = n.entry->addref();
       c.pointer[which] = child;
-      c.pointer[!which] = n.pointer[!which];
+      c.pointer[!which] =
+          this->child<std::memory_order_relaxed>(node, !which, latestVersion);
       c.updated.store(false, std::memory_order_relaxed);
       c.updateVersion = version;
       assert(copy == 0 || copy >= kMinAddressable);
@@ -649,11 +650,12 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
       // The reason these aren't data races is that concurrent readers are
       // reading < `version`
       if (updated && n.replacedPointer != which) {
+        auto result = doCopy();
         // We can't update n.replacedPointer without introducing a data race
         // (unless we packed it into the atomic?) so we copy. pointer[2] becomes
         // unreachable, but need to tell the garbage collector.
         n.pointer[2] = 0;
-        return doCopy();
+        return result;
       } else if (updated) {
         n.pointer[2] = child;
       } else {
@@ -808,12 +810,13 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
   void remove(Finger &finger) {
 
 #ifndef NDEBUG
-    uint32_t expected;
+    Entry *expected;
     {
       Finger copy;
       finger.copyTo(copy);
       move<std::memory_order_relaxed>(copy, latestVersion, true);
-      expected = copy.searchPathSize() > 0 ? copy.backNode() : 0;
+      expected =
+          copy.searchPathSize() > 0 ? mm.base[copy.backNode()].entry : nullptr;
     }
 #endif
 
@@ -850,7 +853,7 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
     // propagate up the search path, all the way to the root since we may have
     // more rotations to do even if an update doesn't change a node pointer
     auto node = finger.backNode();
-    const auto oldSize = finger.searchPathSize();
+    const auto oldSize = finger.searchPathSize() - (node == 0);
     for (;;) {
       if (finger.searchPathSize() == 1) {
         // Made it to the root
@@ -873,12 +876,17 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
         finger.push(c, false);
       }
     } else {
-      move<std::memory_order_relaxed>(finger, latestVersion, true);
+      while (finger.searchPathSize() > 1 && finger.backDirection() == true) {
+        finger.pop();
+      }
+      finger.pop();
     }
 
 #ifndef NDEBUG
     if (finger.searchPathSize() > 0) {
-      assert(finger.backNode() == expected);
+      assert(mm.base[finger.backNode()].entry == expected);
+    } else {
+      assert(expected == nullptr);
     }
 #endif
   }
@@ -1342,7 +1350,7 @@ void VersionedMap::Impl::printInOrderHelper(int64_t version, uint32_t node,
     return;
   }
   printInOrderHelper(version,
-                     child<std::memory_order_relaxed>(node, false, version),
+                     child<std::memory_order_relaxed>(node, true, version),
                      depth + 1);
   for (int i = 0; i < depth; ++i) {
     printf("  ");
@@ -1360,7 +1368,7 @@ void VersionedMap::Impl::printInOrderHelper(int64_t version, uint32_t node,
   }
   printf("\n");
   VersionedMap::Impl::printInOrderHelper(
-      version, child<std::memory_order_relaxed>(node, true, version),
+      version, child<std::memory_order_relaxed>(node, false, version),
       depth + 1);
 }