search

corrosion-rs / corrosion

Marrying Rust and CMake - Easy Rust and C/C++ Integration!
https://corrosion-rs.github.io/corrosion/
MIT License
1.1k stars 106 forks source link

[Feature request]: Support *-pc-windows-gnullvm targets #551

Closed PieroV closed 2 months ago

PieroV commented 3 months ago

Hi! Thanks for this project, it's very nice.

I tried it with the x86_64-pc-windows-gnullvm target, which is very similar to x86_64-pc-windows-gnu but uses only LLVM tools. It worked almost out of the box, but I had to change this line in Corrosion.cmake:

        elseif(Rust_CARGO_TARGET_ENV STREQUAL "gnu" OR Rust_CARGO_TARGET_ENV STREQUAL "gnullvm")
            set(is_windows_gnu TRUE)
        endif()

(I had to add the OR Rust_CARGO_TARGET_ENV STREQUAL "gnullvm" part).

In addition to that, I had to manually pass these variables to cmake:

Rust_CARGO_TARGET=x86_64-pc-windows-gnullvm
Rust_CARGO_TARGET_LINK_NATIVE_LIBS='ws2_32;ntoskrnl;userenv'

I would be happy to open a PR to support it officially, if this could be helpful to someone else, but I don't know where the initial value of the native libraries is set (for me it was gcc_s;util;rt;pthread;m;dl;c, which matches my host libraries, but I'm cross compiling).

If I understand correctly, this command is run in corrosion/required_libs, but it fails for me:

> cargo rustc --verbose --color never --target=x86_64-pc-windows-gnullvm --print=native-static-libs
error: the `print` flag is unstable, and only available on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
See https://github.com/rust-lang/cargo/issues/9357 for more information about the `print` flag.

Also, I'd need some help for writing any needed tests.

jschwe commented 3 months ago

Please also note https://github.com/corrosion-rs/corrosion/pull/546 and my comments there. Specifically I'm interested in

If I understand correctly, this command is run in corrosion/required_libs, but it fails for me:

The command corrosion runs has an additional -- before the --print. The cargo flag is unstable, but the rustc flag is stable, and those are separated by a --.

> cargo rustc --verbose --color never --target=<triple> -- --print=native-static-libs

Could you rerun this command with the -- and report the output? Please also note that configuring CMake in debug mode will also provide additional helpful output (cmake -S<source_dir> -B<build_dir> --log-level=DEBUG <your_other_options>）

ws2_32;ntoskrnl;userenv

Are these libraries also required for a minimal hello world example? If not, then it's likely that these libraries are added by dependencies (which probably report that they need to link to a library via a build-script). Such libraries missing is a known limitation of Corrosion, that needs upstream CMake additions to fix reliably.

PieroV commented 3 months ago

Thanks for the prompt answer! I tried to look for issues but I missed a PR was already existing, sorry!

Please also note #546 and my comments there. Specifically I'm interested in

  • What are the Clang / CMake compiler targets, which corresponds to Rusts gnullvm?

Clang and CMake use x86_64-pc-windows-gnu also for this target. Clang manages to find the correct runtime libraries by itself when they are in their appropriate directory. The reference for creating an all-LLVM mingw pipeline is this repository.

  • Is using gcc to compile for *-gnu and linking a Rust library compiled for gnullvm sound?

I have never tried. The main problems might be with unwinding, but I believe it's sort of expected not to work over FFI boundaries. Usually LLVM runtimes are made to be ABI compatible with GNU. FWIW, Firefox has been doing the opposite for years (use an LLVM-based mingw on the C/C++ side and the GNU target on Rust) in their tier 2 target (the official binaries are Clang/msvc, but the CI builds also for mingw).

  • Why is there no IMPLIB? How does CMake handle that?

Sorry, I don't know what you're talking about :sweat_smile:. However I've been using an LLVM based toolchain for a while, and I've never had to change anything that sounds like this (usually, if something supports GCC+mingw, it also works out of the box with Clang).

I've seen in the PR IMPLIB has been set to empty for the gnullvm target. I set it to the same as GNU in my test. As far as I know, the main difference between gnu and gnullvm are the runtime (libcompilerrt + libunwind instead of libgcc_s), but the rest of the tooling is the same.

If I understand correctly, this command is run in corrosion/required_libs, but it fails for me:

The command corrosion runs has an additional -- before the --print. The cargo flag is unstable, but the rustc flag is stable, and those are separated by a --.

> cargo rustc --verbose --color never --target=<triple> -- --print=native-static-libs

Could you rerun this command with the -- and report the output? Please also note that configuring CMake in debug mode will also provide additional helpful output (cmake -S<source_dir> -B<build_dir> --log-level=DEBUG <your_other_options>）

Yeah, sorry, I somehow missed that!

> cargo rustc --verbose --color never --target=x86_64-pc-windows-gnullvm -- --print=native-static-libs
   Compiling required_libs v0.1.0 (/home/piero/sdem/build/corrosion/required_libs)
     Running `rustc --crate-name required_libs --edition=2018 lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=282 --crate-type staticlib --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 --print=native-static-libs -C metadata=869712cafa1f4636 -C extra-filename=-869712cafa1f4636 --out-dir /home/piero/sdem/build/corrosion/required_libs/target/x86_64-pc-windows-gnullvm/debug/deps --target x86_64-pc-windows-gnullvm -C incremental=/home/piero/sdem/build/corrosion/required_libs/target/x86_64-pc-windows-gnullvm/debug/incremental -L dependency=/home/piero/sdem/build/corrosion/required_libs/target/x86_64-pc-windows-gnullvm/debug/deps -L dependency=/home/piero/sdem/build/corrosion/required_libs/target/debug/deps`
note: Link against the following native artifacts when linking against this static library. The order and any duplication can be significant on some platforms.

note: native-static-libs: -lkernel32 -ladvapi32 -lkernel32 -lntdll -luserenv -lws2_32 -lkernel32 -lws2_32 -lkernel32 -lunwind

    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.14s

ws2_32;ntoskrnl;userenv

Are these libraries also required for a minimal hello world example? If not, then it's likely that these libraries are added by dependencies (which probably report that they need to link to a library via a build-script). Such libraries missing is a known limitation of Corrosion, that needs upstream CMake additions to fix reliably.

IIRC, they were needed by something like std::sys::, but I will try again later.

Thanks again for your answer!

PieroV commented 3 months ago

ws2_32;ntoskrnl;userenv

Are these libraries also required for a minimal hello world example? If not, then it's likely that these libraries are added by dependencies (which probably report that they need to link to a library via a build-script). Such libraries missing is a known limitation of Corrosion, that needs upstream CMake additions to fix reliably.

IIRC, they were needed by something like std::sys::, but I will try again later.

Thanks again for your answer!

I setup a new project. Instead of using FetchContent, I cloned corrosion and applied my patch before running CMake:

diff --git a/cmake/Corrosion.cmake b/cmake/Corrosion.cmake
index 3ad5bc1..f5132ea 100644
--- a/cmake/Corrosion.cmake
+++ b/cmake/Corrosion.cmake
@@ -355,7 +355,7 @@ function(_corrosion_add_library_target)
         set(is_windows TRUE)
         if(Rust_CARGO_TARGET_ENV STREQUAL "msvc")
             set(is_windows_msvc TRUE)
-        elseif(Rust_CARGO_TARGET_ENV STREQUAL "gnu")
+        elseif(Rust_CARGO_TARGET_ENV STREQUAL "gnu" OR Rust_CARGO_TARGET_ENV STREQUAL "gnullvm")
             set(is_windows_gnu TRUE)
         endif()
     elseif(Rust_CARGO_TARGET_OS STREQUAL "darwin")

At that point, Corrosion found the needed libraries correctly:

> cmake .. -DCMAKE_SYSTEM_NAME=Windows -DCMAKE_C_COMPILER=x86_64-w64-mingw32-clang -DCMAKE_CXX_COMPILER=x86_64-w64-mingw32-clang++ -DRust_CARGO_TARGET=x86_64-pc-windows-gnullvm
-- Determining required link libraries for target x86_64-pc-windows-gnullvm
-- Required static libs for target x86_64-pc-windows-gnullvm: kernel32;advapi32;kernel32;ntdll;userenv;ws2_32;kernel32;ws2_32;kernel32;unwind
-- Required link flags for target x86_64-pc-windows-gnullvm: 
-- Determining required link libraries for target x86_64-unknown-linux-gnu
-- Required static libs for host target x86_64-unknown-linux-gnu: gcc_s;util;rt;pthread;m;dl;c
-- Found Rust: /var/tmp/dist/rust/bin/rustc (found version "1.78.0") 
-- The C compiler identification is Clang 18.1.5
-- The CXX compiler identification is Clang 18.1.5
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /var/tmp/dist/mingw-w64-clang/bin/x86_64-w64-mingw32-clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /var/tmp/dist/mingw-w64-clang/bin/x86_64-w64-mingw32-clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done
-- Generating done
-- Build files have been written to: /home/piero/hello/build

ntoskrnl hasn't been added, but I guess something else is linking to it, or somehow providing the features for which I initially linked it and I can't replicate the error I had in the hello project.

As for the implib, Cmake generates indeed a libxyz.dll.a, So, I tried to run cargo directly with cdylib instead of staticlib, and it generated it as well, but in the deps directory. In particular, it generated these files:

And Corrosion doesn't find the implib:

[  0%] Built target cargo-prebuild_hello_rs
   Compiling hello_rs v0.1.0 (/home/piero/hello/hello_rs)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.07s
Copying byproducts `libhello_rs.dll.a` to /home/piero/hello/build
Error copying file (if different) from "/home/piero/hello/build/./cargo/build/x86_64-pc-windows-gnullvm/debug/libhello_rs.dll.a" to "/home/piero/hello/build".
make[2]: *** [CMakeFiles/_cargo-build_hello_rs.dir/build.make:76: _cargo-build_hello_rs] Error 1
make[1]: *** [CMakeFiles/Makefile2:107: CMakeFiles/_cargo-build_hello_rs.dir/all] Error 2
make: *** [Makefile:91: all] Error 2

Copying to the directory it expects resolved the problem. However, I also tried to create an example program that links to the DLL without the .a, and it works as well.

jschwe commented 2 months ago

As for the implib, Cmake generates indeed a libxyz.dll.a, So, I tried to run cargo directly with cdylib instead of staticlib, and it generated it as well, but in the deps directory. In particular, it generated these files:

target/x86_64-pc-windows-gnullvm/debug/deps/libhello_rs.dll.a

Thanks a lot for testing. I believe probably cargo needs a patch to copy the implib to the same location (as I don't want to start digging into the deps directory). I don't really know what I'm doing, but I gave it a shot in https://github.com/rust-lang/cargo/pull/14451 and hope that it is sufficient.

PieroV commented 2 months ago

Thanks a lot for testing. I believe probably cargo needs a patch to copy the implib to the same location (as I don't want to start digging into the deps directory).

Makes sense.

I don't really know what I'm doing, but I gave it a shot in rust-lang/cargo#14451 and hope that it is sufficient.

Great, and it's already been merged approved! gnullvm is a fairly new target (it's been raised from tier 3 to tier 2 without host tools quite recently), so it probably needs some refinements.

jschwe commented 2 months ago

I have a draft PR which adds gnullvm to CI, but it looks like there are also other problems besides the implib issue: https://github.com/corrosion-rs/corrosion/pull/553

If someone has time to investigate, or knows what additional setup might be needed, I'd be glad for any comments.

jschwe commented 2 months ago

Fixed in #553