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

Add support for Windows on Arm64 (WoA) build #413

Closed

Conversation

chirontt
Copy link
Contributor

New environment triplet win32/win32/aarch64 is added.

Changes are made to the 'org.eclipse.swt' bundle to support successful compile of the new SWT native libraries (*.dll) for Arm64.

New environment triplet win32/win32/aarch64 is added.

Changes are made to the 'org.eclipse.swt' bundle to support successful
compile of the new SWT native libraries (*.dll) for Arm64.
@@ -63,7 +63,7 @@ BOOL Validate_AllowDarkModeForWindow(const BYTE* functionPtr)
* an ATOM value of 0xA91E which is unlikely to change
*/

#ifdef _M_X64
#if defined(_M_X64) || defined(_M_ARM64)
Copy link
Member

Choose a reason for hiding this comment

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

This and other similar changes are not exactly correct, because on aarch64 disasm can't match the expected bytes. If you're not going to implement it now, it would be more correct to change this to

#elif defined(_M_ARM64)
    // Not implemented yet
    return FALSE;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand the above objection at all.

What is the meaning of "aarch64 disasm can't match the expected bytes"? What is "aarch64 disasm" to do with this change in the C code?

What does it mean by "If you're not going to implement it now"? implement what exactly? The modified code compiles successfully with the Microsoft C/C++ native compiler for Arm64, so what kind of "implement" is needed here? I'm quite confused, to be honest.

If you object to the use of those predefined macros in the C code, they are specifically meant to be used with the Microsoft C/C++ compiler, targeting the Windows OS, as documented here. This C code is not for any other OS but the Windows OS, so naturally the code is specifically designed to be compiled by the dominant compiler in the Windows system (Microsoft Visual Studio Suite).

The build.bat command file was also designed for Microsoft Visual Studio compiler tools, and my changes are to continue the usage of the same tools, for generating the native binaries for the Windows on Arm64 platform.

If you object to the use of Microsoft C/C++ tools here for the Windows OS, then this would become a huge topic that this PR can't address.

Copy link
Member

Choose a reason for hiding this comment

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

This and other similar changes are not exactly correct, because on aarch64 disasm can't match the expected bytes. If you're not going to implement it now, it would be more correct to change this to

#elif defined(_M_ARM64)
    // Not implemented yet
    return FALSE;

@SyntevoAlex
Looks like acronym "disasm" caused some confusion:
aarch64 "disasm" actually stands for aarch64 disassembler

@chirontt
IMHO, above comment only meant to re-arrange arm64 code something as below:
#ifdef _M_X64
{
// Existing conditions for TRUE cases, which my apply only to _M_X64 only
return TRUE;
}
return FALSE;
#elif defined(_M_ARM64)
{
// TODO: Add Conditions for any known TRUE cases for arm64
// Not implemented yet
}
return FALSE;
#else
#error Unsupported processor type
#endif

Please correct me if I got it wrong. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The code in question tests compiled code (machine instructions, which can be seen as assembly code, which I described as disasm for shorthand). It verifies that machine code in a Windows library matches what's expected. This is because the APIs are not documented, but we still need them, so we need to check if at least it's still the API we expected.

Surely aarch64 machine code differs from x86_64 machine code. That's the whole point of a different architecture. Otherwise you could just run x86_64 native libraries on aarch64.

Therefore, even if the code "compiles" for you, it will not work. To fix that, you have to implement checks specific to aarch64.

If you're not implementing that now, then change the code as I suggested, so that it's more clear that it's not implemented and doesn't look like "it's good".

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, as a result of NOT implementing the new checks, Dark Theme will stop working properly (scrollbars will be light, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can see what you meant now. Thanks for the explanation. And here's a screenshot of Eclipse SDK in dark mode (built myself and running on my WoA box):

image

As it shows, the scrollbars are not dark, so the code blocks within the

#if defined(_M_X64) || defined(_M_ARM64)

don't work properly in Windows Arm64; it may also not work if it just does return FALSE; instead, as suggested (I'll rebuild it and check it out later.)

In the mean time, I'll withdraw this PR, as it's become clear now that it's not useful, because the build infrastructure for building Windows on Arm64 artifacts is not yet in place to support it.

Copy link
Contributor

Choose a reason for hiding this comment

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

aarch64 disasm taken care of at #1048

@chirontt
Copy link
Contributor Author

PR withdrawn, as build infrastructure is not yet in place to support building Windows on Arm64 artifacts.

@chirontt chirontt closed this Sep 28, 2022
@SyntevoAlex
Copy link
Member

What kind of build infrastructure is needed? Shouldn't be too hard to get it installed, I think.

@SyntevoAlex
Copy link
Member

I feel that the effort to support Windows aarch64 is generally useful, so it would be a pity if the PR is merely discarded.

rather than sharing the custom code with the x64 platform,
pending custom implementation need be added later for Arm64 platform.
@akurtakov
Copy link
Member

I have explained it in eclipse-platform/eclipse.platform.releng.aggregator#577 (comment) . We can have sources but not include binaries in any deliverable until we have infrastructure to rebuild them on demand. @chirontt , it's not an all or nothing, this patch doesn't try to include in our deliverables any build artifact so it's fine from releng POV as soon as win32 knowledgable committers give it a go.
P.S. referencing all windows on arm committs in single issue (like the one I referred too) would make it easier for everyone to see slightly bigger picture for the state of affairs on the topic.

@SyntevoAlex
Copy link
Member

SyntevoAlex commented Oct 6, 2022

There's no need for arm64 build machine, I was able to build arm64 on my x64 machine.

To do that, one needs to install MSVC v143 - VS 2022 C++ ARM64 build tools (latest) in their Visual Studio (VS2019 probably has a matching entry).

image

After that, the following edits were necessary:

  1. In build.bat, I changed set BUILD_ARCH=arm64 to set BUILD_ARCH=x64_arm64
  2. I downloaded arm64 JDK from https://download.visualstudio.microsoft.com/download/pr/2b5319cd-948a-4079-8b97-eede6f970d68/31ea515b9692dc74e878d3697341d120/microsoft-jdk-17.0.4.1-windows-aarch64.zip

Notes:

  1. build.bat needs to detect current machine's arch and select one of:
    set BUILD_ARCH=arm64
    set BUILD_ARCH=x64_arm64
  2. JDK is only needed for a couple *.lib files, such as jawt.lib.
    It would be inconvenient to have two JDKs installed and then try to identify which is which.
    It would make sense to instead add the necessary *.h *.lib files directly into our repo, I think?
    Not sure if their license allows that, probably yes, but this needs to be checked.
  3. eclipse.platform.swt.binaries repo needs adjustments to include a bundle for arm64.
    I was able to just copy org.eclipse.swt.win32.win32.x86_64 and trivially edit it.
  4. A minor rant: please use capital letters in keywords like set in build.bat

I'm interested in this PR, so if you're able to polish it, I will ensure to get it merged.

@karianna
Copy link

karianna commented Oct 7, 2022

Hoping to help find machines to build on (will be a few more weeks at least though) - tracking in eclipse-platform/eclipse.platform.releng.aggregator#577

@chirontt
Copy link
Contributor Author

chirontt commented Oct 7, 2022

@SyntevoAlex I did consider doing cross-compiling in the build.bat, but then for linking, the need for a required Arm64 JDK becomes complicated in an x64 box or VM (as mentioned in your Notes 2), so I dropped this cross-compile idea. To me, it's simpler to just build the Arm64 binaries using an Arm64 box or VM. I hope @karianna 's suggestion above will come to fruition.

As for your Notes 1, if we want to consider cross-compiling, detecting the current machine's arch is simple, by checking the PROCESSOR_ARCHITECTURE environment variable in build.bat, and then set up BUILD_ARCH accordingly for cross-compiling.

@SyntevoAlex
Copy link
Member

It would be nice to have cross-compiling option.
It would also be nice to get rid of JDK dependency and just have the necessary files directly in the repo.
None of that is necessary for that PR, just some wishes.

@HannesWell
Copy link
Member

If you rebase this PR please consider to adjust the Jenkins pipeline enhanced with PR #514, so that the windows-arm build is triggered correctly. You probably want to add one more element in the matrix axis.

@jukzi
Copy link
Contributor

jukzi commented Jun 15, 2023

@chirontt Will be there progress on this PR? Otherwise we close it.

@chirontt
Copy link
Contributor Author

@jukzi I did close it back in Sep last year, when it became apparent that there wouldn't be any WoA hardware available for the build farm to proceed. Then @SyntevoAlex re-opened it the following month.

@SyntevoAlex any progress on this WoA hardware problem for the build farm?

@SyntevoAlex
Copy link
Member

I'm not related to hardware team and I'm not aware if there's progress.

Didn't mean to reopen a closed issue, I merely commented and maybe clicked a wrong button and then GitHub reopened the issue.

Anyways, I still think that cross-compiling is the way to go. Implementing it shouldn't be hard, if you're interested to work on it, of course.

@chirontt
Copy link
Contributor Author

Even if cross-compiling is possible, it doesn't solve the problem of running tests where they require a real (or virtual) WoA hardware to execute. I guess the hardware would be available in the build farm when Eclipse Adoptium starts producing a JDK release for Windows aarch64.

I'm closing this PR for now.

@chirontt chirontt closed this Jun 29, 2023
@SyntevoAlex
Copy link
Member

Continued in #1019

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.

None yet

8 participants