Data race checker: Added recursion support

Author: drowaudio

include/scl/utils/CMakeLists.txt

@@ -7,16 +7,16 @@ include(../../../cmake/icm_testing.cmake)
 # Global include for icm based tests
 include_directories("../../include")
 
-file(GLOB test_files LIST_DIRECTORIES false "${CMAKE_CURRENT_SOURCE_DIR}" *_test.cpp)
-message(test_file: ${test_files})
+file(GLOB test_files LIST_DIRECTORIES false "${CMAKE_CURRENT_SOURCE_DIR}" *.test.cpp)
+message(test_files: ${test_files})
 
 foreach(test_file ${test_files})
     get_filename_component(name_without_extension "${test_file}" NAME_WE)
     icm_add_test(
         NAME ${name_without_extension}
         SOURCES ${test_file})
 
-    if(MATCHES ${test_file} fail_*)
+    if(${test_file} MATCHES fail_*)
         set_tests_properties(${name_without_extension} PROPERTIES
             WILL_FAIL TRUE)
     endif()

include/scl/utils/data_race_checker.h

@@ -23,6 +23,13 @@
 // A writer enters a function and there is an active writer
 // A writer enters a function and there is an active reader
 //
+// N.B. This checks for recursion but there is a false positive if:
+// - A write starts
+// - A read on a separate thread starts
+// - That read finishes
+// - The original write continues but a data race is flagged
+// Although there is technically no race, this is probably unintended behaviour
+//
 // Wait-free? (maybe the CMPX is only lock-free?)
 
 namespace scl {
@@ -36,6 +43,7 @@ namespace scl {
 struct check_state
 {
     std::atomic<size_t> num_readers { 0 };
+    std::atomic<std::thread::id> last_thread_id { std::thread::id() };
     std::atomic<bool> is_writing { false };
 };
 
@@ -52,22 +60,37 @@ bool can_read (const check_state& state)
 //==========================================
 void read_started (check_state& state)
 {
+    auto this_thread_id = std::this_thread::get_id();
+    auto last_thread_id = state.last_thread_id.exchange (this_thread_id);
+
     ++state.num_readers; // must be first
 
     if (state.is_writing)
-        DATA_RACE_DETECTED
-    // read during active write
+    {
+        if (last_thread_id != this_thread_id)
+        { DATA_RACE_DETECTED }
+        // read during active write
+    }
 }
 
 void write_started (check_state& state)
 {
+    auto this_thread_id = std::this_thread::get_id();
+    auto last_thread_id = state.last_thread_id.exchange (this_thread_id);
+
     if (state.is_writing.exchange (true))  // must be first
-        DATA_RACE_DETECTED
-    // write during active write
+    {
+        if (last_thread_id != this_thread_id)
+        { DATA_RACE_DETECTED }
+        // write during active write
+    }
 
     if (state.num_readers > 0)
-        DATA_RACE_DETECTED
-    // write during active read
+    {
+        if (last_thread_id != this_thread_id)
+        { DATA_RACE_DETECTED }
+        // write during active read
+    }
 }
 
 void read_ended (check_state& state)

include/scl/utils/data_race_checker.test.h

@@ -36,11 +36,52 @@ class test_vector
     data.push_back(value);
   }
 
+  // Test delegating a write-write
+  void push_back_2(const T& v1, const T& v2) {
+    scl::scoped_check<scl::check_type::write> _ (scl::data_race_registry<>::get_state (this));
+    push_back(v1);
+    push_back(v2);
+  }
+
+  // Test delegating a write-read
+  T& push_back_and_return(const T& value) {
+    scl::scoped_check<scl::check_type::write> _ (scl::data_race_registry<>::get_state (this));
+    data.push_back(value);
+    return data.back();
+  }
+
   void pop_back() {
     scl::scoped_check<scl::check_type::write> _ (scl::data_race_registry<>::get_state (this));
     data.pop_back();
   }
 
+  // Test delegating a write-read
+  const T& push_back_return_old_back(const T& value) {
+    scl::scoped_check<scl::check_type::write> _ (scl::data_race_registry<>::get_state (this));
+
+    auto& old_back = back();
+    push_back(value);
+    return old_back;
+  }
+
+  T& back() {
+    scl::scoped_check<scl::check_type::read> _ (scl::data_race_registry<>::get_state (this));
+    return data.back();
+  }
+
+  const T& back() const {
+    scl::scoped_check<scl::check_type::read> _ (scl::data_race_registry<>::get_state (this));
+    return data.back();
+  }
+
+  // Test delegating a read-write
+  // This wouldn't normally occur as you'd have a const function calling a non-const function
+  const T& back_with_default_write() {
+    scl::scoped_check<scl::check_type::read> _ (scl::data_race_registry<>::get_state (this));
+    push_back(T());
+    return data.back();
+  }
+
   T& operator[](size_t index) {
     scl::scoped_check<scl::check_type::read> _ (scl::data_race_registry<>::get_state (this));
     return data[index];
@@ -61,6 +102,13 @@ class test_vector
     return data.capacity();
   }
 
+  // Test delegating a read-read
+  std::pair<size_t, size_t> size_and_capacity() const
+  {
+    scl::scoped_check<scl::check_type::read> _ (scl::data_race_registry<>::get_state (this));
+    return { size(), capacity() };
+  }
+
 private:
   std::vector<T> data;
 };
@@ -74,7 +122,7 @@ inline void test_no_data_race()
      vec.push_back (c);
 
   // Then only read from the vector - no data race
-std::vector<std::thread> threads;
+  std::vector<std::thread> threads;
 
   for (auto _ : std::ranges::iota_view (0, 3))
     threads.emplace_back([&]
@@ -123,3 +171,57 @@ inline void test_data_race()
   for (auto& t : threads)
     t.join();
 }
+
+inline void test_no_data_race_read_read()
+{
+  test_vector<size_t> vec;
+
+  // Fill the vector first
+  for (auto c : std::ranges::iota_view (0uz, 1'000uz))
+    vec.push_back (c);
+
+  // Then only read from the vector - no data race
+  std::vector<std::thread> threads;
+
+  for (auto _ : std::ranges::iota_view (0, 3))
+    threads.emplace_back([&]
+                       {
+                         for (auto _ : std::ranges::iota_view (0uz, 1'000'000uz))
+                         {
+                           volatile auto c = 0uz;
+
+                           if (! vec.empty())
+                             c = vec[vec.size() - 1];
+
+                           auto sz_cp = vec.size_and_capacity();
+                           c = sz_cp.first;
+                           c = sz_cp.second;
+                        }
+                      });
+
+  for (auto& t : threads)
+    t.join();
+}
+
+inline void test_no_data_race_read_write()
+{
+  test_vector<size_t> vec;
+  volatile auto c = 0uz;
+  vec.push_back (auto (c));
+  c = vec.back_with_default_write();
+}
+
+inline void test_no_data_race_write_read()
+{
+  test_vector<size_t> vec;
+  volatile auto c = 0uz;
+  vec.push_back(auto(c));
+  c = vec.push_back_return_old_back(auto (c));
+}
+
+inline void test_no_data_race_write_write()
+{
+  test_vector<size_t> vec;
+  volatile auto c = 0uz;
+  vec.push_back_2(auto (c), auto (c));
+}

include/scl/utils/fail_data_race_checker.test.cpp

@@ -3,7 +3,7 @@
 //
 
 // Use exit(1) as ctest seems to count a raised signal as a pass...
-#define DATA_RACE_DETECTED std::exit(1);
+#define DATA_RACE_DETECTED { std::println ("ERROR: data race detected"); std::exit(1); }
 #include "data_race_checker.test.h"
 
 int main()

include/scl/utils/pass_data_race_checker.test.cpp

@@ -2,10 +2,16 @@
 // Created by David Rowland on 11/02/2025.
 //
 
-#define DATA_RACE_DETECTED std::exit(1);
+#define DATA_RACE_DETECTED { std::println ("ERROR: data race detected"); std::exit(1); }
 #include "data_race_checker.test.h"
 
 int main()
 {
   test_no_data_race();
+
+  // Single threaded-tests
+  test_no_data_race_read_read();
+  test_no_data_race_read_write();
+  test_no_data_race_write_read();
+  test_no_data_race_write_write();
 }