fix tests (oops) and also make them more robust

else localFunction in test_local_function is likely inlined and the tests fail

.forgejo/workflows/demo.yaml

@@ -1,8 +0,0 @@
-on: [push]
-jobs:
-  test:
-    runs-on: docker
-    steps:
-      - run: echo All good!
-      - uses: actions/checkout@v4
-      - run: ls -la

.idea/vcs.xml

@@ -2,5 +2,6 @@
 <project version="4">
   <component name="VcsDirectoryMappings">
     <mapping directory="$PROJECT_DIR$" vcs="Git" />
+    <mapping directory="$PROJECT_DIR$/lib/catch2" vcs="Git" />
   </component>
 </project>

tests/CMakeLists.txt

@@ -7,6 +7,7 @@ add_executable(mach_detours_tests
         test_threads.cpp
         test_transaction.cpp
 )
+target_compile_options(mach_detours_tests PRIVATE "-O0")
 
 # The target function must be in a shared library because otherwise it might be in the same code page as the test.cpp functions.
 # Between attach and commit the target function's code page is not executable, which can mean our test driver code would not
@@ -15,6 +16,7 @@ add_library(mach_detours_tests_lib SHARED
         lib_function.c
         lib_function.h
 )
+target_compile_options(mach_detours_tests_lib PRIVATE "-O0")
 
 target_link_libraries(mach_detours_tests
         PRIVATE

tests/test_local_function.cpp

@@ -4,6 +4,9 @@
 
 #include <mach_detours.h>
 
+// Note that this file must be compiled without optimizations (or at least without inlining)
+// else localFunction() calls below are likely going to be inlined at the call site, and detours will not work.
+
 static int localFunctionCounter = 0;
 int localFunction()
 {

tests/test_threads.cpp

@@ -1,5 +1,6 @@
 // Copyright (c) Lysann Tranvouez.  All rights reserved.
 
+#include <algorithm>
 #include <catch2/catch_test_macros.hpp>
 
 #include <mach_detours.h>
@@ -29,33 +30,62 @@ static const char* localFunctionDetour()
 }
 static const char* (*realLocalFunction)() = localFunction;
 
+struct threadData
+{
+    std::atomic<int> loopCounter;
+    bool cancel = false;
+};
+
 static void* threadFunc(void* pUserArg)
 {
-    const bool* pCancel = static_cast<const bool*>(pUserArg);
+    auto& data = *static_cast<threadData*>(pUserArg);
-    while (!*pCancel) {
+    while (!data.cancel) {
         localFunction();
         localFunction();
         localFunction();
         localFunction();
         localFunction();
+        ++data.loopCounter;
     }
     return nullptr;
 }
 
+template<size_t numThreads>
+bool resetLoopCountersAndWaitForLoop(std::array<threadData, numThreads>& threadDatas, std::optional<std::chrono::seconds> timeout = std::nullopt)
+{
+    for (auto i = 0; i < numThreads; i++) {
+        threadDatas[i].loopCounter = 0;
+    }
+
+    const auto endTime = timeout ? std::make_optional(std::chrono::steady_clock::now() + *timeout) : std::nullopt;
+    while (true) {
+        // we check for >= 2 to be sure we did a full loop - we don't want to execute just the line incrementing the counter!
+        const bool anyLooped = std::ranges::any_of(threadDatas, [](const threadData& data){ return data.loopCounter >= 2; });
+        if (anyLooped) {
+            return true;
+        }
+        if (endTime && std::chrono::steady_clock::now() > *endTime) {
+            return false;
+        }
+        usleep( 100 );
+    }
+}
+
 TEST_CASE( "Handling other threads", "[attach][local][threads]" )
 {
-    bool cancelThreads = false;
+    using namespace std::chrono_literals;
 
     constexpr auto numThreads = 150;
     pthread_t threads[numThreads];
+    std::array<threadData, numThreads> threadDatas{};
     for (auto i = 0; i < numThreads; i++) {
-        pthread_create(&threads[i], nullptr, threadFunc, (void*)&cancelThreads);
+        pthread_create(&threads[i], nullptr, threadFunc, (void*)&threadDatas[i]);
     }
 
     SECTION( "running threads modify a value" )
     {
         const int saved_b = g_b;
-        usleep( 1000 ); // let the threads update g_b a bit
+        resetLoopCountersAndWaitForLoop(threadDatas);
         CHECK( saved_b != g_b );
     }
 
@@ -79,13 +109,13 @@ TEST_CASE( "Handling other threads", "[attach][local][threads]" )
         }
 
         int saved_b = g_b;
-        usleep( 1000 ); // let any running thread update g_b a bit
+        resetLoopCountersAndWaitForLoop(threadDatas, 1s);
         CHECK( saved_b == g_b );
 
         CHECK( detour_transaction_commit() == err_none );
 
         saved_b = g_b;
-        usleep( 1000 ); // let the threads update g_b a bit
+        resetLoopCountersAndWaitForLoop(threadDatas);
         CHECK( saved_b != g_b );
     }
 
@@ -95,7 +125,7 @@ TEST_CASE( "Handling other threads", "[attach][local][threads]" )
         CHECK( detour_transaction_abort() == err_none );
 
         const int saved_b = g_b;
-        usleep( 1000 ); // let the threads update g_b a bit
+        resetLoopCountersAndWaitForLoop(threadDatas);
         CHECK( saved_b != g_b );
     }
 
@@ -108,15 +138,17 @@ TEST_CASE( "Handling other threads", "[attach][local][threads]" )
         CHECK( detour_attach_and_commit(reinterpret_cast<detour_func_t*>(&realLocalFunction), reinterpret_cast<detour_func_t>(localFunctionDetour)) == err_none );
 
         const int saved_b = g_b;
-        usleep( 1000 ); // let the threads update g_b a bit
+        resetLoopCountersAndWaitForLoop(threadDatas);
-        CHECK( saved_b != g_b );
+        CHECK( saved_b == g_b );
 
         // clean up
         CHECK( detour_transaction_begin_managed() == err_none );
         CHECK( detour_detach_and_commit(reinterpret_cast<detour_func_t*>(&realLocalFunction), reinterpret_cast<detour_func_t>(localFunctionDetour)) == err_none );
     }
 
-    cancelThreads = true;
+    for (auto i = 0; i < numThreads; i++) {
+        threadDatas[i].cancel = true;
+    }
     for (auto i = 0; i < numThreads; i++) {
         pthread_join(threads[i], nullptr);
     }