Avoid potential race in SET_MARK_BIT_EXIT_IF_SET if parallel marking

* include/private/gc_atomic_ops.h [GC_BUILTIN_ATOMIC] (AO_char_load,
AO_HAVE_char_load, AO_char_store, AO_HAVE_char_store): Define macro.
* include/private/gc_pmark.h [USE_MARK_BYTES && PARALLEL_MARK
&& AO_HAVE_char_store && !AO_USE_PTHREAD_DEFS]
(SET_MARK_BIT_EXIT_IF_SET): Use AO_char_load/store to access hb_marks.
* include/private/gc_pmark.h [!USE_MARK_BYTES && PARALLEL_MARK
&& THREAD_SANITIZER] (OR_WORD_EXIT_IF_SET): Use AO_load to fetch the
word at addr.
* tests/test_atomic_ops.c [AO_HAVE_char_load || AO_HAVE_char_store]
(main): Define c local variable.
* tests/test_atomic_ops.c [AO_HAVE_char_load] (main): Test AO_char_load.
* tests/test_atomic_ops.c [AO_HAVE_char_store] (main): Test
AO_char_store.

diff --git a/include/private/gc_atomic_ops.h b/include/private/gc_atomic_ops.h
index 28dce954486063e164b8d88cae6112a99d0a8b2d..776d4551905426528744c9b27b7de37e4709802b 100644 (file)
--- a/include/private/gc_atomic_ops.h
+++ b/include/private/gc_atomic_ops.h
@@ -74,6 +74,11 @@
 # define AO_store_release_write(p, v) AO_store_release(p, v)
 # define AO_HAVE_store_release_write
 
+# define AO_char_load(p) __atomic_load_n(p, __ATOMIC_RELAXED)
+# define AO_HAVE_char_load
+# define AO_char_store(p, v) __atomic_store_n(p, v, __ATOMIC_RELAXED)
+# define AO_HAVE_char_store
+
 # ifdef AO_REQUIRE_CAS
     AO_INLINE int
     AO_compare_and_swap(volatile AO_t *p, AO_t ov, AO_t nv)

diff --git a/include/private/gc_pmark.h b/include/private/gc_pmark.h
index 1a8841dfeef9ee67f186301fabdf33d75aab90b2..f12851a55a0a221d9f7d68ca7e35835b6e3b0714 100644 (file)
--- a/include/private/gc_pmark.h
+++ b/include/private/gc_pmark.h
@@ -148,21 +148,45 @@ GC_INNER mse * GC_signal_mark_stack_overflow(mse *msp);
 
 /* Set mark bit, exit (using "break" statement) if it is already set.   */
 #ifdef USE_MARK_BYTES
-  /* There is a race here, and we may set                               */
-  /* the bit twice in the concurrent case.  This can result in the      */
-  /* object being pushed twice.  But that's only a performance issue.   */
-# define SET_MARK_BIT_EXIT_IF_SET(hhdr, bit_no) \
-    { /* cannot use do-while(0) here */ \
-        char * mark_byte_addr = (char *)hhdr -> hb_marks + (bit_no); \
+# if defined(PARALLEL_MARK) && defined(AO_HAVE_char_store) \
+     && !defined(AO_USE_PTHREAD_DEFS)
+    /* There is a race here, and we may set the bit twice in the        */
+    /* concurrent case.  This can result in the object being pushed     */
+    /* twice.  But that is only a performance issue.                    */
+#   define SET_MARK_BIT_EXIT_IF_SET(hhdr, bit_no) \
+      { /* cannot use do-while(0) here */ \
+        volatile unsigned char * mark_byte_addr = \
+                        (unsigned char *)(hhdr)->hb_marks + (bit_no); \
+        /* Unordered atomic load and store are sufficient here. */ \
+        if (AO_char_load(mark_byte_addr) != 0) \
+          break; /* go to the enclosing loop end */ \
+        AO_char_store(mark_byte_addr, 1); \
+      }
+# else
+#   define SET_MARK_BIT_EXIT_IF_SET(hhdr, bit_no) \
+      { /* cannot use do-while(0) here */ \
+        char * mark_byte_addr = (char *)(hhdr)->hb_marks + (bit_no); \
         if (*mark_byte_addr != 0) break; /* go to the enclosing loop end */ \
         *mark_byte_addr = 1; \
-    }
+      }
+# endif /* !PARALLEL_MARK */
 #else
 # ifdef PARALLEL_MARK
     /* This is used only if we explicitly set USE_MARK_BITS.            */
     /* The following may fail to exit even if the bit was already set.  */
     /* For our uses, that's benign:                                     */
-#   define OR_WORD_EXIT_IF_SET(addr, bits) \
+#   ifdef THREAD_SANITIZER
+#     define OR_WORD_EXIT_IF_SET(addr, bits) \
+        { /* cannot use do-while(0) here */ \
+          if (!((word)AO_load((volatile AO_t *)(addr)) & (bits))) { \
+                /* Atomic load is just to avoid TSan false positive. */ \
+            AO_or((volatile AO_t *)(addr), (AO_t)(bits)); \
+          } else { \
+            break; /* go to the enclosing loop end */ \
+          } \
+        }
+#   else
+#     define OR_WORD_EXIT_IF_SET(addr, bits) \
         { /* cannot use do-while(0) here */ \
           if (!(*(addr) & (bits))) { \
             AO_or((volatile AO_t *)(addr), (AO_t)(bits)); \
@@ -170,6 +194,7 @@ GC_INNER mse * GC_signal_mark_stack_overflow(mse *msp);
             break; /* go to the enclosing loop end */ \
           } \
         }
+#   endif /* !THREAD_SANITIZER */
 # else
 #   define OR_WORD_EXIT_IF_SET(addr, bits) \
         { /* cannot use do-while(0) here */ \
@@ -182,7 +207,7 @@ GC_INNER mse * GC_signal_mark_stack_overflow(mse *msp);
 # endif /* !PARALLEL_MARK */
 # define SET_MARK_BIT_EXIT_IF_SET(hhdr, bit_no) \
     { /* cannot use do-while(0) here */ \
-        word * mark_word_addr = hhdr -> hb_marks + divWORDSZ(bit_no); \
+        word * mark_word_addr = (hhdr)->hb_marks + divWORDSZ(bit_no); \
         OR_WORD_EXIT_IF_SET(mark_word_addr, \
                 (word)1 << modWORDSZ(bit_no)); /* contains "break" */ \
     }

diff --git a/tests/test_atomic_ops.c b/tests/test_atomic_ops.c
index a70fe47145b50704bc598284d2fe1662a57c69d0..23bb405b7076f47f976b3c4ae8efd1c1b7ceeb65 100644 (file)
--- a/tests/test_atomic_ops.c
+++ b/tests/test_atomic_ops.c
@@ -38,6 +38,9 @@
 
   int main(void) {
     AO_t x = 13;
+#   if defined(AO_HAVE_char_load) || defined(AO_HAVE_char_store)
+      unsigned char c = 117;
+#   endif
 #   ifdef AO_HAVE_test_and_set_acquire
         AO_TS_t z = AO_TS_INITIALIZER;
 
@@ -49,6 +52,13 @@
 #   ifdef AO_HAVE_nop_full
       AO_nop_full();
 #   endif
+#   ifdef AO_HAVE_char_load
+      TA_assert(AO_char_load(&c) == 117);
+#   endif
+#   ifdef AO_HAVE_char_store
+      AO_char_store(&c, 119);
+      TA_assert(c == 119);
+#   endif
 #   ifdef AO_HAVE_load_acquire
       TA_assert(AO_load_acquire(&x) == 13);
 #   endif