From 36c6c7f25960ecec135d70a5ad4df01b0551e773 Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Tue, 10 Apr 2018 11:48:51 +0300 Subject: [PATCH] Avoid potential data race during GC_dump execution * include/gc.h (GC_dump): Separate declaration from GC_dump_named; add comment that it acquires the lock. * include/gc.h (GC_dump_named): Refine comment (add note that the lock is not acquired by this function). * include/private/gc_priv.h (GC_dump): Remove commented out declaration. * include/private/gc_priv.h [!NO_DEBUGGING] (COND_DUMP): Call GC_dump_named(NULL) instead of GC_dump(). * misc.c (GC_init) [GC_ASSERTIONS && GC_ALWAYS_MULTITHREADED]: Call COND_DUMP while holding the allocation lock. * misc.c [!NO_DEBUGGING] (GC_dump): Place LOCK/UNLOCK around GC_dump_named() call. --- include/gc.h | 9 +++++++-- include/private/gc_priv.h | 6 +++--- misc.c | 40 +++++++++++++++++++++++----------------- 3 files changed, 33 insertions(+), 22 deletions(-) diff --git a/include/gc.h b/include/gc.h index c612848..fbac4c3 100644 --- a/include/gc.h +++ b/include/gc.h @@ -1566,12 +1566,17 @@ GC_API void * GC_CALL GC_is_valid_displacement(void * /* p */); /* Explicitly dump the GC state. This is most often called from the */ /* debugger, or by setting the GC_DUMP_REGULARLY environment variable, */ /* but it may be useful to call it from client code during debugging. */ -/* If name is specified (and non-NULL), it is printed to help */ +/* The current collection number is printed in the header of the dump. */ +/* Acquires the GC lock to avoid data races. */ +/* Defined only if the library has been compiled without NO_DEBUGGING. */ +GC_API void GC_CALL GC_dump(void); + +/* The same as GC_dump but allows to specify the name of dump and does */ +/* not acquire the lock. If name is non-NULL, it is printed to help */ /* identifying individual dumps. Otherwise the current collection */ /* number is used as the name. */ /* Defined only if the library has been compiled without NO_DEBUGGING. */ GC_API void GC_CALL GC_dump_named(const char * /* name */); -GC_API void GC_CALL GC_dump(void); /* = GC_dump_named(NULL) */ /* Dump information about each block of every GC memory section. */ /* Defined only if the library has been compiled without NO_DEBUGGING. */ diff --git a/include/private/gc_priv.h b/include/private/gc_priv.h index 1d682a9..5ac127a 100644 --- a/include/private/gc_priv.h +++ b/include/private/gc_priv.h @@ -2212,7 +2212,6 @@ void GC_print_block_list(void); void GC_print_hblkfreelist(void); void GC_print_heap_sects(void); void GC_print_static_roots(void); -/* void GC_dump(void); - declared in gc.h */ extern word GC_fo_entries; /* should be visible in extra/MacOS.c */ @@ -2524,8 +2523,9 @@ GC_INNER void *GC_store_debug_info_inner(void *p, word sz, const char *str, #ifndef NO_DEBUGGING GC_EXTERN GC_bool GC_dump_regularly; /* Generate regular debugging dumps. */ -# define COND_DUMP if (EXPECT(GC_dump_regularly, FALSE)) GC_dump(); \ - else COND_DUMP_CHECKS +# define COND_DUMP if (EXPECT(GC_dump_regularly, FALSE)) { \ + GC_dump_named(NULL); \ + } else COND_DUMP_CHECKS #else # define COND_DUMP COND_DUMP_CHECKS #endif diff --git a/misc.c b/misc.c index 55937dd..2c4b6ce 100644 --- a/misc.c +++ b/misc.c @@ -871,6 +871,9 @@ GC_API void GC_CALL GC_init(void) /* LOCK(); -- no longer does anything this early. */ word initial_heap_sz; IF_CANCEL(int cancel_state;) +# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) + DCL_LOCK_STATE; +# endif if (EXPECT(GC_is_initialized, TRUE)) return; # ifdef REDIRECT_MALLOC @@ -1281,31 +1284,30 @@ GC_API void GC_CALL GC_init(void) GC_pcr_install(); # endif GC_is_initialized = TRUE; +# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) + LOCK(); /* just to set GC_lock_holder */ +# endif # if defined(GC_PTHREADS) || defined(GC_WIN32_THREADS) -# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) - DCL_LOCK_STATE; - LOCK(); /* just to set GC_lock_holder */ - GC_thr_init(); - UNLOCK(); -# else - GC_thr_init(); -# endif + GC_thr_init(); # ifdef PARALLEL_MARK /* Actually start helper threads. */ +# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) + UNLOCK(); +# endif GC_start_mark_threads_inner(); +# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) + LOCK(); +# endif # endif # endif COND_DUMP; /* Get black list set up and/or incremental GC started */ - if (!GC_dont_precollect || GC_incremental) { -# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) - LOCK(); - GC_gcollect_inner(); - UNLOCK(); -# else - GC_gcollect_inner(); -# endif - } + if (!GC_dont_precollect || GC_incremental) { + GC_gcollect_inner(); + } +# if defined(GC_ASSERTIONS) && defined(GC_ALWAYS_MULTITHREADED) + UNLOCK(); +# endif # ifdef STUBBORN_ALLOC GC_stubborn_init(); # endif @@ -2190,7 +2192,11 @@ GC_API void * GC_CALL GC_do_blocking(GC_fn_type fn, void * client_data) #if !defined(NO_DEBUGGING) GC_API void GC_CALL GC_dump(void) { + DCL_LOCK_STATE; + + LOCK(); GC_dump_named(NULL); + UNLOCK(); } GC_API void GC_CALL GC_dump_named(const char *name) -- 2.7.4