Re: [PATCH] compat/bswap.h: detect ARM64 when using MSVC

Hi brian & Daniel,

On Sat, 7 Nov 2020, brian m. carlson wrote:

On 2020-11-07 at 22:19:16, Daniel Gurney wrote:

quoted

Signed-off-by: Daniel Gurney <redacted>
---
 compat/bswap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index c0bb744adc..512f6f4b99 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h

@@ -74,7 +74,7 @@ static inline uint64_t git_bswap64(uint64_t x)
 }
 #endif

-#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
+#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))

 #include <stdlib.h>

I think this is fine as it is, but I have a question here: why, if we're
using MSVC, is that not sufficient to enable this?  In other words, why
can't this line simply be this:

  #elif defined(_MSC_VER)

That is a good question. As far as I can see, this code path has been
introduced in 0fcabdeb52b (Use faster byte swapping when compiling with
MSVC, 2009-10-19). Then commit looks like this:

    Author: Sebastian Schuberth [off-list ref]
    Date:   Mon Oct 19 18:37:05 2009 +0200
    Subject: Use faster byte swapping when compiling with MSVC

    When compiling with MSVC on x86-compatible, use an intrinsic for
    byte swapping. In contrast to the GCC path, we do not prefer
    inline assembly here as it is not supported for the x64 platform.

    Signed-off-by: Sebastian Schuberth [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

    diff --git a/compat/bswap.h b/compat/bswap.h
    index 5cc4acbfcce..279e0b48b15 100644
    --- a/compat/bswap.h
    +++ b/compat/bswap.h
    @@ -28,6 +28,16 @@ static inline uint32_t default_swab32(uint32_t val)
     	} \
     	__res; })

    +#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))
    +
    +#include <stdlib.h>
    +
    +#define bswap32(x) _byteswap_ulong(x)
    +
    +#endif
    +
    +#ifdef bswap32
    +
     #undef ntohl
     #undef htonl
     #define ntohl(x) bswap32(x)

I Cc:ed Sebastian, to confirm my hunch: Looking a bit above that hunk, I
see that this merely imitates the way things are done for GCC:

    #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))

    [...]

Now, let's have a slightly harder look at the patch under consideration:
what this does is to re-use the (known to be little-endian) code path of
x86 and x86_64 to define the `bswap32()` and `bswap64()` macros also for
ARM64.

Further down, the presence of these macros is taken for a sign that we
_are_ little-endian and therefore the `ntol()` family of functions isn't a
no-op, but has to swap the order, and that's what those macros are then
used for.

The biggest question now is: are we certain that `_M_ARM64` implies
little-endian?

I remember that ARM (the 32-bit variety, that is) has support for
switching endianness on the fly. Happily, MSVC talks specifically about
_not_ supporting that:
https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions

Likewise, it says the same about ARM64 (mentioning that it would be much
harder to switch endianness there to begin with):
https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions

So does that make us confident that we can just add that `_M_ARM64` part?
Yes. Does it make me confident that we can just drop all of the
architecture-dependent conditions? No, it does not. There _were_ versions
of MSVC that could compile code for PowerPC, for example, which _is_
big-endian.

As far as I know, Windows has always run on little-endian hardware.

I think that depends on your point of view... IIRC an early version of
Windows NT (or was it still VMS Plus?) ran on DEC Alpha, which I seem to
_vaguely_ remember was big-endian.

It looks like MSVC did run on the M68000 series and MIPS[0] at some
point. Are those really versions of MSVC we care about and think Git can
practically support, given the fact that we require so many C99
constructs that are not practically available in old versions of MSVC?
If not, wouldn't it make sense to simplify?

Indeed, and it is more than just the C99 constructs that prevent us from
supporting other MSVC versions: we do not support compiling with MSVC out
of the box. Nowadays, we rely on CMake to generate the appropriate files
to allow us to build with MSVC because our Makefile-based approach very
much hardcodes GCC (or compatible) options.

[0] Wikipedia does not specify the endiannesses supported by the MIPS
edition.

I have another vague memory about MIPS (a wonderful SGI machine I had the
pleasure of banging my head against, for lack of Python support and Git
requiring Python back then) being big-endian, too.

Short version: while I managed to convince myself that _currently_ there
are no big-endian platforms that we can support via MSVC, I would like to
stay within the boundaries of caution and _not_ drop those `defined(_M_*)`
parts.

Ciao,
Dscho