Re: [PATCH 1/5] add SWAP macro

["René Scharfe" <l.s.r@web.de>]

Am 30.04.2017 um 05:11 schrieb Jeff King:
> On Sat, Apr 29, 2017 at 08:16:17PM +0200, René Scharfe wrote:
> 
>>> I dunno. I could go either way. Or we could leave it as-is, and let
>>> valgrind find the problem. That has zero run-time cost, but of course
>>> nobody bothers to run valgrind outside of the test suite, so the inputs
>>> are not usually very exotic.
>>
>> It would be  problematic on platforms where memcpy has to erase the
>> destination before writing new values (I don't know any example).
>>
>> We could use two temporary buffers.  The object code is the same with
>> GCC around 5 and Clang 3.2 or higher -- at least for prio-queue.c.
> 
> Hmm, yeah, that's an easy solution that covers the overlap case, too. If
> the generated code is the same, that seems like it might not be bad (I
> am a little sad how complex this simple swap operation is getting,
> though).

Patch below.

All is well if the compiler can (and is allowed to) see through the
memcpy calls, otherwise we get a performance hit and this change makes
that slow path slower.  FWIW, I didn't see any slowdown in our perf
tests, though.

It's not too late to switch to a macro that takes a type parameter, as
Dscho suggested earlier:

  #define swap_t(T, a, b) do {T t_ = (a); (a) = (b); (b) = t_;} while (0)

Much simpler and with full type check, but harder to use (needs correct
type parameter and its other parameters must not have side effects).

-- >8 --
Subject: [PATCH] avoid calling memcpy on overlapping objects in SWAP

Copy both objects into their own buffers before swapping to make sure we
don't call memcpy with overlapping memory ranges, as that's undefined.
Compilers that inline the memcpy calls optimize the extra operations
away.  That allows calls like SWAP(x, x) without ill effect and without
extra checks.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 git-compat-util.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index bd04564..a843f04 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -528,13 +528,15 @@ static inline int ends_with(const char *str, const char *suffix)
 }
 
 #define SWAP(a, b) do {						\
-	void *_swap_a_ptr = &(a);				\
-	void *_swap_b_ptr = &(b);				\
-	unsigned char _swap_buffer[sizeof(a)];			\
-	memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));		\
-	memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +		\
-	       BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));	\
-	memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));		\
+	void *swap_a_ptr_ = &(a);				\
+	void *swap_b_ptr_ = &(b);				\
+	unsigned char swap_a_buffer_[sizeof(a)];		\
+	unsigned char swap_b_buffer_[sizeof(a) +		\
+		BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b))];	\
+	memcpy(swap_a_buffer_, swap_a_ptr_, sizeof(a));		\
+	memcpy(swap_b_buffer_, swap_b_ptr_, sizeof(a));		\
+	memcpy(swap_a_ptr_, swap_b_buffer_, sizeof(a));		\
+	memcpy(swap_b_ptr_, swap_a_buffer_, sizeof(a));		\
 } while (0)
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
2.1.4