Document and test thread safety properties

Closes #2

.clangd

@@ -1,2 +1,2 @@
 CompileFlags:
-  Add: [-DENABLE_MAIN, -UNDEBUG, -DENABLE_FUZZ, -fexceptions]
+  Add: [-DENABLE_MAIN, -UNDEBUG, -DENABLE_FUZZ, -DTHREAD_TEST, -fexceptions]

CMakeLists.txt

@@ -167,6 +167,21 @@ if(BUILD_TESTING)
     add_test(NAME conflict_set_fuzz_${hash} COMMAND fuzz_driver ${TEST})
   endforeach()
 
+  # tsan
+
+  if(NOT CMAKE_CROSSCOMPILING)
+    add_executable(tsan_driver ConflictSet.cpp FuzzTestDriver.cpp)
+    target_compile_options(tsan_driver PRIVATE ${TEST_FLAGS} -fsanitize=thread)
+    target_link_options(tsan_driver PRIVATE -fsanitize=thread)
+    target_compile_definitions(tsan_driver PRIVATE ENABLE_FUZZ THREAD_TEST)
+    target_include_directories(tsan_driver
+                               PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include)
+    foreach(TEST ${CORPUS_TESTS})
+      get_filename_component(hash ${TEST} NAME)
+      add_test(NAME conflict_set_tsan_${hash} COMMAND tsan_driver ${TEST})
+    endforeach()
+  endif()
+
   add_executable(driver TestDriver.cpp)
   target_compile_options(driver PRIVATE ${TEST_FLAGS})
   target_link_libraries(driver PRIVATE ${PROJECT_NAME})

ConflictSet.cpp

@@ -998,17 +998,17 @@ Node *nextLogical(Node *node) {
 
 // Invalidates `self`, replacing it with a node of at least capacity.
 // Does not return nodes to freelists when kUseFreeList is false.
-template <bool kUseFreeList>
 void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
                                 NodeAllocators *allocators,
-                                ConflictSet::Impl *impl) {
+                                ConflictSet::Impl *impl,
+                                const bool kUseFreeList) {
   switch (self->getType()) {
   case Type_Node0: {
     auto *self0 = (Node0 *)self;
     auto *newSelf = allocators->node0.allocate(capacity);
     newSelf->copyChildrenAndKeyFrom(*self0);
     getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
       allocators->node0.release(self0);
     } else {
       removeNode(self0);
@@ -1021,7 +1021,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
     auto *newSelf = allocators->node3.allocate(capacity);
     newSelf->copyChildrenAndKeyFrom(*self3);
     getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
       allocators->node3.release(self3);
     } else {
       removeNode(self3);
@@ -1034,7 +1034,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
     auto *newSelf = allocators->node16.allocate(capacity);
     newSelf->copyChildrenAndKeyFrom(*self16);
     getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
       allocators->node16.release(self16);
     } else {
       removeNode(self16);
@@ -1047,7 +1047,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
     auto *newSelf = allocators->node48.allocate(capacity);
     newSelf->copyChildrenAndKeyFrom(*self48);
     getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
       allocators->node48.release(self48);
     } else {
       removeNode(self48);
@@ -1060,7 +1060,7 @@ void freeAndMakeCapacityAtLeast(Node *&self, int capacity,
     auto *newSelf = allocators->node256.allocate(capacity);
     newSelf->copyChildrenAndKeyFrom(*self256);
     getInTree(self, impl) = newSelf;
-    if constexpr (kUseFreeList) {
+    if (kUseFreeList) {
       allocators->node256.release(self256);
     } else {
       removeNode(self256);
@@ -1083,11 +1083,9 @@ void maybeDecreaseCapacity(Node *&self, NodeAllocators *allocators,
   if (self->getCapacity() <= maxCapacity) {
     return;
   }
-  freeAndMakeCapacityAtLeast</*kUseFreeList*/ false>(self, maxCapacity,
+  freeAndMakeCapacityAtLeast(self, maxCapacity, allocators, impl, false);
-                                                     allocators, impl);
 }
 
-template <Type kType>
 void maybeDownsize(Node *self, NodeAllocators *allocators,
                    ConflictSet::Impl *impl, Node *&dontInvalidate) {
 
@@ -1095,9 +1093,7 @@ void maybeDownsize(Node *self, NodeAllocators *allocators,
   fprintf(stderr, "maybeDownsize: %s\n", getSearchPathPrintable(self).c_str());
 #endif
 
-  assert(self->getType() == kType);
+  switch (self->getType()) {
-  static_assert(kType != Type_Node0);
-  switch (kType) {
   case Type_Node0:           // GCOVR_EXCL_LINE
     __builtin_unreachable(); // GCOVR_EXCL_LINE
   case Type_Node3: {
@@ -1113,8 +1109,7 @@ void maybeDownsize(Node *self, NodeAllocators *allocators,
 
       if (minCapacity > child->getCapacity()) {
         const bool update = child == dontInvalidate;
-        freeAndMakeCapacityAtLeast</*kUseFreeList*/ true>(child, minCapacity,
+        freeAndMakeCapacityAtLeast(child, minCapacity, allocators, impl, true);
-                                                          allocators, impl);
         if (update) {
           dontInvalidate = child;
         }
@@ -1201,24 +1196,7 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,
 
   if (self->numChildren != 0) {
     const bool update = result == dontInvalidate;
-    switch (self->getType()) {
+    maybeDownsize(self, allocators, impl, result);
-    case Type_Node0:           // GCOVR_EXCL_LINE
-      __builtin_unreachable(); // GCOVR_EXCL_LINE
-    case Type_Node3:
-      maybeDownsize<Type_Node3>(self, allocators, impl, result);
-      break;
-    case Type_Node16:
-      maybeDownsize<Type_Node16>(self, allocators, impl, result);
-      break;
-    case Type_Node48:
-      maybeDownsize<Type_Node48>(self, allocators, impl, result);
-      break;
-    case Type_Node256:
-      maybeDownsize<Type_Node256>(self, allocators, impl, result);
-      break;
-    default:                   // GCOVR_EXCL_LINE
-      __builtin_unreachable(); // GCOVR_EXCL_LINE
-    }
     if (update) {
       dontInvalidate = result;
     }
@@ -1244,11 +1222,6 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,
 
     --parent->numChildren;
     assert(parent->numChildren > 0 || parent->entryPresent);
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node3>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
   } break;
   case Type_Node16: {
     auto *parent16 = static_cast<Node16 *>(parent);
@@ -1265,13 +1238,6 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,
 
     // By kMinChildrenNode16
     assert(parent->numChildren > 0);
-
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node16>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
-
   } break;
   case Type_Node48: {
     auto *parent48 = static_cast<Node48 *>(parent);
@@ -1292,12 +1258,6 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,
 
     // By kMinChildrenNode48
     assert(parent->numChildren > 0);
-
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node48>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
   } break;
   case Type_Node256: {
     auto *parent256 = static_cast<Node256 *>(parent);
@@ -1309,16 +1269,17 @@ Node *erase(Node *self, NodeAllocators *allocators, ConflictSet::Impl *impl,
     // By kMinChildrenNode256
     assert(parent->numChildren > 0);
 
-    const bool update = result == dontInvalidate;
-    maybeDownsize<Type_Node256>(parent, allocators, impl, result);
-    if (update) {
-      dontInvalidate = result;
-    }
   } break;
   default:                   // GCOVR_EXCL_LINE
     __builtin_unreachable(); // GCOVR_EXCL_LINE
   }
 
+  const bool update = result == dontInvalidate;
+  maybeDownsize(parent, allocators, impl, result);
+  if (update) {
+    dontInvalidate = result;
+  }
+
   return result;
 }
 
@@ -1698,6 +1659,9 @@ bool checkRangeStartsWith(Node *n, std::span<const uint8_t> key, int begin,
       goto downLeftSpine;
     } else {
       n = nextSibling(n);
+      if (n == nullptr) {
+        return true;
+      }
       goto downLeftSpine;
     }
   }
@@ -1715,6 +1679,9 @@ bool checkRangeStartsWith(Node *n, std::span<const uint8_t> key, int begin,
         goto downLeftSpine;
       } else {
         n = nextSibling(n);
+        if (n == nullptr) {
+          return true;
+        }
         goto downLeftSpine;
       }
     }
@@ -1732,9 +1699,6 @@ bool checkRangeStartsWith(Node *n, std::span<const uint8_t> key, int begin,
   __builtin_unreachable(); // GCOVR_EXCL_LINE
 
 downLeftSpine:
-  if (n == nullptr) {
-    return true;
-  }
   for (;;) {
     if (n->entryPresent) {
       return n->entry.rangeVersion <= readVersion;
@@ -1802,6 +1766,10 @@ struct CheckRangeLeftSide {
           return true;
         } else {
           n = nextSibling(n);
+          if (n == nullptr) {
+            ok = true;
+            return true;
+          }
           return downLeftSpine();
         }
       }
@@ -1829,6 +1797,10 @@ struct CheckRangeLeftSide {
             return true;
           } else {
             n = nextSibling(n);
+            if (n == nullptr) {
+              ok = true;
+              return true;
+            }
             return downLeftSpine();
           }
         }
@@ -1865,10 +1837,6 @@ struct CheckRangeLeftSide {
 
   bool downLeftSpine() {
     phase = DownLeftSpine;
-    if (n == nullptr) {
-      ok = true;
-      return true;
-    }
     return false;
   }
 };

FuzzTestDriver.cpp

@@ -2,10 +2,12 @@
 #include <cstdint>
 #include <fstream>
 #include <sstream>
+#include <thread>
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
 
 int main(int argc, char **argv) {
+  auto doTest = [&]() {
     for (int i = 1; i < argc; ++i) {
       std::ifstream t(argv[i], std::ios::binary);
       std::stringstream buffer;
@@ -13,4 +15,12 @@ int main(int argc, char **argv) {
       auto str = buffer.str();
       LLVMFuzzerTestOneInput((const uint8_t *)str.data(), str.size());
     }
+  };
+#ifdef THREAD_TEST
+  std::thread thread2{doTest};
+#endif
+  doTest();
+#ifdef THREAD_TEST
+  thread2.join();
+#endif
 }

Internal.h

@@ -10,10 +10,12 @@
 #include <cstdlib>
 #include <cstring>
 #include <inttypes.h>
+#include <latch>
 #include <map>
 #include <set>
 #include <span>
 #include <string>
+#include <thread>
 #include <unordered_set>
 #include <utility>
 #include <vector>
@@ -652,12 +654,26 @@ template <class ConflictSetImpl> struct TestDriver {
       auto *results2 =
           new (arena) ConflictSet::Result[numPointReads + numRangeReads];
 
+#ifdef THREAD_TEST
+      auto *results3 =
+          new (arena) ConflictSet::Result[numPointReads + numRangeReads];
+      std::latch ready{1};
+      std::thread thread2{[&]() {
+        ready.count_down();
+        cs.check(reads, results3, numPointReads + numRangeReads);
+      }};
+      ready.wait();
+#endif
+
       CALLGRIND_START_INSTRUMENTATION;
       cs.check(reads, results1, numPointReads + numRangeReads);
       CALLGRIND_STOP_INSTRUMENTATION;
 
       refImpl.check(reads, results2, numPointReads + numRangeReads);
-      for (int i = 0; i < numPointReads + numRangeReads; ++i) {
+
+      auto compareResults = [reads](ConflictSet::Result *results1,
+                                    ConflictSet::Result *results2, int count) {
+        for (int i = 0; i < count; ++i) {
           if (results1[i] != results2[i]) {
             if (reads[i].end.len == 0) {
               fprintf(stderr,
@@ -674,10 +690,24 @@ template <class ConflictSetImpl> struct TestDriver {
                   printable(reads[i].begin).c_str(),
                   printable(reads[i].end).c_str(), reads[i].readVersion);
             }
+            return false;
+          }
+        }
+        return true;
+      };
+
+      if (!compareResults(results1, results2, numPointReads + numRangeReads)) {
         ok = false;
         return true;
       }
+
+#ifdef THREAD_TEST
+      thread2.join();
+      if (!compareResults(results3, results2, numPointReads + numRangeReads)) {
+        ok = false;
+        return true;
       }
+#endif
     }
     return false;
   }

include/ConflictSet.h

@@ -19,7 +19,15 @@ limitations under the License.
 #include <stdint.h>
 
 #ifdef __cplusplus
-
+/** A data structure for optimistic concurrency control on ranges of
+ * bitwise-lexicographically-ordered keys.
+ *
+ * Thread safety:
+ *   - It's safe to operate on two different ConflictSets in two different
+ * threads concurrently
+ *   - It's safe to have multiple threads operating on the same ConflictSet
+ * concurrently if and only if all threads only call `check`.
+ */
 struct __attribute__((__visibility__("default"))) ConflictSet {
   enum Result {
     /** The result of a check which does not intersect any conflicting writes */
@@ -92,6 +100,15 @@ private:
 
 #else
 
+/** A data structure for optimistic concurrency control on ranges of
+ * bitwise-lexicographically-ordered keys.
+ *
+ * Thread safety:
+ *   - It's safe to operate on two different ConflictSets in two different
+ * threads concurrently
+ *   - It's safe to have multiple threads operating on the same ConflictSet
+ * concurrently if and only if all threads only call `check`.
+ */
 typedef struct ConflictSet ConflictSet;
 
 typedef enum {