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

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);