diff options
author | Martin Baulig <martin@novell.com> | 2006-05-17 14:51:31 +0400 |
---|---|---|
committer | Martin Baulig <martin@novell.com> | 2006-05-17 14:51:31 +0400 |
commit | 1663a14d56b8b4ad9b43b6a00d3efcb0adf4b149 (patch) | |
tree | 9310a891d3dd6fec6147f50c7acca7119d801d4d /libgc | |
parent | 20f9256680027576ec788c8906c746c14a253a83 (diff) |
2006-05-17 Martin Baulig <martin@ximian.com>
Fix a weird race condition which prevented XSP from working inside
the debugger - see doc/debugger-issues.txt for details.
* include/gc.h: Moved the "libgc-mono-debugger.h" #include down
after the gc_pthread_redirects.h one.
* include/libgc-mono-debugger.h
(GCThreadFunctions): Added `thread_created' and `thread_exited'.
(GC_mono_debugger_add_all_threads): New function prototype.
* pthread_stop_world.c (gc_thread_vtable): Allow the vtable and
any function in it be NULL; use NULL as the default vtable.
(GC_mono_debugger_add_all_threads): New public function.
* pthread_support.c (GC_new_thread): Use calloc() instead of
GC_INTERNAL_MALLOC() to allocate the `GC_thread' structure.
(GC_delete_thread): Call `gc_thread_vtable->thread_exited()'.
(GC_thr_init): Call `gc_thread_vtable->thread_created()'.
(GC_start_routine_head): Likewise; use calloc() instead of
GC_INTERNAL_MALLOC() to allocate the `start_info'.
svn path=/trunk/mono/; revision=60766
Diffstat (limited to 'libgc')
-rw-r--r-- | libgc/ChangeLog | 23 | ||||
-rw-r--r-- | libgc/doc/debugger-issues.txt | 85 | ||||
-rw-r--r-- | libgc/include/gc.h | 6 | ||||
-rw-r--r-- | libgc/include/libgc-mono-debugger.h | 7 | ||||
-rw-r--r-- | libgc/pthread_stop_world.c | 48 | ||||
-rw-r--r-- | libgc/pthread_support.c | 20 |
6 files changed, 160 insertions, 29 deletions
diff --git a/libgc/ChangeLog b/libgc/ChangeLog index 6f43e63fa2d..57131e0988c 100644 --- a/libgc/ChangeLog +++ b/libgc/ChangeLog @@ -1,3 +1,26 @@ +2006-05-17 Martin Baulig <martin@ximian.com> + + Fix a weird race condition which prevented XSP from working inside + the debugger - see doc/debugger-issues.txt for details. + + * include/gc.h: Moved the "libgc-mono-debugger.h" #include down + after the gc_pthread_redirects.h one. + + * include/libgc-mono-debugger.h + (GCThreadFunctions): Added `thread_created' and `thread_exited'. + (GC_mono_debugger_add_all_threads): New function prototype. + + * pthread_stop_world.c (gc_thread_vtable): Allow the vtable and + any function in it be NULL; use NULL as the default vtable. + (GC_mono_debugger_add_all_threads): New public function. + + * pthread_support.c (GC_new_thread): Use calloc() instead of + GC_INTERNAL_MALLOC() to allocate the `GC_thread' structure. + (GC_delete_thread): Call `gc_thread_vtable->thread_exited()'. + (GC_thr_init): Call `gc_thread_vtable->thread_created()'. + (GC_start_routine_head): Likewise; use calloc() instead of + GC_INTERNAL_MALLOC() to allocate the `start_info'. + 2006-04-05 Zoltan Varga <vargaz@gmail.com> * include/private/gcconfig.h (LINUX and SPARC): Applied patch from diff --git a/libgc/doc/debugger-issues.txt b/libgc/doc/debugger-issues.txt new file mode 100644 index 00000000000..a739393ee35 --- /dev/null +++ b/libgc/doc/debugger-issues.txt @@ -0,0 +1,85 @@ +I spent the last couple of days debugging a very weird race condition. + +The problem only occured when running XSP (SVN revision 60518) inside +the debugger and only when using special parameters. + +I'm using Mono from SVN revision 60564 (that's from last Thursday), +XSP from SVN revision 60518 and manually installed xsp.exe.mdb and +Mono.WebServer.dll.mdb into $prefix/lib/xsp/1.0/. + +With this setup, I'm running XSP with + + mdb -args /work/asgard/INSTALL/lib/xsp/1.0/xsp.exe --root /work/asgard/INSTALL/lib/xsp/test/ + +Note that adding options like --nonstop or changing the --root may +make the problem go away or make it crash somewhere else. + +Then I insert a breakpoint on line 476 (that's the line before the +Console.ReadLine()) and continue. + +Using `set env GC_DONT_GC 1' inside mdb makes the problem go away and +running a stand-alone mono with -O=shared (and all the other +optimization flags the debugger is using) works fine. + +So my first guess was that this is a GC issue. + +After implementing hardware breakpoints in the debugger, I was finally +able to track this down. If I understand things correctly, the +problem goes like this: + +Some code inside XSP calls mono_thread_pool_add() - inside that +method, we GC-allocate an `ASyncCall *ac' structure, store the `msg' +and `state' objects in it and create a `MonoAsyncResult *ares'. + +Then we call mono_thread_create() passing it async_invoke_thread() and +the `ares'. + +mono_thread_create() stores them as `func' and `start_arg' in the +g_new()-allocated `start_info' and calls CreateThread() which calls +pthread_create(). + +pthread_create() is in fact a wrapper in libgc - it calls the "real" +pthread_create() and then blocks on a semaphore until the thread is +actually started. + +Now - somehow - and I still don't fully understand why - the parent +"loses" all references to the `ac' and `ares' after calling the real +pthread_create(). + +If I understand this correctly, mono_thread_pool_add() only stores +them in registers and not on the stack, so the `start_info' contains +the only references to them. The `start_info', however, is just +passed to the clone() system call and not accessed anymore after that. + +This means that all references to the `ac' and the `ares' may +disappear from the parent's stack between the clone() and sem_wait() +system calls. Under normal circumstances, this is no problem since +the child's stack is created with a reference to the `start_info'. + +I said under normal circumstances, because this is where race +condition #1 comes into the picture: + +The GC's pthread_create() passes a wrapper called GC_start_func() +around the original `start_func' to the real pthread_create(). This +wrapper calls GC_new_thread() and stores some information about the +newly created thread in its internal structures - this information is +also used to determine the child's stack. + +After that, it posts the semaphore on which the parent thread is +blocking, we release the allocation lock and everything is fine. + +However - GC_new_thread() uses GC_INTERNAL_MALLOC() to allocate the +`GC_thread' structure - and GC_INTERNAL_MALLOC() may in fact trigger a +collection ! + +Doing a collection at this time means we don't know about the child's +stack yet - and if the parent doesn't keep a reference to the `ares' +anymore, it's gone .... + +Fixing this was really easy, all I had to do is make GC_new_thread() +use calloc() instead of GC_INTERNAL_MALLOC(). + +The second issue is a debugger-only problem: we need to tell the +debugger about newly created threads while still holding the +allocation lock to ensure that no collection may happen in the +meantime. diff --git a/libgc/include/gc.h b/libgc/include/gc.h index e210f13c02e..ae7b8d0632f 100644 --- a/libgc/include/gc.h +++ b/libgc/include/gc.h @@ -887,9 +887,6 @@ GC_API void (*GC_is_valid_displacement_print_proc) GC_API void (*GC_is_visible_print_proc) GC_PROTO((GC_PTR p)); -#define _IN_LIBGC_GC_H -#include "libgc-mono-debugger.h" - /* For pthread support, we generally need to intercept a number of */ /* thread library calls. We do that here by macro defining them. */ @@ -902,6 +899,9 @@ GC_API void (*GC_is_visible_print_proc) #endif #endif +#define _IN_LIBGC_GC_H +#include "libgc-mono-debugger.h" + # if defined(PCR) || defined(GC_SOLARIS_THREADS) || \ defined(GC_PTHREADS) || defined(GC_WIN32_THREADS) /* Any flavor of threads except SRC_M3. */ diff --git a/libgc/include/libgc-mono-debugger.h b/libgc/include/libgc-mono-debugger.h index 624e845005c..ffeefd7a30a 100644 --- a/libgc/include/libgc-mono-debugger.h +++ b/libgc/include/libgc-mono-debugger.h @@ -7,13 +7,18 @@ typedef struct { void (* initialize) (void); + void (* thread_created) (pthread_t tid, void *stack_ptr); + void (* thread_exited) (pthread_t tid, void *stack_ptr); + void (* stop_world) (void); - void (* push_all_stacks) (void); void (* start_world) (void); } GCThreadFunctions; extern GCThreadFunctions *gc_thread_vtable; +extern void +GC_mono_debugger_add_all_threads (void); + #else #error "This header is only intended to be used by the Mono Debugger" #endif diff --git a/libgc/pthread_stop_world.c b/libgc/pthread_stop_world.c index 68f1dce4306..6291d47fb2d 100644 --- a/libgc/pthread_stop_world.c +++ b/libgc/pthread_stop_world.c @@ -294,7 +294,7 @@ void GC_restart_handler(int sig) /* world is stopped. Should not fail if it isn't. */ void GC_push_all_stacks() { - gc_thread_vtable->push_all_stacks(); + pthread_push_all_stacks(); } /* There seems to be a very rare thread stopping problem. To help us */ @@ -353,7 +353,7 @@ static void pthread_stop_world() #if DEBUG_THREADS GC_printf1("Stopping the world from 0x%lx\n", pthread_self()); #endif - + n_live_threads = GC_suspend_all(); if (GC_retry_signals) { @@ -412,7 +412,10 @@ void GC_stop_world() /* We should have previously waited for it to become zero. */ # endif /* PARALLEL_MARK */ ++GC_stop_count; - gc_thread_vtable->stop_world (); + if (gc_thread_vtable && gc_thread_vtable->stop_world) + gc_thread_vtable->stop_world (); + else + pthread_stop_world (); # ifdef PARALLEL_MARK GC_release_mark_lock(); # endif @@ -478,7 +481,10 @@ static void pthread_start_world() void GC_start_world() { - gc_thread_vtable->start_world(); + if (gc_thread_vtable && gc_thread_vtable->start_world) + gc_thread_vtable->start_world(); + else + pthread_start_world (); } static void pthread_stop_init() { @@ -527,20 +533,28 @@ static void pthread_stop_init() { /* We hold the allocation lock. */ void GC_stop_init() { - gc_thread_vtable->initialize (); + if (gc_thread_vtable && gc_thread_vtable->initialize) + gc_thread_vtable->initialize (); + else + pthread_stop_init (); +} + +GCThreadFunctions *gc_thread_vtable = NULL; + +void +GC_mono_debugger_add_all_threads (void) +{ + GC_thread p; + int i; + + if (gc_thread_vtable && gc_thread_vtable->thread_created) { + for (i = 0; i < THREAD_TABLE_SZ; i++) { + for (p = GC_threads[i]; p != 0; p = p -> next) { + gc_thread_vtable->thread_created (p->id, &p->stop_info.stack_ptr); + } + } + } } -/* - * This is used by the Mono Debugger to stop/start the world. - */ -GCThreadFunctions pthread_thread_vtable = { - pthread_stop_init, - - pthread_stop_world, - pthread_push_all_stacks, - pthread_start_world -}; - -GCThreadFunctions *gc_thread_vtable = &pthread_thread_vtable; #endif diff --git a/libgc/pthread_support.c b/libgc/pthread_support.c index 58a662e773b..7f3c31efd81 100644 --- a/libgc/pthread_support.c +++ b/libgc/pthread_support.c @@ -663,8 +663,7 @@ GC_thread GC_new_thread(pthread_t id) result = &first_thread; first_thread_used = TRUE; } else { - result = (struct GC_Thread_Rep *) - GC_INTERNAL_MALLOC(sizeof(struct GC_Thread_Rep), NORMAL); + result = calloc (1, sizeof (struct GC_Thread_Rep)); } if (result == 0) return(0); result -> id = id; @@ -692,7 +691,9 @@ void GC_delete_thread(pthread_t id) } else { prev -> next = p -> next; } - GC_INTERNAL_FREE(p); + if (gc_thread_vtable && gc_thread_vtable->thread_exited) + gc_thread_vtable->thread_exited (id, &p->stop_info.stack_ptr); + free(p); } /* If a thread has been joined, but we have not yet */ @@ -714,7 +715,7 @@ void GC_delete_gc_thread(pthread_t id, GC_thread gc_id) } else { prev -> next = p -> next; } - GC_INTERNAL_FREE(p); + free(p); } /* Return a GC_thread corresponding to a given pthread_t. */ @@ -767,7 +768,7 @@ void GC_remove_all_threads_but_me(void) GC_destroy_thread_local(p); } # endif /* THREAD_LOCAL_ALLOC */ - if (p != &first_thread) GC_INTERNAL_FREE(p); + if (p != &first_thread) free(p); } } GC_threads[hv] = me; @@ -969,6 +970,8 @@ void GC_thr_init() t -> stop_info.stack_ptr = (ptr_t)(&dummy); # endif t -> flags = DETACHED | MAIN_THREAD; + if (gc_thread_vtable && gc_thread_vtable->thread_created) + gc_thread_vtable->thread_created (pthread_self (), &t->stop_info.stack_ptr); GC_stop_init(); @@ -1274,6 +1277,8 @@ void * GC_start_routine_head(void * arg, void *base_addr, /* This is also < 100% convincing. We should also read this */ /* from /proc, but the hook to do so isn't there yet. */ # endif /* IA64 */ + if (gc_thread_vtable && gc_thread_vtable->thread_created) + gc_thread_vtable->thread_created (my_pthread, &me->stop_info.stack_ptr); UNLOCK(); if (start) *start = si -> start_routine; @@ -1356,8 +1361,7 @@ WRAP_FUNC(pthread_create)(pthread_t *new_thread, /* responsibility. */ LOCK(); - si = (struct start_info *)GC_INTERNAL_MALLOC(sizeof(struct start_info), - NORMAL); + si = calloc (1, sizeof (struct start_info)); UNLOCK(); if (!parallel_initialized) GC_init_parallel(); if (0 == si) return(ENOMEM); @@ -1417,7 +1421,7 @@ WRAP_FUNC(pthread_create)(pthread_t *new_thread, } sem_destroy(&(si -> registered)); LOCK(); - GC_INTERNAL_FREE(si); + free(si); UNLOCK(); return(result); |