From: Ivan Maidanski Date: Thu, 7 Dec 2017 08:14:46 +0000 (+0300) Subject: Minimize use of AO_ATTR_NO_SANITIZE_THREAD in atomic_ops_malloc/stack X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6ffda1db;p=libatomic_ops Minimize use of AO_ATTR_NO_SANITIZE_THREAD in atomic_ops_malloc/stack (fix commit c058d9d) * src/atomic_ops_malloc.c [AO_THREAD_SANITIZER] (AO_malloc): Use AO_store() only if AO_USE_ALMOST_LOCK_FREE; add comment. * src/atomic_ops_malloc.c (AO_free): Remove TSan workaround (do not check AO_THREAD_SANITIZER macro). * src/atomic_ops_stack.c (AO_copy_before_cas): New inline function. * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE] (AO_stack_push_explicit_aux_release): Remove AO_ATTR_NO_SANITIZE_THREAD. * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE && AO_HAVE_compare_double_and_swap_double] (AO_stack_push_release, AO_stack_pop_acquire): Likewise. * src/atomic_ops_stack.c [AO_USE_ALMOST_LOCK_FREE] (AO_stack_push_explicit_aux_release): Replace *x=next with AO_copy_before_cas(x,&next). * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE] (AO_stack_push_release): Likewise. * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE] (AO_stack_pop_acquire): Replace next=*cptr with AO_copy_before_cas(&next,cptr). * src/atomic_ops_stack.c [!USE_ALMOST_LOCK_FREE && AO_HAVE_compare_and_swap_double] (AO_stack_pop_acquire): Reformat code. --- diff --git a/src/atomic_ops_malloc.c b/src/atomic_ops_malloc.c index f78ff40..06ed833 100644 --- a/src/atomic_ops_malloc.c +++ b/src/atomic_ops_malloc.c @@ -317,7 +317,8 @@ AO_malloc(size_t sz) add_chunk_as(chunk, log_sz); result = AO_stack_pop(AO_free_list+log_sz); } -# ifdef AO_THREAD_SANITIZER +# if defined(AO_THREAD_SANITIZER) && defined(AO_USE_ALMOST_LOCK_FREE) + /* A data race with AO_stack_pop() called above is a false positive. */ AO_store(result, log_sz); # else *result = log_sz; @@ -339,11 +340,7 @@ AO_free(void *p) return; base = (AO_t *)p - 1; -# ifdef AO_THREAD_SANITIZER - log_sz = (int)AO_load(base); -# else - log_sz = (int)(*base); -# endif + log_sz = (int)(*base); # ifdef AO_TRACE_MALLOC fprintf(stderr, "%p: AO_free(%p sz:%lu)\n", (void *)pthread_self(), p, log_sz > LOG_MAX_SIZE ? (unsigned)log_sz : 1UL << log_sz); diff --git a/src/atomic_ops_stack.c b/src/atomic_ops_stack.c index b57ed85..a957642 100644 --- a/src/atomic_ops_stack.c +++ b/src/atomic_ops_stack.c @@ -22,6 +22,14 @@ #define AO_REQUIRE_CAS #include "atomic_ops_stack.h" +/* The function call must be a part of a do-while loop with a CAS */ +/* designating the condition of the loop (see the use cases below). */ +AO_ATTR_NO_SANITIZE_THREAD +AO_INLINE void AO_copy_before_cas(AO_t *pdest, AO_t *psrc) +{ + *pdest = *psrc; +} + #ifdef AO_USE_ALMOST_LOCK_FREE void AO_pause(int); /* defined in atomic_ops.c */ @@ -46,7 +54,6 @@ /* to be inserted. */ /* Both list headers and link fields contain "perturbed" pointers, i.e. */ /* pointers with extra bits "or"ed into the low order bits. */ -AO_ATTR_NO_SANITIZE_THREAD void AO_stack_push_explicit_aux_release(volatile AO_t *list, AO_t *x, AO_stack_aux *a) { @@ -94,7 +101,7 @@ void AO_stack_push_explicit_aux_release(volatile AO_t *list, AO_t *x, do { next = AO_load(list); - *x = next; /* data race is OK here */ + AO_copy_before_cas(x, &next); } while (AO_EXPECT_FALSE(!AO_compare_and_swap_release(list, next, x_bits))); } @@ -207,14 +214,13 @@ AO_stack_pop_explicit_aux_acquire(volatile AO_t *list, AO_stack_aux * a) volatile /* non-static */ AO_t AO_noop_sink; #endif -AO_ATTR_NO_SANITIZE_THREAD void AO_stack_push_release(AO_stack_t *list, AO_t *element) { AO_t next; do { next = AO_load(&(list -> ptr)); - *element = next; /* data race is OK here */ + AO_copy_before_cas(element, &next); } while (AO_EXPECT_FALSE(!AO_compare_and_swap_release(&(list -> ptr), next, (AO_t)element))); /* This uses a narrow CAS here, an old optimization suggested */ @@ -227,7 +233,6 @@ void AO_stack_push_release(AO_stack_t *list, AO_t *element) # endif } -AO_ATTR_NO_SANITIZE_THREAD AO_t *AO_stack_pop_acquire(AO_stack_t *list) { # ifdef __clang__ @@ -244,8 +249,9 @@ AO_t *AO_stack_pop_acquire(AO_stack_t *list) /* Version must be loaded first. */ cversion = AO_load_acquire(&(list -> version)); cptr = (AO_t *)AO_load(&(list -> ptr)); - if (cptr == 0) return 0; - next = *cptr; /* data race is OK here */ + if (NULL == cptr) + return NULL; + AO_copy_before_cas(&next, cptr); } while (AO_EXPECT_FALSE(!AO_compare_double_and_swap_double_release(list, cversion, (AO_t)cptr, cversion+1, (AO_t)next))); @@ -274,7 +280,7 @@ void AO_stack_push_release(AO_stack_t *list, AO_t *element) /* Again version must be loaded first, for different reason. */ version = AO_load_acquire(&(list -> version)); next_ptr = AO_load(&(list -> ptr)); - *element = next_ptr; + AO_copy_before_cas(element, &next_ptr); } while (!AO_compare_and_swap_double_release( list, version, version+1, (AO_t) element)); @@ -289,14 +295,15 @@ AO_t *AO_stack_pop_acquire(AO_stack_t *list) do { cversion = AO_load_acquire(&(list -> version)); cptr = (AO_t *)AO_load(&(list -> ptr)); - if (cptr == 0) return 0; - next = *cptr; - } while (!AO_compare_double_and_swap_double_release - (list, cversion, (AO_t) cptr, cversion+1, next)); + if (NULL == cptr) + return NULL; + AO_copy_before_cas(&next, cptr); + } while (!AO_compare_double_and_swap_double_release(list, + cversion, (AO_t)cptr, + cversion+1, next)); return cptr; } - #endif /* AO_HAVE_compare_and_swap_double */ #endif /* ! USE_ALMOST_LOCK_FREE */