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

Initial Windows on ARM (AArch64) Support #249

Merged
merged 30 commits into from
Dec 2, 2023
Merged

Initial Windows on ARM (AArch64) Support #249

merged 30 commits into from
Dec 2, 2023

Conversation

hmelder
Copy link
Collaborator

@hmelder hmelder commented Nov 25, 2023

This pull request provides initial support for Windows on ARM.

The code was tested on the following Systems:

  1. Windows 10 Version 22598 MP (5 procs) Free ARM 64-bit (AArch64)
    Edition build lab: 22598.1.arm64fre.ni_release.220408-1503
  2. Debian GNU/Linux trixie/sid aarch64 (Kernel: 6.5.0-3-arm64)

I have documented the use of the SEH directives at the beginning of the file.

Further reading material (sorted by relevance):

Thank you @mstorsjo for the detailed explaination of the .seh* directives and the link to the LLVM commit.

@hmelder
Copy link
Collaborator Author

hmelder commented Nov 25, 2023

I need to rebase the branch.

CMake reports the wrong CPU architecture as we run it as an Intel process:

-- Selecting Windows SDK version 10.0.22621.0 to target Windows 10.0.22598.
-- The C compiler identification is MSVC 19.33.31517.0
-- The CXX compiler identification is MSVC 19.33.31517.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.33.31517/bin/HostARM64/ARM64/cl.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.33.31517/bin/HostARM64/ARM64/cl.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMAKE_SYSTEM_PROCESSOR: AMD64
-- Configuring done (3.4s)
-- Generating done (0.0s)
-- Build files have been written to: C:/Users/vm/test/build

The check 226455b fails

@hmelder hmelder assigned hmelder and unassigned hmelder Nov 25, 2023
@hmelder
Copy link
Collaborator Author

hmelder commented Nov 25, 2023

Arch check is now fixed for #247

objc_msgSend.aarch64.S Outdated Show resolved Hide resolved
Copy link
Member

@triplef triplef left a comment

Choose a reason for hiding this comment

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

Lgtm, though I can’t really comment on the assembly changes. Thank you for putting this together!

I fixed the CI issues on master, would be great if you could rebase again.

Unfortunately GitHub Actions so far doesn’t support WoA runners (see actions/runner-images#768), but would it be possible to at least cross-compile for ARM on the current CI (without running the tests)?

@anthony-linaro
Copy link

Repost from correct account :)

How are tests run? An option is docker+qemu+wine on an ubuntu x64 machine, such as here in sse2neon: DLTcollab/sse2neon#598

Might not work in all cases, but possibly worth a try

@hmelder
Copy link
Collaborator Author

hmelder commented Nov 27, 2023

Repost from correct account :)

How are tests run? An option is docker+qemu+wine on an ubuntu x64 machine, such as here in sse2neon: DLTcollab/sse2neon#598

Might not work in all cases, but possibly worth a try

That would be an interesting approach! Self-hosting is also an option but requires someone to pay for an arm64 VM :/

Copy link
Member

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

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

I still don't really like the amount of hackery that we have to work around a CMake bug, but we've merged hackery to work around CMake before. Can you open an issue that tracks the upstream CMake issue so that we can remove it once CMake is fixed?

CMakeLists.txt Show resolved Hide resolved
objc_msgSend.aarch64.S Show resolved Hide resolved
@hmelder
Copy link
Collaborator Author

hmelder commented Nov 28, 2023

We could try the CMAKE_HOST_SYSTEM_PROCESSOR variable as well but this will cause problems when cross compiling.

I found this in the CMAKE_SYSTEM_PROCESSOR documentation which is related to our problem:

In many cases, this will correspond to the target architecture for the build, but this is not guaranteed. (E.g. on Windows, the host may be AMD64 even when using a MSVC cl compiler with a 32-bit target.)

I will open an issue, but I have low expectations.

@hmelder
Copy link
Collaborator Author

hmelder commented Nov 28, 2023

It comes down to how CMake retrieves the architecture from the compiler.

@anthony-linaro
Copy link

anthony-linaro commented Nov 28, 2023

Repost from correct account (again, sorry all)

You guys are running inside a vcvarsall window, right? In blender, where I did something similar, I just checked "$ENV{VSCMD_ARG_TGT_ARCH}"

@hmelder
Copy link
Collaborator Author

hmelder commented Nov 28, 2023

Repost from correct account (again, sorry all)

You guys are running inside a vcvarsall window, right?

Yes:

vm@localhost's password:
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.3.0-pre.2.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'arm64'

@mstorsjo
Copy link

It comes down to how CMake retrieves the architecture from the compiler.

For CMAKE_HOST_SYSTEM_PROCESSOR, cmake pretty much explicitly doesn’t check the compiler, only the host environment. And CMAKE_SYSTEM_PROCESSOR is a plain copy of the former, if not cross compiling.

For detecting things from the actual compiler, there’s only CMAKE_C_COMPILER_ARCHITECTURE_ID, which only is available in some cases.

@anthony-linaro
Copy link

If it's always going to be in a vcvarsall window on Windows, I would suggest "$ENV{VSCMD_ARG_TGT_ARCH}" then - avoids CMake completely

@hmelder
Copy link
Collaborator Author

hmelder commented Nov 28, 2023

If it's always going to be in a vcvarsall window on Windows, I would suggest "$ENV{VSCMD_ARG_TGT_ARCH}" then - avoids CMake completely

So we need to error out, when we do not detect a vcvarsall enviroment, or fall back to some other value. Also not an ideal solution.

It is ridiculous how such an elementary thing is just completely broken in CMake.

@triplef
Copy link
Member

triplef commented Nov 28, 2023

So we need to error out, when we do not detect a vcvarsall enviroment

That seems ok to me since I think one always needs a Visual Studio environment on Windows, right?

@anthony-linaro
Copy link

Yes, I think that's a reasonable assumption - I haven't come across anything that doesn't use it (although I'm happy to be proved wrong) - even stuff that doesn't use CL

@mstorsjo
Copy link

So we need to error out, when we do not detect a vcvarsall enviroment

That seems ok to me since I think one always needs a Visual Studio environment on Windows, right?

If using mingw based tools (either GNU tools, or llvm tools with mingw headers), then one does not use a visual studio environment at all.

@triplef
Copy link
Member

triplef commented Nov 28, 2023

If using mingw based tools (either GNU tools, or llvm tools with mingw headers), then one does not use a visual studio environment at all.

Good point, but libobjc2 doesn’t currently support MinGW.

@mstorsjo
Copy link

If using mingw based tools (either GNU tools, or llvm tools with mingw headers), then one does not use a visual studio environment at all.

Good point, but libobjc2 doesn’t currently support MinGW.

Oh, ok, fair enough.

@triplef
Copy link
Member

triplef commented Nov 30, 2023

As we now have suitable hardware I could provide a Windows ARM self-hosted runner for the CI here (still need to set it up but maybe next week). Would you be able to give me sufficient GH project access to set this up @davidchisnall? Feel free to ping me directly.

@davidchisnall
Copy link
Member

@triplef, I've made you admin. I think that's sufficient to add runners.

@hmelder hmelder merged commit ab23f14 into master Dec 2, 2023
41 checks passed
@hmelder hmelder deleted the woa_support branch December 2, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants