-
Notifications
You must be signed in to change notification settings - Fork 779
Windows arm64: enable building with MSVC #2673
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
Conversation
| - run: rustup toolchain install --no-self-update --profile=minimal 1.66.0 | ||
|
|
||
| - run: echo "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\Llvm\x64\bin" >> $GITHUB_PATH | ||
| - run: echo "VSINSTALLDIR=C:\Program Files\Microsoft Visual Studio\2022\Enterprise" >> $GITHUB_ENV |
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 won't be needed anymore once rust-lang/cc-rs#1506 gets merged. It will then be possible to get the path to Visual Studio's clang.exe like this:
cc::windows_registry::find(asm_target, "clang.exe");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.
Let's wait for the cc-rs changes. Then we can increase our cc-rs version dependency and rely on it to find this for us.
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.
The cc-rs change that I think we need is rust-lang/cc-rs#1367.
| #elif defined(OPENSSL_AARCH64) && defined(_MSC_VER) && !defined(__clang__) | ||
| /* Manual 64-bit addition for MSVC ARM64 (no __uint128_t) */ | ||
| Limb sum = a + carry_in; | ||
| Carry carry1 = (sum < a) ? 1 : 0; /* carry from a + carry_in */ | ||
| *r = sum + b; | ||
| Carry carry2 = (*r < sum) ? 1 : 0; /* carry from sum + b */ | ||
| ret = carry1 | carry2; |
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.
Given the lack of __uint128_t on MSVC as well as not having _addcarry_u64 or _subborrow_u64 on MSVC arm64, I used AI to come up with this workaround and the ones below. I think this looks good, but would very much appreciate anyone with C knowledge to chime in 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.
used AI
I cannot accept PRs of unknown provenance.
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.
Upstream BoringSSL has some changes that we should bring in related to this. But, again, it looks like they are just avoiding MSVC on AArch64.
|
FYI, the original idea was to use Clang-cl with the MSVC toolchain for 2 main reasons:
I'm not sure what specifically has changed in Ring 0.17 (a lot, from what I know), so maybe today it is easier to build it with MSVC. |
| curl -LOs https://static.rust-lang.org/rustup/dist/aarch64-pc-windows-msvc/rustup-init.exe | ||
| ./rustup-init.exe -y --default-toolchain none --no-modify-path | ||
| echo "$USERPROFILE/.cargo/bin" >> "$GITHUB_PATH" | ||
| shell: sh |
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.
Thanks for pointing this out; see PR #2675.
| run_command(c); | ||
| } | ||
|
|
||
| fn clang_win(file: &Path, arch: &str, include_dir: &Path, out_dir: &Path) { |
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 instead of this, it would be better to create two separate libraries: one for assembly code, and one for C code. Then configure the cc::Build for the assembly code library to force the use of clang as the C compiler for Windows AArch64. This way, we maximize the use of cc-rs's infrastructure, e.g. handling compiler options and whatnot.
| #if defined(OPENSSL_X86_64) | ||
| #pragma intrinsic(_umul128) | ||
| #elif defined(OPENSSL_AARCH64) | ||
| #pragma intrinsic(__umulh) |
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'm not sure why BoringSSL avoids doing this. My guess is they aren't testing AArch64 Windows builds using MSVC, but instead forcing the use of clang.
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'm not sure why BoringSSL avoids doing this
They actually do use it, looking at the most recent version of internal.h on their main branch: https://boringssl.googlesource.com/boringssl/+/refs/heads/main/crypto/fipsmodule/bn/internal.h#419
My guess is they aren't testing AArch64 Windows builds using MSVC, but instead forcing the use of clang.
They do build arm64 Windows using both MSVC and Clang in their CI. They even seem to prefer MSVC over Clang:
On Windows, MSVC from Visual Studio 2022 or later with Windows 10 SDK 2104 or later are supported, but using the latest versions is recommended. Recent versions of GCC (6.1+) and Clang should work on non-Windows platforms, and maybe on Windows too.
I've just configured an example pipeline in GitHub Actions to build BoringSSL using both MSVC and Clang on arm64. The build succeeds using both compilers. Note that with MSVC, OPENSSL_NO_ASM needs to be defined. They do that in their own CI setup as well.
@Alovchin91 just noting that it doesn't currently use |
|
@MarijnS95 Right. I can hardly remember much of our discussion (you can find it here and maybe something also in #1167), but I guess maybe back in the v0.16 days the C code was compiled separately, so it didn't really matter whether we used I'm not sure if it's a good idea to move forward with MSVC since it apparently still lacks 128-bit support. But as a drop-in replacement, Is it really a problem though that the user could explicitly set Sorry for too many words, pulling this from memory takes a lot of effort 😅 |
|
@Alovchin91 reading through those links you seem to mostly mention
I don't understand exactly what you mean here with "separately". Since the compiler is replaced inside That is also the reason to check what |
I mean as a separate step. Indeed, I see the use of Looking a bit more at that code, it might also have been that we simply relied on Probably this check should have covered that case: But then this is an actual mistake, because if it's |
|
@Alovchin91 yeah, that Since this PR is closed, we better jump back to #2216 / #2215 / rust-lang/cc-rs#882 / rust-lang/cc-rs#1367 to discuss possible approaches and solutions. |
Closes #2674
Closes #2215
Currently, a workaround is in place to always build with Visual Studio's
clang.exeinstead of MSVC.This is causing problems in some cases, because the
aarch64-pc-windows-msvctarget typically only uses MSVC-provided tooling and has no hard requirement onclang.exeat all. It gets even trickier when it tries to pass MSVC flags like/nologo,/DCOMPILER_MSVC, etc. toclang.exe, which it doesn't support. Sure,clang-cl.exemight work, but that can come with its own set of challenges.I learned that the
ringlibrary basically has two ways of using it:I was thinking:
This PR implements exactly that. With some minor changes, I was able to get things to compile using MSVC, using pre-compiled object files generated with Visual Studio's
clang.exe.CC @awakecoding @Alovchin91