Add ApiTest.cpp

ApiTest.cpp

@@ -0,0 +1,90 @@
+#include "VersionedMap.h"
+
+#include "Internal.h"
+#include "PrintMutation.h"
+
+#include <assert.h>
+#include <inttypes.h>
+#include <latch>
+#include <string.h>
+#include <thread>
+
+void setAndClearPrev() {
+  debugVerboseEnabled = false;
+  weaselab::VersionedMap versionedMap{0};
+
+  auto setAndClear = [&versionedMap](int64_t version) {
+    const int64_t k = __builtin_bswap64(version);
+    const int64_t zero = __builtin_bswap64(0);
+    weaselab::VersionedMap::Mutation m[] = {
+        {(const uint8_t *)&zero, (const uint8_t *)&k, 8, 8,
+         weaselab::VersionedMap::Clear},
+        {(const uint8_t *)&k, (const uint8_t *)&k, 8, 8,
+         weaselab::VersionedMap::Set},
+    };
+    versionedMap.addMutations(m, sizeof(m) / sizeof(m[0]), version);
+    assert(versionedMap.getVersion() == version);
+  };
+
+  int64_t version = 0;
+  while (version < 1000) {
+    ++version;
+    setAndClear(version);
+  }
+
+  std::latch ready{2};
+
+  auto writer = std::thread{[&]() {
+    ready.arrive_and_wait();
+
+    int64_t version = 1000;
+    while (version < 100000) {
+      ++version;
+      setAndClear(version);
+    }
+  }};
+
+  ready.arrive_and_wait();
+
+  version = 0;
+  while (version < 1000) {
+    ++version;
+
+    auto iter = versionedMap.begin(version);
+
+    auto m = *iter++;
+    int64_t k = __builtin_bswap64(version);
+    assert(m.version == version);
+    assert(m.param1Len == 8);
+    assert(m.param2Len == 8);
+    int64_t zero = __builtin_bswap64(0);
+    assert(memcmp(m.param1, &zero, 8) == 0);
+    assert(memcmp(m.param2, &k, 8) == 0);
+    assert(m.type == weaselab::VersionedMap::Clear);
+
+    m = *iter++;
+    assert(m.version == version);
+    assert(m.param1Len == 8);
+    assert(m.param2Len == 8);
+    assert(memcmp(m.param1, &k, 8) == 0);
+    assert(memcmp(m.param2, &k, 8) == 0);
+    assert(m.type == weaselab::VersionedMap::Set);
+
+    assert(iter == versionedMap.end(version));
+  }
+
+  writer.join();
+
+  debugVerboseEnabled = true;
+  printf("Before gc: %" PRId64 "\n", versionedMap.getBytes());
+  versionedMap.setOldestVersion(versionedMap.getVersion());
+  printf("After gc: %" PRId64 "\n", versionedMap.getBytes());
+  assert(versionedMap.getOldestVersion() == versionedMap.getVersion());
+  for (auto iter = versionedMap.begin(versionedMap.getVersion()),
+            end = versionedMap.end(versionedMap.getVersion());
+       iter != end; ++iter) {
+    printMutation(*iter);
+  }
+}
+
+int main() { setAndClearPrev(); }

CMakeLists.txt

@@ -87,12 +87,12 @@ add_custom_command(
   COMMAND_EXPAND_LISTS)
 
 add_library(${PROJECT_NAME} SHARED ${CMAKE_BINARY_DIR}/${PROJECT_NAME}.o)
+target_include_directories(
+  ${PROJECT_NAME} PUBLIC $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>)
 set_target_properties(
   ${PROJECT_NAME} PROPERTIES LIBRARY_OUTPUT_DIRECTORY
                              "${CMAKE_BINARY_DIR}/versioned-map")
-if(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
+set_target_properties(${PROJECT_NAME} PROPERTIES LINKER_LANGUAGE C)
-  set_target_properties(${PROJECT_NAME} PROPERTIES LINKER_LANGUAGE C)
-endif()
 
 if(HAS_VERSION_SCRIPT)
   target_link_options(${PROJECT_NAME} PRIVATE
@@ -100,9 +100,9 @@ if(HAS_VERSION_SCRIPT)
 endif()
 
 add_library(${PROJECT_NAME}-static STATIC ${CMAKE_BINARY_DIR}/${PROJECT_NAME}.o)
-if(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
+target_include_directories(
-  set_target_properties(${PROJECT_NAME}-static PROPERTIES LINKER_LANGUAGE C)
+  ${PROJECT_NAME}-static PUBLIC $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/include>)
-endif()
+set_target_properties(${PROJECT_NAME}-static PROPERTIES LINKER_LANGUAGE C)
 
 if(NOT APPLE)
   add_custom_command(
@@ -115,6 +115,8 @@ if(NOT APPLE)
       "Proceeding with all symbols global in static library")
 endif()
 
+set(TEST_FLAGS -Wall -Wextra -Wunreachable-code -Wpedantic -UNDEBUG)
+
 include(CTest)
 
 option(BUILD_TREE_VIS "Build tree visualization" OFF)
@@ -130,16 +132,24 @@ if(BUILD_TESTING)
   add_executable(rootset_test RootSet.cpp)
   target_compile_definitions(rootset_test PRIVATE ENABLE_ROOTSET_TESTS)
   target_compile_options(rootset_test PRIVATE -fsanitize=address,undefined
-                                              -UNDEBUG)
+                                              ${TEST_FLAGS})
   target_link_options(rootset_test PRIVATE -fsanitize=address,undefined)
   add_test(NAME rootset_test COMMAND rootset_test)
 
   add_executable(rootset_test_tsan RootSet.cpp)
   target_compile_definitions(rootset_test_tsan PRIVATE ENABLE_ROOTSET_TESTS)
-  target_compile_options(rootset_test_tsan PRIVATE -fsanitize=thread -UNDEBUG)
+  target_compile_options(rootset_test_tsan PRIVATE -fsanitize=thread
+                                                   ${TEST_FLAGS})
   target_link_options(rootset_test_tsan PRIVATE -fsanitize=thread)
   add_test(NAME rootset_test_tsan COMMAND rootset_test)
 
+  add_executable(api_test ApiTest.cpp)
+  target_link_libraries(api_test PRIVATE ${PROJECT_NAME})
+  target_compile_options(api_test PRIVATE -fsanitize=address,undefined
+                                          ${TEST_FLAGS})
+  target_link_options(api_test PRIVATE -fsanitize=address,undefined)
+  add_test(NAME api_test COMMAND api_test)
+
   # symbol visibility tests
   if(NOT CMAKE_BUILD_TYPE STREQUAL Debug)
     if(APPLE)

Internal.h

@@ -17,10 +17,8 @@
 #define DEBUG_VERBOSE 0
 #endif
 
-#if DEBUG_VERBOSE
 // Use to toggle debug verbose dynamically
 inline bool debugVerboseEnabled = true;
-#endif
 
 // This header contains code that we want to reuse outside of ConflictSet.cpp or
 // want to exclude from coverage since it's only testing related.

PrintMutation.h

@@ -0,0 +1,35 @@
+#pragma once
+
+#include "VersionedMap.h"
+#include <inttypes.h>
+#include <stdio.h>
+
+inline void
+printMutation(const weaselab::VersionedMap::Iterator::VersionedMutation &m) {
+  switch (m.type) {
+  case weaselab::VersionedMap::Set:
+    printf("set ");
+    for (int i = 0; i < m.param1Len; ++i) {
+      printf("x%02x", m.param1[i]);
+    }
+    printf(" -> '");
+    for (int i = 0; i < m.param2Len; ++i) {
+      printf("x%02x", m.param2[i]);
+    }
+    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]);
+    }
+    printf(", ");
+    for (int i = 0; i < m.param2Len; ++i) {
+      printf("x%02x", m.param2[i]);
+    }
+    printf(") @ %" PRId64 "\n", m.version);
+    break;
+  default:                   // GCOVR_EXCL_LINE
+    __builtin_unreachable(); // GCOVR_EXCL_LINE
+  }
+}

RootSet.cpp

@@ -188,14 +188,14 @@ int main() {
 
   RootSet rs;
   std::latch ready{1 + kNumReaders};
-  std::atomic<int> version;
+  std::atomic<uint32_t> version;
-  std::vector<std::atomic<int>> doneVersions(kNumReaders);
+  std::vector<std::atomic<uint32_t>> doneVersions(kNumReaders);
   std::thread writer([&]() {
     ready.arrive_and_wait();
     for (int i = 0; i < kNumVersions; ++i) {
       rs.add(i / 10, i);
       version.store(i);
-      int min = std::numeric_limits<int>::max();
+      uint32_t min = -1;
       for (auto &v : doneVersions) {
         min = std::min(min, v.load());
       }

VersionedMap.cpp

@@ -427,33 +427,32 @@ struct Finger {
 
   void setSearchPathSizeUnsafe(int size) { searchPathSize_ = size; }
 
-  Finger() : searchPathSize_(0) {}
+  Finger() { clear(); }
 
-  Finger(const Finger &other) {
+  void clear() {
 #ifndef NDEBUG
     memset(searchPath, 0, sizeof(searchPath));
     memset(direction, 0, sizeof(direction));
 #endif
-    memcpy(searchPath, other.searchPath,
+    searchPathSize_ = 0;
-           other.searchPathSize_ * sizeof(searchPath[0]));
-    memcpy(direction, other.direction,
-           other.searchPathSize_ * sizeof(direction[0]));
-    searchPathSize_ = other.searchPathSize_;
   }
 
-  Finger &operator=(const Finger &other) {
+  void copyTo(Finger &result) {
 #ifndef NDEBUG
-    memset(searchPath, 0, sizeof(searchPath));
+    memset(result.searchPath, 0, sizeof(searchPath));
-    memset(direction, 0, sizeof(direction));
+    memset(result.direction, 0, sizeof(direction));
 #endif
-    memcpy(searchPath, other.searchPath,
+    memcpy(result.searchPath, searchPath,
-           other.searchPathSize_ * sizeof(searchPath[0]));
+           searchPathSize_ * sizeof(searchPath[0]));
-    memcpy(direction, other.direction,
+    memcpy(result.direction, direction, searchPathSize_ * sizeof(direction[0]));
-           other.searchPathSize_ * sizeof(direction[0]));
+    result.searchPathSize_ = searchPathSize_;
-    searchPathSize_ = other.searchPathSize_;
-    return *this;
   }
 
+  Finger(const Finger &) = delete;
+  Finger &operator=(const Finger &) = delete;
+  Finger(Finger &&) = delete;
+  Finger &operator=(Finger &&) = delete;
+
 private:
   uint32_t searchPath[kPathLengthUpperBound];
   bool direction[kPathLengthUpperBound];
@@ -483,6 +482,7 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
 
   template <std::memory_order kOrder>
   uint32_t child(uint32_t node, bool which, int64_t at) const {
+    assert(node != 0);
     static_assert(kOrder == std::memory_order_acquire ||
                   kOrder == std::memory_order_relaxed);
     auto &n = mm.base[node];
@@ -579,11 +579,11 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
   };
 
   template <std::memory_order kOrder, class T>
-  Finger search(Key key, T root, int64_t version) const {
+  void search(Key key, T root, int64_t version, Finger &finger) const {
+    finger.clear();
     static_assert(std::is_same_v<T, uint32_t>);
-    Finger finger;
     if (root == 0) {
-      return finger;
+      return;
     }
     bool ignored;
     finger.push(root, ignored);
@@ -601,7 +601,6 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
       }
       finger.push(child<kOrder>(n, c > 0, version), c > 0);
     }
-    return finger;
   }
 
   // If `val` is set, then this is a point mutation at `latestVersion`.
@@ -633,7 +632,8 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
     if (val.has_value()) {
       pointVersion = latestVersion;
       if (inserted) {
-        auto copy = finger;
+        Finger copy;
+        finger.copyTo(copy);
         move<std::memory_order_relaxed>(copy, latestVersion, true);
         if (copy.searchPathSize() == 0) {
           rangeVersion = -1;
@@ -694,6 +694,16 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
   // entry.
   void remove(Finger &finger) {
 
+#ifndef NDEBUG
+    uint32_t expected;
+    {
+      Finger copy;
+      finger.copyTo(copy);
+      move<std::memory_order_relaxed>(copy, latestVersion, true);
+      expected = copy.searchPathSize() > 0 ? copy.backNode() : 0;
+    }
+#endif
+
     // True if finger is pointing to an entry > than the entry we're removing
     // after we rotate it down
     bool greaterThan;
@@ -743,7 +753,7 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
     }
     finger.setSearchPathSizeUnsafe(oldSize);
 
-    if (greaterThan) {
+    if (greaterThan && finger.backNode() != 0) {
       uint32_t c;
       while ((c = child<std::memory_order_relaxed>(finger.backNode(), false,
                                                    latestVersion)) != 0) {
@@ -753,10 +763,11 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
       move<std::memory_order_relaxed>(finger, latestVersion, true);
     }
 
-    if (finger.backNode() == 0) {
+#ifndef NDEBUG
-      finger.pop();
+    if (finger.searchPathSize() > 0) {
-      assert(finger.searchPathSize() == 0);
+      assert(finger.backNode() == expected);
     }
+#endif
   }
 
   uint32_t newNode(int64_t version, int64_t rangeVersion, const uint8_t *key,
@@ -802,8 +813,9 @@ struct __attribute__((__visibility__("hidden"))) VersionedMap::Impl {
       case Clear: {
         insert({m.param1, m.param1Len}, {{nullptr, -1}});
         if (m.param2Len > 0) {
-          auto iter = search<std::memory_order_relaxed>(
+          Finger iter;
-              {m.param1, m.param1Len}, latestRoot, latestVersion);
+          search<std::memory_order_relaxed>({m.param1, m.param1Len}, latestRoot,
+                                            latestVersion, iter);
           move<std::memory_order_relaxed>(iter, latestVersion, true);
           while (iter.searchPathSize() > 0 &&
                  mm.base[iter.backNode()] < Key{m.param2, m.param2Len}) {
@@ -881,6 +893,17 @@ struct VersionedMap::Iterator::Impl {
   int mutationCount;
   int mutationIndex;
   VersionedMutation mutations[2];
+
+  void copyTo(Impl &result) {
+    result.cmp = cmp;
+    result.map = map;
+    result.version = version;
+    result.mutationCount = mutationCount;
+    result.mutationIndex = mutationIndex;
+    result.mutations[0] = mutations[0];
+    result.mutations[1] = mutations[1];
+    finger.copyTo(result.finger);
+  }
 };
 
 VersionedMap::Iterator::~Iterator() {
@@ -891,7 +914,9 @@ VersionedMap::Iterator::~Iterator() {
 }
 
 VersionedMap::Iterator::Iterator(const Iterator &other)
-    : impl(new(safe_malloc(sizeof(Impl))) Impl(*other.impl)) {}
+    : impl(new(safe_malloc(sizeof(Impl))) Impl()) {
+  other.impl->copyTo(*impl);
+}
 
 VersionedMap::Iterator &
 VersionedMap::Iterator::operator=(const Iterator &other) {
@@ -899,7 +924,8 @@ VersionedMap::Iterator::operator=(const Iterator &other) {
     impl->~Impl();
     safe_free(impl, sizeof(*impl));
   }
-  impl = new (safe_malloc(sizeof(Impl))) Impl(*other.impl);
+  impl = new (safe_malloc(sizeof(Impl))) Impl();
+  other.impl->copyTo(*impl);
   return *this;
 }
 
@@ -932,7 +958,8 @@ VersionedMap::Iterator::operator*() const {
 void materializeMutations(VersionedMap::Iterator::Impl *impl, const Entry *prev,
                           const Entry *next) {
   if (prev == nullptr) {
-    auto copy = impl->finger;
+    Finger copy;
+    impl->finger.copyTo(copy);
     impl->map->move<std::memory_order_acquire>(copy, impl->version, false);
     if (copy.searchPathSize() > 0) {
       prev = impl->map->mm.base[copy.backNode()].entry;
@@ -941,7 +968,8 @@ void materializeMutations(VersionedMap::Iterator::Impl *impl, const Entry *prev,
     }
   }
   if (next == nullptr) {
-    auto copy = impl->finger;
+    Finger copy;
+    impl->finger.copyTo(copy);
     impl->map->move<std::memory_order_acquire>(copy, impl->version, true);
     if (copy.searchPathSize() > 0) {
       next = impl->map->mm.base[copy.backNode()].entry;
@@ -1058,7 +1086,8 @@ void VersionedMap::Impl::firstGeq(const Key *key, const int64_t *version,
       iterator[i].impl =
           new (safe_malloc(sizeof(Iterator::Impl))) Iterator::Impl();
     }
-    auto finger = search<std::memory_order_acquire>(key[i], root, version[i]);
+    Finger &finger = iterator[i].impl->finger;
+    search<std::memory_order_acquire>(key[i], root, version[i], finger);
 
     if (finger.searchPathSize() == 0) {
       iterator[i].impl->cmp = 1;
@@ -1072,13 +1101,12 @@ void VersionedMap::Impl::firstGeq(const Key *key, const int64_t *version,
       iterator[i].impl->cmp = 0;
     }
 
-    iterator[i].impl->finger = finger;
     iterator[i].impl->version = version[i];
     iterator[i].impl->map = this;
 
     const Entry *prev = nullptr;
     for (;;) {
-      if (iterator[i].impl->finger.searchPathSize() > 0) {
+      if (finger.searchPathSize() > 0) {
         materializeMutations(iterator[i].impl, prev, nullptr);
         if (iterator[i].impl->mutationCount > 0) {
           break;
@@ -1086,11 +1114,9 @@ void VersionedMap::Impl::firstGeq(const Key *key, const int64_t *version,
       } else {
         break;
       }
-      prev = iterator[i]
+      prev = iterator[i].impl->map->mm.base[finger.backNode()].entry;
-                 .impl->map->mm.base[iterator[i].impl->finger.backNode()]
-                 .entry;
       iterator[i].impl->map->move<std::memory_order_acquire>(
-          iterator[i].impl->finger, iterator[i].impl->version, true);
+          finger, iterator[i].impl->version, true);
     }
     if (iterator[i].impl->cmp == 0) {
       iterator[i].impl->mutationIndex = iterator[i].impl->mutationCount - 1;
@@ -1230,6 +1256,8 @@ struct __attribute__((visibility("default"))) PeakPrinter {
 #ifdef ENABLE_MAIN
 #include <nanobench.h>
 
+#include "PrintMutation.h"
+
 void breakpoint_me() {}
 
 int main() {
@@ -1272,33 +1300,7 @@ int main() {
     printf("Bytes: %" PRId64 "\n", versionedMap.getBytes());
     breakpoint_me();
     for (auto end = versionedMap.end(v); iter != end; ++iter) {
-      const auto &m = *iter;
+      printMutation(*iter);
-      switch (m.type) {
-      case weaselab::VersionedMap::Set:
-        printf("set ");
-        for (int i = 0; i < m.param1Len; ++i) {
-          printf("x%02x", m.param1[i]);
-        }
-        printf(" -> '");
-        for (int i = 0; i < m.param2Len; ++i) {
-          printf("x%02x", m.param2[i]);
-        }
-        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]);
-        }
-        printf(", ");
-        for (int i = 0; i < m.param2Len; ++i) {
-          printf("x%02x", m.param2[i]);
-        }
-        printf(") @ %" PRId64 "\n", m.version);
-        break;
-      default:                   // GCOVR_EXCL_LINE
-        __builtin_unreachable(); // GCOVR_EXCL_LINE
-      }
     }
   }
   return 0;