Remove almost all instances of "volatile" keyword.
authorRyan C. Gordon <icculus@icculus.org>
Sun, 03 Jan 2016 06:50:50 -0500
changeset 10003 d91a2c45825e
parent 10002 c9cce8633f13
child 10004 8f2f519d1e61
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
src/events/SDL_events.c
src/haptic/windows/SDL_windowshaptic.c
src/haptic/windows/SDL_windowshaptic_c.h
src/haptic/windows/SDL_xinputhaptic.c
src/timer/SDL_timer.c
src/video/android/SDL_androidtouch.c
test/testatomic.c
test/testlock.c
test/testmultiaudio.c
test/torturethread.c
--- a/src/dynapi/SDL_dynapi.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/dynapi/SDL_dynapi.c	Sun Jan 03 06:50:50 2016 -0500
@@ -293,7 +293,7 @@
      *  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
--- a/src/events/SDL_events.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/events/SDL_events.c	Sun Jan 03 06:50:50 2016 -0500
@@ -73,15 +73,15 @@
 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_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 @@
         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_EventQ.lock = SDL_CreateMutex();
     }
     if (SDL_EventQ.lock == NULL) {
-        return (-1);
+        return -1;
     }
 #endif /* !SDL_THREADS_DISABLED */
 
@@ -180,9 +180,9 @@
     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 @@
 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 @@
         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 @@
 
     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 @@
     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 @@
 SDL_FlushEvents(Uint32 minType, Uint32 maxType)
 {
     /* Don't look after we've quit */
-    if (!SDL_EventQ.active) {
+    if (!SDL_AtomicGet(&SDL_EventQ.active)) {
         return;
     }
 
--- a/src/haptic/windows/SDL_windowshaptic.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/haptic/windows/SDL_windowshaptic.c	Sun Jan 03 06:50:50 2016 -0500
@@ -255,7 +255,7 @@
     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;
         }
--- a/src/haptic/windows/SDL_windowshaptic_c.h	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/haptic/windows/SDL_windowshaptic_c.h	Sun Jan 03 06:50:50 2016 -0500
@@ -42,8 +42,8 @@
     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;
 };
 
 
--- a/src/haptic/windows/SDL_xinputhaptic.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/haptic/windows/SDL_xinputhaptic.c	Sun Jan 03 06:50:50 2016 -0500
@@ -146,7 +146,7 @@
 {
     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 @@
 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);
 }
--- a/src/timer/SDL_timer.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/timer/SDL_timer.c	Sun Jan 03 06:50:50 2016 -0500
@@ -35,7 +35,7 @@
     void *param;
     Uint32 interval;
     Uint32 scheduled;
-    volatile SDL_bool canceled;
+    SDL_atomic_t canceled;
     struct _SDL_Timer *next;
 } SDL_Timer;
 
@@ -60,9 +60,9 @@
     /* 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 @@
         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 @@
             /* We're going to do something with this timer */
             data->timers = current->next;
 
-            if (current->canceled) {
+            if (SDL_AtomicGet(&current->canceled)) {
                 interval = 0;
             } else {
                 interval = current->callback(current->interval, current->param);
@@ -179,7 +179,7 @@
                 }
                 freelist_tail = current;
 
-                current->canceled = SDL_TRUE;
+                SDL_AtomicSet(&current->canceled, 1);
             }
         }
 
@@ -207,7 +207,7 @@
 {
     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 @@
             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_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_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 @@
     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_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);
--- a/src/video/android/SDL_androidtouch.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/src/video/android/SDL_androidtouch.c	Sun Jan 03 06:50:50 2016 -0500
@@ -50,7 +50,7 @@
     *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,
--- a/test/testatomic.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/test/testatomic.c	Sun Jan 03 06:50:50 2016 -0500
@@ -284,7 +284,7 @@
     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 @@
     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 @@
             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 @@
             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 @@
 {
     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 @@
     }
 
     /* 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) {
--- a/test/testlock.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/test/testlock.c	Sun Jan 03 06:50:50 2016 -0500
@@ -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 @@
 terminate(int sig)
 {
     signal(SIGINT, terminate);
-    doterminate = 1;
+    SDL_AtomicSet(&doterminate, 1);
 }
 
 void
@@ -54,7 +54,7 @@
     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 @@
 {
     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 @@
         /* 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 @@
     }
     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);
--- a/test/testmultiaudio.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/test/testmultiaudio.c	Sun Jan 03 06:50:50 2016 -0500
@@ -25,7 +25,7 @@
 {
     SDL_AudioDeviceID dev;
     int soundpos;
-    volatile int done;
+    SDL_atomic_t done;
 } callback_data;
 
 callback_data cbd[64];
@@ -46,14 +46,14 @@
     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 @@
 #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 @@
     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;
             }
         }
--- a/test/torturethread.c	Sat Jan 02 12:17:33 2016 -0800
+++ b/test/torturethread.c	Sun Jan 03 06:50:50 2016 -0500
@@ -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 @@
     }
 
     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 @@
     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 @@
     }
 
     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++) {