-
Notifications
You must be signed in to change notification settings - Fork 163
Compile SWT natives for Windows on Arm64 (WoA). #1045
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
Compile SWT natives for Windows on Arm64 (WoA). #1045
Conversation
#elif defined(_M_ARM64) | ||
/* Not implemented yet */ | ||
functionPtr; /* to prevent: warning C4100: 'functionPtr': unreferenced formal parameter */ | ||
return FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should return TRUE here and SWT on Windows on ARM should require Windows 10 1903 or higher.
Or we can implement the disassembly hack that is done for x64 with ARM64 code.
For more on the matter, see:
mintty/mintty#983 (comment)
and
https://stackoverflow.com/a/58547831/827532
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we state that SWT on Windows on ARM require Windows 1903+ then we can return TRUE there and not do the assembly hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also. I personally would've not done that assembly hack. If you look at one of the links above, this is way less hacky:
auto ord135 = GetProcAddress(hUxtheme, MAKEINTRESOURCEA(135));
if (g_buildNumber < 18334)
_AllowDarkModeForApp = reinterpret_cast<fnAllowDarkModeForApp>(ord135);
else
_SetPreferredAppMode = reinterpret_cast<fnSetPreferredAppMode>(ord135);
In the Validate_AllowDarkModeForWindowWithTelemetryId()
method all they would have to do was to just query the build version of Windows to decide if TRUE or FALSE stating that uxtheme.dll has the expected function at ordinal 135.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR with new commit returning TRUE for Arm64, so no need for any further implementation. Windows on Arm64 is really only Windows 11 going forward, so there's no need for any checking for Windows 10 1903+ in Arm64 because Windows 10 is no longer supported in Arm64 by Microsoft.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that what @SyntevoAlex is saying is that since these three functions are undocumented (they are not even exported by name, only by ordinal) they could be moved to another ordinal completely or even removed in the future builds of Windows 11 , thus it's unsafe to just return TRUE without checking first if the function is indeed the one expected at a given ordinal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the functions are still NOT documented, so they still need to be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntevoAlex @hmartinez82 I don't understand any of the proposed changes to the os_custom.c
file.
I'm thinking of removing this os_custom.c
file from this PR, so that you guys can submit it in a separate new PR with your proposed changes for Arm64. And my PR will wait for your new PR to be submitted first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chirontt I can do that, but I don't know how to compile this. I have no Maven or Java experience :(
That's why I copied the code here. I can try to explain to you were those numbers 0x18 and 0x1C are coming from:
The validation check for x64, was done by looking at the machine code of those three functions, AllowDarkModeForWindow()
, AllowDarkModeForWindowWithTelemetryId()
, SetPreferredAppMode()
.
This was done by someone one Visual Studio, in an x64 machine.
With the disassembly of the code, one can see which bytes are at each address after the start of the function (the start of the function is the value returned by GetProcAddress()
)
Here's SetPreferredAppMode()
for instance on x64:
As you can see, you have to open the disassembly view to be able to Step Into
something you don't have the source code for. You do that by stepping in the Disassembly View until you get to the call
and then you hit Step Into
and finally see the first instruction at the beginning of the function.
The numbers circled in blue in the picture above are being tested in this line for instance https://github.com/eclipse-platform/eclipse.platform.swt/pull/1045/files#diff-d6068e15e4497052c1129ba671192ebce3920a189d39a48d0cb66e151019f9b8R227 in Validate_SetPreferredAppMode()
I then did all this dance, for the three functions in ARM64, got the disassembly of them all (see my pasted ASM code further down) and came up with three possible validations. Those three opcodes that I used for checking are 0x18 bytes from the beginning of theAllowDarkModeForWindow()
, AllowDarkModeForWindowWithTelemetryId()
and 0x1C bytes from the beginning of SetPreferredAppMode()
. Since in ARM64 all instructions are fixed length (unlike x86 and x86_64) it was easy to just convert them to an uint32_t and compare directly to the expected opcodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntevoAlex @chirontt Please review #1048 :)
All of the above comments are wrong. The problem is that API is not documented. Ordinal numbers are unreliable and may change any time, that's why APIs are additionally validated. Otherwise it's possible that one day, there would be entirely different API on ordinal 135, and Eclipse would just immediately crash on every start. By returning TRUE, you just disable safety checks and say "it's fine", regardless of whether it's fine or not. |
This is (in short way) explained in code with code comment:
|
So we need an ARM64 equivalent of the hack? I can try looking into it. |
Right. One would need to dissasemble ARM windows DLLs and decide which part of it uniquely identifies desired functions. |
Or you can just return FALSE for now. It can be supported later. |
If you choose to return FALSE, please add another code comment explaining why. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review Java specific build files because I have no experience with these.
IF "x.%OUTPUT_DIR%"=="x." ( | ||
CALL :ECHO "ERROR: OUTPUT_DIR environment variable must be set for manual cross-compile." | ||
EXIT /B 1 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this code can be simplified.
Realistically I don't expect any new archs in upcoming years, so there will be just two - amd64 and arm64. So I suggest to have just one new variable that would override %PROCESSOR_ARCHITECTURE%
. For example, COMPILE_ARCH
could be the only new variable. If it's empty, it falls back to current processor arch:
IF "%COMPILE_ARCH%"=="" SET COMPILE_ARCH=%PROCESSOR_ARCHITECTURE%
this way, you only use one var instead of two, and there's less code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing it this way means the cross-compiling idea is abandoned. Is this what you really want?
The required OUTPUT_DIR variable must be set, either internally within this build.bat
file, or externally set by the caller of this build.bat
script. Perhaps we can make this build.bat
script smarter, in working out the required OUTPUT_DIR value internally from the COMPILE_ARCH (or XC_HOST_TARGET) value, by parsing either x64_arm64
or arm64_x64
and working out the needed OUTPUT_DIR value from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-compiled arm64 is the same as native-compiled arm64. Therefore, if arm64 is compiled, output dirs will be for arm64. Even if it's compiled on amd64.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, on an x64 box, passing 'arm64' to initialize and compile the C code for Arm64 will certainly fail to work (I've tried it). For cross-compile, MSVC must know from which host to which target explicitly, hence it requires 'x64_arm64' on an x64 box, or 'arm64_x64' on an Arm64 box, for cross-compiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so the expected value for BUILD_ARCH
is for example x64_arm64
, I didn't catch that initially. I think that it still makes sense to compose BUILD_ARCH
from %PROCESSOR_ARCHITECTURE%
and requested target arch, because the code would be easier to understand. What do you think?
REM Compose host architecture string for MSVC
IF "%PROCESSOR_ARCHITECTURE%"=="AMD64" (
SET HOST_ARCH=x64
) ELSE IF "%PROCESSOR_ARCHITECTURE%"=="ARM64" (
SET HOST_ARCH=arm64
) ELSE (
CALL :ECHO "ERROR: Unknown host architecture: %PROCESSOR_ARCHITECTURE%."
EXIT /B 1
)
REM %TARGET_ARCH% may be specified by the caller for cross-compiling.
REM If not, build for builder machine's architecture
IF "%TARGET_ARCH%"=="" (
SET TARGET_ARCH=%HOST_ARCH%
)
REM Compose build argument for MSVC
IF "%TARGET_ARCH%"=="%HOST_ARCH%" (
SET BUILD_ARCH=%TARGET_ARCH%
) ELSE (
SET BUILD_ARCH=%HOST_ARCH%_%TARGET_ARCH%
)
REM Select build directory based on target arch
IF "%TARGET_ARCH%"=="x64" (
IF "x.%OUTPUT_DIR%"=="x." SET OUTPUT_DIR=..\..\..\org.eclipse.swt.win32.win32.x86_64
) ELSE IF "%TARGET_ARCH%"=="arm64" (
IF "x.%OUTPUT_DIR%"=="x." SET OUTPUT_DIR=..\..\..\org.eclipse.swt.win32.win32.aarch64
) ELSE (
CALL :ECHO "ERROR: Unknown target architecture: %TARGET_ARCH%."
EXIT /B 1
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it some more, I figured that my approach has an even bigger advantage: user only specifies what he wants, and doesn't have to care about how to make this happen. Script will automatically figure the need to use cross-compiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, in turn, will simplify builder scripts: builder will just say "build that and that", regardless of what they're running on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, your approach is a bit verbose, but it's quite clear in intention, so I have no problem with it. And current Maven build scripts would still work with it without change (this was my intention all along.) And doing cross-compiling has to be a manual operation anyway, that the user need to explicitly specify an extra environment variable in order to trigger it, so I won't expect the current automated build scripts to do this cross-compiling, but the facility is there for automated cross-compile by the build scripts if required.
I'll merge this with the PR and squash it. And the PR's description need be changed too, to reflect the updated contents.
Another thing: I plan to apply these changes of build.bat
, to the build.bat of the Equinox project, to generate the Equinox launcher native for Arm64. If you have different idea (for the Equinox project), let me know in advance, thanks.
@SyntevoAlex any recommendations here? Since the instructions have fixed width, I was going to just compare the expected opcodes at a certain offset from the start of the functions. Any other suggestion? |
I think that suggested checks are reasonable. |
@chirontt here are the verifications to be used instead of just returning
For
For
@niraj-modi Are these satisfactory? |
I've restored the |
#1048 was merged. |
f426a5c
to
1524c8d
Compare
Branch squashed and rebased to latest master; PR's description updated also, to correctly describe the updated contents. To summarize the description, if you're on an x64 box and want to cross-compile to produce the SWT natives for Arm64, here are the commands: set TARGET_ARCH=arm64
cd binaries\org.eclipse.swt.win32.win32.aarch64
mvn clean process-resources -Dnative=win32.win32.aarch64 -DSWT_JAVA_HOME=\path\to\arm64_jdk If you're authorized, then you could commit the generated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I guess we'll merge it when master is open again. Probably there's no rush to merge it ASAP?
1524c8d
to
be6bd28
Compare
Branch rebased to latest master; ready to merge. |
Thanks for working on this.
The dll's should not be added manually only the Jenkins build should do that. With the current build pipeline the manually submitted would then probably be deleted on the next change that requires recompilation. |
@jonahgraham can you tell about the state of the hardware here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module needs to be mentioned in https://github.com/eclipse-platform/eclipse.platform.swt/blob/master/binaries/pom.xml as well to be included in the build.
7b4e3d7
to
6b4cd63
Compare
The build of the native binaries for Windows on ARM is now working compiling against a beta jdk-17 win32.aarch64. What's the opinion of the other committers about using that to build the SWT natives for WoA for some time until Temurin provides official releases for them? Do you think that would be a ok or should we wait until there is an official GA release for those binaries? I'm torn on this one. It is not the cleanest solution, but it would allow more testing of the WoA jdk17, which would maybe help the temurin maintainers to gain more confidence about it.
I recently run the build without the |
e0c4bc0
to
6b4cd63
Compare
I totally support this idea. I understand that JDK only plays minimal role in produced DLLs, mostly it just provides a header and a library to describe the shape of JNI interface. I expect that these files would be mostly identical in any flavor of aarch64 JDK. |
By "mostly identical" I mean that I'm convinced that DLLs compiled with one JDK or another JDK will work the same way. |
6b4cd63
to
44c37c5
Compare
Sorry, I take back my comment about the (no)need for Line 31 in 103b10e
So we need to add the |
44c37c5
to
9976ce7
Compare
Yes, exactly. I looked into WebView2Loader topic again yesterday but didn't have the time to answer here, yet.
Thank you @hmartinez82 for that hint. I have now I have now added the arm64 variant of the In order to get IP/License clearance to add the Once that is approved this should be ready for submission. |
9976ce7
to
9b90d36
Compare
@HannesWell Thanks for adding the |
That's a good catch. And that gave me the idea that it would be better to share the legal-files for the fragments of a platform in a common location. I therefore created #1144. |
9b90d36
to
d4b1768
Compare
With #1144 merged, I've rebased and updated this PR's branch to bring it up to date. |
d4b1768
to
4513c52
Compare
This adds the org.eclipse.swt.win32.aarch64 fragment and contains changes to the build.bat file, to support compiling the SWT natives for the Windows on Arm64 (WoA) platform. On a WoA box, run the following commands to produce the SWT natives (swt*.dll) for WoA: cd binaries\org.eclipse.swt.win32.win32.aarch64 mvn clean process-resources -Dnative=win32.win32.aarch64 and the swt*.dll files for WoA will be created in the current directory. Cross-compiling between the x64 and Arm64 platforms, where either of them can be host or target, is also possible, after setting up the TARGET_ARCH environment variable with correct target architecture value, and with the SWT_JAVA_HOME property pointing to an available JDK of the target architecture. For example, to cross-compile on an x64 host and produce SWT natives for Arm64 target, run the following commands: set TARGET_ARCH=arm64 cd binaries\org.eclipse.swt.win32.win32.aarch64 mvn clean process-resources -Dnative=win32.win32.aarch64 -DSWT_JAVA_HOME=\path\to\arm64_jdk and the swt*.dll files for WoA will be created in the current directory. Note the above SWT_JAVA_HOME property in the command line which points to an available Arm64 JDK of the target architecture. Similarly, to cross-compile on an Arm64 host and produce SWT natives for x64 target, run the following commands: set TARGET_ARCH=x64 cd binaries\org.eclipse.swt.win32.win32.x86_64 mvn clean process-resources -Dnative=win32.win32.x86_64 -DSWT_JAVA_HOME=\path\to\x64_jdk and the swt*.dll files for x64 will be created in the current directory. Note that for cross-compiling between x64 and Arm64 to work, install the MSVC compiler version 2022, with mandatory build tools for both platforms included in the installation. Once the SWT natives for Arm64 are generated, the SWT binaries module's files can be produced with the following build commands in current directory: mvn clean verify -DskipTests=true and the module's jar/zip files will be produced in the target directory: org.eclipse.swt.win32.win32.aarch64-<version>.jar org.eclipse.swt.win32.win32.aarch64-<version>-sources.jar swt-<version>-win32-win32-aarch64.zip Also add the WebView2Loader.dll version 1.0.1150.38 (matching the swt.win32.x86_64 fragment), obtained from the NuGet package microsoft.web.webview2 https://nuget.info/packages/Microsoft.Web.WebView2/1.0.1150.38 located in the sub-folder build/native/arm64
4513c52
to
22037ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #1144 merged, I've rebased and updated this PR's branch to bring it up to date.
Thank you.
I have just pushed another small update for the Jenkins file to have the work-around for the different source of the win32.aarch64
JDK located near the URL definition of the JDKs used by default.
Unfortunately I could not yet get more details about an estimation when Temurin will officially support WoA (see adoptium/adoptium-support#616 (comment)), so it looks like we have to continue with that workaround until it is available and justj can adopted it.
But as it was discussed, it shouldn't have an impact.
In order to get IP/License clearance to add the
WebView2Loader.dll
I created* https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/14072
The IP clearance was granted so everything is finally ready.
I did a final pass over the changes and all looks good to me (although I'm not an expert in configuring MSVC, but AFAICT it looks good).
Unfortunately there is currently an issue with the Eclipse build infrastructure that prevents the build from succeeding. But once that is resolved and we have a green build I think this is ready for submission.
The infrastructure issue has been resolved and all builds had passed. Thank you @chirontt for this nice contribution and everyone that helped. Please note that This should only be done once the Equinox launcher is available and and I-build for WoA is at least prepared. |
Where can one download the ARM64 binaries? |
At some point they should be visible in I-builds pages e.g. https://download.eclipse.org/eclipse/downloads/drops4/I20240329-0530/#SWT but not yet there. |
Right, they are contained in my PR (for WoA) for that Please review it. |
They are now generated and stored in this repo: |
This PR contains changes to the
build.bat
file, to support compiling the SWT natives for the WoA platform, and associated files to produce the SWT binaries module's jar/zip files for WoA.On a WoA box, run the following commands to produce the SWT natives (
swt*.dll
) for WoA:cd binaries\org.eclipse.swt.win32.win32.aarch64 mvn clean process-resources -Dnative=win32.win32.aarch64
and the
swt*.dll
files for WoA will be created in the current directory.Cross-compiling between the x64 and Arm64 platforms, where either of them can be host or target, is also possible, after setting up the
TARGET_ARCH
environment variable with correct "target architecture" value, and with theSWT_JAVA_HOME
property pointing to an available JDK of the target architecture.For example, to cross-compile on an x64 host and produce SWT natives for Arm64 target, run the following commands:
and the
swt*.dll
files for WoA will be created in the current directory. Note the aboveSWT_JAVA_HOME
property in the command line which points to an available Arm64 JDK of the target architecture. If needed, an Arm64 JDK for WoA can be downloaded from Microsoft, Liberica, or Zulu.Similarly, to cross-compile on an Arm64 host and produce SWT natives for x64 target, run the following commands:
and the
swt*.dll
files for x64 will be created in the current directory.Note that for cross-compiling between x64 and Arm64 to work, install the MSVC compiler version 2022, with mandatory build tools for both platforms included in the installation. Further more, there must exist a JDK installation of the
target
platform available on the host machine, so that theSWT_JAVA_HOME
variable can point to it during cross-compiling and linking.Once the SWT natives for Arm64 are generated, the SWT binaries module's files can be produced with the following build commands in current directory:
and the module's jar/zip files will be produced in the
target
directory: