From 0d73f2fffa98ae290982958660c479ad28f16b61 Mon Sep 17 00:00:00 2001 From: DanyDollaro <53940883+DanyDollaro@users.noreply.github.com> Date: Mon, 30 Sep 2024 20:57:16 +0200 Subject: [PATCH] Added mutex tests (#1501) * Added mutex tests. Add errorcheck in safe mode for Posix threads. Make non-recursive locks fail when used recursively on Windows. Fix thread pool tests. Simple locking count. --------- Co-authored-by: Christoffer Lerno --- lib/std/core/runtime.c3 | 1 + lib/std/os/posix/threads.c3 | 4 +- lib/std/threads/os/thread_posix.c3 | 6 ++ lib/std/threads/os/thread_win32.c3 | 33 +++---- releasenotes.md | 2 + test/unit/stdlib/threads/mutex.c3 | 148 +++++++++++++++++++++++++++++ test/unit/stdlib/threads/pool.c3 | 20 +++- 7 files changed, 188 insertions(+), 26 deletions(-) create mode 100644 test/unit/stdlib/threads/mutex.c3 diff --git a/lib/std/core/runtime.c3 b/lib/std/core/runtime.c3 index e4b5ebd05..d12daccf1 100644 --- a/lib/std/core/runtime.c3 +++ b/lib/std/core/runtime.c3 @@ -221,6 +221,7 @@ fn bool run_tests(TestUnit[] tests) name.appendf("Testing %s ", unit.name); name.append_repeat('.', max_name - unit.name.len + 2); io::printf("%s ", name.str_view()); + (void)io::stdout().flush(); if (libc::setjmp(&context.buf) == 0) { if (catch err = unit.func()) diff --git a/lib/std/os/posix/threads.c3 b/lib/std/os/posix/threads.c3 index 330940ef8..9e1fd1969 100644 --- a/lib/std/os/posix/threads.c3 +++ b/lib/std/os/posix/threads.c3 @@ -3,8 +3,8 @@ import std::thread; import libc; const PTHREAD_MUTEX_NORMAL = 0; -const PTHREAD_MUTEX_ERRORCHECK = 1; -const PTHREAD_MUTEX_RECURSIVE = 2; +const PTHREAD_MUTEX_ERRORCHECK = env::LINUX ? 2 : 1; +const PTHREAD_MUTEX_RECURSIVE = env::LINUX ? 1 : 2; def PosixThreadFn = fn void*(void*); distinct Pthread_t = void*; diff --git a/lib/std/threads/os/thread_posix.c3 b/lib/std/threads/os/thread_posix.c3 index ff442ec8c..61b1f7fbb 100644 --- a/lib/std/threads/os/thread_posix.c3 +++ b/lib/std/threads/os/thread_posix.c3 @@ -24,6 +24,12 @@ fn void! NativeMutex.init(&self, MutexType type) { if (posix::pthread_mutexattr_settype(&attr, posix::PTHREAD_MUTEX_RECURSIVE)) return ThreadFault.INIT_FAILED?; } + else + { + $if env::COMPILER_SAFE_MODE: + if (posix::pthread_mutexattr_settype(&attr, posix::PTHREAD_MUTEX_ERRORCHECK)) return ThreadFault.INIT_FAILED?; + $endif + } if (posix::pthread_mutex_init(&self.mutex, &attr)) return ThreadFault.INIT_FAILED?; self.initialized = true; } diff --git a/lib/std/threads/os/thread_win32.c3 b/lib/std/threads/os/thread_win32.c3 index d9f3fe385..8181bc68c 100644 --- a/lib/std/threads/os/thread_win32.c3 +++ b/lib/std/threads/os/thread_win32.c3 @@ -12,7 +12,7 @@ struct NativeMutex } // Size is less than a Win32_HANDLE so due to alignment // there is no benefit to pack these into a bitstruct. - bool already_locked; + uint locks; bool recursive; bool timed; } @@ -40,7 +40,7 @@ struct NativeConditionVariable fn void! NativeMutex.init(&mtx, MutexType type) { - mtx.already_locked = false; + mtx.locks = 0; mtx.recursive = (bool)(type & thread::MUTEX_RECURSIVE); mtx.timed = (bool)(type & thread::MUTEX_TIMED); if (!mtx.timed) @@ -53,6 +53,7 @@ fn void! NativeMutex.init(&mtx, MutexType type) fn void! NativeMutex.destroy(&mtx) { + mtx.locks = 0; if (!mtx.timed) { win32::deleteCriticalSection(&mtx.critical_section); @@ -79,11 +80,11 @@ fn void! NativeMutex.lock(&mtx) } } - if (!mtx.recursive) + if (!mtx.recursive && mtx.locks) { - while (mtx.already_locked) win32::sleep(1); + return ThreadFault.LOCK_FAILED?; } - mtx.already_locked = true; + mtx.locks++; } @@ -103,20 +104,11 @@ fn void! NativeMutex.lock_timeout(&mtx, ulong ms) default: return ThreadFault.LOCK_FAILED?; } - if (!mtx.recursive) + if (!mtx.recursive && mtx.locks) { - usz left_timeout = ms - 1; - while (mtx.already_locked) - { - if (left_timeout-- == 0) - { - win32::releaseMutex(mtx.handle); - return ThreadFault.LOCK_TIMEOUT?; - } - win32::sleep(1); - } - mtx.already_locked = true; + return ThreadFault.LOCK_FAILED?; } + mtx.locks++; } fn bool NativeMutex.try_lock(&mtx) @@ -128,7 +120,7 @@ fn bool NativeMutex.try_lock(&mtx) if (!success) return false; if (!mtx.recursive) { - if (mtx.already_locked) + if (mtx.locks) { if (mtx.timed) { @@ -140,14 +132,15 @@ fn bool NativeMutex.try_lock(&mtx) } return false; } - mtx.already_locked = true; } + mtx.locks++; return true; } fn void! NativeMutex.unlock(&mtx) { - mtx.already_locked = false; + if (!mtx.locks) return ThreadFault.UNLOCK_FAILED?; + mtx.locks--; if (!mtx.timed) { win32::leaveCriticalSection(&mtx.critical_section); diff --git a/releasenotes.md b/releasenotes.md index d6cfa5a66..65dda63e1 100644 --- a/releasenotes.md +++ b/releasenotes.md @@ -66,6 +66,8 @@ - Improved error messages on `Foo a = foo { 1 };` #1496 - Bug in json decoder escape handling. - Fix bug when reading zip manifest, that would not return a zero terminated string. #1490 +- Fix thread tests. +- Detect recursion errors on non-recursive mutexes in safe mode. ### Stdlib changes - Additional init functions for hashmap. diff --git a/test/unit/stdlib/threads/mutex.c3 b/test/unit/stdlib/threads/mutex.c3 new file mode 100644 index 000000000..9867bb4cf --- /dev/null +++ b/test/unit/stdlib/threads/mutex.c3 @@ -0,0 +1,148 @@ +module thread_test; + +import std::thread; +import std::os; + +const TEST_MAGNITUDE = 10; + + +fn void! lock_control_test() @test +{ + Mutex m; + m.init()!; + m.lock()!; + assert(@catch(m.lock())); +} + +fn void! unlock_control_test() @test +{ + Mutex m; + m.init()!; + assert(@catch(m.unlock())); +} + +fn void! lock_with_double_unlock_test() @test +{ + Mutex m; + m.init()!; + + m.lock()!; + m.unlock()!; + assert(@catch(m.unlock())); +} + +fn void! own_mutex(Mutex* m) +{ + m.lock()!!; + m.unlock()!!; +} + +fn void! ensure_owner_checks() @test +{ + Mutex m; + m.init()!; + + Thread[3 * TEST_MAGNITUDE] threads; + + foreach(&t : threads) + { + t.create((ThreadFn)&own_mutex, &m)!; + } + + foreach(&t : threads) + { + t.join()!; + } + + own_mutex(&m)!; +} + +struct ArgsWrapper1 +{ + Mutex* m; + ulong* v; +} + +fn void shared_mutex_increment(ArgsWrapper1* args) +{ + args.m.lock()!!; + args.v++; + args.m.unlock()!!; +} + +fn void shared_mutex_decrement(ArgsWrapper1* args) +{ + args.m.lock()!!; + args.v--; + args.m.unlock()!!; +} + +fn void! shared_mutex() @test +{ + Mutex m; + m.init()!; + m.lock()!; + + ulong v; + + ArgsWrapper1 args = + { + .m = &m, + .v = &v + }; + + // An even number of threads must be chosen + Thread[6 * TEST_MAGNITUDE] threads; + for (int i = 0; i < threads.len / 2; i++) + { + (&threads[i]).create((ThreadFn)&shared_mutex_increment, &args)!; + } + for (int i = (threads.len / 2); i < threads.len; i++) + { + (&threads[i]).create((ThreadFn)&shared_mutex_decrement, &args)!; + } + + m.unlock()!; + foreach(&t : threads) + { + t.join()!; + } + assert(v == 0); +} + +// Recursive mutex + +fn void! acquire_recursively(RecursiveMutex* m) +{ + // TODO: The recursive mutex functions can not directly be called via pointer + + for (usz i = 0; i < 5 * TEST_MAGNITUDE; i++) + { + ((Mutex*)m).lock()!!; + } + + for (usz i = 0; i < 5 * TEST_MAGNITUDE; i++) + { + ((Mutex*)m).unlock()!!; + } +} + +fn void! test_recursive_mutex() @test +{ + RecursiveMutex m; + m.init()!; + defer m.destroy()!!; + + Thread[3 * TEST_MAGNITUDE] threads; + foreach(&t : threads) + { + t.create((ThreadFn)&acquire_recursively, &m)!; + } + + foreach(&t : threads) + { + t.join()!; + } + + return acquire_recursively(&m); +} diff --git a/test/unit/stdlib/threads/pool.c3 b/test/unit/stdlib/threads/pool.c3 index 669bcdb4d..31cb3076c 100644 --- a/test/unit/stdlib/threads/pool.c3 +++ b/test/unit/stdlib/threads/pool.c3 @@ -17,7 +17,7 @@ fn void! init_destroy() @test fn void! push_destroy() @test { - for (usz i = 0; i < 20; i++) + for FOO: (usz i = 0; i < 20; i++) { x = 0; int y = 20; @@ -26,9 +26,19 @@ fn void! push_destroy() @test defer pool.destroy()!!; work_done.lock()!!; pool.push(&do_work, &y)!; - work_done.lock()!!; - assert(x == y, "%d: %d != %d", i, x, y); work_done.unlock()!!; + for (int j = 0; j < 1000; j++) + { + work_done.lock()!!; + if (@atomic_load(x) == @atomic_load(y)) + { + (void)work_done.unlock(); + break FOO; + } + (void)work_done.unlock(); + thread::yield(); + } + assert(false, "y never changed"); } } @@ -42,6 +52,7 @@ fn void! push_stop() @test pool.init()!; work_done.lock()!!; pool.push(&do_work, &y)!; + work_done.unlock()!!; pool.stop_and_destroy()!!; assert(x == y, "%d: %d != %d", i, x, y); } @@ -56,11 +67,12 @@ fn void startup() @init { } fn void shutdown() @finalizer { - work_done.destroy()!!; + (void)work_done.destroy(); } fn int do_work(void* arg) { + work_done.lock()!!; x = *(int*)arg; work_done.unlock()!!; return 0;