[PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[Sebastian Andrzej Siewior]

The top of that file contains optimized bswap32/64 only for a few little
endian machines. Since the commit cited below there is one for every
architecture supporting the __builtin_bswap* directives.

Later in the file, the ntohl* macros are replaced with the bswap* macros
should they be provided. Since they are now provided even on big endian
machines they replace the nop with a swap.

Move the ntohl*/ htonl* replacement once it is determined that it is a
little architecture where the swap is performed.

Fixes: 6547d1c9cbafa ("bswap.h: add support for built-in bswap functions")
Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---

This builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The
testsuite passes.

 compat/bswap.h | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index 9e0f98e00b93a..5789d12c2c356 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -95,24 +95,6 @@ static inline uint64_t git_bswap64(uint64_t x)
 
 #endif
 
-#if defined(bswap32)
-
-#undef ntohl
-#undef htonl
-#define ntohl(x) bswap32(x)
-#define htonl(x) bswap32(x)
-
-#endif
-
-#if defined(bswap64)
-
-#undef ntohll
-#undef htonll
-#define ntohll(x) bswap64(x)
-#define htonll(x) bswap64(x)
-
-#else
-
 #undef ntohll
 #undef htonll
 
@@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)
 # define ntohll(n) (n)
 # define htonll(n) (n)
 #else
-# define ntohll(n) default_bswap64(n)
-# define htonll(n) default_bswap64(n)
-#endif
 
+# if defined(bswap32)
+#  undef ntohl
+#  undef htonl
+#  define ntohl(x) bswap32(x)
+#  define htonl(x) bswap32(x)
+# endif
+
+# if defined(bswap64)
+#  undef ntohll
+#  undef htonll
+#  define ntohll(x) bswap64(x)
+#  define htonll(x) bswap64(x)
+# else
+#  define ntohll(n) default_bswap64(n)
+#  define htonll(n) default_bswap64(n)
+# endif
 #endif
 
 static inline uint16_t get_be16(const void *ptr)
-- 
2.49.0

Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[Junio C Hamano]

Sebastian Andrzej Siewior <sebastian@breakpoint.cc> writes:

> The top of that file contains optimized bswap32/64 only for a few little
> endian machines. Since the commit cited below there is one for every
> architecture supporting the __builtin_bswap* directives.
>
> Later in the file, the ntohl* macros are replaced with the bswap* macros
> should they be provided. Since they are now provided even on big endian
> machines they replace the nop with a swap.
>
> Move the ntohl*/ htonl* replacement once it is determined that it is a
> little architecture where the swap is performed.
>
> Fixes: 6547d1c9cbafa ("bswap.h: add support for built-in bswap functions")
> Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> ---
>
> This builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The
> testsuite passes.

What's missing from the above proposed log message is what problem
this patch is trying to address.  "The testsuite passes" may refer
to the state with this patch, but the message does not talk about
the state without the patch.  We'd prefer to see the log message
begin more like so:

    Since 6547d1c9 (bswap.h: add support for built-in bswap
    functions, 2025-04-23) tweaked the way the bswap32/64 macros are
    defined, on platforms with __builtin_bswap32/64 supported, the
    bswap32/64 macros are defined even on big endian platforms.

    However this file assumes that bswap32/64 are defined ONLY on
    little endian machines and uses that assumption to redefine
    ntohl/ntohll macros.  The said commit broke t1234-be.sh test
    gets broken, among many others on s390x.

Especially pay close attention to how we name the commit in prose,
not as "the cited commit" (because we do not 'cite' them and frown
upon Fixes: trailer on this project).

Also, I do not know what tests were broken and on what platforms for
you that triggered you to do this patch, so take the second paragraph
above as all made up example that only illustrates the level of
detail expected in a proposed log message.

After the "observation of the current state of the problematic code"
is given, we'd describe the solution.

    Make sure that we detect byte order of the platform first and
    override ntohll only on little endian platfgorms with bswap64
    by moving things around.

or something, perhaps.



>  #endif

It is a bit hard to see as the original does not indent the
#directives consistently, but this "#endif" closes the
#if..#elif..#endif to define bswap32/bswap64 for some platforms.  We
are only inside the top-level "#ifdef COMPAT_BSWAP_H" at this point,
so ...

> -#if defined(bswap32)
> -
> -#undef ntohl
> -#undef htonl
> -#define ntohl(x) bswap32(x)
> -#define htonl(x) bswap32(x)
> -
> -#endif
> -
> -#if defined(bswap64)
> -
> -#undef ntohll
> -#undef htonll
> -#define ntohll(x) bswap64(x)
> -#define htonll(x) bswap64(x)
> -
> -#else

... we undefine these two macros for _everybody_ here.  Also let me
take a mental note that we only undef these 64-bit functions and
leave ntohl/htonl intact.

>  #undef ntohll
>  #undef htonll

This is related to the "oddity" I'll mention at the end.  

After this part, there is a #if..#elif..#endif cascade to ensure
GIT_BYTE_ORDER is defined, which is unchanged and not shown in the
context.

> @@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)
>  # define ntohll(n) (n)
>  # define htonll(n) (n)
>  #else
> -# define ntohll(n) default_bswap64(n)
> -# define htonll(n) default_bswap64(n)
> -#endif

"#if GIT_BYTE_ORDER == GIT_BIGENDIAN" is before the pre-context of
this hunk.  We are extending the else clause (i.e. little endian
support) with the following:

> +# if defined(bswap32)
> +#  undef ntohl
> +#  undef htonl
> +#  define ntohl(x) bswap32(x)
> +#  define htonl(x) bswap32(x)
> +# endif
> +
> +# if defined(bswap64)
> +#  undef ntohll
> +#  undef htonll
> +#  define ntohll(x) bswap64(x)
> +#  define htonll(x) bswap64(x)
> +# else
> +#  define ntohll(n) default_bswap64(n)
> +#  define htonll(n) default_bswap64(n)
> +# endif
>  #endif

I think the patch is an improvement from the current state, but the
resulting code is still somewhat odd in that ntohll() and htonll()
are overridden for everybody (even for big endian boxes we make sure
it is identity function), but we override ntohl() and htonl() only
on platforms where bswap32 is defined.

Thanks.

Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[Sebastian Andrzej Siewior]

On 2025-06-06 14:05:37 [-0700], Junio C Hamano wrote:
> > This builds on top of v2.50.0-rc1 on s390x and -rc0 and x86-64. The
> > testsuite passes.
> 
> What's missing from the above proposed log message is what problem
> this patch is trying to address.  "The testsuite passes" may refer
> to the state with this patch, but the message does not talk about
> the state without the patch.  We'd prefer to see the log message
> begin more like so:
> 
>     Since 6547d1c9 (bswap.h: add support for built-in bswap
>     functions, 2025-04-23) tweaked the way the bswap32/64 macros are
>     defined, on platforms with __builtin_bswap32/64 supported, the
>     bswap32/64 macros are defined even on big endian platforms.
> 
>     However this file assumes that bswap32/64 are defined ONLY on
>     little endian machines and uses that assumption to redefine
>     ntohl/ntohll macros.  The said commit broke t1234-be.sh test
>     gets broken, among many others on s390x.
> 
> Especially pay close attention to how we name the commit in prose,
> not as "the cited commit" (because we do not 'cite' them and frown
> upon Fixes: trailer on this project).

Okay.
> Also, I do not know what tests were broken and on what platforms for
> you that triggered you to do this patch, so take the second paragraph
> above as all made up example that only illustrates the level of
> detail expected in a proposed log message.

I run into this while debugging t4014-format-patch.sh due to another
issue.

> After the "observation of the current state of the problematic code"
> is given, we'd describe the solution.
> 
>     Make sure that we detect byte order of the platform first and
>     override ntohll only on little endian platfgorms with bswap64
>     by moving things around.
> 
> or something, perhaps.

Okay.

> >  #endif
> 
> It is a bit hard to see as the original does not indent the
> #directives consistently, but this "#endif" closes the
> #if..#elif..#endif to define bswap32/bswap64 for some platforms.  We
> are only inside the top-level "#ifdef COMPAT_BSWAP_H" at this point,
> so ...
> 
> > -#if defined(bswap32)
> > -
> > -#undef ntohl
> > -#undef htonl
> > -#define ntohl(x) bswap32(x)
> > -#define htonl(x) bswap32(x)
> > -
> > -#endif
> > -
> > -#if defined(bswap64)
> > -
> > -#undef ntohll
> > -#undef htonll
> > -#define ntohll(x) bswap64(x)
> > -#define htonll(x) bswap64(x)
> > -
> > -#else
> 
> ... we undefine these two macros for _everybody_ here.  Also let me
> take a mental note that we only undef these 64-bit functions and
> leave ntohl/htonl intact.

How so? The ntohl/ htonl are also replaced with bswap32 Or do I miss
something.

> >  #undef ntohll
> >  #undef htonll
> 
> This is related to the "oddity" I'll mention at the end.  
> 
> After this part, there is a #if..#elif..#endif cascade to ensure
> GIT_BYTE_ORDER is defined, which is unchanged and not shown in the
> context.
> 
> > @@ -151,10 +133,23 @@ static inline uint64_t git_bswap64(uint64_t x)
> >  # define ntohll(n) (n)
> >  # define htonll(n) (n)
> >  #else
> > -# define ntohll(n) default_bswap64(n)
> > -# define htonll(n) default_bswap64(n)
> > -#endif
> 
> "#if GIT_BYTE_ORDER == GIT_BIGENDIAN" is before the pre-context of
> this hunk.  We are extending the else clause (i.e. little endian
> support) with the following:
> 
> > +# if defined(bswap32)
> > +#  undef ntohl
> > +#  undef htonl
> > +#  define ntohl(x) bswap32(x)
> > +#  define htonl(x) bswap32(x)
> > +# endif
> > +
> > +# if defined(bswap64)
> > +#  undef ntohll
> > +#  undef htonll
> > +#  define ntohll(x) bswap64(x)
> > +#  define htonll(x) bswap64(x)
> > +# else
> > +#  define ntohll(n) default_bswap64(n)
> > +#  define htonll(n) default_bswap64(n)
> > +# endif
> >  #endif
> 
> I think the patch is an improvement from the current state, but the
> resulting code is still somewhat odd in that ntohll() and htonll()
> are overridden for everybody (even for big endian boxes we make sure
> it is identity function), but we override ntohl() and htonl() only
> on platforms where bswap32 is defined.

Ah, the ntohll/ htonll gets undef and defined later. That is the
"oddity" as you put it.
Do you want this reposted with an improved commit message or do you want
also the undef for ntohll and the identity define removed since it is
not required? I could add it as a follow-up not merge the fix and the
improvement in one patch but as you wish.

We migh also remove this file because it appears that ntohl() comes
already as the built-in:
| $ cat a.c
| #include <arpa/inet.h>
| 
| unsigned int nto32(unsigned int x)
| {
|         return ntohl(x);
| }
| $ gcc -o a.i -E a.c -O2
| $ grep -A4 nto32 a.i 
| unsigned int nto32(unsigned int x)
| {
|  return 
| # 5 "a.c" 3 4
|        __bswap_32 (
| $ grep -A3 "__bswap_32 (_" a.i 
| __bswap_32 (__uint32_t __bsx)
| {
| 
|   return __builtin_bswap32 (__bsx);

This is from the glibc 2.41 header file. But then ntohll() is
non-standard and needs manual care.

> Thanks.

Sebastian

Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[Junio C Hamano]

Sebastian Andrzej Siewior <sebastian@breakpoint.cc> writes:

>> ... we undefine these two macros for _everybody_ here.  Also let me
>> take a mental note that we only undef these 64-bit functions and
>> leave ntohl/htonl intact.
>
> How so? The ntohl/ htonl are also replaced with bswap32 Or do I miss
> something.

Only sometimes, unlike the 64-bit one that is always replaced.

>> >  #undef ntohll
>> >  #undef htonll
>> 
>> This is related to the "oddity" I'll mention at the end.  

>> > +# if defined(bswap32)
>> > +#  undef ntohl
>> > +#  undef htonl
>> > +#  define ntohl(x) bswap32(x)
>> > +#  define htonl(x) bswap32(x)
>> > +# endif

This is ntohl/htonl thing.  Because we do not have
default_bswap32(), unlike the 64-bit side, we do not touch
ntohl/htonl when bswap32 is not available.

That is the oddity I mentioned.

> Ah, the ntohll/ htonll gets undef and defined later. That is the
> "oddity" as you put it.
> Do you want this reposted with an improved commit message or do you want
> also the undef for ntohll and the identity define removed since it is
> not required?

I do not have a strong preference either way myself.  As long as it
is clearly documented what we are doing here (e.g., we probably
should tell anybody who includes this header file that bswap32 and
bswap64 are an implementation detail inside this file and they
should not use them themselves, or something like that).

Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[Junio C Hamano]

This seems to break Windows rather badly.

https://github.com/git/git/actions/runs/15511228119/job/43672455770#step:4:92

I'll eject it from my tree for now.

Re: [PATCH] bswap.h: Move the overwriting of the ntohl*/ htonl* macros.

[Sebastian Andrzej Siewior]

On 2025-06-07 14:02:29 [-0700], Junio C Hamano wrote:
> This seems to break Windows rather badly.
> 
> https://github.com/git/git/actions/runs/15511228119/job/43672455770#step:4:92
> 
> I'll eject it from my tree for now.

Okay. I prepared a revert the commit introducing the regression and then
tried to clean it up a bit taking this fallout into consideration.

Will post it tomorrow after some testing.

Sebastian