From 613ec90c8ea2a31d26c90bf5d3dd8a86329d211a Mon Sep 17 00:00:00 2001 From: Ivan Maidanski Date: Fri, 1 Dec 2017 19:23:13 +0300 Subject: [PATCH] Fix data race in mark_thread when updating mark_no * mark.c [PARALLEL_MARK] (GC_mark_local, GC_help_marker): Update description comment; remove GC_acquire_mark_lock() call at the function beginning; remove GC_release_mark_lock() call on return. * mark.c [PARALLEL_MARK] (GC_do_parallel_mark): Remove GC_acquire/release_mark_lock() calls around GC_mark_local() call. * pthread_support.c [PARALLEL_MARK] (GC_mark_thread): Remove GC_release_mark_lock() call. * win32_threads.c [PARALLEL_MARK] (GC_mark_thread): Likewise. --- mark.c | 11 ++--------- pthread_support.c | 1 - win32_threads.c | 1 - 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/mark.c b/mark.c index efd3e221..a495facf 100644 --- a/mark.c +++ b/mark.c @@ -1102,15 +1102,13 @@ STATIC void GC_do_local_mark(mse *local_mark_stack, mse *local_top) /* Mark using the local mark stack until the global mark stack is empty */ /* and there are no active workers. Update GC_first_nonempty to reflect */ -/* progress. */ -/* Caller does not hold mark lock. */ +/* progress. Caller holds the mark lock. */ /* Caller has already incremented GC_helper_count. We decrement it, */ /* and maintain GC_active_count. */ STATIC void GC_mark_local(mse *local_mark_stack, int id) { mse * my_first_nonempty; - GC_acquire_mark_lock(); GC_active_count++; my_first_nonempty = (mse *)AO_load(&GC_first_nonempty); GC_ASSERT((word)GC_mark_stack <= (word)my_first_nonempty); @@ -1175,7 +1173,6 @@ STATIC void GC_mark_local(mse *local_mark_stack, int id) GC_helper_count--; if (0 == GC_helper_count) need_to_notify = TRUE; GC_VERBOSE_LOG_PRINTF("Finished mark helper %d\n", id); - GC_release_mark_lock(); if (need_to_notify) GC_notify_all_marker(); return; } @@ -1222,11 +1219,9 @@ STATIC void GC_do_parallel_mark(void) GC_active_count = 0; GC_helper_count = 1; GC_help_wanted = TRUE; - GC_release_mark_lock(); GC_notify_all_marker(); /* Wake up potential helpers. */ GC_mark_local(main_local_mark_stack, 0); - GC_acquire_mark_lock(); GC_help_wanted = FALSE; /* Done; clean up. */ while (GC_helper_count > 0) { @@ -1243,6 +1238,7 @@ STATIC void GC_do_parallel_mark(void) /* Try to help out the marker, if it's running. */ /* We do not hold the GC lock, but the requestor does. */ +/* And we hold the mark lock. */ GC_INNER void GC_help_marker(word my_mark_no) { # define my_id my_id_mse.mse_descr.w @@ -1251,7 +1247,6 @@ GC_INNER void GC_help_marker(word my_mark_no) /* Note: local_mark_stack is quite big (up to 128 KiB). */ GC_ASSERT(GC_parallel); - GC_acquire_mark_lock(); while (GC_mark_no < my_mark_no || (!GC_help_wanted && GC_mark_no == my_mark_no)) { GC_wait_marker(); @@ -1260,11 +1255,9 @@ GC_INNER void GC_help_marker(word my_mark_no) if (GC_mark_no != my_mark_no || my_id > (unsigned)GC_markers_m1) { /* Second test is useful only if original threads can also */ /* act as helpers. Under Linux they can't. */ - GC_release_mark_lock(); return; } GC_helper_count = (unsigned)my_id + 1; - GC_release_mark_lock(); GC_mark_local(local_mark_stack, (int)my_id); /* GC_mark_local decrements GC_helper_count. */ # undef my_id diff --git a/pthread_support.c b/pthread_support.c index 9105bcd7..ba4b590c 100644 --- a/pthread_support.c +++ b/pthread_support.c @@ -359,7 +359,6 @@ STATIC void * GC_mark_thread(void * id) GC_acquire_mark_lock(); if (0 == --GC_fl_builder_count) /* count may have a negative value */ GC_notify_all_builder(); - GC_release_mark_lock(); for (;; ++my_mark_no) { /* GC_mark_no is passed only to allow GC_help_marker to terminate */ diff --git a/win32_threads.c b/win32_threads.c index 40eee3c4..bc8abdf6 100644 --- a/win32_threads.c +++ b/win32_threads.c @@ -1748,7 +1748,6 @@ GC_INNER void GC_get_next_stack(char *start, char *limit, GC_acquire_mark_lock(); if (0 == --GC_fl_builder_count) /* count may have a negative value */ GC_notify_all_builder(); - GC_release_mark_lock(); for (;; ++my_mark_no) { if (my_mark_no - GC_mark_no > (word)2) { -- 2.40.0