Homogenize semantics for atomic-related functions

There were some subtle semantic differences between the various
implementations of atomic functions. Now they behave the same, have
tests and are better documented to avoid this from happening again in
the future.

Of note:

* The semantics chosen for `git_atomic_compare_and_swap` match
  `InterlockedCompareExchangePointer`/`__sync_cal_compare_and_swap` now.
* The semantics chosen for `git_atomic_add` match
  `InterlockedAdd`/`__atomic_add_fetch`.
* `git_atomic_swap` and `git_atomic_load` still have a bit of semantic
  difference with the gcc builtins / msvc interlocked operations, since
  they require an l-value (not a pointer). If desired, this can be
  homogenized.

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