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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
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
