thread: Locking mutexes and rwlocks are now void functions.

Almost nothing checks these return values, and there's no reason a valid
lock should fail to operate. The cases where a lock isn't valid (it's a
bogus pointer, it was previously destroyed, a thread is unlocking a lock it
doesn't own, etc) are undefined behavior and always were, and should be
treated as an application bug.

Reference Issue #8096.
This commit is contained in:
Ryan C. Gordon
2023-10-25 10:00:26 -04:00
parent 082ef41566
commit 899eb0d042
21 changed files with 496 additions and 746 deletions

View File

@@ -20,7 +20,7 @@
*/
#include "SDL_internal.h"
/* An implementation of mutexes using semaphores */
// An implementation of mutexes using semaphores
#include "SDL_systhread_c.h"
@@ -31,13 +31,9 @@ struct SDL_Mutex
SDL_Semaphore *sem;
};
/* Create a mutex */
SDL_Mutex *SDL_CreateMutex(void)
{
SDL_Mutex *mutex;
/* Allocate mutex memory */
mutex = (SDL_Mutex *)SDL_calloc(1, sizeof(*mutex));
SDL_Mutex *mutex = (SDL_Mutex *)SDL_calloc(1, sizeof(*mutex));
#ifndef SDL_THREADS_DISABLED
if (mutex) {
@@ -52,12 +48,11 @@ SDL_Mutex *SDL_CreateMutex(void)
} else {
SDL_OutOfMemory();
}
#endif /* !SDL_THREADS_DISABLED */
#endif // !SDL_THREADS_DISABLED
return mutex;
}
/* Free the mutex */
void SDL_DestroyMutex(SDL_Mutex *mutex)
{
if (mutex) {
@@ -68,94 +63,72 @@ void SDL_DestroyMutex(SDL_Mutex *mutex)
}
}
/* Lock the mutex */
int SDL_LockMutex(SDL_Mutex *mutex) SDL_NO_THREAD_SAFETY_ANALYSIS /* clang doesn't know about NULL mutexes */
void SDL_LockMutex(SDL_Mutex *mutex) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
#ifdef SDL_THREADS_DISABLED
return 0;
#else
SDL_threadID this_thread;
if (mutex == NULL) {
return 0;
}
this_thread = SDL_ThreadID();
if (mutex->owner == this_thread) {
++mutex->recursive;
} else {
/* The order of operations is important.
We set the locking thread id after we obtain the lock
so unlocks from other threads will fail.
*/
SDL_WaitSemaphore(mutex->sem);
mutex->owner = this_thread;
mutex->recursive = 0;
}
return 0;
#endif /* SDL_THREADS_DISABLED */
}
/* try Lock the mutex */
int SDL_TryLockMutex(SDL_Mutex *mutex)
{
#ifdef SDL_THREADS_DISABLED
return 0;
#else
int retval = 0;
SDL_threadID this_thread;
if (mutex == NULL) {
return 0;
}
this_thread = SDL_ThreadID();
if (mutex->owner == this_thread) {
++mutex->recursive;
} else {
/* The order of operations is important.
We set the locking thread id after we obtain the lock
so unlocks from other threads will fail.
*/
retval = SDL_TryWaitSemaphore(mutex->sem);
if (retval == 0) {
#ifndef SDL_THREADS_DISABLED
if (mutex != NULL) {
SDL_threadID this_thread = SDL_ThreadID();
if (mutex->owner == this_thread) {
++mutex->recursive;
} else {
/* The order of operations is important.
We set the locking thread id after we obtain the lock
so unlocks from other threads will fail.
*/
SDL_WaitSemaphore(mutex->sem);
mutex->owner = this_thread;
mutex->recursive = 0;
}
}
return retval;
#endif /* SDL_THREADS_DISABLED */
}
/* Unlock the mutex */
int SDL_UnlockMutex(SDL_Mutex *mutex) SDL_NO_THREAD_SAFETY_ANALYSIS /* clang doesn't know about NULL mutexes */
int SDL_TryLockMutex(SDL_Mutex *mutex)
{
#ifdef SDL_THREADS_DISABLED
return 0;
#else
if (mutex == NULL) {
return 0;
int retval = 0;
#ifndef SDL_THREADS_DISABLED
if (mutex != NULL) {
SDL_threadID this_thread = SDL_ThreadID();
if (mutex->owner == this_thread) {
++mutex->recursive;
} else {
/* The order of operations is important.
We set the locking thread id after we obtain the lock
so unlocks from other threads will fail.
*/
retval = SDL_TryWaitSemaphore(mutex->sem);
if (retval == 0) {
mutex->owner = this_thread;
mutex->recursive = 0;
}
}
}
/* If we don't own the mutex, we can't unlock it */
if (SDL_ThreadID() != mutex->owner) {
return SDL_SetError("mutex not owned by this thread");
}
if (mutex->recursive) {
--mutex->recursive;
} else {
/* The order of operations is important.
First reset the owner so another thread doesn't lock
the mutex and set the ownership before we reset it,
then release the lock semaphore.
*/
mutex->owner = 0;
SDL_PostSemaphore(mutex->sem);
}
return 0;
#endif /* SDL_THREADS_DISABLED */
#endif // SDL_THREADS_DISABLED
return retval;
}
void SDL_UnlockMutex(SDL_Mutex *mutex) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
#ifndef SDL_THREADS_DISABLED
if (mutex != NULL) {
// If we don't own the mutex, we can't unlock it
if (SDL_ThreadID() != mutex->owner) {
SDL_assert(!"Tried to unlock a mutex we don't own!");
return; // (undefined behavior!) SDL_SetError("mutex not owned by this thread");
}
if (mutex->recursive) {
--mutex->recursive;
} else {
/* The order of operations is important.
First reset the owner so another thread doesn't lock
the mutex and set the ownership before we reset it,
then release the lock semaphore.
*/
mutex->owner = 0;
SDL_PostSemaphore(mutex->sem);
}
}
#endif // SDL_THREADS_DISABLED
}

View File

@@ -20,7 +20,7 @@
*/
#include "SDL_internal.h"
/* An implementation of rwlocks using mutexes, condition variables, and atomics. */
// An implementation of rwlocks using mutexes, condition variables, and atomics.
#include "SDL_systhread_c.h"
@@ -30,7 +30,7 @@
* will be chosen at runtime), the function names need to be
* suffixed
*/
/* !!! FIXME: this is quite a tapdance with macros and the build system, maybe we can simplify how we do this. --ryan. */
// !!! FIXME: this is quite a tapdance with macros and the build system, maybe we can simplify how we do this. --ryan.
#ifndef SDL_THREAD_GENERIC_RWLOCK_SUFFIX
#define SDL_CreateRWLock_generic SDL_CreateRWLock
#define SDL_DestroyRWLock_generic SDL_DestroyRWLock
@@ -95,63 +95,48 @@ void SDL_DestroyRWLock_generic(SDL_RWLock *rwlock)
}
}
int SDL_LockRWLockForReading_generic(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS /* clang doesn't know about NULL mutexes */
void SDL_LockRWLockForReading_generic(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
#ifndef SDL_THREADS_DISABLED
if (!rwlock) {
return SDL_InvalidParamError("rwlock");
} else if (SDL_LockMutex(rwlock->lock) == -1) {
return -1;
if (rwlock) {
// !!! FIXME: these don't have to be atomic, we always gate them behind a mutex.
SDL_LockMutex(rwlock->lock);
SDL_assert(SDL_AtomicGet(&rwlock->writer_count) == 0); // shouldn't be able to grab lock if there's a writer!
SDL_AtomicAdd(&rwlock->reader_count, 1);
SDL_UnlockMutex(rwlock->lock); // other readers can attempt to share the lock.
}
SDL_assert(SDL_AtomicGet(&rwlock->writer_count) == 0); /* shouldn't be able to grab lock if there's a writer! */
SDL_AtomicAdd(&rwlock->reader_count, 1);
SDL_UnlockMutex(rwlock->lock); /* other readers can attempt to share the lock. */
#endif
return 0;
}
int SDL_LockRWLockForWriting_generic(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS /* clang doesn't know about NULL mutexes */
void SDL_LockRWLockForWriting_generic(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
#ifndef SDL_THREADS_DISABLED
if (!rwlock) {
return SDL_InvalidParamError("rwlock");
} else if (SDL_LockMutex(rwlock->lock) == -1) {
return -1;
}
if (rwlock) {
SDL_LockMutex(rwlock->lock);
while (SDL_AtomicGet(&rwlock->reader_count) > 0) { // while something is holding the shared lock, keep waiting.
SDL_WaitCondition(rwlock->condition, rwlock->lock); // release the lock and wait for readers holding the shared lock to release it, regrab the lock.
}
while (SDL_AtomicGet(&rwlock->reader_count) > 0) { /* while something is holding the shared lock, keep waiting. */
SDL_WaitCondition(rwlock->condition, rwlock->lock); /* release the lock and wait for readers holding the shared lock to release it, regrab the lock. */
// we hold the lock!
SDL_AtomicAdd(&rwlock->writer_count, 1); // we let these be recursive, but the API doesn't require this. It _does_ trust you unlock correctly!
}
/* we hold the lock! */
SDL_AtomicAdd(&rwlock->writer_count, 1); /* we let these be recursive, but the API doesn't require this. It _does_ trust you unlock correctly! */
#endif
return 0;
}
int SDL_TryLockRWLockForReading_generic(SDL_RWLock *rwlock)
{
#ifndef SDL_THREADS_DISABLED
int rc;
if (rwlock) {
const int rc = SDL_TryLockMutex(rwlock->lock);
if (rc != 0) {
// !!! FIXME: there is a small window where a reader has to lock the mutex, and if we hit that, we will return SDL_RWLOCK_TIMEDOUT even though we could have shared the lock.
return rc;
}
if (!rwlock) {
return SDL_InvalidParamError("rwlock");
SDL_assert(SDL_AtomicGet(&rwlock->writer_count) == 0); // shouldn't be able to grab lock if there's a writer!
SDL_AtomicAdd(&rwlock->reader_count, 1);
SDL_UnlockMutex(rwlock->lock); // other readers can attempt to share the lock.
}
rc = SDL_TryLockMutex(rwlock->lock);
if (rc != 0) {
/* !!! FIXME: there is a small window where a reader has to lock the mutex, and if we hit that, we will return SDL_RWLOCK_TIMEDOUT even though we could have shared the lock. */
return rc;
}
SDL_assert(SDL_AtomicGet(&rwlock->writer_count) == 0); /* shouldn't be able to grab lock if there's a writer! */
SDL_AtomicAdd(&rwlock->reader_count, 1);
SDL_UnlockMutex(rwlock->lock); /* other readers can attempt to share the lock. */
#endif
return 0;
@@ -160,46 +145,41 @@ int SDL_TryLockRWLockForReading_generic(SDL_RWLock *rwlock)
int SDL_TryLockRWLockForWriting_generic(SDL_RWLock *rwlock)
{
#ifndef SDL_THREADS_DISABLED
int rc;
if (rwlock) {
const int rc = SDL_TryLockMutex(rwlock->lock);
if (rc != 0) {
return rc;
}
if (!rwlock) {
return SDL_InvalidParamError("rwlock");
} else if ((rc = SDL_TryLockMutex(rwlock->lock)) != 0) {
return rc;
if (SDL_AtomicGet(&rwlock->reader_count) > 0) { // a reader is using the shared lock, treat it as unavailable.
SDL_UnlockMutex(rwlock->lock);
return SDL_RWLOCK_TIMEDOUT;
}
// we hold the lock!
SDL_AtomicAdd(&rwlock->writer_count, 1); // we let these be recursive, but the API doesn't require this. It _does_ trust you unlock correctly!
}
if (SDL_AtomicGet(&rwlock->reader_count) > 0) { /* a reader is using the shared lock, treat it as unavailable. */
SDL_UnlockMutex(rwlock->lock);
return SDL_RWLOCK_TIMEDOUT;
}
/* we hold the lock! */
SDL_AtomicAdd(&rwlock->writer_count, 1); /* we let these be recursive, but the API doesn't require this. It _does_ trust you unlock correctly! */
#endif
return 0;
}
int SDL_UnlockRWLock_generic(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS /* clang doesn't know about NULL mutexes */
void SDL_UnlockRWLock_generic(SDL_RWLock *rwlock) SDL_NO_THREAD_SAFETY_ANALYSIS // clang doesn't know about NULL mutexes
{
#ifndef SDL_THREADS_DISABLED
if (!rwlock) {
return SDL_InvalidParamError("rwlock");
if (rwlock) {
SDL_LockMutex(rwlock->lock); // recursive lock for writers, readers grab lock to make sure things are sane.
if (SDL_AtomicGet(&rwlock->reader_count) > 0) { // we're a reader
SDL_AtomicAdd(&rwlock->reader_count, -1);
SDL_BroadcastCondition(rwlock->condition); // alert any pending writers to attempt to try to grab the lock again.
} else if (SDL_AtomicGet(&rwlock->writer_count) > 0) { // we're a writer
SDL_AtomicAdd(&rwlock->writer_count, -1);
SDL_UnlockMutex(rwlock->lock); // recursive unlock.
}
SDL_UnlockMutex(rwlock->lock);
}
SDL_LockMutex(rwlock->lock); /* recursive lock for writers, readers grab lock to make sure things are sane. */
if (SDL_AtomicGet(&rwlock->reader_count) > 0) { /* we're a reader */
SDL_AtomicAdd(&rwlock->reader_count, -1);
SDL_BroadcastCondition(rwlock->condition); /* alert any pending writers to attempt to try to grab the lock again. */
} else if (SDL_AtomicGet(&rwlock->writer_count) > 0) { /* we're a writer */
SDL_AtomicAdd(&rwlock->writer_count, -1);
SDL_UnlockMutex(rwlock->lock); /* recursive unlock. */
}
SDL_UnlockMutex(rwlock->lock);
#endif
return 0;
}

View File

@@ -27,12 +27,12 @@
SDL_RWLock *SDL_CreateRWLock_generic(void);
void SDL_DestroyRWLock_generic(SDL_RWLock *rwlock);
int SDL_LockRWLockForReading_generic(SDL_RWLock *rwlock);
int SDL_LockRWLockForWriting_generic(SDL_RWLock *rwlock);
void SDL_LockRWLockForReading_generic(SDL_RWLock *rwlock);
void SDL_LockRWLockForWriting_generic(SDL_RWLock *rwlock);
int SDL_TryLockRWLockForReading_generic(SDL_RWLock *rwlock);
int SDL_TryLockRWLockForWriting_generic(SDL_RWLock *rwlock);
int SDL_UnlockRWLock_generic(SDL_RWLock *rwlock);
void SDL_UnlockRWLock_generic(SDL_RWLock *rwlock);
#endif /* SDL_THREAD_GENERIC_RWLOCK_SUFFIX */
#endif // SDL_THREAD_GENERIC_RWLOCK_SUFFIX
#endif /* SDL_sysrwlock_c_h_ */
#endif // SDL_sysrwlock_c_h_