Skip to content

Commit 74708a8

Browse files
lhchavezlhchavez
committed
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.
1 parent fabacb7 commit 74708a8

File tree

5 files changed

+234
-52
lines changed

5 files changed

+234
-52
lines changed

‎src/attrcache.c

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ static int attr_cache_remove(git_attr_cache *cache, git_attr_file *file)
127127
{
128128
int error = 0;
129129
git_attr_file_entry *entry;
130-
git_attr_file *old = NULL;
130+
git_attr_file *oldfile = NULL;
131131

132132
if (!file)
133133
return 0;
@@ -136,13 +136,13 @@ static int attr_cache_remove(git_attr_cache *cache, git_attr_file *file)
136136
return error;
137137

138138
if ((entry = attr_cache_lookup_entry(cache, file->entry->path)) != NULL)
139-
old = git_atomic_compare_and_swap(&entry->file[file->source.type], file, NULL);
139+
oldfile = git_atomic_compare_and_swap(&entry->file[file->source.type], file, NULL);
140140

141141
attr_cache_unlock(cache);
142142

143-
if (old) {
144-
GIT_REFCOUNT_OWN(old, NULL);
145-
git_attr_file__free(old);
143+
if (oldfile == file) {
144+
GIT_REFCOUNT_OWN(file, NULL);
145+
git_attr_file__free(file);
146146
}
147147

148148
return error;
@@ -401,8 +401,7 @@ int git_attr_cache__init(git_repository *repo)
401401
(ret = git_pool_init(&cache->pool, 1)) < 0)
402402
goto cancel;
403403

404-
cache = git_atomic_compare_and_swap(&repo->attrcache, NULL, cache);
405-
if (cache)
404+
if (git_atomic_compare_and_swap(&repo->attrcache, NULL, cache) != NULL)
406405
goto cancel; /* raced with another thread, free this but no error */
407406

408407
git_config_free(cfg);

‎src/diff_driver.c

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -141,18 +141,23 @@ static int diff_driver_funcname(const git_config_entry *entry, void *payload)
141141
static git_diff_driver_registry *git_repository_driver_registry(
142142
git_repository *repo)
143143
{
144-
if (!repo->diff_drivers) {
145-
git_diff_driver_registry *reg = git_diff_driver_registry_new();
146-
reg = git_atomic_compare_and_swap(&repo->diff_drivers, NULL, reg);
144+
git_diff_driver_registry *reg = git_atomic_load(repo->diff_drivers), *newreg;
145+
if (reg)
146+
return reg;
147147

148-
if (reg != NULL) /* if we race, free losing allocation */
149-
git_diff_driver_registry_free(reg);
150-
}
151-
152-
if (!repo->diff_drivers)
148+
newreg = git_diff_driver_registry_new();
149+
if (!newreg) {
153150
git_error_set(GIT_ERROR_REPOSITORY, "unable to create diff driver registry");
154-
155-
return repo->diff_drivers;
151+
return newreg;
152+
}
153+
reg = git_atomic_compare_and_swap(&repo->diff_drivers, NULL, newreg);
154+
if (!reg) {
155+
reg = newreg;
156+
} else {
157+
/* if we race, free losing allocation */
158+
git_diff_driver_registry_free(newreg);
159+
}
160+
return reg;
156161
}
157162

158163
static int diff_driver_alloc(

‎src/repository.c

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,8 +1093,7 @@ int git_repository_config__weakptr(git_config **out, git_repository *repo)
10931093
if (!error) {
10941094
GIT_REFCOUNT_OWN(config, repo);
10951095

1096-
config = git_atomic_compare_and_swap(&repo->_config, NULL, config);
1097-
if (config != NULL) {
1096+
if (git_atomic_compare_and_swap(&repo->_config, NULL, config) != NULL) {
10981097
GIT_REFCOUNT_OWN(config, NULL);
10991098
git_config_free(config);
11001099
}
@@ -1164,8 +1163,7 @@ int git_repository_odb__weakptr(git_odb **out, git_repository *repo)
11641163
return error;
11651164
}
11661165

1167-
odb = git_atomic_compare_and_swap(&repo->_odb, NULL, odb);
1168-
if (odb != NULL) {
1166+
if (git_atomic_compare_and_swap(&repo->_odb, NULL, odb) != NULL) {
11691167
GIT_REFCOUNT_OWN(odb, NULL);
11701168
git_odb_free(odb);
11711169
}
@@ -1209,8 +1207,7 @@ int git_repository_refdb__weakptr(git_refdb **out, git_repository *repo)
12091207
if (!error) {
12101208
GIT_REFCOUNT_OWN(refdb, repo);
12111209

1212-
refdb = git_atomic_compare_and_swap(&repo->_refdb, NULL, refdb);
1213-
if (refdb != NULL) {
1210+
if (git_atomic_compare_and_swap(&repo->_refdb, NULL, refdb) != NULL) {
12141211
GIT_REFCOUNT_OWN(refdb, NULL);
12151212
git_refdb_free(refdb);
12161213
}
@@ -1257,8 +1254,7 @@ int git_repository_index__weakptr(git_index **out, git_repository *repo)
12571254
if (!error) {
12581255
GIT_REFCOUNT_OWN(index, repo);
12591256

1260-
index = git_atomic_compare_and_swap(&repo->_index, NULL, index);
1261-
if (index != NULL) {
1257+
if (git_atomic_compare_and_swap(&repo->_index, NULL, index) != NULL) {
12621258
GIT_REFCOUNT_OWN(index, NULL);
12631259
git_index_free(index);
12641260
}

‎src/thread.h

Lines changed: 84 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ typedef git_atomic32 git_atomic_ssize;
7474
# include "unix/pthread.h"
7575
#endif
7676

77+
/*
78+
* Atomically sets the contents of *a to be val.
79+
*/
7780
GIT_INLINE(void) git_atomic32_set(git_atomic32 *a, int val)
7881
{
7982
#if defined(GIT_WIN32)
@@ -87,6 +90,10 @@ GIT_INLINE(void) git_atomic32_set(git_atomic32 *a, int val)
8790
#endif
8891
}
8992

93+
/*
94+
* Atomically increments the contents of *a by 1, and stores the result back into *a.
95+
* @return the result of the operation.
96+
*/
9097
GIT_INLINE(int) git_atomic32_inc(git_atomic32 *a)
9198
{
9299
#if defined(GIT_WIN32)
@@ -100,10 +107,14 @@ GIT_INLINE(int) git_atomic32_inc(git_atomic32 *a)
100107
#endif
101108
}
102109

110+
/*
111+
* Atomically adds the contents of *a and addend, and stores the result back into *a.
112+
* @return the result of the operation.
113+
*/
103114
GIT_INLINE(int) git_atomic32_add(git_atomic32 *a, int32_t addend)
104115
{
105116
#if defined(GIT_WIN32)
106-
return InterlockedExchangeAdd(&a->val, addend);
117+
return InterlockedAdd(&a->val, addend);
107118
#elif defined(GIT_BUILTIN_ATOMIC)
108119
return __atomic_add_fetch(&a->val, addend, __ATOMIC_SEQ_CST);
109120
#elif defined(GIT_BUILTIN_SYNC)
@@ -113,6 +124,10 @@ GIT_INLINE(int) git_atomic32_add(git_atomic32 *a, int32_t addend)
113124
#endif
114125
}
115126

127+
/*
128+
* Atomically decrements the contents of *a by 1, and stores the result back into *a.
129+
* @return the result of the operation.
130+
*/
116131
GIT_INLINE(int) git_atomic32_dec(git_atomic32 *a)
117132
{
118133
#if defined(GIT_WIN32)
@@ -126,6 +141,10 @@ GIT_INLINE(int) git_atomic32_dec(git_atomic32 *a)
126141
#endif
127142
}
128143

144+
/*
145+
* Atomically gets the contents of *a.
146+
* @return the contents of *a.
147+
*/
129148
GIT_INLINE(int) git_atomic32_get(git_atomic32 *a)
130149
{
131150
#if defined(GIT_WIN32)
@@ -143,16 +162,13 @@ GIT_INLINE(void *) git_atomic__compare_and_swap(
143162
void * volatile *ptr, void *oldval, void *newval)
144163
{
145164
#if defined(GIT_WIN32)
146-
volatile void *foundval;
147-
foundval = InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval);
148-
return (foundval == oldval) ? oldval : newval;
165+
return InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval);
149166
#elif defined(GIT_BUILTIN_ATOMIC)
150-
bool success = __atomic_compare_exchange(ptr, &oldval, &newval, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
151-
return success ? oldval : newval;
167+
void *foundval = oldval;
168+
__atomic_compare_exchange(ptr, &foundval, &newval, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
169+
return foundval;
152170
#elif defined(GIT_BUILTIN_SYNC)
153-
volatile void *foundval;
154-
foundval = __sync_val_compare_and_swap(ptr, oldval, newval);
155-
return (foundval == oldval) ? oldval : newval;
171+
return __sync_val_compare_and_swap(ptr, oldval, newval);
156172
#else
157173
# error "Unsupported architecture for atomic operations"
158174
#endif
@@ -164,11 +180,11 @@ GIT_INLINE(volatile void *) git_atomic__swap(
164180
#if defined(GIT_WIN32)
165181
return InterlockedExchangePointer(ptr, newval);
166182
#elif defined(GIT_BUILTIN_ATOMIC)
167-
void * volatile foundval;
183+
void * volatile foundval = NULL;
168184
__atomic_exchange(ptr, &newval, &foundval, __ATOMIC_SEQ_CST);
169185
return foundval;
170186
#elif defined(GIT_BUILTIN_SYNC)
171-
return __sync_lock_test_and_set(ptr, newval);
187+
return (volatile void *)__sync_lock_test_and_set(ptr, newval);
172188
#else
173189
# error "Unsupported architecture for atomic operations"
174190
#endif
@@ -178,9 +194,7 @@ GIT_INLINE(volatile void *) git_atomic__load(void * volatile *ptr)
178194
{
179195
#if defined(GIT_WIN32)
180196
void *newval = NULL, *oldval = NULL;
181-
volatile void *foundval = NULL;
182-
foundval = InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval);
183-
return foundval;
197+
return (volatile void *)InterlockedCompareExchangePointer((volatile PVOID *)ptr, newval, oldval);
184198
#elif defined(GIT_BUILTIN_ATOMIC)
185199
return (volatile void *)__atomic_load_n(ptr, __ATOMIC_SEQ_CST);
186200
#elif defined(GIT_BUILTIN_SYNC)
@@ -192,10 +206,14 @@ GIT_INLINE(volatile void *) git_atomic__load(void * volatile *ptr)
192206

193207
#ifdef GIT_ARCH_64
194208

209+
/*
210+
* Atomically adds the contents of *a and addend, and stores the result back into *a.
211+
* @return the result of the operation.
212+
*/
195213
GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend)
196214
{
197215
#if defined(GIT_WIN32)
198-
return InterlockedExchangeAdd64(&a->val, addend);
216+
return InterlockedAdd64(&a->val, addend);
199217
#elif defined(GIT_BUILTIN_ATOMIC)
200218
return __atomic_add_fetch(&a->val, addend, __ATOMIC_SEQ_CST);
201219
#elif defined(GIT_BUILTIN_SYNC)
@@ -205,6 +223,9 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend)
205223
#endif
206224
}
207225

226+
/*
227+
* Atomically sets the contents of *a to be val.
228+
*/
208229
GIT_INLINE(void) git_atomic64_set(git_atomic64 *a, int64_t val)
209230
{
210231
#if defined(GIT_WIN32)
@@ -218,6 +239,10 @@ GIT_INLINE(void) git_atomic64_set(git_atomic64 *a, int64_t val)
218239
#endif
219240
}
220241

242+
/*
243+
* Atomically gets the contents of *a.
244+
* @return the contents of *a.
245+
*/
221246
GIT_INLINE(int64_t) git_atomic64_get(git_atomic64 *a)
222247
{
223248
#if defined(GIT_WIN32)
@@ -297,11 +322,10 @@ GIT_INLINE(int) git_atomic32_get(git_atomic32 *a)
297322
GIT_INLINE(void *) git_atomic__compare_and_swap(
298323
void * volatile *ptr, void *oldval, void *newval)
299324
{
300-
if (*ptr == oldval)
325+
void *foundval = *ptr;
326+
if (foundval == oldval)
301327
*ptr = newval;
302-
else
303-
oldval = newval;
304-
return oldval;
328+
return foundval;
305329
}
306330

307331
GIT_INLINE(volatile void *) git_atomic__swap(
@@ -339,17 +363,50 @@ GIT_INLINE(int64_t) git_atomic64_get(git_atomic64 *a)
339363

340364
#endif
341365

342-
/* Atomically replace oldval with newval
343-
* @return oldval if it was replaced or newval if it was not
366+
/*
367+
* Atomically replace the contents of *ptr (if they are equal to oldval) with
368+
* newval. ptr must point to a pointer or a value that is the same size as a
369+
* pointer. This is semantically compatible with:
370+
*
371+
* #define git_atomic_compare_and_swap(ptr, oldval, newval) \
372+
* ({ \
373+
* void *foundval = *ptr; \
374+
* if (foundval == oldval) \
375+
* *ptr = newval; \
376+
* foundval; \
377+
* })
378+
*
379+
* @return the original contents of *ptr.
344380
*/
345-
#define git_atomic_compare_and_swap(P,O,N) \
346-
git_atomic__compare_and_swap((void * volatile *)P, O, N)
381+
#define git_atomic_compare_and_swap(ptr, oldval, newval) \
382+
git_atomic__compare_and_swap((void * volatile *)ptr, oldval, newval)
347383

348-
#define git_atomic_swap(ptr, val) \
349-
(void *)git_atomic__swap((void * volatile *)&ptr, val)
384+
/*
385+
* Atomically replace the contents of v with newval. v must be the same size as
386+
* a pointer. This is semantically compatible with:
387+
*
388+
* #define git_atomic_swap(v, newval) \
389+
* ({ \
390+
* volatile void *old = v; \
391+
* v = newval; \
392+
* old; \
393+
* })
394+
*
395+
* @return the original contents of v.
396+
*/
397+
#define git_atomic_swap(v, newval) \
398+
(void *)git_atomic__swap((void * volatile *)&(v), newval)
350399

351-
#define git_atomic_load(ptr) \
352-
(void *)git_atomic__load((void * volatile *)&ptr)
400+
/*
401+
* Atomically reads the contents of v. v must be the same size as a pointer.
402+
* This is semantically compatible with:
403+
*
404+
* #define git_atomic_load(v) v
405+
*
406+
* @return the contents of v.
407+
*/
408+
#define git_atomic_load(v) \
409+
(void *)git_atomic__load((void * volatile *)&(v))
353410

354411
#if defined(GIT_THREADS)
355412

0 commit comments

Comments
 (0)