From: hboehm Date: Mon, 7 May 2007 23:23:50 +0000 (+0000) Subject: 2007-05-07 Hans Boehm X-Git-Tag: gc7_0alpha9~9 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=3d7e4a8c6095be9a8c5fbc31bb597e9e627c6e0a;p=gc 2007-05-07 Hans Boehm * mark.c (GC_mark_some wrapper): Restructure for readability, handle GC_started_thread_while_stopped. * misc.c (Win32 GC_write): Lock GC_write_cs only if needed. * win32_threads.c: (client_has_run): remove, GC_started_thread_while_stopped, GC_attached_thread: add. (GC_push_all_stacks): Add verbose output. (DllMain): Avoid initializing collector or the like. Never update both thread tables. * doc/README.win32: Adjust GC_win32_dll_threads rules. --- diff --git a/ChangeLog b/ChangeLog index 5d19ce93..b82412cd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2007-05-07 Hans Boehm + + * mark.c (GC_mark_some wrapper): Restructure for readability, handle + GC_started_thread_while_stopped. + * misc.c (Win32 GC_write): Lock GC_write_cs only if needed. + * win32_threads.c: (client_has_run): remove, + GC_started_thread_while_stopped, GC_attached_thread: add. + (GC_push_all_stacks): Add verbose output. + (DllMain): Avoid initializing collector or the like. + Never update both thread tables. + * doc/README.win32: Adjust GC_win32_dll_threads rules. + 2007-05-07 Hans Boehm * pthread_stop_world.c (GC_push_all_stacks): Print thread count with diff --git a/doc/README.win32 b/doc/README.win32 index e1d27793..c6bdabf8 100644 --- a/doc/README.win32 +++ b/doc/README.win32 @@ -164,7 +164,8 @@ This version of the collector by default handles threads similarly to other platforms. James Clark's code which tracks threads attached to the collector DLL still exists, but requires that both - the collector is built in a DLL with GC_DLL defined, and -- GC_win32_dll_threads be set to true before GC initialization. +- GC_win32_dll_threads be set to true before GC initialization, which + in turn must happen before creating additional threads. We generally recommend avoiding this if possible, since it seems to be less than 100% reliable. diff --git a/mark.c b/mark.c index 10df6749..43f114bb 100644 --- a/mark.c +++ b/mark.c @@ -462,6 +462,11 @@ static void alloc_mark_stack(size_t); } # endif /* __GNUC__ && MSWIN32 */ +#ifdef GC_WIN32_THREADS + extern GC_bool GC_started_thread_while_stopped(void); + /* In win32_threads.c. Did we invalidate mark phase with an */ + /* unexpected thread start? */ +#endif # ifdef WRAP_MARK_SOME GC_bool GC_mark_some(ptr_t cold_gc_frame) @@ -484,6 +489,21 @@ static void alloc_mark_stack(size_t); /* USE_PROC_FOR_LIBRARIES. */ __try { + ret_val = GC_mark_some_inner(cold_gc_frame); + } __except (GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION ? + EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) { + goto handle_ex; + } +# ifdef GC_WIN32_THREADS + /* With DllMain-based thread tracking, a thread may have */ + /* started while we were marking. This is logically equivalent */ + /* to the exception case; our results are invalid and we have */ + /* to start over. This cannot be prevented since we can't */ + /* block in DllMain. */ + if (GC_started_thread_while_stopped()) goto handle_ex; +# endif + rm_handler: + return ret_val; # else /* __GNUC__ */ @@ -497,6 +517,15 @@ static void alloc_mark_stack(size_t); er.ex_reg.handler = mark_ex_handler; asm volatile ("movl %%fs:0, %0" : "=r" (er.ex_reg.prev)); asm volatile ("movl %0, %%fs:0" : : "r" (&er)); + ret_val = GC_mark_some_inner(cold_gc_frame); + /* Prevent GCC from considering the following code unreachable */ + /* and thus eliminating it. */ + if (er.alt_path == 0) + goto handle_ex; + rm_handler: + /* Uninstall the exception handler */ + asm volatile ("mov %0, %%fs:0" : : "r" (er.ex_reg.prev)); + return ret_val; # endif /* __GNUC__ */ # else /* !MSWIN32 */ @@ -510,63 +539,27 @@ static void alloc_mark_stack(size_t); /* I'm not sure if this could still work ... */ GC_setup_temporary_fault_handler(); if(SETJMP(GC_jmp_buf) != 0) goto handle_ex; + ret_val = GC_mark_some_inner(cold_gc_frame); + rm_handler: + GC_reset_fault_handler(); + return ret_val; # endif /* !MSWIN32 */ - ret_val = GC_mark_some_inner(cold_gc_frame); - -# ifdef MSWIN32 -# ifndef __GNUC__ - - } __except (GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION ? - EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) { - -# else /* __GNUC__ */ - - /* Prevent GCC from considering the following code unreachable */ - /* and thus eliminating it. */ - if (er.alt_path != 0) - goto rm_handler; - -handle_ex: - /* Execution resumes from here on an access violation. */ - -# endif /* __GNUC__ */ -# else /* !MSWIN32 */ - goto rm_handler; handle_ex: -# endif /* __GNUC__ */ - - if (GC_print_stats) { - GC_log_printf("Caught ACCESS_VIOLATION in marker. " - "Memory mapping disappeared.\n"); - } - - /* We have bad roots on the stack. Discard mark stack. */ - /* Rescan from marked objects. Redetermine roots. */ - GC_invalidate_mark_state(); - scan_ptr = 0; - - ret_val = FALSE; - -# if defined(MSWIN32) -# if !defined(__GNUC__) - + /* Exception handler starts here for all cases. */ + if (GC_print_stats) { + GC_log_printf("Caught ACCESS_VIOLATION in marker. " + "Memory mapping disappeared.\n"); } -# else /* __GNUC__ && MSWIN32 */ - -rm_handler: - /* Uninstall the exception handler */ - asm volatile ("mov %0, %%fs:0" : : "r" (er.ex_reg.prev)); - -# endif /* __GNUC__ */ -# else /* !MSWIN32 */ -rm_handler: - GC_reset_fault_handler(); -# endif /* !MSWIN32 */ + /* We have bad roots on the stack. Discard mark stack. */ + /* Rescan from marked objects. Redetermine roots. */ + GC_invalidate_mark_state(); + scan_ptr = 0; - return ret_val; + ret_val = FALSE; + goto rm_handler; // Back to platform-specific code. } #endif /* WRAP_MARK_SOME */ diff --git a/misc.c b/misc.c index 0967e1ad..5ab8cf11 100644 --- a/misc.c +++ b/misc.c @@ -834,8 +834,9 @@ out: DWORD written; if (len == 0) return 0; - EnterCriticalSection(&GC_write_cs); + if (GC_need_to_lock) EnterCriticalSection(&GC_write_cs); if (GC_stdout == INVALID_HANDLE_VALUE) { + if (GC_need_to_lock) LeaveCriticalSection(&GC_write_cs); return -1; } else if (GC_stdout == 0) { char * file_name = GETENV("GC_LOG_FILE"); @@ -863,7 +864,7 @@ out: # if defined(_MSC_VER) && defined(_DEBUG) _CrtDbgReport(_CRT_WARN, NULL, 0, NULL, "%.*s", len, buf); # endif - LeaveCriticalSection(&GC_write_cs); + if (GC_need_to_lock) LeaveCriticalSection(&GC_write_cs); return tmp ? (int)written : -1; } diff --git a/win32_threads.c b/win32_threads.c index 1215f91d..edfe80ef 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -92,10 +92,10 @@ /* We have two versions of the thread table. Which one */ /* we us depends on whether or not GC_win32_dll_threads */ -/* is set. The one complication is that at process */ -/* startup, we use both, since the client hasn't yet */ -/* had a chance to tell us which one (s)he wants. */ -static GC_bool client_has_run = FALSE; +/* is set. Note that before initialization, we don't */ +/* add any entries to either table, even if DllMain is */ +/* called. The main thread will be added on */ +/* initialization. */ /* The type of the first argument to InterlockedExchange. */ /* Documented to be LONG volatile *, but at least gcc likes */ @@ -120,9 +120,7 @@ void GC_init_parallel(void); /* Thread-local allocation really wants to lock at thread */ /* entry and exit. */ # endif - GC_need_to_lock = TRUE; - /* Cannot intercept thread creation. */ - GC_ASSERT(!client_has_run); + GC_ASSERT(!parallel_initialized); GC_win32_dll_threads = TRUE; } #else @@ -181,6 +179,32 @@ typedef volatile struct GC_Thread_Rep * GC_vthread; volatile GC_bool GC_please_stop = FALSE; +/* + * We track thread attachments while the world is supposed to be stopped. + * Unfortunately, we can't stop them from starting, since blocking in + * DllMain seems to cause the world to deadlock. Thus we have to recover + * If we notice this in the middle of marking. + */ + +AO_t GC_attached_thread = 0; +/* Return TRUE if an thread was attached since we last asked or */ +/* since GC_attached_thread was explicitly reset. */ +GC_bool GC_started_thread_while_stopped(void) +{ + AO_t result; + + if (GC_win32_dll_threads) { + AO_nop_full(); /* Prior heap reads need to complete earlier. */ + result = AO_load(&GC_attached_thread); + if (result) { + AO_store(&GC_attached_thread, FALSE); + } + return (result); + } else { + return FALSE; + } +} + /* Thread table used if GC_win32_dll_threads is set. */ /* This is a fixed size array. */ /* Since we use runtime conditionals, both versions */ @@ -269,7 +293,7 @@ static GC_thread GC_register_my_thread_inner(struct GC_stack_base *sb, # endif # endif - if (GC_win32_dll_threads || !client_has_run) { + if (GC_win32_dll_threads) { int i; /* It appears to be unsafe to acquire a lock here, since this */ /* code is apparently not preeemptible on some systems. */ @@ -309,22 +333,20 @@ static GC_thread GC_register_my_thread_inner(struct GC_stack_base *sb, GC_max_thread_index = MAX_THREADS - 1; } me = dll_thread_table + i; - } - if (!GC_win32_dll_threads || !client_has_run) { - GC_ASSERT(I_HOLD_LOCK() || !client_has_run); + } else /* Not using DllMain */ { + GC_ASSERT(I_HOLD_LOCK()); me = GC_new_thread(thread_id); } - # ifdef CYGWIN32 me -> pthread_id = pthread_self(); # endif if (!DuplicateHandle(GetCurrentProcess(), - GetCurrentThread(), - GetCurrentProcess(), - (HANDLE*)&(me -> handle), - 0, - 0, - DUPLICATE_SAME_ACCESS)) { + GetCurrentThread(), + GetCurrentProcess(), + (HANDLE*)&(me -> handle), + 0, + 0, + DUPLICATE_SAME_ACCESS)) { DWORD last_error = GetLastError(); GC_err_printf("Last error code: %d\n", last_error); ABORT("DuplicateHandle failed"); @@ -332,21 +354,27 @@ static GC_thread GC_register_my_thread_inner(struct GC_stack_base *sb, me -> stack_base = sb -> mem_base; /* Up until this point, GC_push_all_stacks considers this thread */ /* invalid. */ - if (me -> stack_base == NULL) - ABORT("Bad stack base in GC_register_my_thread_inner"); /* Up until this point, this entry is viewed as reserved but invalid */ /* by GC_delete_thread. */ me -> id = thread_id; # if defined(THREAD_LOCAL_ALLOC) - GC_init_thread_local((GC_tlfs)(&(me->tlfs))); + GC_init_thread_local((GC_tlfs)(&(me->tlfs))); # endif - GC_ASSERT(!GC_please_stop || GC_win32_dll_threads); + if (me -> stack_base == NULL) + ABORT("Bad stack base in GC_register_my_thread_inner"); + if (GC_win32_dll_threads) { + if (GC_please_stop) { + AO_store(&GC_attached_thread, TRUE); + AO_nop_full(); // Later updates must become visible after this. + } + /* We'd like to wait here, but can't, since waiting in DllMain */ + /* provokes deadlocks. */ + /* Thus we force marking to be restarted instead. */ + } else { + GC_ASSERT(!GC_please_stop); /* Otherwise both we and the thread stopping code would be */ /* holding the allocation lock. */ - /* If this thread is being created while we are trying to stop */ - /* the world, wait here. Hopefully this can't happen on any */ - /* systems that don't allow us to block here. */ - while (GC_please_stop) Sleep(20); + } return (GC_thread)(me); } @@ -667,9 +695,11 @@ void GC_stop_world(void) EnterCriticalSection(&GC_write_cs); # endif if (GC_win32_dll_threads) { - /* Any threads being created during this loop will end up sleeping */ - /* in the thread registration code until GC_please_stop becomes */ - /* false. This is not ideal, but hopefully correct. */ + /* Any threads being created during this loop will end up setting */ + /* GC_attached_thread when they start. This will force marking to */ + /* restart. */ + /* This is not ideal, but hopefully correct. */ + GC_attached_thread = FALSE; for (i = 0; i <= GC_get_max_thread_index(); i++) { GC_vthread t = dll_thread_table + i; if (t -> stack_base != 0 @@ -825,6 +855,7 @@ void GC_push_all_stacks(void) { DWORD me = GetCurrentThreadId(); GC_bool found_me = FALSE; + size_t nthreads = 0; if (GC_win32_dll_threads) { int i; @@ -833,6 +864,7 @@ void GC_push_all_stacks(void) for (i = 0; i <= my_max; i++) { GC_thread t = (GC_thread)(dll_thread_table + i); if (t -> in_use) { + ++nthreads; GC_push_stack_for(t); if (t -> id == me) found_me = TRUE; } @@ -843,11 +875,20 @@ void GC_push_all_stacks(void) for (i = 0; i < THREAD_TABLE_SZ; i++) { for (t = GC_threads[i]; t != 0; t = t -> next) { + ++nthreads; GC_push_stack_for(t); if (t -> id == me) found_me = TRUE; } } } + if (GC_print_stats == VERBOSE) { + GC_log_printf("Pushed %d thread stacks ", nthreads); + if (GC_win32_dll_threads) { + GC_log_printf("based on DllMain thread tracking\n"); + } else { + GC_log_printf("\n"); + } + } if (!found_me) ABORT("Collecting from unknown thread."); } @@ -945,7 +986,6 @@ GC_API HANDLE WINAPI GC_CreateThread( /* make sure GC is initialized (i.e. main thread is attached, tls initialized) */ - client_has_run = TRUE; if (GC_win32_dll_threads) { return CreateThread(lpThreadAttributes, dwStackSize, lpStartAddress, lpParameter, dwCreationFlags, lpThreadId); @@ -990,7 +1030,6 @@ uintptr_t GC_beginthreadex( /* make sure GC is initialized (i.e. main thread is attached, tls initialized) */ - client_has_run = TRUE; if (GC_win32_dll_threads) { return _beginthreadex(security, stack_size, start_address, arglist, initflag, thrdaddr); @@ -1108,7 +1147,7 @@ int GC_pthread_join(pthread_t pthread_id, void **retval) { (int)pthread_self(), GetCurrentThreadId(), (int)pthread_id); # endif - client_has_run = TRUE; + if (!parallel_initialized) GC_init_parallel(); /* Thread being joined might not have registered itself yet. */ /* After the join,thread id may have been recycled. */ /* FIXME: It would be better if this worked more like */ @@ -1145,7 +1184,6 @@ GC_pthread_create(pthread_t *new_thread, if (!parallel_initialized) GC_init_parallel(); /* make sure GC is initialized (i.e. main thread is attached) */ - client_has_run = TRUE; if (GC_win32_dll_threads) { return pthread_create(new_thread, attr, start_routine, arg); } @@ -1257,7 +1295,7 @@ void GC_thread_exit_proc(void *arg) /* nothing required here... */ int GC_pthread_sigmask(int how, const sigset_t *set, sigset_t *oset) { - client_has_run = TRUE; + if (!parallel_initialized) GC_init_parallel(); return pthread_sigmask(how, set, oset); } @@ -1266,7 +1304,7 @@ int GC_pthread_detach(pthread_t thread) int result; GC_thread thread_gc_id; - client_has_run = TRUE; + if (!parallel_initialized) GC_init_parallel(); LOCK(); thread_gc_id = GC_lookup_pthread(thread); UNLOCK(); @@ -1287,8 +1325,11 @@ int GC_pthread_detach(pthread_t thread) /* * We avoid acquiring locks here, since this doesn't seem to be preemptable. - * Pontus Rydin suggested wrapping the thread start routine instead, which - * we do in other places. + * This may run with an uninitialized collector, in which case we don't do much. + * This implies that no threads other than the main one should be created + * with an uninitialized collector. (The alternative of initializing + * the collector here seems dangerous, since DllMain is limited in what it + * can do.) */ #ifdef GC_DLL GC_API BOOL WINAPI DllMain(HINSTANCE inst, ULONG reason, LPVOID reserved) @@ -1296,17 +1337,18 @@ GC_API BOOL WINAPI DllMain(HINSTANCE inst, ULONG reason, LPVOID reserved) struct GC_stack_base sb; DWORD thread_id; int sb_result; + static int entry_count = 0; - if (client_has_run && !GC_win32_dll_threads) return TRUE; + if (parallel_initialized && !GC_win32_dll_threads) return TRUE; switch (reason) { - case DLL_PROCESS_ATTACH: - GC_init(); /* Force initialization before thread attach. */ - /* fall through */ - case DLL_THREAD_ATTACH: - GC_ASSERT(GC_thr_initialized); + case DLL_THREAD_ATTACH: + GC_ASSERT(entry_count == 0 || parallel_initialized); + ++entry_count; /* and fall through: */ + case DLL_PROCESS_ATTACH: + /* This may run with the collector uninitialized. */ thread_id = GetCurrentThreadId(); - if (GC_main_thread != thread_id) { + if (parallel_initialized && GC_main_thread != thread_id) { /* Don't lock here. */ sb_result = GC_get_stack_base(&sb); GC_ASSERT(sb_result == GC_SUCCESS); @@ -1317,14 +1359,14 @@ GC_API BOOL WINAPI DllMain(HINSTANCE inst, ULONG reason, LPVOID reserved) } /* o.w. we already did it during GC_thr_init(), called by GC_init() */ break; - case DLL_THREAD_DETACH: + case DLL_THREAD_DETACH: /* We are hopefully running in the context of the exiting thread. */ - client_has_run = TRUE; + GC_ASSERT(parallel_initialized); if (!GC_win32_dll_threads) return TRUE; GC_delete_thread(GetCurrentThreadId()); break; - case DLL_PROCESS_DETACH: + case DLL_PROCESS_DETACH: { int i; @@ -1356,9 +1398,16 @@ void GC_init_parallel(void) { if (parallel_initialized) return; parallel_initialized = TRUE; - /* GC_init() calls us back, so set flag first. */ + if (!GC_is_initialized) GC_init(); + if (GC_win32_dll_threads) { + GC_need_to_lock = TRUE; + /* Cannot intercept thread creation. Hence we don't know if other */ + /* threads exist. However, client is not allowed to create other */ + /* threads before collector initialization. Thus it's OK not to */ + /* lock before this. */ + } /* Initialize thread local free lists if used. */ # if defined(THREAD_LOCAL_ALLOC) LOCK();