From: Ivan Maidanski <ivmai@mail.ru>
Date: Fri, 1 Dec 2017 16:23:13 +0000 (+0300)
Subject: Fix data race in mark_thread when updating mark_no
X-Git-Tag: v7.6.2~20
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=266c2891;p=gc

Fix data race in mark_thread when updating mark_no
(Cherry-pick commit 613ec90c from 'master' branch.)

* 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.
---

diff --git a/mark.c b/mark.c
index eaef7e6b..5a137b3a 100644
--- a/mark.c
+++ b/mark.c
@@ -1070,15 +1070,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);
@@ -1143,7 +1141,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;
                 }
@@ -1193,11 +1190,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(local_mark_stack, 0);
-    GC_acquire_mark_lock();
     GC_help_wanted = FALSE;
     /* Done; clean up.  */
     while (GC_helper_count > 0) {
@@ -1214,6 +1209,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
@@ -1222,7 +1218,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();
@@ -1231,11 +1226,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 65d64f8d..f9ccd9cb 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 0fb4cf0b..294acade 100644
--- a/win32_threads.c
+++ b/win32_threads.c
@@ -1744,7 +1744,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) {