Skip to content

Support Windows/ARM64 #1904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion compat/bswap.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,19 @@ static inline uint64_t default_bswap64(uint64_t val)
#undef bswap32
#undef bswap64

#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))
/**
* __has_builtin is available since Clang 10 and GCC 10.
* Below is a fallback for older compilers.
*/
#ifndef __has_builtin
#define __has_builtin(x) 0
#endif

#if __has_builtin(__builtin_bswap32) && __has_builtin(__builtin_bswap64)
#define bswap32(x) __builtin_bswap32((x))
#define bswap64(x) __builtin_bswap64((x))

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

#define bswap32 git_bswap32
static inline uint32_t git_bswap32(uint32_t x)
Expand Down
18 changes: 14 additions & 4 deletions config.mak.uname
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,11 @@ ifeq ($(uname_S),Windows)
ifeq (MINGW32,$(MSYSTEM))
prefix = /mingw32
else
prefix = /mingw64
ifeq (CLANGARM64,$(MSYSTEM))
prefix = /clangarm64
else
prefix = /mingw64
endif
endif
# Prepend MSVC 64-bit tool-chain to PATH.
#
Expand Down Expand Up @@ -485,7 +489,7 @@ ifeq ($(uname_S),Windows)
NO_POSIX_GOODIES = UnfortunatelyYes
NATIVE_CRLF = YesPlease
DEFAULT_HELP_FORMAT = html
ifeq (/mingw64,$(subst 32,64,$(prefix)))
ifeq (/mingw64,$(subst 32,64,$(subst clangarm,mingw,$(prefix))))
# Move system config into top-level /etc/
ETC_GITCONFIG = ../etc/gitconfig
ETC_GITATTRIBUTES = ../etc/gitattributes
Expand Down Expand Up @@ -724,6 +728,10 @@ ifeq ($(uname_S),MINGW)
prefix = /mingw64
HOST_CPU = x86_64
BASIC_LDFLAGS += -Wl,--pic-executable,-e,mainCRTStartup
else ifeq (CLANGARM64,$(MSYSTEM))
prefix = /clangarm64
HOST_CPU = aarch64
BASIC_LDFLAGS += -Wl,--pic-executable,-e,mainCRTStartup
else
COMPAT_CFLAGS += -D_USE_32BIT_TIME_T
BASIC_LDFLAGS += -Wl,--large-address-aware
Expand All @@ -738,8 +746,10 @@ ifeq ($(uname_S),MINGW)
HAVE_LIBCHARSET_H = YesPlease
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Mon, Apr 21, 2025 at 12:39:07PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> 
> It does not compile there, and seeing as nedmalloc has been pretty much
> unmaintained since at least November 2017, as per
> https://github.com/ned14/nedmalloc/issues/20#issuecomment-343432314,
> there is also no hope that any fixes will materialize there.

This kind of raises the question whether we want to keep on maintaining
nedmalloc in our codebase at all. Is there any strong reason to have it?

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Patrick,

On Tue, 22 Apr 2025, Patrick Steinhardt wrote:

> On Mon, Apr 21, 2025 at 12:39:07PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[email protected]>
> > 
> > It does not compile there, and seeing as nedmalloc has been pretty much
> > unmaintained since at least November 2017, as per
> > https://github.com/ned14/nedmalloc/issues/20#issuecomment-343432314,
> > there is also no hope that any fixes will materialize there.
> 
> This kind of raises the question whether we want to keep on maintaining
> nedmalloc in our codebase at all. Is there any strong reason to have it?

To the contrary, There is a very strong reason to drop it: nedmalloc is
unmaintained.

There is just a teeny tiny blocker before it can be dropped, though:

$ git grep -n USE_NED_ALLOCATOR upstream/master -- ':(exclude)Makefile'
upstream/master:config.mak.uname:478:   # USE_NED_ALLOCATOR = YesPlease
upstream/master:config.mak.uname:741:   USE_NED_ALLOCATOR = YesPlease
upstream/master:contrib/buildsystems/CMakeLists.txt:258:                                USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP

The commented-out one is the MSVC build (I had experimental Git for
Windows patches to enable nedmalloc even in MSVC builds, which I abandoned
in favor of https://github.com/git-for-windows/git/pull/4580 to enable
mimalloc in MSVC builds, but I abandoned that effort, too, because Git
decided to favor Meson over first-class MSVC support and I decided to
focus on avoiding to have my time wasted by the Git project).

As you are quite aware (because it caused plenty of trouble with your
reftable patch series), Git for Windows switched to mimalloc quite a while
ago (https://github.com/microsoft/mimalloc).

When I switched Git for Windows to mimalloc, I did (re-)run a couple of
performance tests to see whether having a custom allocator is still
necessary, and from my (unfortunately too vague) recollection, Windows
11's default allocator seems to have performed quite well in comparison.
Which is in stark contrast to the results of the performance tests I ran
when originally integrating nedmalloc. So: In theory, Git for Windows
could drop building with a custom allocator, iff it wasn't for older
Windows version that are still supported.

Which means that I would like to upstream the vendored-in mimalloc first,
with the patch to use it when building on Windows by default, before
dropping nedmalloc from Git's source code.

Ciao,
Johannes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> -	USE_NED_ALLOCATOR = YesPlease
> +	ifneq (CLANGARM64,$(MSYSTEM))
> +		USE_NED_ALLOCATOR = YesPlease
> +	endif
>          ifeq (/mingw64,$(subst 32,64,$(prefix)))

Notice the funny indentation above?  It turns out that the one in
the context that looks funnily indented uses the "correct"
indentation, which is quite counter-intuitive and confusing X-<.

I forgot about the rule while reviewing the previous round, but we
had to prepare for newer GNU make with commits like c18400c6
(Makefile(s): avoid recipe prefix in conditional statements,
2024-04-08).  In short, the conditionals like ifn?eq, ifn?def, else,
endif should not be indented with HT and we instead want them to be
indented with SP.

    $ git diff master... config.mak.uname |
      grep -E -e '^[+]       +(ifn?eq|ifn?def|else|endif)'

found them in a few patches in the series that touch
config.mak.uname

    msvc: do handle builds on Windows/ARM64
    mingw: do not use nedmalloc on Windows/ARM64

Fix-up for "mingw: do not use nedmalloc on Windows/ARM64"

 config.mak.uname | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git c/config.mak.uname w/config.mak.uname
index 6222d2c5a4..3ec82d95e6 100644
--- c/config.mak.uname
+++ w/config.mak.uname
@@ -742,9 +742,9 @@ ifeq ($(uname_S),MINGW)
 	HAVE_LIBCHARSET_H = YesPlease
 	USE_GETTEXT_SCHEME = fallthrough
 	USE_LIBPCRE = YesPlease
-	ifneq (CLANGARM64,$(MSYSTEM))
+        ifneq (CLANGARM64,$(MSYSTEM))
 		USE_NED_ALLOCATOR = YesPlease
-	endif
+        endif
         ifeq (/mingw64,$(subst 32,64,$(prefix)))
 		# Move system config into top-level /etc/
 		ETC_GITCONFIG = ../etc/gitconfig


Fix-up for "msvc: do handle builds on Windows/ARM64"
    
 config.mak.uname | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git c/config.mak.uname w/config.mak.uname
index 4831e9ccf6..4ef453ebcd 100644
--- c/config.mak.uname
+++ w/config.mak.uname
@@ -432,11 +432,11 @@ ifeq ($(uname_S),Windows)
         ifeq (MINGW32,$(MSYSTEM))
 		prefix = /mingw32
         else
-		ifeq (CLANGARM64,$(MSYSTEM))
+                ifeq (CLANGARM64,$(MSYSTEM))
 			prefix = /clangarm64
-		else
+                else
 			prefix = /mingw64
-		endif
+                endif
         endif
 	# Prepend MSVC 64-bit tool-chain to PATH.
 	#

Taken as a whole, here is a range-diff of what I will queue based on
this iteration.

Thanks.

1:  da1408a34e ! 1:  734bf24007 mingw: do not use nedmalloc on Windows/ARM64
    @@ Commit message
         there is also no hope that any fixes will materialize there.
     
         Signed-off-by: Johannes Schindelin <[email protected]>
    +    [jc: adjust config.mak.uname for c18400c6]
         Signed-off-by: Junio C Hamano <[email protected]>
     
      ## config.mak.uname ##
    @@ config.mak.uname: ifeq ($(uname_S),MINGW)
      	USE_GETTEXT_SCHEME = fallthrough
      	USE_LIBPCRE = YesPlease
     -	USE_NED_ALLOCATOR = YesPlease
    -+	ifneq (CLANGARM64,$(MSYSTEM))
    ++        ifneq (CLANGARM64,$(MSYSTEM))
     +		USE_NED_ALLOCATOR = YesPlease
    -+	endif
    ++        endif
              ifeq (/mingw64,$(subst 32,64,$(prefix)))
      		# Move system config into top-level /etc/
      		ETC_GITCONFIG = ../etc/gitconfig
2:  e27caa3dca ! 2:  8945fba590 msvc: do handle builds on Windows/ARM64
    @@ Commit message
         is time to do the same in the MS Visual C part.
     
         Signed-off-by: Johannes Schindelin <[email protected]>
    +    [jc: adjust config.mak.uname for c18400c6]
         Signed-off-by: Junio C Hamano <[email protected]>
     
      ## config.mak.uname ##
    @@ config.mak.uname: ifeq ($(uname_S),Windows)
      		prefix = /mingw32
              else
     -		prefix = /mingw64
    -+		ifeq (CLANGARM64,$(MSYSTEM))
    ++                ifeq (CLANGARM64,$(MSYSTEM))
     +			prefix = /clangarm64
    -+		else
    ++                else
     +			prefix = /mingw64
    -+		endif
    ++                endif
              endif
      	# Prepend MSVC 64-bit tool-chain to PATH.
      	#
3:  f1f6c1f2fa ! 3:  619950d421 mingw(arm64): do move the `/etc/git*` location
    @@ config.mak.uname: ifeq ($(uname_S),Windows)
      	ETC_GITCONFIG = ../etc/gitconfig
      	ETC_GITATTRIBUTES = ../etc/gitattributes
     @@ config.mak.uname: ifeq ($(uname_S),MINGW)
    - 	ifneq (CLANGARM64,$(MSYSTEM))
    +         ifneq (CLANGARM64,$(MSYSTEM))
      		USE_NED_ALLOCATOR = YesPlease
    - 	endif
    +         endif
     -        ifeq (/mingw64,$(subst 32,64,$(prefix)))
     +        ifeq (/mingw64,$(subst 32,64,$(subst clangarm,mingw,$(prefix))))
      		# Move system config into top-level /etc/
4:  687bd4ea96 = 4:  436a42215e max_tree_depth: lower it for clangarm64 on Windows

USE_GETTEXT_SCHEME = fallthrough
USE_LIBPCRE = YesPlease
USE_NED_ALLOCATOR = YesPlease
ifeq (/mingw64,$(subst 32,64,$(prefix)))
ifneq (CLANGARM64,$(MSYSTEM))
USE_NED_ALLOCATOR = YesPlease
endif
ifeq (/mingw64,$(subst 32,64,$(subst clangarm,mingw,$(prefix))))
# Move system config into top-level /etc/
ETC_GITCONFIG = ../etc/gitconfig
ETC_GITATTRIBUTES = ../etc/gitattributes
Expand Down
10 changes: 10 additions & 0 deletions environment.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ int max_allowed_tree_depth =
* the stack overflow can occur.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Mon, Apr 21, 2025 at 12:39:10PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/environment.c b/environment.c
> index 9e4c7781be0..cc853950bb2 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -82,9 +82,21 @@ int max_allowed_tree_depth =
>  	 * the stack overflow can occur.
>  	 */
>  	512;
> +#else
> +#if defined(GIT_WINDOWS_NATIVE) && defined(__clang__) && defined(__aarch64__)

Tiny nit, only because it puzzled me for a second: this should probably
be `#elif`.

> +	/*
> +	 * Similar to Visual C, it seems that on Windows/ARM64 the clang-based
> +	 * builds have a smaller stack space available. When running out of
> +	 * that stack space, a `STATUS_STACK_OVERFLOW` is produced. When the
> +	 * Git command was run from an MSYS2 Bash, this unfortunately results
> +	 * in an exit code 127. Let's prevent that by lowering the maximal
> +	 * tree depth; This value seems to be low enough.
> +	 */
> +	1280;
>  #else
>  	2048;
>  #endif
> +#endif

Hm. This whole construct feels rather awful, if you ask me. Instead of
papering over the issue it would be nice if we eventually fixed the root
cause, which is that we use recursion on a data structure that has an
unbounded depth in theory.

Anyway, that is clearly outside of the scope of this patch series, so
the bandaid is good enough for now.

Patrick

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Patrick,

On Tue, 22 Apr 2025, Patrick Steinhardt wrote:

> On Mon, Apr 21, 2025 at 12:39:10PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/environment.c b/environment.c
> > index 9e4c7781be0..cc853950bb2 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -82,9 +82,21 @@ int max_allowed_tree_depth =
> >  	 * the stack overflow can occur.
> >  	 */
> >  	512;
> > +#else
> > +#if defined(GIT_WINDOWS_NATIVE) && defined(__clang__) && defined(__aarch64__)
> 
> Tiny nit, only because it puzzled me for a second: this should probably
> be `#elif`.

I will change it.

> > +	/*
> > +	 * Similar to Visual C, it seems that on Windows/ARM64 the clang-based
> > +	 * builds have a smaller stack space available. When running out of
> > +	 * that stack space, a `STATUS_STACK_OVERFLOW` is produced. When the
> > +	 * Git command was run from an MSYS2 Bash, this unfortunately results
> > +	 * in an exit code 127. Let's prevent that by lowering the maximal
> > +	 * tree depth; This value seems to be low enough.
> > +	 */
> > +	1280;
> >  #else
> >  	2048;
> >  #endif
> > +#endif
> 
> Hm. This whole construct feels rather awful, if you ask me. Instead of
> papering over the issue it would be nice if we eventually fixed the root
> cause, which is that we use recursion on a data structure that has an
> unbounded depth in theory.

True.

It is also quite awful that I cannot find a way to represent
`STATUS_STACK_OVERFLOW` by anything else than exit code 127, which always
misleads me into thinking that an executable or a DLL might be missing.
But I did not find any.

> Anyway, that is clearly outside of the scope of this patch series, so
> the bandaid is good enough for now.

True enough!

Thank you,
Johannes

*/
512;
#elif defined(GIT_WINDOWS_NATIVE) && defined(__clang__) && defined(__aarch64__)
/*
* Similar to Visual C, it seems that on Windows/ARM64 the clang-based
* builds have a smaller stack space available. When running out of
* that stack space, a `STATUS_STACK_OVERFLOW` is produced. When the
* Git command was run from an MSYS2 Bash, this unfortunately results
* in an exit code 127. Let's prevent that by lowering the maximal
* tree depth; This value seems to be low enough.
*/
1280;
#else
2048;
#endif
Expand Down
Loading