v2, 6 partsbswap.h: Add support for __BYTE_ORDER__

Hi,

since the add of bswap32/64() support based on __builtin support, the usage of ntohl() macros is broken on big endian machines because the macros are always overwritten providing a swap.

The first patch in the series reverts the change and the following patches try to improve the situation by allowing to always provide an optimized version.

I've been looking at recent compiler and they manage to recognize the manual shifting and use an optimize opcode if available. The ntohl version provided by glibc already provides an "optimized" version which makes an optimisation in git almost not needed. One of the motivation behind overwriting/ providing an optimized version was to provide a macro instead of using a function call. One libc that is still providing ntohl as a function call is musl. So it might makes sense to keep it. While ntohl() is provided by the libc, the ntohll() is not. I found it only on Windows provided by winsock.h.

I haven't touched the put/get_be*() macros. gcc & clang are both smart enough to swap the content accordingly and perform a single store/ load. Only the msvc seems to strugle here and performs multiple bytes stores/ loads and shifts.

Sebastian Andrzej Siewior (6):
  Revert "bswap.h: add support for built-in bswap functions"
  bswap.h: Add support for __BYTE_ORDER__
  bswap.h: Define GIT_LITTLE_ENDIAN on msvc as little endian
  bswap.h: Always overwrite ntohl/ ntohll macros
  bswap.h: Remove optimized x86 version of bswap32/64
  bswap.h: Provide a built-in based version of bswap32/64 if possible

 compat/bswap.h | 118 ++++++++++++++++++-------------------------------
 1 file changed, 44 insertions(+), 74 deletions(-)

The __BYTE_ORDER__ define is provided by gcc (since ~v4.6), clang (since ~v3.2) and icc (since ~16.0.3). It is not provided by msvc as of v19.43 / 17.13.6. The __BYTE_ORDER and BYTE_ORDER macros are libc specific and are not available on all supported platforms such as mingw.

Add support for the __BYTE_ORDER__ macro as a fallback.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 6 ++++++
 1 file changed, 6 insertions(+)

On 2025-06-11 at 22:14:38, Sebastian Andrzej Siewior wrote:

One additional option you can add if you want (it's completely optional) is that if `__STDC_VERSION__` is 202311L or larger, then you can `#include <stdbit.h>`, which has `__STDC_ENDIAN_LITTLE__`, `__STDC_ENDIAN_BIG__`, and `__STDC_ENDIAN_NATIVE__`.

That will work on a modern GCC or clang with an appropriate compiler flag. I don't know about MSVC, but it might be an option for people who want to use more esoteric compilers which support standards but aren't very compatible with GCC and clang.

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Another standard that is too new to be common, but POSIX.1-2024 added <endian.h> with BYTE_ORDER, LITTLE_ENDIAN, and BIG_ENDIAN [1].

In practice, some systems will have it because glibc had it for a long time.

Collin

[1] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/endian.h.html

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

I think it is more confusing than illuninating to have the last sentence in the above paragraph. There probably are tons of other compilers that do not support it, and this patch does not target any of them, including MSVC, which is the topic of the next step anyway.

So, I'm inclined to just remove the sentence, or replace it with something like "Even recent versions of MSVC do not support it, which will be dealt with in the next patch."

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 bswap31/64 are defined ONLY on little endian machines and uses that assumption to redefine ntohl/ntohll macros. The said commit broke t4014-format-patch.sh test, among many others on s390x.

Revert the commit.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

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

31/64 -> 32/64? Just sanity checking the typofix I plan to make locally while applying this patch.

The Microsoft Visual C++ (MSVC) compiler (as of Visual Studio 2022 version 17.13.6) does not define __BYTE_ORDER__ and its C-library does not define __BYTE_ORDER. The compiler is supported only an arm64 and x86 which are all little endian.

Define GIT_BYTE_ORDER on msvc as little endian to avoid further checks.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

"an arm" -> "on arm"? Just sanity checking before making local typofix while queueing.

On x86 the bswap32/64 macro is implemenated based on the x86 opcode wich performs the required shifting in just one opcode. The other CPUs fallback to the generic shifting as implemented by default_swab32() and default_bswap64() if needed.

I've been looking at how good a compiler is at recognizing the default
shift and emitting an optimized operation:
- x86, arm64 msvc v19.20
  default_swab32() optimized
  default_bswap64() shifts
  _byteswap_uint64() otimized

- x86, arm64 msvc v19.37
  default_swab32() optimized
  default_bswap64() optimized
  _byteswap_uint64() otimized

- arm64, gcc-4.9.4: optimized
- x86-64, gcc-4.4.7: shifts
- x86-64, gcc-4.5.3: optimized
- x86-64, clang-3.0: optimized

Given that gcc-4.5 and clang-3.0 are fairly old, any recent compiler should recognize the shift.

Remove the optimized x86 version and rely on the compiler.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 41 +----------------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

On Thu, Jun 12, 2025, at 00:14, Sebastian Andrzej Siewior wrote:

s/implemenated/implemented/

s/wich/which/

Is it supposed to say `otimized` twice?

On 2025-06-26 17:55:43 [+0200], Kristoffer Haugsbakk wrote:

Nope. Fixed. Thank you.

Sebastian

The compiler is in general able to recognize the endian shift and replace it with an optimized opcode if possible. On certain architectures such as RiscV or MIPS the situation can get complicated. They don't provide an optimized opcode and masking the "higher" bits may required loading a constant which needs shifting. This causes the compiler to emit a lot of instructions for the operation.

The provided builtin directive on these architecture calls a function which does the operation instead of emitting the code for operation.

Bring back the change from commit 6547d1c9 (bswap.h: add support for built-in bswap functions, 2025-04-23). The bswap32/64 macro can now be defined unconditionally so it won't regress on big endian architectures.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

On x86 the bswap32/64 macro is implemenated based on the x86 opcode wich performs the required shifting in just one opcode. The other CPUs fallback to the generic shifting as implemented by default_swab32() and default_bswap64() if needed.

I've been looking at how good a compiler is at recognizing the default
shift and emitting an optimized operation:
- x86, arm64 msvc v19.20
  default_swab32() optimized
  default_bswap64() shifts
  _byteswap_uint64() otimized

- x86, arm64 msvc v19.37
  default_swab32() optimized
  default_bswap64() optimized
  _byteswap_uint64() otimized

- arm64, gcc-4.9.4: optimized
- x86-64, gcc-4.4.7: shifts
- x86-64, gcc-4.5.3: optimized
- x86-64, clang-3.0: optimized

Given that gcc-4.5 and clang-3.0 are fairly old, any recent compiler should recognize the shift.

Remove the optimized x86 version and rely on the compiler.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 41 +----------------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

The ntohl and htonl macros are redefined because the provided macros were not always optimal. Sometimes it was a function call, sometimes it was a macro which did the shifting. Using the 'bswap' opcode on x86 provides probably better performance than performing the shifting. These macros are only overwritten on x86 if the "optimized" version is available.

The ntohll and htonll macros are not available on every platform (at least glibc does not provide them) which means they need to be defined once the endianness of the system is determined.

In order to get a more symmetrical setup, redfine the macros once the endianness of the system has been determined.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 compat/bswap.h | 54 ++++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 28 deletions(-)

On 2025-06-11 at 22:14:40, Sebastian Andrzej Siewior wrote:

I believe that the peephole optimizer will almost always optimize them to the bswap or equivalent opcode, much like it recognizes how to generate rotate opcodes from two shifts and an or, so they should actually be equivalent.

GCC and clang both emit simple bswap instructions with `-O2`, which is the optimization level we use: https://godbolt.org/z/1r8P1Pqo7.

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Good observation. In short, we do not have to redefine these in terms of bswap32/64 for performance as the compilers should do a reasonable job.

The updated organization to separate the two concerns in this file, namely, (1) figure out the best way to write bswap32/64, and (2) override (or supply on platforms that do not offer) host-network byte order helpers, does clean things up a lot, so I still like to see it, though.

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

I am inclined to take only this one for the upcoming release, as a pure regression fix. It's not like with the change reverted, those platforms with __builtin_bswap32/64 will stop building correctly. The worst is that they fall back to the implementation they have always been using until any and all released versions of Git.

The rest looked promising and with very good materials, but let's leave them for the cycle after 2.50 ships.

Thanks.

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

Now, I see many comments and suggestions on the thread since this v2 iteration was posted:

https://lore.kernel.org/git/20250611221444.1567638-1-sebastian@breakpoint.cc/

and I think the ball is in the author's court to respond.

I'll mark the topic as Stalled in the draft of the next issue of "What's cooking" report.

Thanks.

On 2025-07-07 15:43:20 [-0700], Junio C Hamano wrote:

Sorry for the delay, thank you for the reminder.

Sebastian