Merge pull request 'Make SHOW_MEMORY builds work: fix deferred node capacity assert and eraseTree use-after-free' (#91) from weaselbot/conflict-set:weaselbot/issue-88 into main

Reviewed-on: #91

CMakeLists.txt

@@ -393,6 +393,24 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
                         PROPERTIES CXX_STANDARD_REQUIRED ON)
   add_test(NAME conflict_set_cxx_api_test COMMAND conflict_set_cxx_api_test)
 
+  # SHOW_MEMORY instrumentation, which the rest of the test suite doesn't
+  # compile with
+  add_executable(conflict_set_show_memory_test ConflictSet.cpp
+                                               ShowMemoryTest.cpp)
+  target_compile_options(conflict_set_show_memory_test PRIVATE ${TEST_FLAGS})
+  target_compile_definitions(conflict_set_show_memory_test
+                             PRIVATE SHOW_MEMORY=1)
+  target_include_directories(conflict_set_show_memory_test
+                             PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+  if(NOT CMAKE_CROSSCOMPILING)
+    target_compile_options(conflict_set_show_memory_test
+                           PRIVATE -fsanitize=address,undefined)
+    target_link_options(conflict_set_show_memory_test PRIVATE
+                        -fsanitize=address,undefined)
+  endif()
+  add_test(NAME conflict_set_show_memory_test
+           COMMAND conflict_set_show_memory_test)
+
   # symbol visibility tests
   if(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
     if(APPLE)

ConflictSet.cpp

@@ -80,17 +80,19 @@ using namespace weaselab;
 #define assume assert
 #endif
 
-#if SHOW_MEMORY
+// The SHOW_MEMORY instrumentation. Declared unconditionally so that each of
+// these has a single definition in every configuration (see the definitions at
+// the end of this file), rather than a no-op stub standing in for the real
+// thing.
 void addNode(struct Node *);
 void removeNode(struct Node *);
 void addKey(struct Node *);
 void removeKey(struct Node *);
-#else
-constexpr void addNode(struct Node *) {}
-constexpr void removeNode(struct Node *) {}
-constexpr void addKey(struct Node *) {}
-constexpr void removeKey(struct Node *) {}
-#endif
+// Like `removeKey`, but with a caller-provided search path length. Used when
+// the node's ancestors may already have been released, which makes walking
+// `parent` pointers invalid.
+void removeKey(struct Node *, int64_t searchPathLength);
+int64_t getSearchPathLength(struct Node *);
 
 // ==================== BEGIN IMPLEMENTATION ====================
 
@@ -307,7 +309,9 @@ struct Node {
   uint8_t *partialKey();
   Type getType() const { return type; }
   int32_t getCapacity() const {
-    assert(!releaseDeferred);
+    // `partialKeyCapacity` is not repurposed by `deferRelease`, so this is also
+    // valid for nodes with `releaseDeferred` set. It's needed to account for
+    // the capacity of a deferred node when it's finally released.
     return partialKeyCapacity;
   }
 
@@ -2753,10 +2757,39 @@ checkMaxBetweenExclusiveImpl<true>(Node256 *n, int begin, int end,
   return self;
 }
 
+#if SHOW_MEMORY
+// Length of the key leading to `child`, given the length of the key leading to
+// its parent. Consistent with `getSearchPathLength`.
+int64_t childSearchPathLength(int64_t parentSearchPathLength, Node *child) {
+  return parentSearchPathLength + 1 + child->partialKeyLen;
+}
+#endif
+
 void eraseTree(Node *root, WriteContext *writeContext) {
   Arena arena;
   auto toFree = vector<Node *>(arena);
   toFree.push_back(root);
+  // Length of the key leading to each node in `toFree`, on a parallel stack.
+  // A node is released before its children are visited, so the `parent`
+  // pointers that `getSearchPathLength` walks are dangling by the time a queued
+  // node is popped. Carry the lengths along instead. Only the SHOW_MEMORY
+  // accounting needs them.
+#if SHOW_MEMORY
+  auto searchPathLengths = vector<int64_t>(arena);
+  searchPathLengths.push_back(getSearchPathLength(root));
+#endif
+
+  // Queue `child` for release, where `parentSearchPathLength` is the length of
+  // the key leading to its parent.
+  auto queueChild = [&](Node *child, int64_t parentSearchPathLength) {
+    toFree.push_back(child);
+#if SHOW_MEMORY
+    searchPathLengths.push_back(
+        childSearchPathLength(parentSearchPathLength, child));
+#else
+    (void)parentSearchPathLength;
+#endif
+  };
 
   while (toFree.size() > 0) {
     auto *n = toFree.back();
@@ -2764,7 +2797,13 @@ void eraseTree(Node *root, WriteContext *writeContext) {
     writeContext->accum.entries_erased += n->entryPresent;
     ++writeContext->accum.nodes_released;
 
-    removeKey(n);
+#if SHOW_MEMORY
+    const int64_t searchPathLength = searchPathLengths.back();
+    searchPathLengths.pop_back();
+#else
+    const int64_t searchPathLength = 0;
+#endif
+    removeKey(n, searchPathLength);
 
     switch (n->getType()) {
     case Type_Node0: {
@@ -2774,28 +2813,39 @@ void eraseTree(Node *root, WriteContext *writeContext) {
     case Type_Node3: {
       auto *n3 = static_cast<Node3 *>(n);
       for (int i = 0; i < n3->numChildren; ++i) {
-        toFree.push_back(n3->children[i]);
+        queueChild(n3->children[i], searchPathLength);
       }
       writeContext->release(n3);
     } break;
     case Type_Node16: {
       auto *n16 = static_cast<Node16 *>(n);
       for (int i = 0; i < n16->numChildren; ++i) {
-        toFree.push_back(n16->children[i]);
+        queueChild(n16->children[i], searchPathLength);
       }
       writeContext->release(n16);
     } break;
     case Type_Node48: {
       auto *n48 = static_cast<Node48 *>(n);
       for (int i = 0; i < n48->numChildren; ++i) {
-        toFree.push_back(n48->children[i]);
+        queueChild(n48->children[i], searchPathLength);
       }
       writeContext->release(n48);
     } break;
     case Type_Node256: {
       auto *n256 = static_cast<Node256 *>(n);
       auto *out = toFree.unsafePrepareAppend(n256->numChildren).data();
-      n256->bitSet.forEachSet([&](int i) { *out++ = n256->children[i]; });
+#if SHOW_MEMORY
+      auto *outSearchPathLengths =
+          searchPathLengths.unsafePrepareAppend(n256->numChildren).data();
+#endif
+      n256->bitSet.forEachSet([&](int i) {
+        Node *child = n256->children[i];
+        *out++ = child;
+#if SHOW_MEMORY
+        *outSearchPathLengths++ =
+            childSearchPathLength(searchPathLength, child);
+#endif
+      });
       assert(out == toFree.end());
       writeContext->release(n256);
     } break;
@@ -5463,8 +5513,9 @@ void internal_addWrites(ConflictSet::Impl *impl,
   impl->totalBytes += mallocBytesDelta;
   impl->memory_bytes.set(impl->totalBytes);
 #if SHOW_MEMORY
+  // Testing-only cross-check of the accounting.
   if (impl->totalBytes != mallocBytes) {
-    abort();
+    abort(); // GCOVR_EXCL_LINE
   }
 #endif
 }
@@ -5475,8 +5526,9 @@ void internal_setOldestVersion(ConflictSet::Impl *impl, int64_t oldestVersion) {
   impl->totalBytes += mallocBytesDelta;
   impl->memory_bytes.set(impl->totalBytes);
 #if SHOW_MEMORY
+  // Testing-only cross-check of the accounting.
   if (impl->totalBytes != mallocBytes) {
-    abort();
+    abort(); // GCOVR_EXCL_LINE
   }
 #endif
 }
@@ -5942,6 +5994,14 @@ checkMaxVersion(Node *root, Node *node, InternalVersionT oldestVersion,
 
 } // namespace
 
+// The SHOW_MEMORY instrumentation. Each of these functions is defined exactly
+// once, with the accounting compiled out when SHOW_MEMORY is disabled. Defining
+// them once (rather than defining no-op stubs when SHOW_MEMORY is disabled)
+// keeps every configuration of this file reporting each function on the same
+// line, which merging coverage from objects compiled with different options
+// requires: gcovr, merging the SHOW_MEMORY test with the rest of the build,
+// otherwise aborts with "Got function ... on multiple lines". The optimizer
+// removes the resulting empty calls.
 #if SHOW_MEMORY
 
 int64_t nodeBytes = 0;
@@ -5970,7 +6030,10 @@ int64_t getNodeSize(struct Node *n) {
   }
 }
 
+#endif
+
 int64_t getSearchPathLength(Node *n) {
+#if SHOW_MEMORY
   assert(n != nullptr);
   int64_t result = 0;
   for (;;) {
@@ -5982,9 +6045,14 @@ int64_t getSearchPathLength(Node *n) {
     n = n->parent;
   }
   return result;
+#else
+  (void)n;
+  return 0;
+#endif
 }
 
 void addNode(Node *n) {
+#if SHOW_MEMORY
   nodeBytes += getNodeSize(n);
   partialCapacityBytes += n->getCapacity();
   if (nodeBytes > peakNodeBytes) {
@@ -5993,14 +6061,22 @@ void addNode(Node *n) {
   if (partialCapacityBytes > peakPartialCapacityBytes) {
     peakPartialCapacityBytes = partialCapacityBytes;
   }
+#else
+  (void)n;
+#endif
 }
 
 void removeNode(Node *n) {
+#if SHOW_MEMORY
   nodeBytes -= getNodeSize(n);
   partialCapacityBytes -= n->getCapacity();
+#else
+  (void)n;
+#endif
 }
 
 void addKey(Node *n) {
+#if SHOW_MEMORY
   if (!n->entryPresent) {
     ++totalKeys;
     keyBytes += getSearchPathLength(n);
@@ -6011,15 +6087,36 @@ void addKey(Node *n) {
       peakKeyBytes = keyBytes;
     }
   }
+#else
+  (void)n;
+#endif
 }
 
 void removeKey(Node *n) {
+#if SHOW_MEMORY
   if (n->entryPresent) {
     --totalKeys;
     keyBytes -= getSearchPathLength(n);
   }
+#else
+  (void)n;
+#endif
 }
 
+void removeKey(Node *n, int64_t searchPathLength) {
+#if SHOW_MEMORY
+  if (n->entryPresent) {
+    --totalKeys;
+    keyBytes -= searchPathLength;
+  }
+#else
+  (void)n;
+  (void)searchPathLength;
+#endif
+}
+
+#if SHOW_MEMORY
+
 struct __attribute__((visibility("default"))) PeakPrinter {
   ~PeakPrinter() {
     printf("--- radix_tree ---\n");

Internal.h

@@ -23,7 +23,9 @@ using namespace weaselab;
 #include <callgrind.h>
 
 #define DEBUG_VERBOSE 0
+#ifndef SHOW_MEMORY
 #define SHOW_MEMORY 0
+#endif
 
 // std::span is not trivially constructible. We want a span that leaves its
 // members uninitialized for performance reasons.
@@ -96,6 +98,11 @@ static_assert(std::is_trivial_v<TrivialSpan>);
 #if SHOW_MEMORY
 inline int64_t mallocBytes = 0;
 inline int64_t peakMallocBytes = 0;
+// Defined in ConflictSet.cpp
+extern int64_t nodeBytes;
+extern int64_t partialCapacityBytes;
+extern int64_t totalKeys;
+extern int64_t keyBytes;
 #endif
 
 inline thread_local int64_t mallocBytesDelta = 0;

ShowMemoryTest.cpp

@@ -0,0 +1,115 @@
+// Exercises the SHOW_MEMORY instrumentation, which the other tests don't
+// compile with (this target passes -DSHOW_MEMORY=1). The instrumentation has
+// its own consistency checks, which abort on failure, and asserts are enabled
+// by TEST_FLAGS. Only one ConflictSet may exist at a time, since the
+// accounting is process-wide.
+#include "ConflictSet.h"
+#include "Internal.h"
+
+#include <cassert>
+#include <cstdint>
+#include <string>
+
+using namespace weaselab;
+
+namespace {
+
+void writePoint(ConflictSet &cs, const std::string &key, int64_t writeVersion) {
+  ConflictSet::WriteRange w{{(const uint8_t *)key.data(), int(key.size())},
+                            {nullptr, 0}};
+  cs.addWrites(&w, 1, writeVersion);
+}
+
+ConflictSet::Result checkPoint(const ConflictSet &cs, const std::string &key,
+                               int64_t readVersion) {
+  ConflictSet::ReadRange r{{(const uint8_t *)key.data(), int(key.size())},
+                           {nullptr, 0},
+                           readVersion};
+  ConflictSet::Result result;
+  cs.check(&r, &result, 1);
+  return result;
+}
+
+// Everything a ConflictSet allocated must have been accounted for and released
+// by the time it's destroyed.
+void checkFullyReleased() {
+  assert(nodeBytes == 0);
+  assert(partialCapacityBytes == 0);
+  assert(totalKeys == 0);
+  assert(keyBytes == 0);
+  assert(mallocBytes == 0);
+}
+
+} // namespace
+
+int main() {
+  // Splitting a node replaces it, which defers the release of the old node.
+  // Releasing a deferred node must account for its capacity, and the key it
+  // held must be accounted for without walking `parent` pointers of nodes that
+  // have already been released.
+  {
+    ConflictSet cs{0};
+    writePoint(cs, "a", 1);
+    writePoint(cs, "ab", 2);
+    assert(checkPoint(cs, "a", 0) == ConflictSet::Conflict);
+    assert(checkPoint(cs, "ab", 0) == ConflictSet::Conflict);
+    assert(checkPoint(cs, "b", 0) == ConflictSet::Commit);
+    assert(nodeBytes > 0);
+    assert(keyBytes > 0);
+  }
+  checkFullyReleased();
+
+  // Exceeding the version window erases the tree mid-workload.
+  {
+    ConflictSet cs{0};
+    writePoint(cs, "abc", 1);
+    const int64_t version = int64_t(1) << 40;
+    writePoint(cs, "abc", version);
+    assert(checkPoint(cs, "abc", 0) == ConflictSet::TooOld);
+    assert(checkPoint(cs, "abc", version) == ConflictSet::Commit);
+  }
+  checkFullyReleased();
+
+  // Erase a tree with many nodes, both mid-workload and at destruction.
+  {
+    ConflictSet cs{0};
+    for (int i = 0; i < 1000; ++i) {
+      writePoint(cs, "prefix/" + std::to_string(i), i + 1);
+    }
+    const int64_t version = int64_t(1) << 40;
+    writePoint(cs, "prefix/1", version);
+    assert(checkPoint(cs, "prefix/1", version) == ConflictSet::Commit);
+
+    for (int i = 0; i < 1000; ++i) {
+      writePoint(cs, "other/" + std::to_string(i), version + i + 1);
+    }
+    assert(checkPoint(cs, "other/999", version) == ConflictSet::Conflict);
+  }
+  checkFullyReleased();
+
+  // Grow a node through every node type, so that the accounting for each node
+  // type's size is exercised, then erase the entries one at a time so that
+  // nodes are released individually rather than as part of a whole tree.
+  {
+    ConflictSet cs{0};
+    std::string key(1, '\0');
+    for (int i = 0; i < 256; ++i) {
+      key[0] = char(i);
+      writePoint(cs, key, i + 1);
+    }
+    for (int i = 0; i < 256; ++i) {
+      key[0] = char(i);
+      assert(checkPoint(cs, key, i) == ConflictSet::Conflict);
+    }
+
+    // Garbage collection erases entries that can only fail with TooOld anyway.
+    // Doing it one version at a time walks the tree, erasing, downsizing and
+    // releasing nodes as it goes.
+    for (int i = 0; i < 256; ++i) {
+      cs.setOldestVersion(i + 1);
+    }
+    key[0] = '\0';
+    assert(checkPoint(cs, key, 0) == ConflictSet::TooOld);
+  }
+  checkFullyReleased();
+}