Skip to content
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 gnullvm and test gnullvm with msys2 in CI #553

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

jschwe
Copy link
Collaborator

@jschwe jschwe commented Aug 25, 2024

List of problems / fixes related to gnullvm:

  • FindRust detects when gnullvm should be merged (FindRust: support gnullvm #552).
  • Set correct implib name for gnullvm (FindRust: support gnullvm #552)
  • Handle implib location on gnullvm. Ideally wait for cargo fix to be merged and reach stable. But perhaps we could also have a short term workaround if it is simple.
  • How to invoke the linker for Rust shared libraries?
    • If corrosion uses the c/c++ linker as the linker driver like we normally do, then -lunwind is not found. Is the default MinGW in CI suitable?
    • If we don't tell Rust which linker to use, it looks for x86_64-w64-mingw32-clang, which is not in PATH, and also not the same as our C/C++ target triple. Again, raises questions on the configuration of our test case.

@mati865
Copy link
Contributor

mati865 commented Aug 26, 2024

There is no LLVM toolchain preinstalled in GHA images. You can use https://github.com/mstorsjo/llvm-mingw or CLANG* environments from MSYS2: https://github.com/msys2/setup-msys2

@PieroV
Copy link

PieroV commented Aug 26, 2024

  • How to invoke the linker for Rust shared libraries?

    • If corrosion uses the c/c++ linker as the linker driver like we normally do, then -lunwind is not found. Is the default MinGW in CI suitable?
    • If we don't tell Rust which linker to use, it looks for x86_64-w64-mingw32-clang, which is not in PATH, and also not the same as our C/C++ target triple. Again, raises questions on the configuration of our test case.

In my tests I added the toolchain to $PATH, but I guess the right thing would be having Cmake telling Cargo/Rust to use that?
Apart from that, that linker should be correct.

Anyway, I can any needed tests.
However, it'll take me a bit longer if I need to build a patched cargo 😅.

I'm using Corrosion for personal projects, but I'm using the artifacts from Tor Browser's build system (I'm a Tor Browser developer during daytime).

@jschwe
Copy link
Collaborator Author

jschwe commented Aug 26, 2024

There is no LLVM toolchain preinstalled in GHA images

When looking at the action logs it seems that CMake does detect clang installed under C:/Program Files/LLVM/bin/clang.exe

-- The C compiler identification is Clang 18.1.8
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: C:/Program Files/LLVM/bin/clang.exe - skipped
-- Detecting C compile features
-- Detecting C compile features - done

Is there any difference between that LLVM and what llvm-mingw / msys2-llvm provide?
The official runner software list lists Microsoft.VisualStudio.Component.VC.Llvm.Clang. So I'm wondering what the difference between microsoft LLVM and other LLVM toolchains is (and if I can detect that from CMake to decide on the toolchain)

@PieroV
Copy link

PieroV commented Aug 26, 2024

Is there any difference between that LLVM and what llvm-mingw / msys2-llvm provide? The official runner software list lists Microsoft.VisualStudio.Component.VC.Llvm.Clang. So I'm wondering what the difference between microsoft LLVM and other LLVM toolchains is (and if I can detect that from CMake to decide on the toolchain)

As far as I know, the binaries provided by LLVM include only the compilers and other tools (linker, windres, etc...), they don't include any runtime and/or standard library implementation.
They will look for an existing Visual Studio installation by default on Windows, but it should work also with mingw or even be able to cross compile with the proper incantation (a mix of --sysroot, --target, and similar arguments).

The llvm-mingw binaries provide an LLVM toolchain plus mingw, libc++ (LLVM's C++ standard library implementation), libunwind, compiler-rt, and maybne some other runtime.
Theoretically, you could use the official binaries with llvm-mingw's runtimes, but I don't know if there's a reason to prefer it over using only llvm-mingw for this use-case.

@jschwe jschwe force-pushed the jschwender/gnullvm_ci branch 2 times, most recently from 8b7b7fb to 21adac8 Compare August 26, 2024 23:22
@mati865
Copy link
Contributor

mati865 commented Aug 26, 2024

should work also with mingw or even be able to cross compile with the proper incantation (a mix of --sysroot, --target, and similar arguments).

Some features will be missing when using mingw-w64 built with GCC though, like Control Flow Guard support. Also, libraries like winpthreads won't be fully compatible because of different TLS and unwinders. This should be fine for most users, but it's better to use purposely built mingw-w64 if doable.

@jschwe jschwe force-pushed the jschwender/gnullvm_ci branch 4 times, most recently from 4532345 to c0ef9a1 Compare September 2, 2024 18:45
jschwe and others added 2 commits September 4, 2024 16:25
install tools from msys2, since preinstalled LLVM toolchain
does not contain libunwind and co.

Co-authored-by: Mateusz Mikuła <[email protected]>
@jschwe jschwe force-pushed the jschwender/gnullvm_ci branch from c0ef9a1 to 6b4a78d Compare September 4, 2024 14:25
@jschwe jschwe marked this pull request as ready for review September 4, 2024 14:25
@jschwe jschwe changed the title Draft: Fix gnullvm / test in CI Fix gnullvm and test gnullvm with msys2 in CI Sep 4, 2024
@jschwe
Copy link
Collaborator Author

jschwe commented Sep 4, 2024

It looks like the upstream cargo fix will still take a while to hit stable, so in the meantime I'm fine with merging the quickfix I added in this branch (which copies the file out of the deps dir). Once the upstream fix has reached stable, I would remove this workaround though, and instead just raise the minimum rust version corrosion requires for gnu-llvm targets.

@jschwe jschwe enabled auto-merge (rebase) September 4, 2024 14:29
@PieroV
Copy link

PieroV commented Sep 4, 2024

Thank you very much for working on this!

@jschwe jschwe merged commit 55a560d into corrosion-rs:master Sep 4, 2024
38 checks passed
@jschwe jschwe deleted the jschwender/gnullvm_ci branch September 4, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants