From 814de0bcab99f82dc637ba7ae34df5a0e9e1138c Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 11 Jul 2013 11:00:41 -0700 Subject: [PATCH 1/2] Update git__swap thread helper This makes git__swap use the __sync_lock_test_and_set primitive with GCC and the InterlockedExchangePointer primitive with MSVC. Previously is used compare_and_swap in a way that was probably unintuitive for most thinking (i.e. it could fail to swap in the value if another thread raced in). Now it will always succeed and the last thread to run in a race will win instead of the first thread. This also fixes up a little confusion between volatile void ** and void * volatile * that came up with the Win32 compiler. --- src/thread-utils.h | 57 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/src/thread-utils.h b/src/thread-utils.h index f8c4dc66d..04e02959f 100644 --- a/src/thread-utils.h +++ b/src/thread-utils.h @@ -38,16 +38,6 @@ typedef git_atomic git_atomic_ssize; #endif -GIT_INLINE(void) git_atomic_set(git_atomic *a, int val) -{ - a->val = val; -} - -GIT_INLINE(int) git_atomic_get(git_atomic *a) -{ - return (int)a->val; -} - #ifdef GIT_THREADS #define git_thread pthread_t @@ -71,6 +61,17 @@ GIT_INLINE(int) git_atomic_get(git_atomic *a) #define git_cond_signal(c) pthread_cond_signal(c) #define git_cond_broadcast(c) pthread_cond_broadcast(c) +GIT_INLINE(void) git_atomic_set(git_atomic *a, int val) +{ +#if defined(GIT_WIN32) + InterlockedExchange(&a->val, (LONG)val); +#elif defined(__GNUC__) + __sync_lock_test_and_set(&a->val, val); +#else +# error "Unsupported architecture for atomic operations" +#endif +} + GIT_INLINE(int) git_atomic_inc(git_atomic *a) { #if defined(GIT_WIN32) @@ -105,7 +106,7 @@ GIT_INLINE(int) git_atomic_dec(git_atomic *a) } GIT_INLINE(void *) git___compare_and_swap( - volatile void **ptr, void *oldval, void *newval) + void * volatile *ptr, void *oldval, void *newval) { volatile void *foundval; #if defined(GIT_WIN32) @@ -118,6 +119,16 @@ GIT_INLINE(void *) git___compare_and_swap( return (foundval == oldval) ? oldval : newval; } +GIT_INLINE(volatile void *) git___swap( + void * volatile *ptr, void *newval) +{ +#if defined(GIT_WIN32) + return InterlockedExchangePointer(ptr, newval); +#else + return __sync_lock_test_and_set(ptr, newval); +#endif +} + #ifdef GIT_ARCH_64 GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) @@ -156,6 +167,11 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) #define git_cond_signal(c) (void)0 #define git_cond_broadcast(c) (void)0 +GIT_INLINE(void) git_atomic_set(git_atomic *a, int val) +{ + a->val = val; +} + GIT_INLINE(int) git_atomic_inc(git_atomic *a) { return ++a->val; @@ -173,7 +189,7 @@ GIT_INLINE(int) git_atomic_dec(git_atomic *a) } GIT_INLINE(void *) git___compare_and_swap( - volatile void **ptr, void *oldval, void *newval) + void * volatile *ptr, void *oldval, void *newval) { if (*ptr == oldval) *ptr = newval; @@ -182,6 +198,14 @@ GIT_INLINE(void *) git___compare_and_swap( return oldval; } +GIT_INLINE(volatile void *) git___swap( + void * volatile *ptr, void *newval) +{ + volatile void *old = *ptr; + *ptr = newval; + return old; +} + #ifdef GIT_ARCH_64 GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) @@ -194,13 +218,18 @@ GIT_INLINE(int64_t) git_atomic64_add(git_atomic64 *a, int64_t addend) #endif +GIT_INLINE(int) git_atomic_get(git_atomic *a) +{ + return (int)a->val; +} + /* Atomically replace oldval with newval * @return oldval if it was replaced or newval if it was not */ #define git__compare_and_swap(P,O,N) \ - git___compare_and_swap((volatile void **)P, O, N) + git___compare_and_swap((void * volatile *)P, O, N) -#define git__swap(ptr, val) git__compare_and_swap(&ptr, ptr, val) +#define git__swap(ptr, val) (void *)git___swap((void * volatile *)&ptr, val) extern int git_online_cpus(void); From 584f2d3013df6744fa7b5c5398a78b96f31e25f4 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Thu, 11 Jul 2013 11:04:42 -0700 Subject: [PATCH 2/2] Fix warnings on Win64 --- src/commit.c | 3 ++- src/diff_driver.c | 2 +- tests-clar/diff/pathspec.c | 14 +++++++------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/commit.c b/src/commit.c index cc912a7be..15a195fe5 100644 --- a/src/commit.c +++ b/src/commit.c @@ -163,7 +163,8 @@ int git_commit__parse(void *_commit, git_odb_object *odb_obj) const char *buffer_start = git_odb_object_data(odb_obj), *buffer; const char *buffer_end = buffer_start + git_odb_object_size(odb_obj); git_oid parent_id; - size_t parent_count = 0, header_len; + uint32_t parent_count = 0; + size_t header_len; /* find end-of-header (counting parents as we go) */ for (buffer = buffer_start; buffer < buffer_end; ++buffer) { diff --git a/src/diff_driver.c b/src/diff_driver.c index 77b0e9f3e..e82dfa50d 100644 --- a/src/diff_driver.c +++ b/src/diff_driver.c @@ -374,7 +374,7 @@ static long diff_context_find( return -1; if (out_size > (long)ctxt->line.size) - out_size = ctxt->line.size; + out_size = (long)ctxt->line.size; memcpy(out, ctxt->line.ptr, (size_t)out_size); return out_size; diff --git a/tests-clar/diff/pathspec.c b/tests-clar/diff/pathspec.c index 332b513b3..4334a8901 100644 --- a/tests-clar/diff/pathspec.c +++ b/tests-clar/diff/pathspec.c @@ -34,7 +34,7 @@ void test_diff_pathspec__0(void) cl_git_pass(git_pathspec_new(&ps, &paths)); cl_git_pass(git_pathspec_match_tree(&matches, a, GIT_PATHSPEC_DEFAULT, ps)); - cl_assert_equal_i(7, git_pathspec_match_list_entrycount(matches)); + cl_assert_equal_i(7, (int)git_pathspec_match_list_entrycount(matches)); cl_assert_equal_s("current_file", git_pathspec_match_list_entry(matches,0)); cl_assert(git_pathspec_match_list_diff_entry(matches,0) == NULL); git_pathspec_match_list_free(matches); @@ -43,13 +43,13 @@ void test_diff_pathspec__0(void) cl_git_pass(git_pathspec_match_diff( &matches, diff, GIT_PATHSPEC_DEFAULT, ps)); - cl_assert_equal_i(7, git_pathspec_match_list_entrycount(matches)); + cl_assert_equal_i(7, (int)git_pathspec_match_list_entrycount(matches)); cl_assert(git_pathspec_match_list_diff_entry(matches, 0) != NULL); cl_assert(git_pathspec_match_list_entry(matches, 0) == NULL); cl_assert_equal_s("current_file", git_pathspec_match_list_diff_entry(matches,0)->new_file.path); cl_assert_equal_i(GIT_DELTA_ADDED, - git_pathspec_match_list_diff_entry(matches,0)->status); + (int)git_pathspec_match_list_diff_entry(matches,0)->status); git_pathspec_match_list_free(matches); git_diff_list_free(diff); @@ -59,13 +59,13 @@ void test_diff_pathspec__0(void) cl_git_pass(git_pathspec_match_diff( &matches, diff, GIT_PATHSPEC_DEFAULT, ps)); - cl_assert_equal_i(3, git_pathspec_match_list_entrycount(matches)); + cl_assert_equal_i(3, (int)git_pathspec_match_list_entrycount(matches)); cl_assert(git_pathspec_match_list_diff_entry(matches, 0) != NULL); cl_assert(git_pathspec_match_list_entry(matches, 0) == NULL); cl_assert_equal_s("subdir/current_file", git_pathspec_match_list_diff_entry(matches,0)->new_file.path); cl_assert_equal_i(GIT_DELTA_DELETED, - git_pathspec_match_list_diff_entry(matches,0)->status); + (int)git_pathspec_match_list_diff_entry(matches,0)->status); git_pathspec_match_list_free(matches); git_diff_list_free(diff); @@ -75,13 +75,13 @@ void test_diff_pathspec__0(void) cl_git_pass(git_pathspec_match_diff( &matches, diff, GIT_PATHSPEC_DEFAULT, ps)); - cl_assert_equal_i(4, git_pathspec_match_list_entrycount(matches)); + cl_assert_equal_i(4, (int)git_pathspec_match_list_entrycount(matches)); cl_assert(git_pathspec_match_list_diff_entry(matches, 0) != NULL); cl_assert(git_pathspec_match_list_entry(matches, 0) == NULL); cl_assert_equal_s("modified_file", git_pathspec_match_list_diff_entry(matches,0)->new_file.path); cl_assert_equal_i(GIT_DELTA_MODIFIED, - git_pathspec_match_list_diff_entry(matches,0)->status); + (int)git_pathspec_match_list_diff_entry(matches,0)->status); git_pathspec_match_list_free(matches); git_diff_list_free(diff);