diff options
author | Edward Thomson <ethomson@edwardthomson.com> | 2021-08-27 08:28:01 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-08-27 08:28:01 -0400 |
commit | 34e685c01d607cc7019217e4b7ddf55041c33914 (patch) | |
tree | f436d9057aa784684161b5a9f7e45d1961a20b74 | |
parent | 3f8bf8be9f47a3ddc9208bc1721a277461546e4d (diff) | |
parent | 74708a813d586df3ba22a81b475f6aba0498e2ef (diff) | |
download | libgit2-34e685c01d607cc7019217e4b7ddf55041c33914.tar.gz |
Merge pull request #5747 from lhchavez/atomic-tests
Homogenize semantics for atomic-related functions
-rw-r--r-- | src/attrcache.c | 13 | ||||
-rw-r--r-- | src/diff_driver.c | 25 | ||||
-rw-r--r-- | src/repository.c | 12 | ||||
-rw-r--r-- | src/thread.h | 111 | ||||
-rw-r--r-- | tests/threads/atomic.c | 125 |
5 files changed, 234 insertions, 52 deletions
diff --git a/src/attrcache.c b/src/attrcache.c index 7fe2bfbdb..2b36b7a9c 100644 --- a/src/attrcache.c +++ b/src/attrcache.c @@ -127,7 +127,7 @@ static int attr_cache_remove(git_attr_cache *cache, git_attr_file *file) { int error = 0; git_attr_file_entry *entry; - git_attr_file *old = NULL; + git_attr_file *oldfile = NULL; if (!file) return 0; @@ -136,13 +136,13 @@ static int attr_cache_remove(git_attr_cache *cache, git_attr_file *file) return error; if ((entry = attr_cache_lookup_entry(cache, file->entry->path)) != NULL) - old = git_atomic_compare_and_swap(&entry->file[file->source.type], file, NULL); + oldfile = git_atomic_compare_and_swap(&entry->file[file->source.type], file, NULL); attr_cache_unlock(cache); - if (old) { - GIT_REFCOUNT_OWN(old, NULL); - git_attr_file__free(old); + if (oldfile == file) { + GIT_REFCOUNT_OWN(file, NULL); + git_attr_file__free(file); } return error; @@ -401,8 +401,7 @@ int git_attr_cache__init(git_repository *repo) (ret = git_pool_init(&cache->pool, 1)) < 0) goto cancel; - cache = git_atomic_compare_and_swap(&repo->attrcache, NULL, cache); - if (cache) + if (git_atomic_compare_and_swap(&repo->attrcache, NULL, cache) != NULL) goto cancel; /* raced with another thread, free this but no error */ git_config_free(cfg); diff --git a/src/diff_driver.c b/src/diff_driver.c index 8e9131feb..a3892d35e 100644 --- a/src/diff_driver.c +++ b/src/diff_driver.c @@ -141,18 +141,23 @@ static int diff_driver_funcname(const git_config_entry *entry, void *payload) static git_diff_driver_registry *git_repository_driver_registry( git_repository *repo) { - if (!repo->diff_drivers) { - git_diff_driver_registry *reg = git_diff_driver_registry_new(); - reg = git_atomic_compare_and_swap(&repo->diff_drivers, NULL, reg); + git_diff_driver_registry *reg = git_atomic_load(repo->diff_drivers), *newreg; + if (reg) + return reg; - if (reg != NULL) /* if we race, free losing allocation */ - git_diff_driver_registry_free(reg); - } - - if (!repo->diff_drivers) + newreg = git_diff_driver_registry_new(); + if (!newreg) { git_error_set(GIT_ERROR_REPOSITORY, "unable to create diff driver registry"); - - return repo->diff_drivers; + return newreg; + } + reg = git_atomic_compare_and_swap(&repo->diff_drivers, NULL, newreg); + if (!reg) { + reg = newreg; + } else { + /* if we race, free losing allocation */ + git_diff_driver_registry_free(newreg); + } + return reg; } static int diff_driver_alloc( diff --git a/src/repository.c b/src/repository.c index 692f71861..9ed46bf4d 100644 --- a/src/repository.c +++ b/src/repository.c @@ -1093,8 +1093,7 @@ int git_repository_config__weakptr(git_config **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(config, repo); - config = git_atomic_compare_and_swap(&repo->_config, NULL, config); - if (config != NULL) { + if (git_atomic_compare_and_swap(&repo->_config, NULL, config) != NULL) { GIT_REFCOUNT_OWN(config, NULL); git_config_free(config); } @@ -1164,8 +1163,7 @@ int git_repository_odb__weakptr(git_odb **out, git_repository *repo) return error; } - odb = git_atomic_compare_and_swap(&repo->_odb, NULL, odb); - if (odb != NULL) { + if (git_atomic_compare_and_swap(&repo->_odb, NULL, odb) != NULL) { GIT_REFCOUNT_OWN(odb, NULL); git_odb_free(odb); } @@ -1209,8 +1207,7 @@ int git_repository_refdb__weakptr(git_refdb **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(refdb, repo); - refdb = git_atomic_compare_and_swap(&repo->_refdb, NULL, refdb); - if (refdb != NULL) { + if (git_atomic_compare_and_swap(&repo->_refdb, NULL, refdb) != NULL) { GIT_REFCOUNT_OWN(refdb, NULL); git_refdb_free(refdb); } @@ -1257,8 +1254,7 @@ int git_repository_index__weakptr(git_index **out, git_repository *repo) if (!error) { GIT_REFCOUNT_OWN(index, repo); - index = git_atomic_compare_and_swap(&repo->_index, NULL, index); - if (index != NULL) { + if (git_atomic_compare_and_swap(&repo->_index, NULL, index) != NULL) { GIT_REFCOUNT_OWN(index, NULL); git_index_free(index); } diff --git a/src/thread.h b/src/thread.h index b4f869243..4b091c0a2 100644 --- a/src/thread.h +++ b/src/thread.h @@ -74,6 +74,9 @@ typedef git_atomic32 git_atomic_ssize; # include "unix/pthread.h" #endif +/* + * Atomically sets the contents of *a to be val. + */ GIT_INLINE(void) git_atomic32_set(git_atomic32 *a, int val) { #if defined(GIT_WIN32) @@ -87,6 +90,10 @@ GIT_INLINE(void) git_atomic32_set(git_atomic32 *a, int val) #endif } +/* + * Atomically increments the contents of *a by 1, and stores the result back into *a. + * @return the result of the operation. + */ GIT_INLINE(int) git_atomic32_inc(git_atomic32 *a) { #if defined(GIT_WIN32) @@ -100,10 +107,14 @@ GIT_INLINE(int) git_atomic32_inc(git_atomic32 *a) #endif } +/* + * Atomically adds the contents of *a and addend, and stores the result back into *a. + * @return the result of the operation. + */ GIT_INLINE(int) git_atomic32_add(git_atomic32 *a, int32_t addend) { #if defined(GIT_WIN32) - return InterlockedExchangeAdd(&a->val, addend); + return InterlockedAdd(&a->val, addend); #elif defined(GIT_BUILTIN_ATOMIC) return __atomic_add_fetch(&a->val, addend, __ATOMIC_SEQ_CST); #elif defined(GIT_BUILTIN_SYNC) @@ -113,6 +124,10 @@ GIT_INLINE(int) git_atomic32_add(git_atomic32 *a, int32_t addend) #endif } +/* + * Atomically decrements the contents of *a by 1, and stores the result back into *a. + * @return the result of the operation. + */ GIT_INLINE(int) git_atomic32_dec(git_atomic32 *a) { #if defined(GIT_WIN32) @@ -126,6 +141,10 @@ GIT_INLINE(int) git_atomic32_dec(git_atomic32 *a) #endif } +/* + * Atomically gets the contents of *a. + * @return the contents of *a. + */ GIT_INLINE(int) git_atomic32_get(git_atomic32 *a) { #if defined(GIT_WIN32) @@ -143,16 +162,13 @@ GIT_INLINE(void *) git_atomic__compare_and_swap( void * volatile *ptr, void *oldval, void *newval) { #if defined(GIT_WIN32) - volatile void *foundval; - foundval = InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval); - return (foundval == oldval) ? oldval : newval; + return InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval); #elif defined(GIT_BUILTIN_ATOMIC) - bool success = __atomic_compare_exchange(ptr, &oldval, &newval, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); - return success ? oldval : newval; + void *foundval = oldval; + __atomic_compare_exchange(ptr, &foundval, &newval, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); + return foundval; #elif defined(GIT_BUILTIN_SYNC) - volatile void *foundval; - foundval = __sync_val_compare_and_swap(ptr, oldval, newval); - return (foundval == oldval) ? oldval : newval; + return __sync_val_compare_and_swap(ptr, oldval, newval); #else # error "Unsupported architecture for atomic operations" #endif @@ -164,11 +180,11 @@ GIT_INLINE(volatile void *) git_atomic__swap( #if defined(GIT_WIN32) return InterlockedExchangePointer(ptr, newval); #elif defined(GIT_BUILTIN_ATOMIC) - void * volatile foundval; + void * volatile foundval = NULL; __atomic_exchange(ptr, &newval, &foundval, __ATOMIC_SEQ_CST); return foundval; #elif defined(GIT_BUILTIN_SYNC) - return __sync_lock_test_and_set(ptr, newval); + return (volatile void *)__sync_lock_test_and_set(ptr, newval); #else # error "Unsupported architecture for atomic operations" #endif @@ -178,9 +194,7 @@ GIT_INLINE(volatile void *) git_atomic__load(void * volatile *ptr) { #if defined(GIT_WIN32) void *newval = NULL, *oldval = NULL; - volatile void *foundval = NULL; - foundval = InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval); - return foundval; + return (volatile void *)InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval); #elif defined(GIT_BUILTIN_ATOMIC) return (volatile void *)__atomic_load_n(ptr, __ATOMIC_SEQ_CST); #elif defined(GIT_BUILTIN_SYNC) @@ -192,10 +206,14 @@ GIT_INLINE(volatile void *) git_atomic__load(void * volatile *ptr) #ifdef GIT_ARCH_64 +/* + * Atomically adds the contents of *a and addend, and stores the result back into *a. + * @return the result of the operation. + */ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) { #if defined(GIT_WIN32) - return InterlockedExchangeAdd64(&a->val, addend); + return InterlockedAdd64(&a->val, addend); #elif defined(GIT_BUILTIN_ATOMIC) return __atomic_add_fetch(&a->val, addend, __ATOMIC_SEQ_CST); #elif defined(GIT_BUILTIN_SYNC) @@ -205,6 +223,9 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) #endif } +/* + * Atomically sets the contents of *a to be val. + */ GIT_INLINE(void) git_atomic64_set(git_atomic64 *a, int64_t val) { #if defined(GIT_WIN32) @@ -218,6 +239,10 @@ GIT_INLINE(void) git_atomic64_set(git_atomic64 *a, int64_t val) #endif } +/* + * Atomically gets the contents of *a. + * @return the contents of *a. + */ GIT_INLINE(int64_t) git_atomic64_get(git_atomic64 *a) { #if defined(GIT_WIN32) @@ -297,11 +322,10 @@ GIT_INLINE(int) git_atomic32_get(git_atomic32 *a) GIT_INLINE(void *) git_atomic__compare_and_swap( void * volatile *ptr, void *oldval, void *newval) { - if (*ptr == oldval) + void *foundval = *ptr; + if (foundval == oldval) *ptr = newval; - else - oldval = newval; - return oldval; + return foundval; } GIT_INLINE(volatile void *) git_atomic__swap( @@ -339,17 +363,50 @@ GIT_INLINE(int64_t) git_atomic64_get(git_atomic64 *a) #endif -/* Atomically replace oldval with newval - * @return oldval if it was replaced or newval if it was not +/* + * Atomically replace the contents of *ptr (if they are equal to oldval) with + * newval. ptr must point to a pointer or a value that is the same size as a + * pointer. This is semantically compatible with: + * + * #define git_atomic_compare_and_swap(ptr, oldval, newval) \ + * ({ \ + * void *foundval = *ptr; \ + * if (foundval == oldval) \ + * *ptr = newval; \ + * foundval; \ + * }) + * + * @return the original contents of *ptr. */ -#define git_atomic_compare_and_swap(P,O,N) \ - git_atomic__compare_and_swap((void * volatile *)P, O, N) +#define git_atomic_compare_and_swap(ptr, oldval, newval) \ + git_atomic__compare_and_swap((void * volatile *)ptr, oldval, newval) -#define git_atomic_swap(ptr, val) \ - (void *)git_atomic__swap((void * volatile *)&ptr, val) +/* + * Atomically replace the contents of v with newval. v must be the same size as + * a pointer. This is semantically compatible with: + * + * #define git_atomic_swap(v, newval) \ + * ({ \ + * volatile void *old = v; \ + * v = newval; \ + * old; \ + * }) + * + * @return the original contents of v. + */ +#define git_atomic_swap(v, newval) \ + (void *)git_atomic__swap((void * volatile *)&(v), newval) -#define git_atomic_load(ptr) \ - (void *)git_atomic__load((void * volatile *)&ptr) +/* + * Atomically reads the contents of v. v must be the same size as a pointer. + * This is semantically compatible with: + * + * #define git_atomic_load(v) v + * + * @return the contents of v. + */ +#define git_atomic_load(v) \ + (void *)git_atomic__load((void * volatile *)&(v)) #if defined(GIT_THREADS) diff --git a/tests/threads/atomic.c b/tests/threads/atomic.c new file mode 100644 index 000000000..4d04a777a --- /dev/null +++ b/tests/threads/atomic.c @@ -0,0 +1,125 @@ +#include "clar_libgit2.h" + +void test_threads_atomic__atomic32_set(void) +{ + git_atomic32 v = {0}; + git_atomic32_set(&v, 1); + cl_assert_equal_i(v.val, 1); +} + +void test_threads_atomic__atomic32_get(void) +{ + git_atomic32 v = {1}; + cl_assert_equal_i(git_atomic32_get(&v), 1); +} + +void test_threads_atomic__atomic32_inc(void) +{ + git_atomic32 v = {0}; + cl_assert_equal_i(git_atomic32_inc(&v), 1); + cl_assert_equal_i(v.val, 1); +} + +void test_threads_atomic__atomic32_add(void) +{ + git_atomic32 v = {0}; + cl_assert_equal_i(git_atomic32_add(&v, 1), 1); + cl_assert_equal_i(v.val, 1); +} + +void test_threads_atomic__atomic32_dec(void) +{ + git_atomic32 v = {1}; + cl_assert_equal_i(git_atomic32_dec(&v), 0); + cl_assert_equal_i(v.val, 0); +} + +void test_threads_atomic__atomic64_set(void) +{ +#ifndef GIT_ARCH_64 + cl_skip(); +#else + git_atomic64 v = {0}; + git_atomic64_set(&v, 1); + cl_assert_equal_i(v.val, 1); +#endif +} + +void test_threads_atomic__atomic64_get(void) +{ +#ifndef GIT_ARCH_64 + cl_skip(); +#else + git_atomic64 v = {1}; + cl_assert_equal_i(git_atomic64_get(&v), 1); +#endif +} + +void test_threads_atomic__atomic64_add(void) +{ +#ifndef GIT_ARCH_64 + cl_skip(); +#else + git_atomic64 v = {0}; + cl_assert_equal_i(git_atomic64_add(&v, 1), 1); + cl_assert_equal_i(v.val, 1); +#endif +} + +void test_threads_atomic__cas_pointer(void) +{ + int *value = NULL; + int newvalue1 = 1, newvalue2 = 2; + + /* value is updated */ + cl_assert_equal_p(git_atomic_compare_and_swap(&value, NULL, &newvalue1), NULL); + cl_assert_equal_p(value, &newvalue1); + + /* value is not updated */ + cl_assert_equal_p(git_atomic_compare_and_swap(&value, NULL, &newvalue2), &newvalue1); + cl_assert_equal_p(value, &newvalue1); +} + +void test_threads_atomic__cas_intptr(void) +{ + intptr_t value = 0; + intptr_t oldvalue; + intptr_t newvalue; + + /* value is updated */ + oldvalue = 0; + newvalue = 1; + cl_assert_equal_i((intptr_t)git_atomic_compare_and_swap(&value, (void *)oldvalue, (void *)newvalue), 0); + cl_assert_equal_i(value, 1); + + /* value is not updated */ + oldvalue = 0; + newvalue = 2; + cl_assert_equal_i((intptr_t)git_atomic_compare_and_swap(&value, (void *)oldvalue, (void *)newvalue), 1); + cl_assert_equal_i(value, 1); +} + +void test_threads_atomic__swap(void) +{ + int *value = NULL; + int newvalue = 1; + + cl_assert_equal_p(git_atomic_swap(value, &newvalue), NULL); + cl_assert_equal_p(value, &newvalue); + + cl_assert_equal_p(git_atomic_swap(value, NULL), &newvalue); + cl_assert_equal_p(value, NULL); +} + +void test_threads_atomic__load_ptr(void) +{ + int value = 1; + int *ptr = &value; + cl_assert_equal_p(git_atomic_load(ptr), &value); +} + +void test_threads_atomic__load_intptr(void) +{ + intptr_t value = 1; + cl_assert_equal_i((intptr_t)git_atomic_load(value), 1); +} |