From: Ivan Maidanski Date: Fri, 9 Mar 2018 20:23:56 +0000 (+0300) Subject: Avoid potential race in SET_MARK_BIT_EXIT_IF_SET if parallel marking X-Git-Tag: v8.0.0~292 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9f377e845b112aed14e9a3c28e52b07b84599e49;p=gc 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 28dce954..776d4551 100644 --- 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 1a8841df..f12851a5 100644 --- 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 a70fe471..23bb405b 100644 --- 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