-
Notifications
You must be signed in to change notification settings - Fork 356
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
Fix cross compilation with clang-cl (v2) #230
base: master
Are you sure you want to change the base?
Conversation
b54ccfa
to
d5b8cf9
Compare
d5b8cf9
to
124a7d8
Compare
sudo ln -s /usr/bin/lld /usr/bin/lld-link | ||
sudo apt-get install --quiet --no-install-recommends -y wine-stable winetricks | ||
winetricks nocrashdialog | ||
cargo install cargo-xwin --target x86_64-unknown-linux-gnu |
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.
Wow, thank you for figuring all this out. It's probably time for me to refactor all of these copy-pasted jobs into a Python script or something, so that additions like this aren't so painful in the future.
} else { | ||
build.file("c/blake3_sse2_x86-64_windows_gnu.S"); | ||
build.file("c/blake3_sse41_x86-64_windows_gnu.S"); | ||
build.file("c/blake3_avx2_x86-64_windows_gnu.S"); |
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.
@sneves are there any potential ABI mismatch issues we might need to worry about with compiling the Windows GNU .S
files into a binary that's otherwise targeting the MSVC ABI? Like different callee-saved register conventions or whatever? Or is it just the assembly syntax that's different between the two?
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.
Syntax only, yes.
Any updates? |
Hey @oconnor663, just wanted to check if there was any chance of this PR being adopted/revisited? I started applying the changes fresh to current master but the code has changed enough that I'm lost for how to translate the last of the original changes in this PR to the current api used in For context bevy recently started using |
Ah, apologies for dropping the ball. I'm going to put a note on my calendar to get to this PR this weekend. Incidentally I've just recently been studying up on how ECS works, and I've been looking at what Bevy does for atomically reserving ranges of IDs. So I'm pumped to see BLAKE3 getting involved :) |
I'm trying to understand how this sort of cross-compilation works and how to test it. I'm getting some confusing results when I try an alternate approach with
If I rebase this PR and run the same tests, the last step fails:
I'm guessing there's more than one way to cross-compile for the MSVC ABI, and whatever |
Has this been fixed yet? Struggling to cross compile my binary because of this |
Not yet, I need help answering the questions above. |
Hey @oconnor663 You may find this answer about differences between the MinGW and MSVC toolchains helpful. Cross offers both toolchains. For the MSVC toolchain, Cross extracts headers and link libraries from the MSVC runtime packages, then runs the actual MSVC compiler under Wine emulation (both You could get this PR to work using Cross, but you'd need to override the linker, C++ compiler and C compiler that Cross provides (the MSVC toolchain under Wine) to use instead the LLVM MSVC-compatible toolchain. Something like this should work: [target.aarch64-unknown-linux-gnu]
pre-build = [
# TODO: Install clang-cl, lld-link, etc.
"export CARGO_TARGET_X86_64_PC_WINDOWS_MSVC_LINKER=lld-link",
# or maybe rust-lld would work?
"export CC_x86_64_pc_windows_msvc=clang-cl",
"export CXX_x86_64_pc_windows_msvc=clang-cl",
] |
It should be possible to "fix" the build script to work by default with MSVC on Windows, MSVC on Wine using Cross, or clang-cl on any architecture (e.g. using fn use_msvc_asm() -> bool {
const MSVC_NAMES: &[&str] = &["", "cl", "cl.exe"];
let target_os = env::var("CARGO_CFG_TARGET_OS").unwrap_or_default();
let target_env = env::var("CARGO_CFG_TARGET_ENV").unwrap_or_default();
let target_windows_msvc = target_os == "windows" && target_env == "msvc";
let host_triple = env::var("HOST").unwrap_or_default();
let target_triple = env::var("TARGET").unwrap_or_default();
let cross_compiling = host_triple != target_triple;
let cc = env::var("CC").unwrap_or_default().to_ascii_lowercase();
if !target_windows_msvc {
// We are not building for Windows with the MSVC toolchain.
false
} else if !cross_compiling && MSVC_NAMES.contains(cc) {
// We are building on Windows with the MSVC toolchain (and not cross-compiling for another architecture or target).
true
} else {
// We are cross-compiling to Windows with the MSVC toolchain.
let target_arch = env::var("CARGO_CFG_TARGET_ARCH").unwrap_or_default();
let target_vendor = env::var("CARGO_CFG_TARGET_VENDOR").unwrap_or_default();
let cc = env::var(format!("CC_{target_arch}_{target_vendor}_windows_msvc")).unwrap_or_default().to_ascii_lowercase();
// Check if we are using the MSVC compiler.
MSVC_NAMES.contains(cc)
}
} Edit: This should support all the Windows targets now, including the UWP and new Windows 7 targets. |
This comment was marked as outdated.
This comment was marked as outdated.
Any updates? Does this PR need any assistance? If so, I'd be happy to help. |
Supersedes #101.
This PR also fixes cross compilation of the C bindings and adds CI testing for 'Linux - Windows MSVC', but skips the 'C bindings intrinsics' test as that fails to build).
Successful build: https://github.com/Sporif/BLAKE3/actions/runs/1944217647