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

[Junio C Hamano <gitster@pobox.com>]

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.