Merge pull request #5747 from lhchavez/atomic-tests

Homogenize semantics for atomic-related functions

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);

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(

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);
 			}

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)
 

tests/threads/atomic.c

+#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);
+}