From ae92918d275d81a7cd4c76237defa8b657f04d81 Mon Sep 17 00:00:00 2001 From: zeromus <zeromus@users.sf.net> Date: Tue, 23 Aug 2016 21:12:49 +0000 Subject: [PATCH] fix bugs in libretro's scond for win32, hopefully. --- .../src/libretro-common/rthreads/rthreads.c | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/desmume/src/libretro-common/rthreads/rthreads.c b/desmume/src/libretro-common/rthreads/rthreads.c index 109ba8bf9..229a871ae 100644 --- a/desmume/src/libretro-common/rthreads/rthreads.c +++ b/desmume/src/libretro-common/rthreads/rthreads.c @@ -80,10 +80,9 @@ struct slock struct scond { #ifdef USE_WIN32_THREADS - /* this might could be done with a semaphore? I'm not sure. */ HANDLE event; - int waiters; - bool waiting_ack; + volatile int waiters; + volatile bool waiting_ack; HANDLE ack; #else pthread_cond_t cond; @@ -314,9 +313,12 @@ scond_t *scond_new(void) return NULL; #ifdef USE_WIN32_THREADS - cond->event = CreateEvent(NULL, FALSE, FALSE, NULL); - cond->waiters = 0; + /* this is very complex because recreating condition variable semantics with win32 parts is not easy (or maybe it is and I just havent seen how) */ + /* the main problem is that a condition variable can be used to wake up a thread, but only if the thread is already waiting. */ + /* whereas a win32 event will 'wake up' a thread in advance (the event will be set in advance, so a 'waiter' wont even have to wait on it) */ + cond->event = CreateEvent(NULL, FALSE, FALSE, NULL); cond->ack = CreateEvent(NULL, FALSE, FALSE, NULL); + cond->waiters = 0; cond->waiting_ack = false; event_created = !!cond->event; #else @@ -365,20 +367,31 @@ void scond_wait(scond_t *cond, slock_t *lock) #ifdef USE_WIN32_THREADS /* remember: we currently have mutex so this will be safe */ cond->waiters++; + if(cond->waiting_ack) + WaitForSingleObject(cond->ack,INFINITE); + ReleaseMutex(lock->lock); /* wait for a signaller */ WaitForSingleObject(cond->event, INFINITE); - /* the algorithm hinges on this uncontrolled variable access. It's too hard to explain why it's safe. (..erm.. I hope it is) */ - cond->waiting_ack = false; + + /* the algorithm hinges on this doing this stuff outside of the mutex */ + /* suppose several people signal right now. Actually, only one of them can. He'll be waiting on an ack signal! *inside the mutex* */ + /* we need to clear waiting_ack before we release him, otherwise it might race to set it to true and beat us */ + /* also: suppose several people are waiting right now (in the above wait on `event`). */ + /* well, only one of them is going to get freed by a signal; it must have been us */ + /* notice that both of the waits for ack are inside the mutex; this guarantees only one of them can be waiting at a time */ + /* that's essential for making this safe */ + //if(cond->waiting_ack) + { + cond->waiting_ack = false; + SetEvent(cond->ack); + } /* reacquire mutex and finish up */ WaitForSingleObject(lock->lock, INFINITE); cond->waiters--; - /* only when the waiter is COMPLETELY FINISHED do we ack a signaller */ - SetEvent(cond->ack); - #else pthread_cond_wait(&cond->cond, &lock->lock); #endif @@ -419,15 +432,14 @@ void scond_signal(scond_t *cond) /* OK, someone is waiting for a signal */ - /* if we're waiting for an ack, we can't proceed until we receive an ack (signifies cond->event is freed up) */ + /* if we're waiting for an ack, we can't proceed until we receive an ack (signifies that the event is freed up from the waiter destined to be waked by it) */ if(cond->waiting_ack) WaitForSingleObject(cond->ack,INFINITE); - /* so someone set the ack event; a waiter is proceeding. we can wait for another ack now... */ + /* before any further waits or signals, we'll need to wait for a waiter to wake up */ cond->waiting_ack = true; - /* ...and set an event to wake up a waiter so he can actually set that ack... */ - /* but definitely not right now, since we still have the mutex. So it may take a while */ + /* the main wakeup event. the winning waiter definitely won't wake up this moment since we're in a mutex. */ SetEvent(cond->event); #else