Re: [PATCH 1/5] add SWAP macro
- From
René Scharfe <l.s.r@web.de>
- Date
- May 2, 2017, 05:29 UTC
- Message-ID
- <57741b45-81b4-5940-3ffb-10c3b067cce3@web.de>
- In-Reply-To
- <20170430031154.qurrbdb3wqrdxd37@sigill.intra.peff.net>
Am 30.04.2017 um 05:11 schrieb Jeff King:
Show 17 quoted lines
> 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