{"thread":{"id":"63595","subject":"[PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","startedAt":"2025-06-06T16:57:22Z","lastAt":"2025-06-09T21:57:20Z","messageCount":6,"participants":["Sebastian Andrzej Siewior","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"519861","messageId":"20250606165718.HOiC2U4X@breakpoint.cc","threadId":"63595","inReplyTo":null,"subject":"[PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","fromName":"Sebastian Andrzej Siewior","fromEmail":"sebastian@breakpoint.cc","sentAt":"2025-06-06T16:57:18Z","receivedAt":"2025-06-06T16:57:22Z","isPatch":true,"sender":{"key":"sebastian@breakpoint.cc","avatar":"https://avatars.githubusercontent.com/u/5932951?v=4"},"body":"The top of that file contains optimized bswap32/64 only for a few little\nendian machines. Since the commit cited below there is one for every\narchitecture supporting the __builtin_bswap* directives.\n\nLater in the file, the ntohl* macros are replaced with the bswap* macros\nshould they be provided. Since they are now provided even on big endian\nmachines they replace the nop with a swap.\n\nMove the ntohl*/ htonl* replacement once it is determined that it is a\nlittle architecture where the swap is performed.\n\nFixes: 6547d1c9cbafa (\"bswap.h: add support for built-in bswap functions\")\nSigned-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>\n---\n\nThis builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The\ntestsuite passes.\n\n compat/bswap.h | 37 ++++++++++++++++---------------------\n 1 file changed, 16 insertions(+), 21 deletions(-)\n\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex 9e0f98e00b93a..5789d12c2c356 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -95,24 +95,6 @@ static inline uint64_t git_bswap64(uint64_t x)\n \n #endif\n \n-#if defined(bswap32)\n-\n-#undef ntohl\n-#undef htonl\n-#define ntohl(x) bswap32(x)\n-#define htonl(x) bswap32(x)\n-\n-#endif\n-\n-#if defined(bswap64)\n-\n-#undef ntohll\n-#undef htonll\n-#define ntohll(x) bswap64(x)\n-#define htonll(x) bswap64(x)\n-\n-#else\n-\n #undef ntohll\n #undef htonll\n \n@@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)\n # define ntohll(n) (n)\n # define htonll(n) (n)\n #else\n-# define ntohll(n) default_bswap64(n)\n-# define htonll(n) default_bswap64(n)\n-#endif\n \n+# if defined(bswap32)\n+#  undef ntohl\n+#  undef htonl\n+#  define ntohl(x) bswap32(x)\n+#  define htonl(x) bswap32(x)\n+# endif\n+\n+# if defined(bswap64)\n+#  undef ntohll\n+#  undef htonll\n+#  define ntohll(x) bswap64(x)\n+#  define htonll(x) bswap64(x)\n+# else\n+#  define ntohll(n) default_bswap64(n)\n+#  define htonll(n) default_bswap64(n)\n+# endif\n #endif\n \n static inline uint16_t get_be16(const void *ptr)\n-- \n2.49.0\n\n"},{"id":"519877","messageId":"xmqqplfg1sym.fsf@gitster.g","threadId":"63595","inReplyTo":"20250606165718.HOiC2U4X@breakpoint.cc","subject":"Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-06T21:05:37Z","receivedAt":"2025-06-06T21:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Andrzej Siewior <sebastian@breakpoint.cc> writes:\n\n> The top of that file contains optimized bswap32/64 only for a few little\n> endian machines. Since the commit cited below there is one for every\n> architecture supporting the __builtin_bswap* directives.\n>\n> Later in the file, the ntohl* macros are replaced with the bswap* macros\n> should they be provided. Since they are now provided even on big endian\n> machines they replace the nop with a swap.\n>\n> Move the ntohl*/ htonl* replacement once it is determined that it is a\n> little architecture where the swap is performed.\n>\n> Fixes: 6547d1c9cbafa (\"bswap.h: add support for built-in bswap functions\")\n> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>\n> ---\n>\n> This builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The\n> testsuite passes.\n\nWhat's missing from the above proposed log message is what problem\nthis patch is trying to address.  \"The testsuite passes\" may refer\nto the state with this patch, but the message does not talk about\nthe state without the patch.  We'd prefer to see the log message\nbegin more like so:\n\n    Since 6547d1c9 (bswap.h: add support for built-in bswap\n    functions, 2025-04-23) tweaked the way the bswap32/64 macros are\n    defined, on platforms with __builtin_bswap32/64 supported, the\n    bswap32/64 macros are defined even on big endian platforms.\n\n    However this file assumes that bswap32/64 are defined ONLY on\n    little endian machines and uses that assumption to redefine\n    ntohl/ntohll macros.  The said commit broke t1234-be.sh test\n    gets broken, among many others on s390x.\n\nEspecially pay close attention to how we name the commit in prose,\nnot as \"the cited commit\" (because we do not 'cite' them and frown\nupon Fixes: trailer on this project).\n\nAlso, I do not know what tests were broken and on what platforms for\nyou that triggered you to do this patch, so take the second paragraph\nabove as all made up example that only illustrates the level of\ndetail expected in a proposed log message.\n\nAfter the \"observation of the current state of the problematic code\"\nis given, we'd describe the solution.\n\n    Make sure that we detect byte order of the platform first and\n    override ntohll only on little endian platfgorms with bswap64\n    by moving things around.\n\nor something, perhaps.\n\n\n\n>  #endif\n\nIt is a bit hard to see as the original does not indent the\n#directives consistently, but this \"#endif\" closes the\n#if..#elif..#endif to define bswap32/bswap64 for some platforms.  We\nare only inside the top-level \"#ifdef COMPAT_BSWAP_H\" at this point,\nso ...\n\n> -#if defined(bswap32)\n> -\n> -#undef ntohl\n> -#undef htonl\n> -#define ntohl(x) bswap32(x)\n> -#define htonl(x) bswap32(x)\n> -\n> -#endif\n> -\n> -#if defined(bswap64)\n> -\n> -#undef ntohll\n> -#undef htonll\n> -#define ntohll(x) bswap64(x)\n> -#define htonll(x) bswap64(x)\n> -\n> -#else\n\n... we undefine these two macros for _everybody_ here.  Also let me\ntake a mental note that we only undef these 64-bit functions and\nleave ntohl/htonl intact.\n\n>  #undef ntohll\n>  #undef htonll\n\nThis is related to the \"oddity\" I'll mention at the end.  \n\nAfter this part, there is a #if..#elif..#endif cascade to ensure\nGIT_BYTE_ORDER is defined, which is unchanged and not shown in the\ncontext.\n\n> @@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)\n>  # define ntohll(n) (n)\n>  # define htonll(n) (n)\n>  #else\n> -# define ntohll(n) default_bswap64(n)\n> -# define htonll(n) default_bswap64(n)\n> -#endif\n\n\"#if GIT_BYTE_ORDER == GIT_BIGENDIAN\" is before the pre-context of\nthis hunk.  We are extending the else clause (i.e. little endian\nsupport) with the following:\n\n> +# if defined(bswap32)\n> +#  undef ntohl\n> +#  undef htonl\n> +#  define ntohl(x) bswap32(x)\n> +#  define htonl(x) bswap32(x)\n> +# endif\n> +\n> +# if defined(bswap64)\n> +#  undef ntohll\n> +#  undef htonll\n> +#  define ntohll(x) bswap64(x)\n> +#  define htonll(x) bswap64(x)\n> +# else\n> +#  define ntohll(n) default_bswap64(n)\n> +#  define htonll(n) default_bswap64(n)\n> +# endif\n>  #endif\n\nI think the patch is an improvement from the current state, but the\nresulting code is still somewhat odd in that ntohll() and htonll()\nare overridden for everybody (even for big endian boxes we make sure\nit is identity function), but we override ntohl() and htonl() only\non platforms where bswap32 is defined.\n\nThanks.\n\n"},{"id":"519879","messageId":"20250606220455.jhicNLuK@breakpoint.cc","threadId":"63595","inReplyTo":"xmqqplfg1sym.fsf@gitster.g","subject":"Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","fromName":"Sebastian Andrzej Siewior","fromEmail":"sebastian@breakpoint.cc","sentAt":"2025-06-06T22:04:55Z","receivedAt":"2025-06-06T22:05:00Z","isPatch":true,"sender":{"key":"sebastian@breakpoint.cc","avatar":"https://avatars.githubusercontent.com/u/5932951?v=4"},"body":"On 2025-06-06 14:05:37 [-0700], Junio C Hamano wrote:\n> > This builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The\n> > testsuite passes.\n> \n> What's missing from the above proposed log message is what problem\n> this patch is trying to address.  \"The testsuite passes\" may refer\n> to the state with this patch, but the message does not talk about\n> the state without the patch.  We'd prefer to see the log message\n> begin more like so:\n> \n>     Since 6547d1c9 (bswap.h: add support for built-in bswap\n>     functions, 2025-04-23) tweaked the way the bswap32/64 macros are\n>     defined, on platforms with __builtin_bswap32/64 supported, the\n>     bswap32/64 macros are defined even on big endian platforms.\n> \n>     However this file assumes that bswap32/64 are defined ONLY on\n>     little endian machines and uses that assumption to redefine\n>     ntohl/ntohll macros.  The said commit broke t1234-be.sh test\n>     gets broken, among many others on s390x.\n> \n> Especially pay close attention to how we name the commit in prose,\n> not as \"the cited commit\" (because we do not 'cite' them and frown\n> upon Fixes: trailer on this project).\n\nOkay.\n> Also, I do not know what tests were broken and on what platforms for\n> you that triggered you to do this patch, so take the second paragraph\n> above as all made up example that only illustrates the level of\n> detail expected in a proposed log message.\n\nI run into this while debugging t4014-format-patch.sh due to another\nissue.\n\n> After the \"observation of the current state of the problematic code\"\n> is given, we'd describe the solution.\n> \n>     Make sure that we detect byte order of the platform first and\n>     override ntohll only on little endian platfgorms with bswap64\n>     by moving things around.\n> \n> or something, perhaps.\n\nOkay.\n\n> >  #endif\n> \n> It is a bit hard to see as the original does not indent the\n> #directives consistently, but this \"#endif\" closes the\n> #if..#elif..#endif to define bswap32/bswap64 for some platforms.  We\n> are only inside the top-level \"#ifdef COMPAT_BSWAP_H\" at this point,\n> so ...\n> \n> > -#if defined(bswap32)\n> > -\n> > -#undef ntohl\n> > -#undef htonl\n> > -#define ntohl(x) bswap32(x)\n> > -#define htonl(x) bswap32(x)\n> > -\n> > -#endif\n> > -\n> > -#if defined(bswap64)\n> > -\n> > -#undef ntohll\n> > -#undef htonll\n> > -#define ntohll(x) bswap64(x)\n> > -#define htonll(x) bswap64(x)\n> > -\n> > -#else\n> \n> ... we undefine these two macros for _everybody_ here.  Also let me\n> take a mental note that we only undef these 64-bit functions and\n> leave ntohl/htonl intact.\n\nHow so? The ntohl/ htonl are also replaced with bswap32 Or do I miss\nsomething.\n\n> >  #undef ntohll\n> >  #undef htonll\n> \n> This is related to the \"oddity\" I'll mention at the end.  \n> \n> After this part, there is a #if..#elif..#endif cascade to ensure\n> GIT_BYTE_ORDER is defined, which is unchanged and not shown in the\n> context.\n> \n> > @@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)\n> >  # define ntohll(n) (n)\n> >  # define htonll(n) (n)\n> >  #else\n> > -# define ntohll(n) default_bswap64(n)\n> > -# define htonll(n) default_bswap64(n)\n> > -#endif\n> \n> \"#if GIT_BYTE_ORDER == GIT_BIGENDIAN\" is before the pre-context of\n> this hunk.  We are extending the else clause (i.e. little endian\n> support) with the following:\n> \n> > +# if defined(bswap32)\n> > +#  undef ntohl\n> > +#  undef htonl\n> > +#  define ntohl(x) bswap32(x)\n> > +#  define htonl(x) bswap32(x)\n> > +# endif\n> > +\n> > +# if defined(bswap64)\n> > +#  undef ntohll\n> > +#  undef htonll\n> > +#  define ntohll(x) bswap64(x)\n> > +#  define htonll(x) bswap64(x)\n> > +# else\n> > +#  define ntohll(n) default_bswap64(n)\n> > +#  define htonll(n) default_bswap64(n)\n> > +# endif\n> >  #endif\n> \n> I think the patch is an improvement from the current state, but the\n> resulting code is still somewhat odd in that ntohll() and htonll()\n> are overridden for everybody (even for big endian boxes we make sure\n> it is identity function), but we override ntohl() and htonl() only\n> on platforms where bswap32 is defined.\n\nAh, the ntohll/ htonll gets undef and defined later. That is the\n\"oddity\" as you put it.\nDo you want this reposted with an improved commit message or do you want\nalso the undef for ntohll and the identity define removed since it is\nnot required? I could add it as a follow-up not merge the fix and the\nimprovement in one patch but as you wish.\n\nWe migh also remove this file because it appears that ntohl() comes\nalready as the built-in:\n| $ cat a.c\n| #include <arpa/inet.h>\n| \n| unsigned int nto32(unsigned int x)\n| {\n|         return ntohl(x);\n| }\n| $ gcc -o a.i -E a.c -O2\n| $ grep -A4 nto32 a.i \n| unsigned int nto32(unsigned int x)\n| {\n|  return \n| # 5 \"a.c\" 3 4\n|        __bswap_32 (\n| $ grep -A3 \"__bswap_32 (_\" a.i \n| __bswap_32 (__uint32_t __bsx)\n| {\n| \n|   return __builtin_bswap32 (__bsx);\n\nThis is from the glibc 2.41 header file. But then ntohll() is\nnon-standard and needs manual care.\n\n> Thanks.\n\nSebastian\n"},{"id":"519880","messageId":"xmqqcybg1orc.fsf@gitster.g","threadId":"63595","inReplyTo":"20250606220455.jhicNLuK@breakpoint.cc","subject":"Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-06T22:36:23Z","receivedAt":"2025-06-06T22:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Andrzej Siewior <sebastian@breakpoint.cc> writes:\n\n>> ... we undefine these two macros for _everybody_ here.  Also let me\n>> take a mental note that we only undef these 64-bit functions and\n>> leave ntohl/htonl intact.\n>\n> How so? The ntohl/ htonl are also replaced with bswap32 Or do I miss\n> something.\n\nOnly sometimes, unlike the 64-bit one that is always replaced.\n\n>> >  #undef ntohll\n>> >  #undef htonll\n>> \n>> This is related to the \"oddity\" I'll mention at the end.  \n\n>> > +# if defined(bswap32)\n>> > +#  undef ntohl\n>> > +#  undef htonl\n>> > +#  define ntohl(x) bswap32(x)\n>> > +#  define htonl(x) bswap32(x)\n>> > +# endif\n\nThis is ntohl/htonl thing.  Because we do not have\ndefault_bswap32(), unlike the 64-bit side, we do not touch\nntohl/htonl when bswap32 is not available.\n\nThat is the oddity I mentioned.\n\n> Ah, the ntohll/ htonll gets undef and defined later. That is the\n> \"oddity\" as you put it.\n> Do you want this reposted with an improved commit message or do you want\n> also the undef for ntohll and the identity define removed since it is\n> not required?\n\nI do not have a strong preference either way myself.  As long as it\nis clearly documented what we are doing here (e.g., we probably\nshould tell anybody who includes this header file that bswap32 and\nbswap64 are an implementation detail inside this file and they\nshould not use them themselves, or something like that).\n"},{"id":"519895","messageId":"xmqqmsajxo2i.fsf@gitster.g","threadId":"63595","inReplyTo":"xmqqcybg1orc.fsf@gitster.g","subject":"Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-07T21:02:29Z","receivedAt":"2025-06-07T21:02:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This seems to break Windows rather badly.\n\nhttps://github.com/git/git/actions/runs/15511228119/job/43672455770#step:4:92\n\nI'll eject it from my tree for now.\n\n"},{"id":"520020","messageId":"20250609215717.gbaaVj7r@breakpoint.cc","threadId":"63595","inReplyTo":"xmqqmsajxo2i.fsf@gitster.g","subject":"Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.","fromName":"Sebastian Andrzej Siewior","fromEmail":"sebastian@breakpoint.cc","sentAt":"2025-06-09T21:57:17Z","receivedAt":"2025-06-09T21:57:20Z","isPatch":true,"sender":{"key":"sebastian@breakpoint.cc","avatar":"https://avatars.githubusercontent.com/u/5932951?v=4"},"body":"On 2025-06-07 14:02:29 [-0700], Junio C Hamano wrote:\n> This seems to break Windows rather badly.\n> \n> https://github.com/git/git/actions/runs/15511228119/job/43672455770#step:4:92\n> \n> I'll eject it from my tree for now.\n\nOkay. I prepared a revert the commit introducing the regression and then\ntried to clean it up a bit taking this fallout into consideration.\n\nWill post it tomorrow after some testing.\n\nSebastian\n"}]}