* [PATCH] bswap: convert to unsigned before shifting in get_be32
@ 2017-07-15 19:11 René Scharfe
2017-07-15 19:22 ` [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions René Scharfe
2017-07-16 0:23 ` [PATCH] bswap: convert to unsigned before shifting in get_be32 Ramsay Jones
0 siblings, 2 replies; 5+ messages in thread
From: René Scharfe @ 2017-07-15 19:11 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
The pointer p is dereferenced and we get an unsigned char. Before
shifting it's automatically promoted to int. Left-shifting a signed
32-bit value bigger than 127 by 24 places is undefined. Explicitly
convert to a 32-bit unsigned type to avoid undefined behaviour if
the highest bit is set.
Found with Clang's UBSan.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
compat/bswap.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/compat/bswap.h b/compat/bswap.h
index d47c003544..4582c1107a 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -166,10 +166,10 @@ static inline uint64_t git_bswap64(uint64_t x)
(*((unsigned char *)(p) + 0) << 8) | \
(*((unsigned char *)(p) + 1) << 0) )
#define get_be32(p) ( \
- (*((unsigned char *)(p) + 0) << 24) | \
- (*((unsigned char *)(p) + 1) << 16) | \
- (*((unsigned char *)(p) + 2) << 8) | \
- (*((unsigned char *)(p) + 3) << 0) )
+ ((uint32_t)*((unsigned char *)(p) + 0) << 24) | \
+ ((uint32_t)*((unsigned char *)(p) + 1) << 16) | \
+ ((uint32_t)*((unsigned char *)(p) + 2) << 8) | \
+ ((uint32_t)*((unsigned char *)(p) + 3) << 0) )
#define put_be32(p, v) do { \
unsigned int __v = (v); \
*((unsigned char *)(p) + 0) = __v >> 24; \
--
2.13.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions
2017-07-15 19:11 [PATCH] bswap: convert to unsigned before shifting in get_be32 René Scharfe
@ 2017-07-15 19:22 ` René Scharfe
2017-07-16 10:27 ` Jeff King
2017-07-16 0:23 ` [PATCH] bswap: convert to unsigned before shifting in get_be32 Ramsay Jones
1 sibling, 1 reply; 5+ messages in thread
From: René Scharfe @ 2017-07-15 19:22 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
Simplify the implementation and allow callers to use expressions with
side-effects by turning the macros get_be16, get_be32 and put_be32 into
inline functions.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
All these redundant casts started to bother me, so I tried to come up
with nice and clean inline functions. Successfully? You tell me.
They are longer, but less cluttered. Would it punish -O0 builds? Is
it all worth it?
compat/bswap.h | 38 ++++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/compat/bswap.h b/compat/bswap.h
index 4582c1107a..7d063e9e40 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -162,19 +162,29 @@ static inline uint64_t git_bswap64(uint64_t x)
#else
-#define get_be16(p) ( \
- (*((unsigned char *)(p) + 0) << 8) | \
- (*((unsigned char *)(p) + 1) << 0) )
-#define get_be32(p) ( \
- ((uint32_t)*((unsigned char *)(p) + 0) << 24) | \
- ((uint32_t)*((unsigned char *)(p) + 1) << 16) | \
- ((uint32_t)*((unsigned char *)(p) + 2) << 8) | \
- ((uint32_t)*((unsigned char *)(p) + 3) << 0) )
-#define put_be32(p, v) do { \
- unsigned int __v = (v); \
- *((unsigned char *)(p) + 0) = __v >> 24; \
- *((unsigned char *)(p) + 1) = __v >> 16; \
- *((unsigned char *)(p) + 2) = __v >> 8; \
- *((unsigned char *)(p) + 3) = __v >> 0; } while (0)
+static inline uint16_t get_be16(const void *ptr)
+{
+ const unsigned char *p = ptr;
+ return (uint16_t)p[0] << 8 |
+ (uint16_t)p[1] << 0;
+}
+
+static inline uint32_t get_be32(const void *ptr)
+{
+ const unsigned char *p = ptr;
+ return (uint32_t)p[0] << 24 |
+ (uint32_t)p[1] << 16 |
+ (uint32_t)p[2] << 8 |
+ (uint32_t)p[3] << 0;
+}
+
+static inline void put_be32(void *ptr, uint32_t value)
+{
+ unsigned char *p = ptr;
+ p[0] = value >> 24;
+ p[1] = value >> 16;
+ p[2] = value >> 8;
+ p[3] = value >> 0;
+}
#endif
--
2.13.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] bswap: convert to unsigned before shifting in get_be32
2017-07-15 19:11 [PATCH] bswap: convert to unsigned before shifting in get_be32 René Scharfe
2017-07-15 19:22 ` [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions René Scharfe
@ 2017-07-16 0:23 ` Ramsay Jones
1 sibling, 0 replies; 5+ messages in thread
From: Ramsay Jones @ 2017-07-16 0:23 UTC (permalink / raw)
To: René Scharfe, Git List; +Cc: Junio C Hamano, Jeff King
On 15/07/17 20:11, René Scharfe wrote:
> The pointer p is dereferenced and we get an unsigned char. Before
> shifting it's automatically promoted to int. Left-shifting a signed
> 32-bit value bigger than 127 by 24 places is undefined. Explicitly
> convert to a 32-bit unsigned type to avoid undefined behaviour if
> the highest bit is set.
>
> Found with Clang's UBSan.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> compat/bswap.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/compat/bswap.h b/compat/bswap.h
> index d47c003544..4582c1107a 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -166,10 +166,10 @@ static inline uint64_t git_bswap64(uint64_t x)
> (*((unsigned char *)(p) + 0) << 8) | \
> (*((unsigned char *)(p) + 1) << 0) )
> #define get_be32(p) ( \
> - (*((unsigned char *)(p) + 0) << 24) | \
> - (*((unsigned char *)(p) + 1) << 16) | \
> - (*((unsigned char *)(p) + 2) << 8) | \
> - (*((unsigned char *)(p) + 3) << 0) )
> + ((uint32_t)*((unsigned char *)(p) + 0) << 24) | \
> + ((uint32_t)*((unsigned char *)(p) + 1) << 16) | \
> + ((uint32_t)*((unsigned char *)(p) + 2) << 8) | \
> + ((uint32_t)*((unsigned char *)(p) + 3) << 0) )
> #define put_be32(p, v) do { \
> unsigned int __v = (v); \
> *((unsigned char *)(p) + 0) = __v >> 24; \
>
Heh, I have a patch that is pretty much identical. I suspect
you can guess why. ;-)
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions
2017-07-15 19:22 ` [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions René Scharfe
@ 2017-07-16 10:27 ` Jeff King
2017-07-16 10:28 ` Jeff King
0 siblings, 1 reply; 5+ messages in thread
From: Jeff King @ 2017-07-16 10:27 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
On Sat, Jul 15, 2017 at 09:22:50PM +0200, René Scharfe wrote:
> Simplify the implementation and allow callers to use expressions with
> side-effects by turning the macros get_be16, get_be32 and put_be32 into
> inline functions.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> All these redundant casts started to bother me, so I tried to come up
> with nice and clean inline functions. Successfully? You tell me.
> They are longer, but less cluttered. Would it punish -O0 builds? Is
> it all worth it?
I do think the end result is a lot more readable. On gcc 6 at least, the
function seems[1] to end up inlined even with -O0.
Interestingly, at -O2 even with -DNO_UNALIGNED_LOADS, gcc converts the
result to a movl and a bswap. Which is the same thing our
unaligned-loads path is trying for. I wonder if we could/should just
drop it (that _would_ punish -O0 on x86, though).
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions
2017-07-16 10:27 ` Jeff King
@ 2017-07-16 10:28 ` Jeff King
0 siblings, 0 replies; 5+ messages in thread
From: Jeff King @ 2017-07-16 10:28 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
On Sun, Jul 16, 2017 at 06:27:04AM -0400, Jeff King wrote:
> On Sat, Jul 15, 2017 at 09:22:50PM +0200, René Scharfe wrote:
>
> > Simplify the implementation and allow callers to use expressions with
> > side-effects by turning the macros get_be16, get_be32 and put_be32 into
> > inline functions.
> >
> > Signed-off-by: Rene Scharfe <l.s.r@web.de>
> > ---
> > All these redundant casts started to bother me, so I tried to come up
> > with nice and clean inline functions. Successfully? You tell me.
> > They are longer, but less cluttered. Would it punish -O0 builds? Is
> > it all worth it?
>
> I do think the end result is a lot more readable. On gcc 6 at least, the
> function seems[1] to end up inlined even with -O0.
For my footnote. I was just going to show the test file I compiled:
#include "git-compat-util.h"
uint32_t foo(const char *x)
{
return get_be32(x);
}
It's possible the optimizer may behave differently on a more complicated
input, but it does show that -O0 is still willing to inline.
-Peff
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-07-16 10:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-15 19:11 [PATCH] bswap: convert to unsigned before shifting in get_be32 René Scharfe
2017-07-15 19:22 ` [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions René Scharfe
2017-07-16 10:27 ` Jeff King
2017-07-16 10:28 ` Jeff King
2017-07-16 0:23 ` [PATCH] bswap: convert to unsigned before shifting in get_be32 Ramsay Jones
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).