[PATCH] compat/bswap.h: Simplify MSVC endianness detection

[PATCH] compat/bswap.h: Simplify MSVC endianness detection

[Daniel Gurney]

Modern MSVC or Windows versions don't support big-endian, so it's
unnecessary to consider architectures when using it.

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..72f225eaa8 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)
 
 #include <stdlib.h>
 
-- 
2.29.2

Re: [PATCH] compat/bswap.h: Simplify MSVC endianness detection

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On 2020-11-07 at 23:47:51, Daniel Gurney wrote:
> Modern MSVC or Windows versions don't support big-endian, so it's
> unnecessary to consider architectures when using it.
> 
> 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..72f225eaa8 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)
>  
>  #include <stdlib.h>
>  

The patch itself seems fine to me, of course.  For the message, it may
be valuable to mention that this enables the proper behavior for ARM64,
which was your original goal.  Also, by convention, we typically use a
lower case letter after the colon in the summary.

And of course, thanks for the patch.  I always have a soft spot for
patches that improve architecture support.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Daniel Gurney]

Modern MSVC or Windows versions don't support big-endian, so it's
unnecessary to consider architectures when using it.

This also makes ARM64 MSVC builds succeed.

Signed-off-by: Daniel Gurney <dgurney99@gmail.com>
---
v2: just edited the commit message as suggested by Brian.

 compat/bswap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/bswap.h b/compat/bswap.h
index c0bb744adc..72f225eaa8 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)
 
 #include <stdlib.h>
 
-- 
2.29.2

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Jeff King]

On Sun, Nov 08, 2020 at 11:57:41AM +0200, Daniel Gurney wrote:

> Modern MSVC or Windows versions don't support big-endian, so it's
> unnecessary to consider architectures when using it.

This made me wonder if we support any non-modern versions (which would
be negatively impacted).

From the earlier thread at [1], it sounds like probably not, but I
wonder if we can offer a stronger argument there (or just define
"modern" a little more clearly).

-Peff

[1] https://lore.kernel.org/git/20201107224747.GF6252@camp.crustytoothpaste.net/

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2266 bytes --]

On 2020-11-10 at 00:31:27, Jeff King wrote:
> On Sun, Nov 08, 2020 at 11:57:41AM +0200, Daniel Gurney wrote:
> 
> > Modern MSVC or Windows versions don't support big-endian, so it's
> > unnecessary to consider architectures when using it.
> 
> This made me wonder if we support any non-modern versions (which would
> be negatively impacted).

I'm pretty sure we don't.  As I said, we're using several C99 features
and that version precedes the C99 standard (and 1999).

> From the earlier thread at [1], it sounds like probably not, but I
> wonder if we can offer a stronger argument there (or just define
> "modern" a little more clearly).

According to Wikipedia[0]:

  Visual C++ 2013 [12.0] finally added support for various C99 features
  in its C mode (including designated initializers, compound literals,
  and the _Bool type), though it was still not complete. Visual C++ 2015
  further improved the C99 support, with full support of the C99
  Standard Library, except for features that require C99 language
  features not yet supported by the compiler.

The version mentioned that supported MIPS, Alpha, and m68k was Visual
C++ 2.0 RISC Edition.  While Wikipedia doesn't mention its release date,
its successor, Visual C++ 4.0, was released in 1995.  The m68k version
ran on Macs using those processors, and Apple abandoned m68k for PowerPC
in 1994[1].

I'm entirely comfortable with requiring that people use a compiler and
operating system newer than 25 years old to compile Git correctly.  As
I've said or implied in previous threads, I'm also fine requiring C99
(vendors having had over two decades to implement it) and only
supporting OSes with vendor security support, although obviously these
latter two items are much more controversial.

I'm fine leaving the commit message as it stands, given the brevity of
the patch and that in the technology field, the affected versions are
not in any way "modern," but of course I wouldn't object to a reroll.
It's fine, should that happen, to include any of this email in the
commit message.

[0] https://en.wikipedia.org/wiki/Microsoft_Visual_C%2B%2B
[1] https://en.wikipedia.org/wiki/Macintosh
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Junio C Hamano]

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

> On 2020-11-10 at 00:31:27, Jeff King wrote:
>> On Sun, Nov 08, 2020 at 11:57:41AM +0200, Daniel Gurney wrote:
>> 
>> > Modern MSVC or Windows versions don't support big-endian, so it's
>> > unnecessary to consider architectures when using it.
>> 
>> This made me wonder if we support any non-modern versions (which would
>> be negatively impacted).
>
> I'm pretty sure we don't.  As I said, we're using several C99 features
> and that version precedes the C99 standard (and 1999).
>
>> From the earlier thread at [1], it sounds like probably not, but I
>> wonder if we can offer a stronger argument there (or just define
>> "modern" a little more clearly).
>
> According to Wikipedia[0]:
>
>   Visual C++ 2013 [12.0] finally added support for various C99 features
>   in its C mode (including designated initializers, compound literals,
>   and the _Bool type), though it was still not complete. Visual C++ 2015
>   further improved the C99 support, with full support of the C99
>   Standard Library, except for features that require C99 language
>   features not yet supported by the compiler.
>
> The version mentioned that supported MIPS, Alpha, and m68k was Visual
> C++ 2.0 RISC Edition.  While Wikipedia doesn't mention its release date,
> its successor, Visual C++ 4.0, was released in 1995.  The m68k version
> ran on Macs using those processors, and Apple abandoned m68k for PowerPC
> in 1994[1].

So, 

	The only versions of MSVC that support big-endian are too
	ancient and do not understand some C99 features we use in
	our codebase, so it is unnecessary...

would be sufficient?

> I'm entirely comfortable with requiring that people use a compiler and
> operating system newer than 25 years old to compile Git correctly.  As
> I've said or implied in previous threads, I'm also fine requiring C99
> (vendors having had over two decades to implement it) and only
> supporting OSes with vendor security support, although obviously these
> latter two items are much more controversial.

Maybe controversial, but worth at least laying the ground rules
ahead of time?

Do we have any specific feature we avoid only due to portability
concerns?  Declaring an identifier in the first part of for() is the
only thing that comes to my mind, but there may be others.  I think
we should consider how well each individual feature is supported by
systems we care about as we feel the need.

Thanks.

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Johannes Schindelin]

Hi Junio,

On Mon, 9 Nov 2020, Junio C Hamano wrote:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > On 2020-11-10 at 00:31:27, Jeff King wrote:
> >> On Sun, Nov 08, 2020 at 11:57:41AM +0200, Daniel Gurney wrote:
> >>
> >> > Modern MSVC or Windows versions don't support big-endian, so it's
> >> > unnecessary to consider architectures when using it.
> >>
> >> This made me wonder if we support any non-modern versions (which would
> >> be negatively impacted).
> >
> > I'm pretty sure we don't.  As I said, we're using several C99 features
> > and that version precedes the C99 standard (and 1999).
> >
> >> From the earlier thread at [1], it sounds like probably not, but I
> >> wonder if we can offer a stronger argument there (or just define
> >> "modern" a little more clearly).
> >
> > According to Wikipedia[0]:
> >
> >   Visual C++ 2013 [12.0] finally added support for various C99 features
> >   in its C mode (including designated initializers, compound literals,
> >   and the _Bool type), though it was still not complete. Visual C++ 2015
> >   further improved the C99 support, with full support of the C99
> >   Standard Library, except for features that require C99 language
> >   features not yet supported by the compiler.
> >
> > The version mentioned that supported MIPS, Alpha, and m68k was Visual
> > C++ 2.0 RISC Edition.  While Wikipedia doesn't mention its release date,
> > its successor, Visual C++ 4.0, was released in 1995.  The m68k version
> > ran on Macs using those processors, and Apple abandoned m68k for PowerPC
> > in 1994[1].
>
> So,
>
> 	The only versions of MSVC that support big-endian are too
> 	ancient and do not understand some C99 features we use in
> 	our codebase, so it is unnecessary...
>
> would be sufficient?
>
> > I'm entirely comfortable with requiring that people use a compiler and
> > operating system newer than 25 years old to compile Git correctly.  As
> > I've said or implied in previous threads, I'm also fine requiring C99
> > (vendors having had over two decades to implement it) and only
> > supporting OSes with vendor security support, although obviously these
> > latter two items are much more controversial.
>
> Maybe controversial, but worth at least laying the ground rules
> ahead of time?
>
> Do we have any specific feature we avoid only due to portability
> concerns?  Declaring an identifier in the first part of for() is the
> only thing that comes to my mind, but there may be others.  I think
> we should consider how well each individual feature is supported by
> systems we care about as we feel the need.

I would feel a bit more comfortable reintroducing the part that
specifically checks for x86, x86_64 and ARM64, for the reasons I outlined
in my reply to a previous version of this patch: just because the MSVC
versions we can currently use to build Git currently only supports little
endian does not mean that all future versions will do. Point in reference:
you can build Linux applications in Visual Studio like _right now_ [*1*].

Ciao,
Dscho

Footnote *1*: It currently uses GCC, but who says it always will?
https://docs.microsoft.com/en-us/cpp/linux/cmake-linux-project

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Jeff King]

On Tue, Nov 10, 2020 at 02:36:20AM +0000, brian m. carlson wrote:

> > > Modern MSVC or Windows versions don't support big-endian, so it's
> > > unnecessary to consider architectures when using it.
> > 
> > This made me wonder if we support any non-modern versions (which would
> > be negatively impacted).
> 
> I'm pretty sure we don't.  As I said, we're using several C99 features
> and that version precedes the C99 standard (and 1999).

Your response was much more thorough than I anticipated. What I was
really going for in the commit message was just laying out "modern" a
bit more clearly, like:

  No version of MSVC or Windows has supported a big-endian platform
  since the mid-90's. Git wouldn't build with these pre-C99 compilers
  these days anyway, so we can assume that MSVC is always little-endian.

> I'm fine leaving the commit message as it stands, given the brevity of
> the patch and that in the technology field, the affected versions are
> not in any way "modern," but of course I wouldn't object to a reroll.
> It's fine, should that happen, to include any of this email in the
> commit message.

I often find that the briefer the patch, the more I need to revisit the
assumptions of the author (myself included!). :)

But I am also OK either way; when commit messages are insufficient, I
often go back to the list archive to get more details, and now it's well
documented here.

-Peff

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Daniel Gurney]

Hello,

On 10/11/2020 16:04, Johannes Schindelin wrote:
 > Point in reference:
 > you can build Linux applications in Visual Studio like _right now_ [*1*].
 >
 > Ciao,
 > Dscho
 >
 > Footnote *1*: It currently uses GCC, but who says it always will?
 > https://docs.microsoft.com/en-us/cpp/linux/cmake-linux-project

When it comes to building C++ code MSVC uses Microsoft's C++ Standard 
Library, and one of its stated non-goals[1] is being ported to other 
systems. I assume the same applies for their C library. Therefore to me 
a scenario where a future version of MSVC would build non-Windows code, 
let alone for a big-endian architecture, seems extremely unlikely.

That said, I understand your overall point of view perfectly, and I'm 
fine with either one of my patches being applied since the end result 
today is the same.

- Daniel

[1] https://github.com/microsoft/STL#non-goals

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Johannes Schindelin]

Hi Daniel,

On Tue, 10 Nov 2020, Daniel Gurney wrote:

> On 10/11/2020 16:04, Johannes Schindelin wrote:
> > Point in reference:
> > you can build Linux applications in Visual Studio like _right now_ [*1*].
> >
> > Ciao,
> > Dscho
> >
> > Footnote *1*: It currently uses GCC, but who says it always will?
> > https://docs.microsoft.com/en-us/cpp/linux/cmake-linux-project
>
> When it comes to building C++ code MSVC uses Microsoft's C++ Standard Library,
> and one of its stated non-goals[1] is being ported to other systems. I assume
> the same applies for their C library. Therefore to me a scenario where a
> future version of MSVC would build non-Windows code, let alone for a
> big-endian architecture, seems extremely unlikely.

Well, we know for a fact that at least XBox is a big-endian system and
ther are rumors out there that Visual Studio _might_ be used to build
software for that system.

Of course, I am not suggesting that we should build Git for XBox. But it
seems dangerous to me to make too many assumptions.

> That said, I understand your overall point of view perfectly, and I'm fine
> with either one of my patches being applied since the end result today is the
> same.

As a maintainer, I am less concerned about the "result today" than I am
about keeping things easy and effortless to maintain. One of your patches
accomplishes that. The other one made it into `next`:
https://github.com/git/git/commit/91a67b86f77

Ciao,
Dscho

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> As a maintainer, I am less concerned about the "result today" than I am
> about keeping things easy and effortless to maintain. One of your patches
> accomplishes that. The other one made it into `next`:
> https://github.com/git/git/commit/91a67b86f77

I do not think reverting it and requeuing

https://lore.kernel.org/git/20201107221916.1428757-1-dgurney99@gmail.com/

would help future folks why we ignore _MSC_VER as any sign usable to
detect endianness, so I'd prefer to see a patch *on top* of 1af265f0
(compat/bswap.h: simplify MSVC endianness detection, 2020-11-08),
which is 91a67b86f77^2, that explains why we prefer to list archs
explicitly in its log message, which would be the primary value of
that commit.

Something along this line, perhaps?

-- >8 --

Subject: compat/bswap.h: do not assume MSVC is little-endian only

Earlier, with 1af265f0 (compat/bswap.h: simplify MSVC endianness
detection, 2020-11-08), we tried to simplify endianness detection
used in compat/bswap.h by assuming that any version Git compiled by
MSVC (detected by _MSC_VER preprocessor macro) is meant to run on
little endian boxes, as the versions of old MSVC that support m68k
and MIPS do not support some C99 features used in the codebase
anyway.

While it might hold true that modern versions of Windows are all
little-endian, MSVC is and/or can be ported to build for big-endian
boxes, so tying _MSC_VER with endianness is a bit too restrictive.

Let's go back to the old way to use _MSC_VER to learn what
preprocessor macros compiler uses to tell us which arch we are
building for, and list these arches that are little-endian
explicitly.

... signed-off-by from you and helped-by from others ...
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
    diffstat
    patch

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> As a maintainer, I am less concerned about the "result today" than I am
>> about keeping things easy and effortless to maintain. One of your patches
>> accomplishes that. The other one made it into `next`:
>> https://github.com/git/git/commit/91a67b86f77
>
> I do not think reverting it and requeuing
>
> https://lore.kernel.org/git/20201107221916.1428757-1-dgurney99@gmail.com/
>
> would help future folks why we ignore _MSC_VER as any sign usable to
> detect endianness, so I'd prefer to see a patch *on top* of 1af265f0
> (compat/bswap.h: simplify MSVC endianness detection, 2020-11-08),
> which is 91a67b86f77^2, that explains why we prefer to list archs
> explicitly in its log message, which would be the primary value of
> that commit.
>
> Something along this line, perhaps?
>
> -- >8 --
>
> Subject: compat/bswap.h: do not assume MSVC is little-endian only
>
> Earlier, with 1af265f0 (compat/bswap.h: simplify MSVC endianness
> detection, 2020-11-08), we tried to simplify endianness detection
> used in compat/bswap.h by assuming that any version Git compiled by
> MSVC (detected by _MSC_VER preprocessor macro) is meant to run on
> little endian boxes, as the versions of old MSVC that support m68k
> and MIPS do not support some C99 features used in the codebase
> anyway.
>
> While it might hold true that modern versions of Windows are all
> little-endian, MSVC is and/or can be ported to build for big-endian
> boxes, so tying _MSC_VER with endianness is a bit too restrictive.
>
> Let's go back to the old way to use _MSC_VER to learn what
> preprocessor macros compiler uses to tell us which arch we are
> building for, and list these arches that are little-endian
> explicitly.
>
> ... signed-off-by from you and helped-by from others ...
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>     diffstat
>     patch

Daniel's patch adds _M_ARM64 to the list, but do we need to do
anything further to tell the endian on such a bi-endian arch, or
does MSVC only support little-endian for that architecture?  

Just double-checking as the "confusion" that started this thread
came from an assumption that MSVC == Windows == big-endian, and you
told us MSVC != Windows.  Now the patch assumes ARM64-on-MSVC is
little-endian only and we want to make sure that assumption is true.

And perhaps it is worth documenting in the log, perhaps

	... that are little-endian explicitly.  Note that ARM64 is
	bi-endian in nature but we treat it little-endian as MSVC
	does not treat the arch as bi-endian.

or something like that at the end (I do not know what MSVC actually
does---just illustrating the level of details I expect in the
explanation).

Thanks.

Re: [PATCH v2] compat/bswap.h: simplify MSVC endianness detection

[Johannes Schindelin]

Hi Junio,

On Tue, 10 Nov 2020, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> As a maintainer, I am less concerned about the "result today" than I am
> >> about keeping things easy and effortless to maintain. One of your patches
> >> accomplishes that. The other one made it into `next`:
> >> https://github.com/git/git/commit/91a67b86f77
> >
> > I do not think reverting it and requeuing
> >
> > https://lore.kernel.org/git/20201107221916.1428757-1-dgurney99@gmail.com/
> >
> > would help future folks why we ignore _MSC_VER as any sign usable to
> > detect endianness, so I'd prefer to see a patch *on top* of 1af265f0
> > (compat/bswap.h: simplify MSVC endianness detection, 2020-11-08),
> > which is 91a67b86f77^2, that explains why we prefer to list archs
> > explicitly in its log message, which would be the primary value of
> > that commit.
> >
> > Something along this line, perhaps?
> >
> > -- >8 --
> >
> > Subject: compat/bswap.h: do not assume MSVC is little-endian only
> >
> > Earlier, with 1af265f0 (compat/bswap.h: simplify MSVC endianness
> > detection, 2020-11-08), we tried to simplify endianness detection
> > used in compat/bswap.h by assuming that any version Git compiled by
> > MSVC (detected by _MSC_VER preprocessor macro) is meant to run on
> > little endian boxes, as the versions of old MSVC that support m68k
> > and MIPS do not support some C99 features used in the codebase
> > anyway.
> >
> > While it might hold true that modern versions of Windows are all
> > little-endian, MSVC is and/or can be ported to build for big-endian
> > boxes, so tying _MSC_VER with endianness is a bit too restrictive.
> >
> > Let's go back to the old way to use _MSC_VER to learn what
> > preprocessor macros compiler uses to tell us which arch we are
> > building for, and list these arches that are little-endian
> > explicitly.
> >
> > ... signed-off-by from you and helped-by from others ...
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
> >     diffstat
> >     patch
>
> Daniel's patch adds _M_ARM64 to the list, but do we need to do
> anything further to tell the endian on such a bi-endian arch, or
> does MSVC only support little-endian for that architecture?

Yes, MSVC only supports little-endian for that architecture. From
https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=msvc-160#endianness:

	Endianness

	As with the ARM32 version of Windows, on ARM64 Windows executes in
	little-endian mode. Switching endianness is difficult to achieve
	without kernel mode support in AArch64, so it's easier to enforce.

> Just double-checking as the "confusion" that started this thread
> came from an assumption that MSVC == Windows == big-endian, and you
> told us MSVC != Windows.  Now the patch assumes ARM64-on-MSVC is
> little-endian only and we want to make sure that assumption is true.

I did not say that MSVC != Windows, but Visual Studio != Windows. But I
did say that I do not want to assume MSVC == Windows for all eternity.

> And perhaps it is worth documenting in the log, perhaps
>
> 	... that are little-endian explicitly.  Note that ARM64 is
> 	bi-endian in nature but we treat it little-endian as MSVC
> 	does not treat the arch as bi-endian.
>
> or something like that at the end (I do not know what MSVC actually
> does---just illustrating the level of details I expect in the
> explanation).

Absolutely. If nobody beats me to the punch, I hope to get around to
prepare the patch that you suggested.

Thanks,
Dscho