From: Ivan Maidanski <ivmai@mail.ru>
Date: Tue, 30 Oct 2018 07:38:56 +0000 (+0300)
Subject: Fix data race in disclaim_weakmap_test statistic counters update
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=95e0747277afa231cad42b9ce73c1385ba3a7f87;p=gc

Fix data race in disclaim_weakmap_test statistic counters update
(fix of commit 6eba4218a)

Issue #239 (bdwgc).

The stat_* variables need to be atomically updated, since the locking
of the map is granular (cf WEAKMAP_MUTEX_COUNT) to allow parallelism
between different key hashes.

* tests/disclaim_weakmap_test.c [!AO_CLEAR] (AO_t): Define.
* tests/disclaim_weakmap_test.c [!AO_HAVE_fetch_and_add1]
(AO_fetch_and_add1): Define.
* tests/disclaim_weakmap_test.c (stat_added, stat_found, stat_removed,
stat_skip_marked, stat_skip_locked): Change type back from unsigned to
volatile AO_t.
* tests/disclaim_weakmap_test.c (weakmap_add, weakmap_disclaim):
Update stat_* counters using AO_fetch_and_add1(); move counters update
outside weakmap_lock/unlock block.
* tests/disclaim_weakmap_test.c (main): Cast stat_* counters to
unsigned type in printf() call.
---

diff --git a/tests/disclaim_weakmap_test.c b/tests/disclaim_weakmap_test.c
index aa593ea5..9f40e452 100644
--- a/tests/disclaim_weakmap_test.c
+++ b/tests/disclaim_weakmap_test.c
@@ -85,6 +85,16 @@
         } \
     } while (0)
 
+/* Define AO primitives in case of no threads support.  */
+#ifndef AO_CLEAR
+  /* AO_t not defined. */
+# define AO_t GC_word
+#endif
+#ifndef AO_HAVE_fetch_and_add1
+# define AO_fetch_and_add1(p) ((*(p))++)
+                /* This is used only to update counters.        */
+#endif
+
 unsigned memhash(void *src, size_t len)
 {
   unsigned acc = 0;
@@ -97,16 +107,11 @@ unsigned memhash(void *src, size_t len)
   return acc;
 }
 
-static unsigned stat_added;
-static unsigned stat_found;
-static unsigned stat_removed;
-static unsigned stat_skip_marked;
-
-#ifdef AO_HAVE_fetch_and_add1
-  static volatile AO_t stat_skip_locked;
-#else
-  static unsigned stat_skip_locked;
-#endif
+static volatile AO_t stat_added;
+static volatile AO_t stat_found;
+static volatile AO_t stat_removed;
+static volatile AO_t stat_skip_locked;
+static volatile AO_t stat_skip_marked;
 
 struct weakmap_link {
   GC_hidden_pointer obj;
@@ -192,8 +197,8 @@ void *weakmap_add(struct weakmap *wm, void *obj)
                wm->obj_size - key_size);
         GC_end_stubborn_change((char *)old_obj + key_size);
       }
-      ++stat_found;
       weakmap_unlock(wm, h);
+      AO_fetch_and_add1(&stat_found);
 #     ifdef DEBUG_DISCLAIM_WEAKMAP
         printf("Found %p, hash=%p\n", old_obj, (void *)(GC_word)h);
 #     endif
@@ -217,8 +222,8 @@ void *weakmap_add(struct weakmap *wm, void *obj)
   new_link->next = *first;
   GC_END_STUBBORN_CHANGE(new_link);
   GC_PTR_STORE_AND_DIRTY(first, new_link);
-  ++stat_added;
   weakmap_unlock(wm, h);
+  AO_fetch_and_add1(&stat_added);
 # ifdef DEBUG_DISCLAIM_WEAKMAP
     printf("Added %p, hash=%p\n", new_obj, (void *)(GC_word)h);
 # endif
@@ -245,30 +250,26 @@ int GC_CALLBACK weakmap_disclaim(void *obj_base)
   /* Lock and check for mark.   */
   h = memhash(obj, wm->key_size);
   if (weakmap_trylock(wm, h) != 0) {
-#   ifdef AO_HAVE_fetch_and_add1
-      AO_fetch_and_add1(&stat_skip_locked);
-#   else
-      ++stat_skip_locked;
-#   endif
+    AO_fetch_and_add1(&stat_skip_locked);
 #   ifdef DEBUG_DISCLAIM_WEAKMAP
       printf("Skipping locked %p, hash=%p\n", obj, (void *)(GC_word)h);
 #   endif
     return 1;
   }
   if (GC_is_marked(obj_base)) {
+    weakmap_unlock(wm, h);
+    AO_fetch_and_add1(&stat_skip_marked);
 #   ifdef DEBUG_DISCLAIM_WEAKMAP
       printf("Skipping marked %p, hash=%p\n", obj, (void *)(GC_word)h);
 #   endif
-    ++stat_skip_marked;
-    weakmap_unlock(wm, h);
     return 1;
   }
 
   /* Remove obj from wm.        */
+  AO_fetch_and_add1(&stat_removed);
 # ifdef DEBUG_DISCLAIM_WEAKMAP
     printf("Removing %p, hash=%p\n", obj, (void *)(GC_word)h);
 # endif
-  ++stat_removed;
   *(GC_word *)obj_base |= INVALIDATE_FLAG;
   for (link = &wm->links[h % wm->capacity];; link = &(*link)->next) {
     void *old_obj;
@@ -462,7 +463,8 @@ int main(void)
 # endif
   weakmap_destroy(pair_hcset);
   printf("%u added, %u found; %u removed, %u locked, %u marked; %u remains\n",
-         stat_added, stat_found, stat_removed, (unsigned)stat_skip_locked,
-         stat_skip_marked, stat_added - stat_removed);
+         (unsigned)stat_added, (unsigned)stat_found, (unsigned)stat_removed,
+         (unsigned)stat_skip_locked, (unsigned)stat_skip_marked,
+         (unsigned)stat_added - (unsigned)stat_removed);
   return 0;
 }