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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi brian & Daniel,

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

> On 2020-11-07 at 22:19:16, Daniel Gurney wrote:
> > Signed-off-by: Daniel Gurney <dgurney99@gmail.com>
> > ---
> >  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 <sschuberth@gmail.com>
    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 <sschuberth@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

    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