ci: Use Clang on Windows #10890
ci: Use Clang on Windows #10890
Conversation
|
Hm can you explain more about what's going on here? (if you can? I'm also no Windows expert...) To confirm are you using the Additionally the C compiler that Roughly to the best of my knowledge there are various CRT libraries on Windows that are ABI-incompatible which means that whatever is selected at compile-time must also be used at runtime which is what's being warned about here. That being said we might be using the ABI-stable bits that are the same across those libraries (I'm not sure). Our usage of the CRT is definitely quite small, it's pretty much just setjmp/longjmp... |
Yes,
I saw this in the CI log of On my local machine it was this: But yes, it makes much more sense that edit: Yes, the C compiler CMake chooses doesn't actually influence anything. So yes, clang vs cl.exe difference. Now I'm wondering if there is an actual solution to this, other than compiling from source to both debug and release? |
|
Ah ok yeah that makes sense, and agreed that the CMake-chosen compiler probably isn't doing anything. Although perhaps indicative that our MSVC tests are not testing what we think they are... In any case, I have vague recollections of solving this in rust-lang/rust a long time ago, but it's now eluding me how exactly it was solved. Could you try turning on this flag perhaps and see if that helps? Skimming Microsoft's docs on this I don't find much that's illuminating... |
diff --git i/crates/wasmtime/build.rs w/crates/wasmtime/build.rs
index 3b005414a..c2ba8f572 100644
--- i/crates/wasmtime/build.rs
+++ w/crates/wasmtime/build.rs
@@ -72,6 +72,7 @@ fn build_c_helpers() {
use wasmtime_versioned_export_macros::versioned_suffix;
let mut build = cc::Build::new();
+ build.static_crt(true);
build.warnings(true);
let arch = std::env::var("CARGO_CFG_TARGET_ARCH").unwrap();
let os = std::env::var("CARGO_CFG_TARGET_OS").unwrap();
Which is quite interesting... edit: Now trying to link against it gives these warnings: edit edit: This is actually the directives: |
|
@jsturtevant could I perhaps tag you in to help out with this from the Windows side of things? (or perhaps if you could forward this along to others who may know as well?) The basic question why @MangoPeachGrape perhaps as a reference point as well, could you past the command/reproduction to generate the linker warning you're seeing as well? |
Build an application that links with In VS2022 it's under "Project" -> "Properties" -> "C/C++" -> "Code Generation" -> "Runtime Library" |
This is a limitation of the Windows build and is documented (not very clearly) in https://learn.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=msvc-160#remarks
The way around this is to produce two artifacts, one with /MD and one with /MDd and the user would select the correct one according to the scenario.
For this, it a bit more nuanced in the way the library ends up getting used due to the above. There is some guidance in https://learn.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=msvc-170#what-problems-exist-if-an-application-uses-more-than-one-crt-version but it going to come down to alot of factors. I did find this article that talks through this issue in detail: https://mygoldenchariot.blogspot.com/2006/07/link-warning-lnk4098-defaultlib-libcd.html |
|
Hm ok I am still left with a question though of what's going on in rustc/llvm. Given the PR description it sounds like that the only difference is whether Also this makes me think that Rust's own usage of system libraries I guess doesn't fall into these categories? Assuming it's the C code causing this directive to show up then that means that Rust doesn't have |
It seems clang uses the static libraries by default. So what I believe is happening is that by switching to clang the build has static link, and the @MangoPeachGrape is using clang for all the other libraries and so it all syncs up and there are no warnings.
Going back to this question, generally MS recommends to not statically link unless required so the tooling sets that up by default. I found a few forum answers that said clang.exe was trying to make sure basic crt that works with helloworld https://discourse.llvm.org/t/change-default-config-of-clang-on-windows/54565
This am I not sure. |
|
Ok thanks for the info! Could this perhaps try the |
Doing this: diff --git i/ci/build-release-artifacts.sh w/ci/build-release-artifacts.sh
index 93063334d..5582ea31e 100755
--- i/ci/build-release-artifacts.sh
+++ w/ci/build-release-artifacts.sh
@@ -66,6 +66,10 @@ else
bin_flags="--features all-arch,component-model"
fi
+if [[ "$build" = "x86_64-windows" ]]; then
+ export RUSTFLAGS="-C link-args=/NODEFAULTLIB:MSVCRT $RUSTFLAGS"
+fi
+
cargo build --release $flags --target $target -p wasmtime-cli $bin_flags --features run
mkdir -p target/c-api-buildresults in a lot of linker unresolved external symbol errors, here's a snippet: |
|
Hm ok well I have no idea what's going on. Maybe just use clang on the |
|
CI seems happy, yay! Can you double-check to confirm the binary artifacts of that build are as you expect? |
|
I think using clang here might be a change from dynamically linking the CRT, to statically linking it by default. This could potentially affect other who are expecting it to be dynamically linked. Would creating two published libraries, one statically linked CRT and one with a dynamically linked CRT also resolve the issue? I guess technically it would be 4, since you need the debug versions of them as well. This appears to be what other tools do as well google/filament#810 |
|
Personally I'd fear the amount of infrastructure required to handle the Windows-specific case of producing 4 static libraries when the only real difference between them is how one small C file is compiled amongst them. At that point I'd sort of rather prefer to push on removing the C from Wasmtime entirely to avoid the need to publish 4x more libraries for Windows. But then again this is still under the murky assumption that for whatever reason |
Yes, and with my app it works linking against both the static and the dynamic lib in both debug and release (
which I would assume points to it still being linked dynamically, but I might be very wrong? |
makes sense
interesting, I guess something else is going on then. I don't know the best way to detect if it is static/dynamic in a lib. I've only really ran into this executables. |
|
Ok given that this is all working for @MangoPeachGrape I'm tempted to go ahead and merge this and consider other possible changes in future follow-ups. I hope to one day also remove the last bits of C from Wasmtime eventually too... @MangoPeachGrape mind updating this PR though to leave a comment in the code itself for why clang is specifically selected? Other than that happy to flag to merge. |
I was running into linker warnings on Windows when linking a prebuilt
wasmtime.libagainst a debug version of my application:But this didn't appear when using self-built
wasmtime.lib, for either debug or release version of my app.It turns out my build was built using Clang, while the prebuilt was using GCC.
Linker directives when built with GCC:
Linker directives when built with Clang:
Please note that I'm not a Windows expert, I don't know if
/DEFAULTLIB:MSVCRTis the correct option, but it seems fine without it.Feel free to let me know if this is not desired! (it might break something)
edit:
It seems to work the same in CI, now with Clang it only contains
/DEFAULTLIB:uuid.lib.Clang would need to be installed for the other targets in the case that this is something you want to go forward with.