From 2d79f88938ca8bbe2eabf05dc4ba98fd0ec63dae Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Wed, 21 Oct 2015 00:48:56 +0300 Subject: [PATCH] Fix lock assertion violation in GC_new_thread if GC_ALWAYS_MULTITHREADED * include/private/gc_priv.h (GC_start_mark_threads_inner): Define macro (or declare function depending on CAN_HANDLE_FORK). * misc.c (GC_init): Surround GC_thr_init call with LOCK/UNLOCK (only if GC_ASSERTIONS and GC_ALWAYS_MULTITHREADED otherwise redundant); call GC_start_mark_threads_inner (if PARALLEL_MARK). * pthread_support.c (GC_mark_thread): Update comment. * win32_threads.c (GC_mark_thread): Likewise. * pthread_support.c (start_mark_threads): Remove macro (moved to gc_priv.h); rename function to GC_start_mark_threads_inner; replace "static" to GC_INNER; check assertion on GC_fl_builder_count only if the markers should actually be started; move the check for disabled parallel markers (available_markers_m1) from GC_thr_init (make it unconditional). * win32_threads.c (start_mark_threads): Likewise. * win32_threads.c (GC_start_mark_threads_inner): Add assertion about the lock status. * pthread_support.c (GC_thr_init): Remove comment about expected lock status; add assertion about holding the lock (duplicating that in GC_new_thread); remove start_mark_threads call (moved to GC_init). * win32_threads.c (GC_thr_init): Likewise. --- include/private/gc_priv.h | 6 ++++++ misc.c | 12 +++++++++++- pthread_support.c | 19 ++++++++----------- win32_threads.c | 32 ++++++++++++-------------------- 4 files changed, 37 insertions(+), 32 deletions(-) diff --git a/include/private/gc_priv.h b/include/private/gc_priv.h index 8f05bc1f..710c58c8 100644 --- a/include/private/gc_priv.h +++ b/include/private/gc_priv.h @@ -2418,6 +2418,12 @@ GC_INNER ptr_t GC_store_debug_info(ptr_t p, word sz, const char *str, /* my_mark_no. Returns if the mark cycle finishes or */ /* was already done, or there was nothing to do for */ /* some other reason. */ + +# ifdef CAN_HANDLE_FORK +# define GC_start_mark_threads_inner GC_start_mark_threads +# else + GC_INNER void GC_start_mark_threads_inner(void); +# endif #endif /* PARALLEL_MARK */ #if defined(GC_PTHREADS) && !defined(GC_WIN32_THREADS) && !defined(NACL) \ diff --git a/misc.c b/misc.c index 29938c0c..88885e30 100644 --- a/misc.c +++ b/misc.c @@ -1258,7 +1258,17 @@ GC_API void GC_CALL GC_init(void) # endif GC_is_initialized = TRUE; # if defined(GC_PTHREADS) || defined(GC_WIN32_THREADS) - GC_thr_init(); +# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) + LOCK(); /* just to set GC_lock_holder */ + GC_thr_init(); + UNLOCK(); +# else + GC_thr_init(); +# endif +# ifdef PARALLEL_MARK + /* Actually start helper threads. */ + GC_start_mark_threads_inner(); +# endif # endif COND_DUMP; /* Get black list set up and/or incremental GC started */ diff --git a/pthread_support.c b/pthread_support.c index 214dc2da..7c5af41f 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -372,7 +372,7 @@ STATIC void * GC_mark_thread(void * id) marker_mach_threads[(word)id] = mach_thread_self(); # endif - /* Inform start_mark_threads() about completion of marker data init. */ + /* Inform GC_start_mark_threads about completion of marker data init. */ GC_acquire_mark_lock(); if (0 == --GC_fl_builder_count) GC_notify_all_builder(); @@ -402,26 +402,25 @@ STATIC pthread_t GC_mark_threads[MAX_MARKERS]; #ifdef CAN_HANDLE_FORK static int available_markers_m1 = 0; -# define start_mark_threads GC_start_mark_threads GC_API void GC_CALL #else # define available_markers_m1 GC_markers_m1 - static void + GC_INNER void #endif -start_mark_threads(void) + GC_start_mark_threads_inner(void) { int i; pthread_attr_t attr; GC_ASSERT(I_DONT_HOLD_LOCK()); - GC_ASSERT(GC_fl_builder_count == 0); -# ifdef CAN_HANDLE_FORK - if (available_markers_m1 <= 0 || GC_parallel) return; + if (available_markers_m1 <= 0) return; /* Skip if parallel markers disabled or already started. */ +# ifdef CAN_HANDLE_FORK + if (GC_parallel) return; # endif + GC_ASSERT(GC_fl_builder_count == 0); INIT_REAL_SYMS(); /* for pthread_create */ - if (0 != pthread_attr_init(&attr)) ABORT("pthread_attr_init failed"); if (0 != pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED)) ABORT("pthread_attr_setdetachstate failed"); @@ -1072,9 +1071,9 @@ static void fork_child_proc(void) static void setup_mark_lock(void); #endif -/* We hold the allocation lock. */ GC_INNER void GC_thr_init(void) { + GC_ASSERT(I_HOLD_LOCK()); if (GC_thr_initialized) return; GC_thr_initialized = TRUE; @@ -1193,8 +1192,6 @@ GC_INNER void GC_thr_init(void) /* Disable true incremental collection, but generational is OK. */ GC_time_limit = GC_TIME_UNLIMITED; setup_mark_lock(); - /* If we are using a parallel marker, actually start helper threads. */ - start_mark_threads(); } # endif } diff --git a/win32_threads.c b/win32_threads.c index d386f076..53d0acfd 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -1721,7 +1721,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_marker_Id[(word)id] = GetCurrentThreadId(); # endif - /* Inform start_mark_threads() about completion of marker data init. */ + /* Inform GC_start_mark_threads about completion of marker data init. */ GC_acquire_mark_lock(); if (0 == --GC_fl_builder_count) GC_notify_all_builder(); @@ -1760,28 +1760,28 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, /* Id not guaranteed to be unique. */ # endif - /* start_mark_threads is the same as in pthread_support.c except */ + /* GC_start_mark_threads is the same as in pthread_support.c except */ /* for thread stack that is assumed to be large enough. */ # ifdef CAN_HANDLE_FORK static int available_markers_m1 = 0; -# define start_mark_threads GC_start_mark_threads GC_API void GC_CALL # else - static void + GC_INNER void # endif - start_mark_threads(void) + GC_start_mark_threads_inner(void) { int i; pthread_attr_t attr; pthread_t new_thread; GC_ASSERT(I_DONT_HOLD_LOCK()); - GC_ASSERT(GC_fl_builder_count == 0); -# ifdef CAN_HANDLE_FORK - if (available_markers_m1 <= 0 || GC_parallel) return; + if (available_markers_m1 <= 0) return; /* Skip if parallel markers disabled or already started. */ +# ifdef CAN_HANDLE_FORK + if (GC_parallel) return; # endif + GC_ASSERT(GC_fl_builder_count == 0); if (0 != pthread_attr_init(&attr)) ABORT("pthread_attr_init failed"); if (0 != pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED)) ABORT("pthread_attr_setdetachstate failed"); @@ -1910,7 +1910,7 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, static HANDLE builder_cv = (HANDLE)0; /* Event with manual reset. */ static HANDLE mark_cv = (HANDLE)0; /* Event with manual reset. */ - static void start_mark_threads(void) + GC_INNER void GC_start_mark_threads_inner(void) { int i; # ifdef MSWINCE @@ -1921,6 +1921,9 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, unsigned thread_id; # endif + GC_ASSERT(I_DONT_HOLD_LOCK()); + if (available_markers_m1 <= 0) return; + GC_ASSERT(GC_fl_builder_count == 0); /* Initialize GC_marker_cv[] fully before starting the */ /* first helper thread. */ @@ -2342,7 +2345,6 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, #endif /* GC_WINMAIN_REDIRECT */ -/* Called by GC_init() - we hold the allocation lock. */ GC_INNER void GC_thr_init(void) { struct GC_stack_base sb; @@ -2454,16 +2456,6 @@ GC_INNER void GC_thr_init(void) GC_ASSERT(0 == GC_lookup_thread_inner(GC_main_thread)); GC_register_my_thread_inner(&sb, GC_main_thread); - -# ifdef PARALLEL_MARK -# ifndef CAN_HANDLE_FORK - if (GC_parallel) -# endif - { - /* If we are using a parallel marker, actually start helper threads. */ - start_mark_threads(); - } -# endif } #ifdef GC_PTHREADS -- 2.40.0