{"thread":{"id":"46394","subject":"[PATCH] bswap: convert to unsigned before shifting in get_be32","startedAt":"2017-07-15T19:12:58Z","lastAt":"2017-07-16T10:28:22Z","messageCount":5,"participants":["René Scharfe","Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324585","messageId":"56f1263c-a0b4-260b-7e23-881e21119041@web.de","threadId":"46394","inReplyTo":null,"subject":"[PATCH] bswap: convert to unsigned before shifting in get_be32","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-15T19:11:14Z","receivedAt":"2017-07-15T19:12:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The pointer p is dereferenced and we get an unsigned char.  Before\nshifting it's automatically promoted to int.  Left-shifting a signed\n32-bit value bigger than 127 by 24 places is undefined.  Explicitly\nconvert to a 32-bit unsigned type to avoid undefined behaviour if\nthe highest bit is set.\n\nFound with Clang's UBSan.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n compat/bswap.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex d47c003544..4582c1107a 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -166,10 +166,10 @@ static inline uint64_t git_bswap64(uint64_t x)\n \t(*((unsigned char *)(p) + 0) << 8) | \\\n \t(*((unsigned char *)(p) + 1) << 0) )\n #define get_be32(p)\t( \\\n-\t(*((unsigned char *)(p) + 0) << 24) | \\\n-\t(*((unsigned char *)(p) + 1) << 16) | \\\n-\t(*((unsigned char *)(p) + 2) <<  8) | \\\n-\t(*((unsigned char *)(p) + 3) <<  0) )\n+\t((uint32_t)*((unsigned char *)(p) + 0) << 24) | \\\n+\t((uint32_t)*((unsigned char *)(p) + 1) << 16) | \\\n+\t((uint32_t)*((unsigned char *)(p) + 2) <<  8) | \\\n+\t((uint32_t)*((unsigned char *)(p) + 3) <<  0) )\n #define put_be32(p, v)\tdo { \\\n \tunsigned int __v = (v); \\\n \t*((unsigned char *)(p) + 0) = __v >> 24; \\\n-- \n2.13.3\n"},{"id":"324586","messageId":"06c5bc39-d2c4-3436-97a7-9faa7c0519ca@web.de","threadId":"46394","inReplyTo":"56f1263c-a0b4-260b-7e23-881e21119041@web.de","subject":"[PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-15T19:22:50Z","receivedAt":"2017-07-15T19:23:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Simplify the implementation and allow callers to use expressions with\nside-effects by turning the macros get_be16, get_be32 and put_be32 into\ninline functions.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nAll these redundant casts started to bother me, so I tried to come up\nwith nice and clean inline functions.  Successfully?  You tell me.\nThey are longer, but less cluttered.  Would it punish -O0 builds?  Is\nit all worth it?\n\n compat/bswap.h | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex 4582c1107a..7d063e9e40 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -162,19 +162,29 @@ static inline uint64_t git_bswap64(uint64_t x)\n \n #else\n \n-#define get_be16(p)\t( \\\n-\t(*((unsigned char *)(p) + 0) << 8) | \\\n-\t(*((unsigned char *)(p) + 1) << 0) )\n-#define get_be32(p)\t( \\\n-\t((uint32_t)*((unsigned char *)(p) + 0) << 24) | \\\n-\t((uint32_t)*((unsigned char *)(p) + 1) << 16) | \\\n-\t((uint32_t)*((unsigned char *)(p) + 2) <<  8) | \\\n-\t((uint32_t)*((unsigned char *)(p) + 3) <<  0) )\n-#define put_be32(p, v)\tdo { \\\n-\tunsigned int __v = (v); \\\n-\t*((unsigned char *)(p) + 0) = __v >> 24; \\\n-\t*((unsigned char *)(p) + 1) = __v >> 16; \\\n-\t*((unsigned char *)(p) + 2) = __v >>  8; \\\n-\t*((unsigned char *)(p) + 3) = __v >>  0; } while (0)\n+static inline uint16_t get_be16(const void *ptr)\n+{\n+\tconst unsigned char *p = ptr;\n+\treturn\t(uint16_t)p[0] << 8 |\n+\t\t(uint16_t)p[1] << 0;\n+}\n+\n+static inline uint32_t get_be32(const void *ptr)\n+{\n+\tconst unsigned char *p = ptr;\n+\treturn\t(uint32_t)p[0] << 24 |\n+\t\t(uint32_t)p[1] << 16 |\n+\t\t(uint32_t)p[2] <<  8 |\n+\t\t(uint32_t)p[3] <<  0;\n+}\n+\n+static inline void put_be32(void *ptr, uint32_t value)\n+{\n+\tunsigned char *p = ptr;\n+\tp[0] = value >> 24;\n+\tp[1] = value >> 16;\n+\tp[2] = value >>  8;\n+\tp[3] = value >>  0;\n+}\n \n #endif\n-- \n2.13.3\n"},{"id":"324592","messageId":"5fd84b40-9dde-0fd2-694c-49a073e87495@ramsayjones.plus.com","threadId":"46394","inReplyTo":"56f1263c-a0b4-260b-7e23-881e21119041@web.de","subject":"Re: [PATCH] bswap: convert to unsigned before shifting in get_be32","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-16T00:23:57Z","receivedAt":"2017-07-16T00:24:05Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/07/17 20:11, René Scharfe wrote:\n> The pointer p is dereferenced and we get an unsigned char.  Before\n> shifting it's automatically promoted to int.  Left-shifting a signed\n> 32-bit value bigger than 127 by 24 places is undefined.  Explicitly\n> convert to a 32-bit unsigned type to avoid undefined behaviour if\n> the highest bit is set.\n> \n> Found with Clang's UBSan.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  compat/bswap.h | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/compat/bswap.h b/compat/bswap.h\n> index d47c003544..4582c1107a 100644\n> --- a/compat/bswap.h\n> +++ b/compat/bswap.h\n> @@ -166,10 +166,10 @@ static inline uint64_t git_bswap64(uint64_t x)\n>  \t(*((unsigned char *)(p) + 0) << 8) | \\\n>  \t(*((unsigned char *)(p) + 1) << 0) )\n>  #define get_be32(p)\t( \\\n> -\t(*((unsigned char *)(p) + 0) << 24) | \\\n> -\t(*((unsigned char *)(p) + 1) << 16) | \\\n> -\t(*((unsigned char *)(p) + 2) <<  8) | \\\n> -\t(*((unsigned char *)(p) + 3) <<  0) )\n> +\t((uint32_t)*((unsigned char *)(p) + 0) << 24) | \\\n> +\t((uint32_t)*((unsigned char *)(p) + 1) << 16) | \\\n> +\t((uint32_t)*((unsigned char *)(p) + 2) <<  8) | \\\n> +\t((uint32_t)*((unsigned char *)(p) + 3) <<  0) )\n>  #define put_be32(p, v)\tdo { \\\n>  \tunsigned int __v = (v); \\\n>  \t*((unsigned char *)(p) + 0) = __v >> 24; \\\n> \n\nHeh, I have a patch that is pretty much identical. I suspect\nyou can guess why. ;-)\n\nATB,\nRamsay Jones\n\n"},{"id":"324605","messageId":"20170716102704.2yywbidmmjv7zfxh@sigill.intra.peff.net","threadId":"46394","inReplyTo":"06c5bc39-d2c4-3436-97a7-9faa7c0519ca@web.de","subject":"Re: [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T10:27:04Z","receivedAt":"2017-07-16T10:27:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 15, 2017 at 09:22:50PM +0200, René Scharfe wrote:\n\n> Simplify the implementation and allow callers to use expressions with\n> side-effects by turning the macros get_be16, get_be32 and put_be32 into\n> inline functions.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> All these redundant casts started to bother me, so I tried to come up\n> with nice and clean inline functions.  Successfully?  You tell me.\n> They are longer, but less cluttered.  Would it punish -O0 builds?  Is\n> it all worth it?\n\nI do think the end result is a lot more readable. On gcc 6 at least, the\nfunction seems[1] to end up inlined even with -O0.\n\nInterestingly, at -O2 even with -DNO_UNALIGNED_LOADS, gcc converts the\nresult to a movl and a bswap. Which is the same thing our\nunaligned-loads path is trying for. I wonder if we could/should just\ndrop it (that _would_ punish -O0 on x86, though).\n\n-Peff\n"},{"id":"324606","messageId":"20170716102815.gl24fbfzlom2ena3@sigill.intra.peff.net","threadId":"46394","inReplyTo":"20170716102704.2yywbidmmjv7zfxh@sigill.intra.peff.net","subject":"Re: [PATCH 2/1] bswap: convert get_be16, get_be32 and put_be32 to inline functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T10:28:16Z","receivedAt":"2017-07-16T10:28:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 16, 2017 at 06:27:04AM -0400, Jeff King wrote:\n\n> On Sat, Jul 15, 2017 at 09:22:50PM +0200, René Scharfe wrote:\n> \n> > Simplify the implementation and allow callers to use expressions with\n> > side-effects by turning the macros get_be16, get_be32 and put_be32 into\n> > inline functions.\n> > \n> > Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> > ---\n> > All these redundant casts started to bother me, so I tried to come up\n> > with nice and clean inline functions.  Successfully?  You tell me.\n> > They are longer, but less cluttered.  Would it punish -O0 builds?  Is\n> > it all worth it?\n> \n> I do think the end result is a lot more readable. On gcc 6 at least, the\n> function seems[1] to end up inlined even with -O0.\n\nFor my footnote. I was just going to show the test file I compiled:\n\n  #include \"git-compat-util.h\"\n  uint32_t foo(const char *x)\n  {\n\treturn get_be32(x);\n  }\n\nIt's possible the optimizer may behave differently on a more complicated\ninput, but it does show that -O0 is still willing to inline.\n\n-Peff\n"}]}