From fa8c83c1c18e0185ffdf3321c7e56b8a450bcedb Mon Sep 17 00:00:00 2001 From: "Ryan C. Gordon" Date: Sun, 3 Jan 2016 06:50:50 -0500 Subject: [PATCH] Remove almost all instances of "volatile" keyword. As Tiffany pointed out in Bugzilla, volatile is not useful for thread safety: https://software.intel.com/en-us/blogs/2007/11/30/volatile-almost-useless-for-multi-threaded-programming/ Some of these volatiles didn't need to be, some were otherwise protected by spinlocks or mutexes, and some got moved over to SDL_atomic_t data, etc. Fixes Bugzilla #3220. --- src/dynapi/SDL_dynapi.c | 2 +- src/events/SDL_events.c | 38 +++++++++++---------- src/haptic/windows/SDL_windowshaptic.c | 2 +- src/haptic/windows/SDL_windowshaptic_c.h | 4 +-- src/haptic/windows/SDL_xinputhaptic.c | 4 +-- src/timer/SDL_timer.c | 43 ++++++++++-------------- src/video/android/SDL_androidtouch.c | 2 +- test/testatomic.c | 12 +++---- test/testlock.c | 12 ++++--- test/testmultiaudio.c | 11 +++--- test/torturethread.c | 8 ++--- 11 files changed, 66 insertions(+), 72 deletions(-) diff --git a/src/dynapi/SDL_dynapi.c b/src/dynapi/SDL_dynapi.c index 1411f8c93..c26baf379 100644 --- a/src/dynapi/SDL_dynapi.c +++ b/src/dynapi/SDL_dynapi.c @@ -293,7 +293,7 @@ SDL_InitDynamicAPI(void) * SDL_CreateThread() would also call this function before building the * new thread). */ - static volatile SDL_bool already_initialized = SDL_FALSE; + static SDL_bool already_initialized = SDL_FALSE; /* SDL_AtomicLock calls SDL mutex functions to emulate if SDL_ATOMIC_DISABLED, which we can't do here, so in such a diff --git a/src/events/SDL_events.c b/src/events/SDL_events.c index ffd103824..bca53759a 100644 --- a/src/events/SDL_events.c +++ b/src/events/SDL_events.c @@ -73,15 +73,15 @@ typedef struct _SDL_SysWMEntry static struct { SDL_mutex *lock; - volatile SDL_bool active; - volatile int count; - volatile int max_events_seen; + SDL_atomic_t active; + SDL_atomic_t count; + int max_events_seen; SDL_EventEntry *head; SDL_EventEntry *tail; SDL_EventEntry *free; SDL_SysWMEntry *wmmsg_used; SDL_SysWMEntry *wmmsg_free; -} SDL_EventQ = { NULL, SDL_TRUE, 0, 0, NULL, NULL, NULL, NULL, NULL }; +} SDL_EventQ = { NULL, { 1 }, { 0 }, 0, NULL, NULL, NULL, NULL, NULL }; /* Public functions */ @@ -98,7 +98,7 @@ SDL_StopEventLoop(void) SDL_LockMutex(SDL_EventQ.lock); } - SDL_EventQ.active = SDL_FALSE; + SDL_AtomicSet(&SDL_EventQ.active, 0); if (report && SDL_atoi(report)) { SDL_Log("SDL EVENT QUEUE: Maximum events in-flight: %d\n", @@ -127,7 +127,7 @@ SDL_StopEventLoop(void) wmmsg = next; } - SDL_EventQ.count = 0; + SDL_AtomicSet(&SDL_EventQ.count, 0); SDL_EventQ.max_events_seen = 0; SDL_EventQ.head = NULL; SDL_EventQ.tail = NULL; @@ -171,7 +171,7 @@ SDL_StartEventLoop(void) SDL_EventQ.lock = SDL_CreateMutex(); } if (SDL_EventQ.lock == NULL) { - return (-1); + return -1; } #endif /* !SDL_THREADS_DISABLED */ @@ -180,9 +180,9 @@ SDL_StartEventLoop(void) SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE); SDL_EventState(SDL_SYSWMEVENT, SDL_DISABLE); - SDL_EventQ.active = SDL_TRUE; + SDL_AtomicSet(&SDL_EventQ.active, 1); - return (0); + return 0; } @@ -191,9 +191,11 @@ static int SDL_AddEvent(SDL_Event * event) { SDL_EventEntry *entry; + const int initial_count = SDL_AtomicGet(&SDL_EventQ.count); + int final_count; - if (SDL_EventQ.count >= SDL_MAX_QUEUED_EVENTS) { - SDL_SetError("Event queue is full (%d events)", SDL_EventQ.count); + if (initial_count >= SDL_MAX_QUEUED_EVENTS) { + SDL_SetError("Event queue is full (%d events)", initial_count); return 0; } @@ -225,10 +227,10 @@ SDL_AddEvent(SDL_Event * event) entry->prev = NULL; entry->next = NULL; } - ++SDL_EventQ.count; - if (SDL_EventQ.count > SDL_EventQ.max_events_seen) { - SDL_EventQ.max_events_seen = SDL_EventQ.count; + final_count = SDL_AtomicAdd(&SDL_EventQ.count, 1) + 1; + if (final_count > SDL_EventQ.max_events_seen) { + SDL_EventQ.max_events_seen = final_count; } return 1; @@ -256,8 +258,8 @@ SDL_CutEvent(SDL_EventEntry *entry) entry->next = SDL_EventQ.free; SDL_EventQ.free = entry; - SDL_assert(SDL_EventQ.count > 0); - --SDL_EventQ.count; + SDL_assert(SDL_AtomicGet(&SDL_EventQ.count) > 0); + SDL_AtomicAdd(&SDL_EventQ.count, -1); } /* Lock the event queue, take a peep at it, and unlock it */ @@ -268,7 +270,7 @@ SDL_PeepEvents(SDL_Event * events, int numevents, SDL_eventaction action, int i, used; /* Don't look after we've quit */ - if (!SDL_EventQ.active) { + if (!SDL_AtomicGet(&SDL_EventQ.active)) { /* We get a few spurious events at shutdown, so don't warn then */ if (action != SDL_ADDEVENT) { SDL_SetError("The event system has been shut down"); @@ -363,7 +365,7 @@ void SDL_FlushEvents(Uint32 minType, Uint32 maxType) { /* Don't look after we've quit */ - if (!SDL_EventQ.active) { + if (!SDL_AtomicGet(&SDL_EventQ.active)) { return; } diff --git a/src/haptic/windows/SDL_windowshaptic.c b/src/haptic/windows/SDL_windowshaptic.c index 0c038fb1b..c6bcba1f3 100644 --- a/src/haptic/windows/SDL_windowshaptic.c +++ b/src/haptic/windows/SDL_windowshaptic.c @@ -255,7 +255,7 @@ SDL_SYS_HapticQuit(void) for (hapticitem = SDL_haptics; hapticitem; hapticitem = hapticitem->next) { if ((hapticitem->hwdata->bXInputHaptic) && (hapticitem->hwdata->thread)) { /* we _have_ to stop the thread before we free the XInput DLL! */ - hapticitem->hwdata->stopThread = 1; + SDL_AtomicSet(&hapticitem->hwdata->stopThread, 1); SDL_WaitThread(hapticitem->hwdata->thread, NULL); hapticitem->hwdata->thread = NULL; } diff --git a/src/haptic/windows/SDL_windowshaptic_c.h b/src/haptic/windows/SDL_windowshaptic_c.h index 89fdd2cb9..f34426442 100644 --- a/src/haptic/windows/SDL_windowshaptic_c.h +++ b/src/haptic/windows/SDL_windowshaptic_c.h @@ -42,8 +42,8 @@ struct haptic_hwdata Uint8 userid; /* XInput userid index for this joystick */ SDL_Thread *thread; SDL_mutex *mutex; - volatile Uint32 stopTicks; - volatile int stopThread; + Uint32 stopTicks; + SDL_atomic_t stopThread; }; diff --git a/src/haptic/windows/SDL_xinputhaptic.c b/src/haptic/windows/SDL_xinputhaptic.c index a46ae5f97..aefbabbda 100644 --- a/src/haptic/windows/SDL_xinputhaptic.c +++ b/src/haptic/windows/SDL_xinputhaptic.c @@ -146,7 +146,7 @@ SDL_RunXInputHaptic(void *arg) { struct haptic_hwdata *hwdata = (struct haptic_hwdata *) arg; - while (!hwdata->stopThread) { + while (!SDL_AtomicGet(&hwdata->stopThread)) { SDL_Delay(50); SDL_LockMutex(hwdata->mutex); /* If we're currently running and need to stop... */ @@ -261,7 +261,7 @@ SDL_XINPUT_HapticOpenFromJoystick(SDL_Haptic * haptic, SDL_Joystick * joystick) void SDL_XINPUT_HapticClose(SDL_Haptic * haptic) { - haptic->hwdata->stopThread = 1; + SDL_AtomicSet(&haptic->hwdata->stopThread, 1); SDL_WaitThread(haptic->hwdata->thread, NULL); SDL_DestroyMutex(haptic->hwdata->mutex); } diff --git a/src/timer/SDL_timer.c b/src/timer/SDL_timer.c index 6189ab8b5..ea24af84d 100644 --- a/src/timer/SDL_timer.c +++ b/src/timer/SDL_timer.c @@ -35,7 +35,7 @@ typedef struct _SDL_Timer void *param; Uint32 interval; Uint32 scheduled; - volatile SDL_bool canceled; + SDL_atomic_t canceled; struct _SDL_Timer *next; } SDL_Timer; @@ -60,9 +60,9 @@ typedef struct { /* Data used to communicate with the timer thread */ SDL_SpinLock lock; SDL_sem *sem; - SDL_Timer * volatile pending; - SDL_Timer * volatile freelist; - volatile SDL_bool active; + SDL_Timer *pending; + SDL_Timer *freelist; + SDL_atomic_t active; /* List of timers - this is only touched by the timer thread */ SDL_Timer *timers; @@ -138,7 +138,7 @@ SDL_TimerThread(void *_data) freelist_tail = NULL; /* Check to see if we're still running, after maintenance */ - if (!data->active) { + if (!SDL_AtomicGet(&data->active)) { break; } @@ -160,7 +160,7 @@ SDL_TimerThread(void *_data) /* We're going to do something with this timer */ data->timers = current->next; - if (current->canceled) { + if (SDL_AtomicGet(¤t->canceled)) { interval = 0; } else { interval = current->callback(current->interval, current->param); @@ -179,7 +179,7 @@ SDL_TimerThread(void *_data) } freelist_tail = current; - current->canceled = SDL_TRUE; + SDL_AtomicSet(¤t->canceled, 1); } } @@ -207,7 +207,7 @@ SDL_TimerInit(void) { SDL_TimerData *data = &SDL_timer_data; - if (!data->active) { + if (!SDL_AtomicGet(&data->active)) { const char *name = "SDLTimer"; data->timermap_lock = SDL_CreateMutex(); if (!data->timermap_lock) { @@ -220,7 +220,7 @@ SDL_TimerInit(void) return -1; } - data->active = SDL_TRUE; + SDL_AtomicSet(&data->active, 1); /* !!! FIXME: this is nasty. */ #if defined(__WIN32__) && !defined(HAVE_LIBC) #undef SDL_CreateThread @@ -249,9 +249,7 @@ SDL_TimerQuit(void) SDL_Timer *timer; SDL_TimerMap *entry; - if (data->active) { - data->active = SDL_FALSE; - + if (SDL_AtomicCAS(&data->active, 1, 0)) { /* active? Move to inactive. */ /* Shutdown the timer thread */ if (data->thread) { SDL_SemPost(data->sem); @@ -291,21 +289,14 @@ SDL_AddTimer(Uint32 interval, SDL_TimerCallback callback, void *param) SDL_Timer *timer; SDL_TimerMap *entry; - if (!data->active) { - int status = 0; - - SDL_AtomicLock(&data->lock); - if (!data->active) { - status = SDL_TimerInit(); - } - SDL_AtomicUnlock(&data->lock); - - if (status < 0) { + SDL_AtomicLock(&data->lock); + if (!SDL_AtomicGet(&data->active)) { + if (SDL_TimerInit() < 0) { + SDL_AtomicUnlock(&data->lock); return 0; } } - SDL_AtomicLock(&data->lock); timer = data->freelist; if (timer) { data->freelist = timer->next; @@ -326,7 +317,7 @@ SDL_AddTimer(Uint32 interval, SDL_TimerCallback callback, void *param) timer->param = param; timer->interval = interval; timer->scheduled = SDL_GetTicks() + interval; - timer->canceled = SDL_FALSE; + SDL_AtomicSet(&timer->canceled, 0); entry = (SDL_TimerMap *)SDL_malloc(sizeof(*entry)); if (!entry) { @@ -377,8 +368,8 @@ SDL_RemoveTimer(SDL_TimerID id) SDL_UnlockMutex(data->timermap_lock); if (entry) { - if (!entry->timer->canceled) { - entry->timer->canceled = SDL_TRUE; + if (!SDL_AtomicGet(&entry->timer->canceled)) { + SDL_AtomicSet(&entry->timer->canceled, 1); canceled = SDL_TRUE; } SDL_free(entry); diff --git a/src/video/android/SDL_androidtouch.c b/src/video/android/SDL_androidtouch.c index a6e0b896a..0ff11ef57 100644 --- a/src/video/android/SDL_androidtouch.c +++ b/src/video/android/SDL_androidtouch.c @@ -50,7 +50,7 @@ static void Android_GetWindowCoordinates(float x, float y, *window_y = (int)(y * window_h); } -static volatile SDL_bool separate_mouse_and_touch = SDL_FALSE; +static SDL_bool separate_mouse_and_touch = SDL_FALSE; static void SeparateEventsHintWatcher(void *userdata, const char *name, diff --git a/test/testatomic.c b/test/testatomic.c index 41cc9ab1b..d371ef31f 100644 --- a/test/testatomic.c +++ b/test/testatomic.c @@ -284,7 +284,7 @@ typedef struct char cache_pad4[SDL_CACHELINE_SIZE-sizeof(SDL_SpinLock)-2*sizeof(SDL_atomic_t)]; #endif - volatile SDL_bool active; + SDL_atomic_t active; /* Only needed for the mutex test */ SDL_mutex *mutex; @@ -305,7 +305,7 @@ static void InitEventQueue(SDL_EventQueue *queue) SDL_AtomicSet(&queue->rwcount, 0); SDL_AtomicSet(&queue->watcher, 0); #endif - queue->active = SDL_TRUE; + SDL_AtomicSet(&queue->active, 1); } static SDL_bool EnqueueEvent_LockFree(SDL_EventQueue *queue, const SDL_Event *event) @@ -538,7 +538,7 @@ static int FIFO_Reader(void* _data) if (DequeueEvent_LockFree(queue, &event)) { WriterData *writer = (WriterData*)event.user.data1; ++data->counters[writer->index]; - } else if (queue->active) { + } else if (SDL_AtomicGet(&queue->active)) { ++data->waits; SDL_Delay(0); } else { @@ -551,7 +551,7 @@ static int FIFO_Reader(void* _data) if (DequeueEvent_Mutex(queue, &event)) { WriterData *writer = (WriterData*)event.user.data1; ++data->counters[writer->index]; - } else if (queue->active) { + } else if (SDL_AtomicGet(&queue->active)) { ++data->waits; SDL_Delay(0); } else { @@ -571,7 +571,7 @@ static int FIFO_Watcher(void* _data) { SDL_EventQueue *queue = (SDL_EventQueue *)_data; - while (queue->active) { + while (SDL_AtomicGet(&queue->active)) { SDL_AtomicLock(&queue->lock); SDL_AtomicIncRef(&queue->watcher); while (SDL_AtomicGet(&queue->rwcount) > 0) { @@ -652,7 +652,7 @@ static void RunFIFOTest(SDL_bool lock_free) } /* Shut down the queue so readers exit */ - queue.active = SDL_FALSE; + SDL_AtomicSet(&queue.active, 0); /* Wait for the readers */ while (SDL_AtomicGet(&readersRunning) > 0) { diff --git a/test/testlock.c b/test/testlock.c index 1106ec3bf..113ba0d4c 100644 --- a/test/testlock.c +++ b/test/testlock.c @@ -23,7 +23,7 @@ static SDL_mutex *mutex = NULL; static SDL_threadID mainthread; static SDL_Thread *threads[6]; -static volatile int doterminate = 0; +static SDL_atomic_t doterminate; /* * SDL_Quit() shouldn't be used with atexit() directly because @@ -45,7 +45,7 @@ void terminate(int sig) { signal(SIGINT, terminate); - doterminate = 1; + SDL_AtomicSet(&doterminate, 1); } void @@ -54,7 +54,7 @@ closemutex(int sig) SDL_threadID id = SDL_ThreadID(); int i; SDL_Log("Process %lu: Cleaning up...\n", id == mainthread ? 0 : id); - doterminate = 1; + SDL_AtomicSet(&doterminate, 1); for (i = 0; i < 6; ++i) SDL_WaitThread(threads[i], NULL); SDL_DestroyMutex(mutex); @@ -66,7 +66,7 @@ Run(void *data) { if (SDL_ThreadID() == mainthread) signal(SIGTERM, closemutex); - while (!doterminate) { + while (!SDL_AtomicGet(&doterminate)) { SDL_Log("Process %lu ready to work\n", SDL_ThreadID()); if (SDL_LockMutex(mutex) < 0) { SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't lock mutex: %s", SDL_GetError()); @@ -82,7 +82,7 @@ Run(void *data) /* If this sleep isn't done, then threads may starve */ SDL_Delay(10); } - if (SDL_ThreadID() == mainthread && doterminate) { + if (SDL_ThreadID() == mainthread && SDL_AtomicGet(&doterminate)) { SDL_Log("Process %lu: raising SIGTERM\n", SDL_ThreadID()); raise(SIGTERM); } @@ -105,6 +105,8 @@ main(int argc, char *argv[]) } atexit(SDL_Quit_Wrapper); + SDL_AtomicSet(&doterminate, 0); + if ((mutex = SDL_CreateMutex()) == NULL) { SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Couldn't create mutex: %s\n", SDL_GetError()); exit(1); diff --git a/test/testmultiaudio.c b/test/testmultiaudio.c index 117ef2696..1b07ba9fc 100644 --- a/test/testmultiaudio.c +++ b/test/testmultiaudio.c @@ -25,7 +25,7 @@ typedef struct { SDL_AudioDeviceID dev; int soundpos; - volatile int done; + SDL_atomic_t done; } callback_data; callback_data cbd[64]; @@ -46,14 +46,14 @@ play_through_once(void *arg, Uint8 * stream, int len) if (len > 0) { stream += cpy; SDL_memset(stream, spec.silence, len); - cbd->done++; + SDL_AtomicSet(&cbd->done, 1); } } void loop() { - if(cbd[0].done) { + if (SDL_AtomicGet(&cbd[0].done)) { #ifdef __EMSCRIPTEN__ emscripten_cancel_main_loop(); #endif @@ -100,8 +100,7 @@ test_multi_audio(int devcount) #ifdef __EMSCRIPTEN__ emscripten_set_main_loop(loop, 0, 1); #else - while (!cbd[0].done) - { + while (!SDL_AtomicGet(&cbd[0].done)) { #ifdef __ANDROID__ /* Empty queue, some application events would prevent pause. */ while (SDL_PollEvent(&event)){} @@ -136,7 +135,7 @@ test_multi_audio(int devcount) while (keep_going) { keep_going = 0; for (i = 0; i < devcount; i++) { - if ((cbd[i].dev) && (!cbd[i].done)) { + if ((cbd[i].dev) && (!SDL_AtomicGet(&cbd[i].done))) { keep_going = 1; } } diff --git a/test/torturethread.c b/test/torturethread.c index 5719a7195..6b98a0a9f 100644 --- a/test/torturethread.c +++ b/test/torturethread.c @@ -21,7 +21,7 @@ #define NUMTHREADS 10 -static char volatile time_for_threads_to_die[NUMTHREADS]; +static SDL_atomic_t time_for_threads_to_die[NUMTHREADS]; /* Call this instead of exit(), so we can clean up SDL: atexit() is evil. */ static void @@ -58,7 +58,7 @@ ThreadFunc(void *data) } SDL_Log("Thread '%d' waiting for signal\n", tid); - while (time_for_threads_to_die[tid] != 1) { + while (SDL_AtomicGet(&time_for_threads_to_die[tid]) != 1) { ; /* do nothing */ } @@ -92,7 +92,7 @@ main(int argc, char *argv[]) for (i = 0; i < NUMTHREADS; i++) { char name[64]; SDL_snprintf(name, sizeof (name), "Parent%d", i); - time_for_threads_to_die[i] = 0; + SDL_AtomicSet(&time_for_threads_to_die[i], 0); threads[i] = SDL_CreateThread(ThreadFunc, name, (void*) (uintptr_t) i); if (threads[i] == NULL) { @@ -102,7 +102,7 @@ main(int argc, char *argv[]) } for (i = 0; i < NUMTHREADS; i++) { - time_for_threads_to_die[i] = 1; + SDL_AtomicSet(&time_for_threads_to_die[i], 1); } for (i = 0; i < NUMTHREADS; i++) {