From 8fe49ff540e3ec5b9073befe2564364ae097f3f4 Mon Sep 17 00:00:00 2001 From: jethead71 Date: Thu, 27 May 2010 18:46:09 +0000 Subject: [PATCH] SDL Simulator: Get thread shutdown and properly handled and fix a minor memory leak that happens when threads exit. git-svn-id: svn://svn.rockbox.org/rockbox/trunk@26336 a1c6a512-1295-4272-9138-f99709370657 --- firmware/export/thread.h | 1 + firmware/target/hosted/sdl/button-sdl.c | 7 +-- firmware/target/hosted/sdl/system-sdl.c | 32 +++++++++----- firmware/target/hosted/sdl/system-sdl.h | 2 +- firmware/target/hosted/sdl/thread-sdl.c | 78 ++++++++++++++++++++++++++------- 5 files changed, 91 insertions(+), 29 deletions(-) diff --git a/firmware/export/thread.h b/firmware/export/thread.h index a473e3a4f..9678c0434 100644 --- a/firmware/export/thread.h +++ b/firmware/export/thread.h @@ -122,6 +122,7 @@ struct regs struct regs { void *t; /* Simulator OS thread */ + void *told; /* Last thread in slot (explained in thead-sdl.c) */ void *s; /* Semaphore for blocking and wakeup */ void (*start)(void); /* Start function */ }; diff --git a/firmware/target/hosted/sdl/button-sdl.c b/firmware/target/hosted/sdl/button-sdl.c index e9fc03792..15633eede 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 d6c0dfa34..d56f1d787 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 e87f09661..53af7b835 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 da43d6ea9..ec6718d6f 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) -- 2.11.4.GIT