{"thread":{"id":"54597","subject":"[PATCH] compat/bswap.h: detect ARM64 when using MSVC","startedAt":"2020-11-07T22:21:39Z","lastAt":"2020-11-10T23:39:29Z","messageCount":6,"participants":["Daniel Gurney","brian m. carlson","Johannes Schindelin","Sebastian Schuberth"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"409326","messageId":"20201107221916.1428757-1-dgurney99@gmail.com","threadId":"54597","inReplyTo":null,"subject":"[PATCH] compat/bswap.h: detect ARM64 when using MSVC","fromName":"Daniel Gurney","fromEmail":"dgurney99@gmail.com","sentAt":"2020-11-07T22:19:16Z","receivedAt":"2020-11-07T22:21:39Z","isPatch":true,"sender":{"key":"dgurney99@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12816807?v=4"},"body":"Signed-off-by: Daniel Gurney \n---\n compat/bswap.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/bswap.h b/compat/bswap.h\nindex c0bb744adc..512f6f4b99 100644\n--- a/compat/bswap.h\n+++ b/compat/bswap.h\n@@ -74,7 +74,7 @@ static inline uint64_t git_bswap64(uint64_t x)\n }\n #endif\n \n-#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))\n+#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))\n \n #include \n \n-- \n2.29.2\n\n"},{"id":"409327","messageId":"20201107224747.GF6252@camp.crustytoothpaste.net","threadId":"54597","inReplyTo":"20201107221916.1428757-1-dgurney99@gmail.com","subject":"Re: [PATCH] compat/bswap.h: detect ARM64 when using MSVC","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-11-07T22:47:47Z","receivedAt":"2020-11-07T22:50:46Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-11-07 at 22:19:16, Daniel Gurney wrote:\n> Signed-off-by: Daniel Gurney \n> ---\n> compat/bswap.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/compat/bswap.h b/compat/bswap.h\n> index c0bb744adc..512f6f4b99 100644\n> --- a/compat/bswap.h\n> +++ b/compat/bswap.h\n> @@ -74,7 +74,7 @@ static inline uint64_t git_bswap64(uint64_t x)\n> }\n> #endif\n> \n> -#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))\n> +#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))\n> \n> #include \n> \n\nI think this is fine as it is, but I have a question here: why, if we're\nusing MSVC, is that not sufficient to enable this? In other words, why\ncan't this line simply be this:\n\n #elif defined(_MSC_VER)\n\nAs far as I know, Windows has always run on little-endian hardware. It\nlooks like MSVC did run on the M68000 series and MIPS[0] at some point.\nAre those really versions of MSVC we care about and think Git can\npractically support, given the fact that we require so many C99\nconstructs that are not practically available in old versions of MSVC?\nIf not, wouldn't it make sense to simplify?\n\n[0] Wikipedia does not specify the endiannesses supported by the MIPS\nedition.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"},{"id":"409328","messageId":"CAGeMgCda8N9qGWqLxX_T+8C4ycczMkRo8mZfoLwSD2DGBPYuLw@mail.gmail.com","threadId":"54597","inReplyTo":"20201107224747.GF6252@camp.crustytoothpaste.net","subject":"Re: [PATCH] compat/bswap.h: detect ARM64 when using MSVC","fromName":"Daniel Gurney","fromEmail":"dgurney99@gmail.com","sentAt":"2020-11-07T23:23:11Z","receivedAt":"2020-11-07T23:23:51Z","isPatch":true,"sender":{"key":"dgurney99@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12816807?v=4"},"body":"> As far as I know, Windows has always run on little-endian hardware.\n\nThat's a fair point, and I doubt there'll be any new developments in\nthis regard. I'll send a new patch with the simplification you\nsuggested.\n"},{"id":"409485","messageId":"nycvar.QRO.7.76.6.2011101418550.18437@tvgsbejvaqbjf.bet","threadId":"54597","inReplyTo":"20201107224747.GF6252@camp.crustytoothpaste.net","subject":"Re: [PATCH] compat/bswap.h: detect ARM64 when using MSVC","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-10T13:58:21Z","receivedAt":"2020-11-10T13:58:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi brian & Daniel,\n\nOn Sat, 7 Nov 2020, brian m. carlson wrote:\n\n> On 2020-11-07 at 22:19:16, Daniel Gurney wrote:\n> > Signed-off-by: Daniel Gurney \n> > ---\n> > compat/bswap.h | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/compat/bswap.h b/compat/bswap.h\n> > index c0bb744adc..512f6f4b99 100644\n> > --- a/compat/bswap.h\n> > +++ b/compat/bswap.h\n> > @@ -74,7 +74,7 @@ static inline uint64_t git_bswap64(uint64_t x)\n> > }\n> > #endif\n> >\n> > -#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))\n> > +#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))\n> >\n> > #include \n> >\n>\n> I think this is fine as it is, but I have a question here: why, if we're\n> using MSVC, is that not sufficient to enable this? In other words, why\n> can't this line simply be this:\n>\n> #elif defined(_MSC_VER)\n\nThat is a good question. As far as I can see, this code path has been\nintroduced in 0fcabdeb52b (Use faster byte swapping when compiling with\nMSVC, 2009-10-19). Then commit looks like this:\n\n Author: Sebastian Schuberth \n Date: Mon Oct 19 18:37:05 2009 +0200\n Subject: Use faster byte swapping when compiling with MSVC\n\n When compiling with MSVC on x86-compatible, use an intrinsic for\n byte swapping. In contrast to the GCC path, we do not prefer\n inline assembly here as it is not supported for the x64 platform.\n\n Signed-off-by: Sebastian Schuberth \n Signed-off-by: Junio C Hamano \n\n diff --git a/compat/bswap.h b/compat/bswap.h\n index 5cc4acbfcce..279e0b48b15 100644\n --- a/compat/bswap.h\n +++ b/compat/bswap.h\n @@ -28,6 +28,16 @@ static inline uint32_t default_swab32(uint32_t val)\n \t} \\\n \t__res; })\n\n +#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))\n +\n +#include \n +\n +#define bswap32(x) _byteswap_ulong(x)\n +\n +#endif\n +\n +#ifdef bswap32\n +\n #undef ntohl\n #undef htonl\n #define ntohl(x) bswap32(x)\n\nI Cc:ed Sebastian, to confirm my hunch: Looking a bit above that hunk, I\nsee that this merely imitates the way things are done for GCC:\n\n #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n\n [...]\n\nNow, let's have a slightly harder look at the patch under consideration:\nwhat this does is to re-use the (known to be little-endian) code path of\nx86 and x86_64 to define the `bswap32()` and `bswap64()` macros also for\nARM64.\n\nFurther down, the presence of these macros is taken for a sign that we\n_are_ little-endian and therefore the `ntol()` family of functions isn't a\nno-op, but has to swap the order, and that's what those macros are then\nused for.\n\nThe biggest question now is: are we certain that `_M_ARM64` implies\nlittle-endian?\n\nI remember that ARM (the 32-bit variety, that is) has support for\nswitching endianness on the fly. Happily, MSVC talks specifically about\n_not_ supporting that:\nhttps://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions\n\nLikewise, it says the same about ARM64 (mentioning that it would be much\nharder to switch endianness there to begin with):\nhttps://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions\n\nSo does that make us confident that we can just add that `_M_ARM64` part?\nYes. Does it make me confident that we can just drop all of the\narchitecture-dependent conditions? No, it does not. There _were_ versions\nof MSVC that could compile code for PowerPC, for example, which _is_\nbig-endian.\n\n> As far as I know, Windows has always run on little-endian hardware.\n\nI think that depends on your point of view... IIRC an early version of\nWindows NT (or was it still VMS Plus?) ran on DEC Alpha, which I seem to\n_vaguely_ remember was big-endian.\n\n> It looks like MSVC did run on the M68000 series and MIPS[0] at some\n> point. Are those really versions of MSVC we care about and think Git can\n> practically support, given the fact that we require so many C99\n> constructs that are not practically available in old versions of MSVC?\n> If not, wouldn't it make sense to simplify?\n\nIndeed, and it is more than just the C99 constructs that prevent us from\nsupporting other MSVC versions: we do not support compiling with MSVC out\nof the box. Nowadays, we rely on CMake to generate the appropriate files\nto allow us to build with MSVC because our Makefile-based approach very\nmuch hardcodes GCC (or compatible) options.\n\n>\n> [0] Wikipedia does not specify the endiannesses supported by the MIPS\n> edition.\n\nI have another vague memory about MIPS (a wonderful SGI machine I had the\npleasure of banging my head against, for lack of Python support and Git\nrequiring Python back then) being big-endian, too.\n\nShort version: while I managed to convince myself that _currently_ there\nare no big-endian platforms that we can support via MSVC, I would like to\nstay within the boundaries of caution and _not_ drop those `defined(_M_*)`\nparts.\n\nCiao,\nDscho\n"},{"id":"409497","messageId":"CAHGBnuM3JeffB73coQhVOC03tiWhg2VY=teSr-Jmnx5-aX48BA@mail.gmail.com","threadId":"54597","inReplyTo":"nycvar.QRO.7.76.6.2011101418550.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] compat/bswap.h: detect ARM64 when using MSVC","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2020-11-10T16:44:21Z","receivedAt":"2020-11-10T16:44:37Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Nov 10, 2020 at 2:58 PM Johannes Schindelin\n wrote:\n\n> > -#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))\n> > +#elif defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64) || defined(_M_ARM64))\n\n> I Cc:ed Sebastian, to confirm my hunch: Looking a bit above that hunk, I\n> see that this merely imitates the way things are done for GCC:\n>\n> #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))\n\nI believe my intention was not necessarily to imitate the way things\nare done for GCC, but to independently be on the safe side by checking\nthat this code is only used on x86-style / little-endian architecture.\n\n> > As far as I know, Windows has always run on little-endian hardware.\n>\n> I think that depends on your point of view... IIRC an early version of\n> Windows NT (or was it still VMS Plus?) ran on DEC Alpha, which I seem to\n> _vaguely_ remember was big-endian.\n\nIMO, strictly speaking, from a semantic point of view this is not\nabout which OS we are running on, but about which compiler is being\nused. So the question here is: Can MSVC compile for a non-little\nendian target platform (incl. things like cross-compilation)? And\nAFAIK the answer is yes, it could in the past / still can nowadays.\n\n> Short version: while I managed to convince myself that _currently_ there\n> are no big-endian platforms that we can support via MSVC, I would like to\n> stay within the boundaries of caution and _not_ drop those `defined(_M_*)`\n> parts.\n\nSame here, I'd prefer to keep these for explicitness, and for\nconsistency with the GCC check.\n\n-- \nSebastian Schuberth\n"},{"id":"409567","messageId":"20201110233850.GJ6252@camp.crustytoothpaste.net","threadId":"54597","inReplyTo":"nycvar.QRO.7.76.6.2011101418550.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] compat/bswap.h: detect ARM64 when using MSVC","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-11-10T23:38:50Z","receivedAt":"2020-11-10T23:39:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-11-10 at 13:58:21, Johannes Schindelin wrote:\n\n> The biggest question now is: are we certain that `_M_ARM64` implies\n> little-endian?\n\nFor Windows? Yes. I'm almost certain Windows has only supported\nlittle-endian architectures, possibly with the exception of gaming\nconsoles.\n\n> I remember that ARM (the 32-bit variety, that is) has support for\n> switching endianness on the fly. Happily, MSVC talks specifically about\n> _not_ supporting that:\n> https://docs.microsoft.com/en-us/cpp/build/overview-of-arm-abi-conventions\n> \n> Likewise, it says the same about ARM64 (mentioning that it would be much\n> harder to switch endianness there to begin with):\n> https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions\n> \n> So does that make us confident that we can just add that `_M_ARM64` part?\n> Yes. Does it make me confident that we can just drop all of the\n> architecture-dependent conditions? No, it does not. There _were_ versions\n> of MSVC that could compile code for PowerPC, for example, which _is_\n> big-endian.\n\nPowerPC can actually be either. Most 64-bit PowerPC machines these days\nare run as little endian, and Windows has always run it in little-endian\nmode. Macs ran it in big-endian mode.\n\n> > As far as I know, Windows has always run on little-endian hardware.\n> \n> I think that depends on your point of view... IIRC an early version of\n> Windows NT (or was it still VMS Plus?) ran on DEC Alpha, which I seem to\n> _vaguely_ remember was big-endian.\n\nAlpha appears to have supported both, but as far as I know, both Windows\nand Linux used it in little-endian mode.\n\n> > [0] Wikipedia does not specify the endiannesses supported by the MIPS\n> > edition.\n> \n> I have another vague memory about MIPS (a wonderful SGI machine I had the\n> pleasure of banging my head against, for lack of Python support and Git\n> requiring Python back then) being big-endian, too.\n\nAnother architecture that supports both endiannesses. Debian supports\nboth, but I believe Windows only supported the little-endian version. I\nhave a small MIPS board that uses the little endian port for Debian.\n\n> Short version: while I managed to convince myself that _currently_ there\n> are no big-endian platforms that we can support via MSVC, I would like to\n> stay within the boundaries of caution and _not_ drop those `defined(_M_*)`\n> parts.\n\nWhile I'm confident in my statements, you're the relevant subsystem\nmaintainer here, so I'm happy to defer to your judgment. I think Junio\ncan just pick up the earlier patch version and we should be good to go,\nsince that patch seemed to meet everyone's needs.\n-- \nbrian m. carlson (he/him or they/them)\nHouston, Texas, US\n"}]}