Skip to content
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

[WIP] Enable building Git for Windows with LLVM/Clang #3427

Closed

Conversation

dennisameling
Copy link

@dennisameling dennisameling commented Sep 15, 2021

While working on Git for Windows ARM64 support, we came across the issue that MSVC-generated builds require Microsoft Visual C++ to be installed. This might have legal implications, not to mention the fact that an external program (VC++) has to be installed, which is problematic for downstream consumers like GitHub Desktop, which expect Git to "just work".

Luckily, since Git now supports CMake, we can also build Git with Clang using llvm-mingw. This can be done with cmake ..... -G "MinGW Makefiles" and then running make -j4 && make install.

Pending problems:

  • windres isn't working correctly (it's basically llvm-rc behind the scenes). It complains about the ProductVersion value that we try to pass to it. Will look into this.

@PhilipOakley
Copy link

Can you also check that any conflicts with #3306 are easy to resolve, as some were for ARM setup compatibility. Many thanks.

@dennisameling
Copy link
Author

Can you also check that any conflicts with #3306 are easy to resolve, as some were for ARM setup compatibility. Many thanks.

I'm not seeing anything strange in #3306, thanks for mentioning it. I'm not seeing any ARM-related fixes in that PR, is that correct? We already specify the VCPKG_ARCH in the GitHub Actions workflow (x64 or arm64). I use a similar command when building things locally, so all good from my side 👍🏼

@PhilipOakley
Copy link

the #3306 fix was related to the previous ARM changes, but for the Visual Studio "open the git folder" workflow, where no parameters are used when VS Ninja starts CMake and runs CMakeLists.txt without params, so needed a check to set the default arch.

Actually, I'd slightly misread one of the changes, which is the vcpkg_install.bat change (rather than the CMakeLists.txt) to add the IF NOT EXIST vcpkg ( line, for which I'm not clear on the reason for (is it catching a failure, or a prior install, or what?) - something for the full commit message when complete.

@dscho
Copy link
Member

dscho commented Oct 4, 2021

* The resulting binaries are huge.

Maybe they could be stripped? And: have you tried building with SKIP_DASHED_BUILT_INS?

* a Git clone partially works, but ends in a segmentation fault:

Were you able to run this in lldb?

@dennisameling
Copy link
Author

dennisameling commented Oct 23, 2021

Maybe they could be stripped? And: have you tried building with SKIP_DASHED_BUILT_INS?

SKIP_DASHED_BUILT_INS worked! The build artifacts are now only 160 MB when unzipped.

Were you able to run this in lldb?

Was able to get lldb to work, super insightful. Pretty sure this has to do with me messing with some header files in this PR. Will work on fixing that first

(lldb) run
Process 16532 launched: 'F:\git-sdk-64\usr\src\git\arelease\bin\git.exe' (x86_64)
Process 16532 stopped
* thread #1, stop reason = Exception 0xc0000005 encountered at address 0x7ff7a3c1254e: Access violation reading location 0x2f7cf354
    frame #0: 0x00007ff7a3c1254e git.exe`tm_to_time_t(tm=0x000000002f7cf340) at date.c:17:17
   14           static const int mdays[] = {
   15               0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
   16           };
-> 17           int year = tm->tm_year - 70;
   18           int month = tm->tm_mon;
   19           int day = tm->tm_mday;
   20

@dennisameling dennisameling force-pushed the build-with-llvm branch 2 times, most recently from 82e2018 to 9ca3275 Compare October 23, 2021 19:39
Nedmalloc has been EOL for a while according to its author (ned14/nedmalloc#20 (comment)):

"nedmalloc is pretty much EOL. Happy to accept patches, but unwilling to fix. You should use the system allocator on any recent OS instead."

It was also leading to compilation errors to ARM64 using llvm-mingw on Windows:

In file included from F:\git-sdk-64\usr\src\git\compat\nedmalloc\nedmalloc.c:63:
F:\git-sdk-64\usr\src\git\compat\nedmalloc/malloc.c.h:1404:57: error: static declaration of '_InterlockedExchange' follows non-static declaration
  static __inline__ __attribute__((always_inline)) long _InterlockedExchange(volatile long * const Target, const long Value)

Git will now use the built-in OS memory allocation on Windows instead.

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
- name: build packages
if: steps.cache-vcpkg.outputs.cache-hit != 'true'
shell: pwsh
run: .\compat\vcbuild\vcpkg_install.bat ${{ matrix.arch }}-windows-llvm-mingw
Copy link
Author

Choose a reason for hiding this comment

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

This whole vcpkg setup is rather hacky and while "it just works" (it uses my fork to enable building with llvm-mingw), I'd like to prevent using it in final builds, also since I don't want to bother you with another maintenance burden. vcpkg's only officially supported compiler on Windows is MSVC, which is yet another reason to not go down this path in the future. I'm pretty sure we can build straight off MINGW-packages when using the llvm-mingw compiler, which I'm also using for Git itself.

The good news is that the MSYS2 team has already been doing a fantastic job enabling all packages we need to build for clangarm64:

Will have a look in the coming days how I can build from MINGW-PACKAGES with the llvm-mingw compiler 👍🏼

Copy link
Author

Choose a reason for hiding this comment

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

It's actually even easier: since a month or two, the MSYS2 team is already providing ARM64 builds of their major packages, for example mingw-w64-clang-aarch64-openssl. I've incorporated that into the CI workflow together with the setup-msys2 GH Action which seems to be working really well 👍🏼 Very exciting!

Copy link
Member

Choose a reason for hiding this comment

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

Nice progress!

@dennisameling dennisameling force-pushed the build-with-llvm branch 7 times, most recently from ea85e40 to 154c582 Compare October 25, 2021 11:23
@dscho
Copy link
Member

dscho commented Oct 25, 2021

Maybe they could be stripped? And: have you tried building with SKIP_DASHED_BUILT_INS?

SKIP_DASHED_BUILT_INS worked! The build artifacts are now only 160 MB when unzipped.

Excellent!

Were you able to run this in lldb?

Was able to get lldb to work, super insightful. Pretty sure this has to do with me messing with some header files in this PR. Will work on fixing that first

(lldb) run
Process 16532 launched: 'F:\git-sdk-64\usr\src\git\arelease\bin\git.exe' (x86_64)
Process 16532 stopped
* thread #1, stop reason = Exception 0xc0000005 encountered at address 0x7ff7a3c1254e: Access violation reading location 0x2f7cf354
    frame #0: 0x00007ff7a3c1254e git.exe`tm_to_time_t(tm=0x000000002f7cf340) at date.c:17:17
   14           static const int mdays[] = {
   15               0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334
   16           };
-> 17           int year = tm->tm_year - 70;
   18           int month = tm->tm_mon;
   19           int day = tm->tm_mday;
   20

Yeah, that looks as if tm was dereferenced using a different idea of struct tm than the function caller's.

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
Clang 12 and GCC 10 added support for built-in bswap32 and bswap64. As bswap.h doesn't support ARM64/aarch64 when using GCC/Clang, this is a good opportunity to start using those built-ins. In the future, we might want to remove the custom git_bswap32 and git_bswap64 altogether and leave that up to the compiler to handle.

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
When developing locally, folks might want to run vcpkg_install.bat multiple times. When the vcpkg folder is already present, cloning it will obviously fail. We would probably prefer to only clone it when it's not already present.

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
While Clang is available on Windows, it uses the Visual Studio linker by default:

"For Windows projects, Visual Studio by default invokes Clang in clang-cl mode. It links with the Microsoft implementation of the Standard Library."

https://docs.microsoft.com/en-us/cpp/build/clang-support-msbuild?view=msvc-160

Since we want to build with the Clang/llvm-mingw toolchain, we have to get rid of some assumptions in the CMakeLists.txt file, as it's basically a GNU compiler and linker.

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
@dennisameling
Copy link
Author

Hi @dscho, I'm planning to continue working on this in the coming weeks. Since we're pretty close to a working build already, could we somehow arrange to get you an ECS LIVA QC710 for testing builds (and potentially adding it to CI)? Hopefully that should allow you to more confidently release ARM64 (beta) versions of Git for Windows.

Are there funds available in the Git for Windows project to get said device? Otherwise I'm happy to sponsor it. Many of my colleagues live in the US so I should be able to get the device shipped to Europe as well. Curious to hear your thoughts on this! 🙏🏼

@dscho
Copy link
Member

dscho commented Dec 17, 2021

Quite honestly, I'd rather try to use QEMU or something. Currently I am trying to follow https://github.com/jeremyd2019/winautoconfig/wiki/Windows-ARM64-VMs-with-QEMU in setting up something, but am stuck on not being able to get the very same Windows build as @jeremyd2019.

@jeremyd2019
Copy link

If you mean running TCG (emulating arm64 on x64), that is painfully slow. For instance, I have found the Git for Windows x64 installer seems to run essentially forever without showing GUI. It works fine in QEMU/KVM on ARM64 hardware (c6gd.metal instance in AWS, need a metal instance to be able to use KVM).

@dscho
Copy link
Member

dscho commented Dec 18, 2021

If you mean running TCG (emulating arm64 on x64), that is painfully slow. For instance, I have found the Git for Windows x64 installer seems to run essentially forever without showing GUI.

Agreed. In the six hours I tried, it did not even make it to the Windows logo.

@jeremyd2019
Copy link

jeremyd2019 commented Dec 18, 2021

Hmm, on my i7-2600 I can get to OOBE and/or the desktop at least. when I leave it to boot overnight. I would have expected better performance from something more modern.

Update: just tested, and got to point where OOBE would have run (if I hadn't used an answer file to disable it) in about 1 hour on that machine.

@jeremyd2019
Copy link

jeremyd2019 commented Dec 19, 2021

Oh, a tip if you're staring at the little dots going round "Getting ready", and wondering what it's up to, you can press Shift-F10 to get a command prompt, and run notepad c:\Windows\Panther\setupact.log. Most of the time I find it's getting the prepackaged 'modern' apps staged, so that's probably the part that takes the longest.

@dscho
Copy link
Member

dscho commented Dec 20, 2021

Update: just tested, and got to point where OOBE would have run (if I hadn't used an answer file to disable it) in about 1 hour on that machine.

Well, I have a few feelings about this. The most important ones:

  • It is way too difficult to set this up. Yes, you (@jeremyd2019) provided some documentation in https://github.com/jeremyd2019/winautoconfig/ and also on Gitter), but let's be honest: the information is still too scattered and too hard to follow.
  • One hour just to boot up that thing? I really expected something more in the realm of a few minutes (which is still much slower than I would expect a physical machine to boot up). With an hour just to boot, there is little chance that I can use this effectively in my work on Git for Windows.

I am currently pursuing a couple of other options that I hope will equip me with (temporary) access to a Windows/ARM64 setup, but with most people already out for the holidays, I don't think that I'll get anywhere before mid-January.

@jeremyd2019
Copy link

Agreed. TCG is just too slow to be useful. And running Windows for ARM in QEMU at all is fairly new and unsupported. That repo was basically just a place for me to keep my notes and scripts for doing so (and also auto-configuring it to be a runner), not really intended to be an exhaustive reference.

@dscho
Copy link
Member

dscho commented Dec 22, 2021

running Windows for ARM in QEMU at all is fairly new and unsupported.

It would currently be the only thing allowing me to work on Git for Windows/ARM64, though. If you have found any alternative, please let me know.

That repo was basically just a place for me to keep my notes and scripts for doing so (and also auto-configuring it to be a runner), not really intended to be an exhaustive reference.

It is sad that this repository is just a good start, and not being polished into truly being the exhaustive resource for people like me who do not want to dig into the details but just get something working so that I can tackle my actual project.

@jeremyd2019
Copy link

running Windows for ARM in QEMU at all is fairly new and unsupported.

It would currently be the only thing allowing me to work on Git for Windows/ARM64, though. If you have found any alternative, please let me know.

The performance you described (of being able to complete the "specialize" deployment phase in a few minutes instead of an hour) can be obtained from a Raspberry Pi.

That repo was basically just a place for me to keep my notes and scripts for doing so (and also auto-configuring it to be a runner), not really intended to be an exhaustive reference.

It is sad that this repository is just a good start, and not being polished into truly being the exhaustive resource for people like me who do not want to dig into the details but just get something working so that I can tackle my actual project.

I will continue to try to improve it, but it is also just enough for me to get something working so I can tackle my actual project. 😉

@dennisameling
Copy link
Author

@dscho anything I can do to help push this over the finish line? I can setup a VM on my Surface Pro X and give you access to it, so you can test things. I can just leave it on as I'm not actively using my SPX at the moment.

Moving forward, one thing we'll need to decide on is whether we'll cross-compile for ARM64 from a x64 host (which is not possible with MSYS2 AFAIK, but it's definitely possible with llvm-mingw as shown in this PR), or whether you want native ARM64 CI machines and do everything with MSYS2, which has a lot of native clangarm64 packages nowadays.

@dscho
Copy link
Member

dscho commented Mar 14, 2022

anything I can do to help push this over the finish line?

@dennisameling I see a couple of commits in this PR that still need work. Not all of them: The last two only need the commit message wrapped to <=76 columns/line, but are otherwise fine.

But the "Stop using nedmalloc on Windows" one, for example, completely neglects the fact that this would cause a serious performance regression. Regular Git for Windows is built with mingw-w64-gcc, meaning: It targets an older MSVCRT version, where the malloc() performance is sub-optimal. Even the latest UCRT versions I tried simply did not hold up to nedmalloc (my usual test is to repack a subset of the Linux kernel repository). For years now, I have been meaning to find the time to integrate mimalloc properly, but it seems as if higher-priority projects keep piling up. Here is my latest state, from October 2019: main...dscho:mimalloc. It did compile as far as I remember, but there were inexplicable hangs in Git's test suite that would obviously need to be addressed. Feel free to pick this up if you have time and are interested in replacing nedmalloc as Git for Windows' default allocator.

Another commit that will need a bit of work is 39bf8c4: I do not want to use regular MSYS2 to build Git for Windows. That would break the Git test suite (probably only when running the test suite, but still). Also, I am loathe to download third-party llvm-build rather than relying on official MSYS2 packages. And things like this probably want to be separated out into their own commits (in this instance, it should probably be done as part of git-extra.install).

And this commit probably wants to be dropped: f2bd10f

@jeremyd2019
Copy link

Also, I am loathe to download third-party llvm-build rather than relying on official MSYS2 packages.

Currently official MSYS2 doesn't provide all the bits necessary to cross-compile. I put together msys2/MINGW-packages#8762 as a sort of proof-of-concept, that does work for cross compiling, but doesn't really fit in with the autobuild system there at least (autobuild doesn't support cross-MSYSTEM dependencies other than the special case of MINGW-packages to MSYS2-packages).

@dscho
Copy link
Member

dscho commented Mar 16, 2022

Currently official MSYS2 doesn't provide all the bits necessary to cross-compile.

Hmm. I really would like to see that first, before starting to consume it to generate ARM64 artifacts in the Git for Windows project.

@dennisameling
Copy link
Author

Closing in favor of #3916, which uses MSYS' CLANGARM64 toolchain instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants