4 files changed, 90 insertions, 29 deletions

diff --git a/firmware/target/hosted/sdl/button-sdl.c b/firmware/target/hosted/sdl/button-sdl.c
index e9fc03792c..15633eede5 100644
--- a/firmware/target/hosted/sdl/button-sdl.c
+++ b/firmware/target/hosted/sdl/button-sdl.c
@@ -86,7 +86,7 @@ static void button_event(int key, bool pressed);
 extern bool debug_wps;
 extern bool mapping;
 
-void gui_message_loop(void)
+bool gui_message_loop(void)
 {
     SDL_Event event;
     static int x,y,xybutton = 0;
@@ -176,8 +176,7 @@ void gui_message_loop(void)
             case SDL_QUIT:
             {
                 sim_exit_irq_handler();
-                exit(EXIT_SUCCESS);
-                break;
+                return false;
             }
             default:
                 /*printf("Unhandled event\n"); */
@@ -185,6 +184,8 @@ void gui_message_loop(void)
         }
         sim_exit_irq_handler();
     }
+
+    return true;
 }
 
 static void button_event(int key, bool pressed)
diff --git a/firmware/target/hosted/sdl/system-sdl.c b/firmware/target/hosted/sdl/system-sdl.c
index d6c0dfa345..d56f1d7874 100644
--- a/firmware/target/hosted/sdl/system-sdl.c
+++ b/firmware/target/hosted/sdl/system-sdl.c
@@ -54,6 +54,8 @@ bool            sim_alarm_wakeup = false;
 const char     *sim_root_dir = NULL;
 extern int      display_zoom;
 
+static SDL_Thread *evt_thread = NULL;
+
 #ifdef DEBUG
 bool debug_audio = false;
 #endif
@@ -64,16 +66,11 @@ int wps_verbose_level = 3;
 
 void sys_poweroff(void)
 {
-    /* Order here is relevent to prevent deadlocks and use of destroyed
-       sync primitives by kernel threads */
-    sim_thread_shutdown();
-    sim_kernel_shutdown();
-    SDL_Quit();
 }
 
 /*
  * Button read loop */
-void gui_message_loop(void);
+bool gui_message_loop(void);
 
 /*
  * This thread will read the buttons in an interrupt like fashion, and
@@ -137,12 +134,28 @@ static int sdl_event_thread(void * param)
 
     /*
      * finally enter the button loop */
-    while(1)
-        gui_message_loop();
+    while(gui_message_loop());
+
+    /* Order here is relevent to prevent deadlocks and use of destroyed
+       sync primitives by kernel threads */
+    sim_thread_shutdown();
+    sim_kernel_shutdown();
 
     return 0;
 }
 
+void sim_do_exit(SDL_mutex *m)
+{
+    /* wait for event thread to finish */
+    SDL_WaitThread(evt_thread, NULL);
+
+    /* cleanup */
+    SDL_DestroyMutex(m);
+
+    SDL_Quit();
+    exit(EXIT_SUCCESS);
+    while(1);
+}
 
 void system_init(void)
 {
@@ -150,11 +163,10 @@ void system_init(void)
 
     if (SDL_Init(SDL_INIT_TIMER))
         panicf("%s", SDL_GetError());
-    atexit(sys_poweroff);
 
     s = SDL_CreateSemaphore(0); /* 0-count so it blocks */
 
-    SDL_CreateThread(sdl_event_thread, s);
+    evt_thread = SDL_CreateThread(sdl_event_thread, s);
 
     /* wait for sdl_event_thread to run so that it can initialize the surfaces
      * and video subsystem needed for SDL events */
diff --git a/firmware/target/hosted/sdl/system-sdl.h b/firmware/target/hosted/sdl/system-sdl.h
index e87f096617..53af7b8355 100644
--- a/firmware/target/hosted/sdl/system-sdl.h
+++ b/firmware/target/hosted/sdl/system-sdl.h
@@ -44,7 +44,7 @@ void sim_exit_irq_handler(void);
 void sim_kernel_shutdown(void);
 void sys_poweroff(void);
 void sys_handle_argv(int argc, char *argv[]);
-void gui_message_loop(void);
+bool gui_message_loop(void);
 
 extern bool background;  /* True if the background image is enabled */
 extern bool showremote;
diff --git a/firmware/target/hosted/sdl/thread-sdl.c b/firmware/target/hosted/sdl/thread-sdl.c
index da43d6ea9f..ec6718d6f8 100644
--- a/firmware/target/hosted/sdl/thread-sdl.c
+++ b/firmware/target/hosted/sdl/thread-sdl.c
@@ -64,10 +64,15 @@ static volatile bool threads_exit = false;
 
 extern long start_tick;
 
+void sim_do_exit(SDL_mutex *m);
+
 void sim_thread_shutdown(void)
 {
     int i;
 
+    /* This *has* to be a push operation from a thread not in the pool
+       so that they may be dislodged from their blocking calls. */
+
     /* Tell all threads jump back to their start routines, unlock and exit
        gracefully - we'll check each one in turn for it's status. Threads
        _could_ terminate via remove_thread or multiple threads could exit
@@ -79,25 +84,44 @@ void sim_thread_shutdown(void)
     /* Take control */
     SDL_LockMutex(m);
 
+    /* Signal all threads on delay or block */
     for (i = 0; i < MAXTHREADS; i++)
     {
         struct thread_entry *thread = &threads[i];
-        /* exit all current threads, except the main one */
-        if (thread->context.t != NULL)
+        if (thread->context.s == NULL)
+            continue;
+        SDL_SemPost(thread->context.s);
+    }
+
+    /* Wait for all threads to finish and cleanup old ones. */
+    for (i = 0; i < MAXTHREADS; i++)
+    {
+        struct thread_entry *thread = &threads[i];
+        SDL_Thread *t = thread->context.t;
+
+        if (t != NULL)
         {
-            /* Signal thread on delay or block */
-            SDL_Thread *t = thread->context.t;
-            SDL_SemPost(thread->context.s);
             SDL_UnlockMutex(m);
             /* Wait for it to finish */
             SDL_WaitThread(t, NULL);
             /* Relock for next thread signal */
             SDL_LockMutex(m);
-        }        
+            /* Already waited and exiting thread would have waited .told,
+             * replacing it with t. */
+            thread->context.told = NULL;
+        }
+        else
+        {
+            /* Wait on any previous thread in this location-- could be one not quite
+             * finished exiting but has just unlocked the mutex. If it's NULL, the
+             * call returns immediately.
+             *
+             * See remove_thread below for more information. */
+            SDL_WaitThread(thread->context.told, NULL);
+        }
     }
 
     SDL_UnlockMutex(m);
-    SDL_DestroyMutex(m);
 }
 
 static void new_thread_id(unsigned int slot_num,
@@ -172,9 +196,22 @@ void init_threads(void)
         return;
     }
 
-    THREAD_SDL_DEBUGF("Main thread: %p\n", thread);
+    /* Tell all threads jump back to their start routines, unlock and exit
+       gracefully - we'll check each one in turn for it's status. Threads
+       _could_ terminate via remove_thread or multiple threads could exit
+       on each unlock but that is safe. */
 
-    return;
+    /* Setup jump for exit */
+    if (setjmp(thread_jmpbufs[0]) == 0)
+    {
+        THREAD_SDL_DEBUGF("Main thread: %p\n", thread);
+        return;
+    }
+
+    SDL_UnlockMutex(m);
+
+    /* doesn't return */
+    sim_do_exit(m);
 }
 
 void sim_thread_exception_wait(void)
@@ -522,7 +559,20 @@ void remove_thread(unsigned int thread_id)
 
     t = thread->context.t;
     s = thread->context.s;
+
+    /* Wait the last thread here and keep this one or SDL will leak it since
+     * it doesn't free its own library allocations unless a wait is performed.
+     * Such behavior guards against the memory being invalid by the time
+     * SDL_WaitThread is reached and also against two different threads having
+     * the same pointer. It also makes SDL_WaitThread a non-concurrent function.
+     *
+     * However, see more below about SDL_KillThread.
+     */
+    SDL_WaitThread(thread->context.told, NULL);
+
     thread->context.t = NULL;
+    thread->context.s = NULL;
+    thread->context.told = t;
 
     if (thread != current)
     {
@@ -560,18 +610,16 @@ void remove_thread(unsigned int thread_id)
         longjmp(thread_jmpbufs[current - threads], 1);
     }
 
+    /* SDL_KillThread frees the old pointer too because it uses SDL_WaitThread
+     * to wait for the host to remove it. */
+    thread->context.told = NULL;
     SDL_KillThread(t);
     restore_irq(oldlevel);
 }
 
 void thread_exit(void)
 {
-    struct thread_entry *t = thread_id_entry(THREAD_ID_CURRENT);
-    /* the main thread cannot be removed since it's created implicitely
-     * by starting the program;
-     * it has no valid jumpbuf to exit, do nothing for now */
-    if (t != &threads[0])
-        remove_thread(t->id);
+    remove_thread(THREAD_ID_CURRENT);
 }
 
 void thread_wait(unsigned int thread_id)