fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI #1015

russelltg · 2024-03-19T02:14:34Z

This PR contains several clang on windows improvements:

properly detect clang vs clang-cl by invoking with -?, instead of checking for _MSC_VER. This is what cmake does, and _MSC_VER is always defined for msvc ABI targets, regardless of the frontend variant used
Fix detection of which output assembler flag to use. It used to use complex logic, including if the C compiler is clang, which was incorrect. Use the flag based on if it's using the msvc assembler
Add clang-cl/clang CI for windows
Detect usage of llvm-ar and pass GNU style flags to it

Original description:

this is somewhat a nasty hack here, but it does work. when (cross) compiling for msvc and you use llvm-ar as the archiver, then you need to use gnu-style flags.

currently it attemps to run llvm-ar like:

running: "/usr/bin/llvm-ar" "-out:/workspaces/ars/build_windows_clang_x86_64_relnoopt/./cargo/build/x86_64-pc-windows-msvc/release/build/link-cplusplus-b04e59fc086748c2/out/liblink-cplusplus.a" "-nologo" "/workspaces/ars/build_windows_clang_x86_64_relnoopt/./cargo/build/x86_64-pc-windows-msvc/release/build/link-cplusplus-b04e59fc086748c2/out/0466faf07398e270-dummy.o"

which doesn't work, as llvm-ar takes in GNU-style ar flags.

i'm definitely open to a more robust way to get the archiver "Family" (maybe something similar to how we detect compiler family? idk) if that would be better

I also moved some APIs to to use Path as it avoided a string->path conversion. Let me know if that's not of interest.

With this and the other PR, I can now cross-compile to windows using clang from linux :)

EDIT: also fixes the path to the archiver not being set on when env_tool finds it

NobodyXu

i'm definitely open to a more robust way to get the archiver "Family" (maybe something similar to how we detect compiler family? idk) if that would be better

I don't know any better way than checking name, or its output of --version.

Perhaps we can try link something and check its output for signature of linker?

With this and the other PR, I can now cross-compile to windows using clang from window

Is it possible to add testing for this in CI?

NobodyXu · 2024-03-19T07:41:25Z

src/lib.rs

@@ -2398,7 +2398,8 @@ impl Build {
        }

        let target = self.get_target()?;
-        if target.contains("msvc") {
+        let (mut ar, cmd, _any_flags) = self.get_ar()?;
+        if target.contains("msvc") && !cmd.to_str().unwrap().contains("llvm-") {


Suggested change

if target.contains("msvc") && !cmd.to_str().unwrap().contains("llvm-") {

if target.contains("msvc") && !cmd.to_str_lossy().contains("llvm-") {

Applied, except to_str_lossy of course doesn't exist as the lossy conversion requires a (conditional) allocation

NobodyXu · 2024-03-19T07:41:58Z

src/lib.rs

-        if target.contains("msvc") {
-            let (mut cmd, program, any_flags) = self.get_ar()?;
+        let (mut cmd, program, any_flags) = self.get_ar()?;
+        if target.contains("msvc") && !program.to_str().unwrap().contains("llvm-") {


Suggested change

if target.contains("msvc") && !program.to_str().unwrap().contains("llvm-") {

if target.contains("msvc") && !program.to_str_lossy().contains("llvm-") {

this is somewhat a nasty hack here, but it does work. when (cross) compiling for msvc and you use llvm-ar as the archiver, then you need to use gnu-style flags. currently it attemps to run llvm-ar like: ``` running: "/usr/bin/llvm-ar" "-out:/workspaces/ars/build_windows_clang_x86_64_relnoopt/./cargo/build/x86_64-pc-windows-msvc/release/build/link-cplusplus-b04e59fc086748c2/out/liblink-cplusplus.a" "-nologo" "/workspaces/ars/build_windows_clang_x86_64_relnoopt/./cargo/build/x86_64-pc-windows-msvc/release/build/link-cplusplus-b04e59fc086748c2/out/0466faf07398e270-dummy.o" ``` which doesn't work, as llvm-ar takes in GNU-style ar flags. i'm definitely open to a more robust way to get the archiver "Family" (maybe something similar to how we detect compiler family? idk) if that would be better

.github/workflows/main.yml

russelltg · 2024-03-19T23:33:36Z

While working on that CI pass, I found that the clang-cl detection doesn't work at all....

Clang defines _MSC_VER when target is *msvc, and actually has no relation to the frontend variant. That combined with us not passing C[XX]FLAGS to the -E invocation just means that all it detects is if the host system is windows, lol....

russelltg · 2024-03-19T23:34:51Z

While I'm working on clang cross compilation for linux->windows, I was going to add the CI pass to windows so I don't have to worry about getting windows SDK/msvc installed in Linux. But since the host is windows it ends up thinking clang is clang-cl!

russelltg · 2024-03-19T23:35:56Z

I know you're waiting on this to release--I think it's still a good change of course, but that CI is not going to work yet. Want me to rip out CI and open a new PR with hopefully some way to fix clang-cl detection, that can be merged after the release?

NobodyXu · 2024-03-20T00:27:53Z

Want me to rip out CI and open a new PR with hopefully some way to fix clang-cl detection, that can be merged after the release?

I would prefer the clang-cl to be fixed as well before cutting a new release to avoid regression.

NobodyXu · 2024-03-20T02:53:45Z

BTW, can you please update the PR title and description?

Thanks!!

russelltg · 2024-03-21T06:17:18Z

Okay, this seems to work now 😅

It ended up being more changes than I thought! Do you want me to split into mutiple PRs? Im also happy to rebase it into a cleaner incremental history for future bisecting

russelltg · 2024-03-21T06:20:24Z

Hmm it's hard to be sure that the env vars are actually being set....

russelltg · 2024-03-21T06:33:56Z

Ah, I don't think it's possible to emit multiple env vars from GITHUB_ENV? At least I can't figure it out.

which is kinda wild

russelltg · 2024-03-21T06:46:06Z

Ok, not quite ready I guess. Will look tomorrow

NobodyXu · 2024-03-21T07:54:06Z

Ah, I don't think it's possible to emit multiple env vars from GITHUB_ENV? At least I can't figure it out.

The doc says just put them in multiple lines, i.e. echo -e "A=...\nB=..." >> "$GITHUB_ENV"

.github/workflows/main.yml

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2024-03-30T12:09:15Z

Updated CI and code, still doesn't work

russelltg · 2024-03-31T22:25:04Z

Ok, phew, there was some more cases to handle here, but it seems to be working.

russelltg · 2024-03-31T23:07:08Z

I'm not sure if you want to keep the changes you made, notably the CI formatting changes. I'm happy to simplify that diff if you want.

NobodyXu · 2024-03-31T23:33:33Z

I'm not sure if you want to keep the changes you made, notably the CI formatting changes. I'm happy to simplify that diff if you want.

That'd be ok for me, thanks for yhe fix!

NobodyXu

Thank you, in LGTM, however since I'm not very familiar with cc test-suite, I'd like another approval from another maintainer @thomcc

russelltg · 2024-03-31T23:54:53Z

Opened llvm/llvm-project#87209 to address clang-cl not taking /Fo: but let's definitely stick with /Fo for maximum compatibility.

thomcc

Thanks, the changes look good. That said, it's a big change, and I don't think we have great test coverage here, so it does make me worry a little. We should land it anyway though.

That said, maybe we should squash-land this so that we can revert more easily if it causes problems. WDYT @NobodyXu?

thomcc · 2024-04-07T09:35:07Z

.github/workflows/main.yml

+          # Some packages were removed, and this is causing the g++multilib
+          # install to fail. Similar issue:
+          # https://github.com/scikit-learn/scikit-learn/issues/13928.
+          sudo add-apt-repository --remove ppa:ubuntu-toolchain-r/test


Well, if it works...

thomcc · 2024-04-07T09:36:25Z

src/command_helpers.rs

-    is_arm: bool,
-) {
-    if msvc && !clang && !gnu && !cuda && !(is_asm && is_arm) {
+pub(crate) struct CmdAddOutputFileArgs {


Thanks for this.

thomcc · 2024-04-07T09:37:38Z

src/lib.rs

-        if target.contains("msvc") {
-            let (mut cmd, program, any_flags) = self.get_ar()?;
+        let (mut cmd, program, any_flags) = self.get_ar()?;
+        if target.contains("msvc") && !program.to_string_lossy().contains("llvm-") {


This will probably need adjusting but for now is fine.

thomcc · 2024-04-07T09:38:29Z

src/lib.rs

@@ -3939,7 +3959,7 @@ fn which(tool: &Path, path_entries: Option<OsString>) -> Option<PathBuf> {
 }

 // search for |prog| on 'programs' path in '|cc| -print-search-dirs' output
-fn search_programs(cc: &mut Command, prog: &str, cargo_output: &CargoOutput) -> Option<PathBuf> {
+fn search_programs(cc: &mut Command, prog: &Path, cargo_output: &CargoOutput) -> Option<PathBuf> {


Ideally the change to use &Path would be in a different pr. But I'm not about to hold this up for that, since it is a good change.

thomcc · 2024-04-07T09:39:03Z

src/tool.rs

+            // https://gitlab.kitware.com/cmake/cmake/-/blob/69a2eeb9dff5b60f2f1e5b425002a0fd45b7cadb/Modules/CMakeDetermineCompilerId.cmake#L267-271
+            let accepts_cl_style_flags =
+                run(Command::new(path).arg("-?").stdout(Stdio::null()), path, &{
+                    // the errors are not errors!


NobodyXu · 2024-04-07T09:49:29Z

That said, maybe we should squash-land this so that we can revert more easily if it causes problems.

Yeah, cc-rs defaults to squash merge so it should be pretty easy to revert.

NobodyXu requested changes Mar 19, 2024

View reviewed changes

russelltg added 4 commits March 19, 2024 08:18

fix usage of get_command

094d406

fmt

d388cb8

initial attempt at clang windows CI

d737de1

russelltg force-pushed the llvm_ar_msvc branch from 4d3fa6d to d737de1 Compare March 19, 2024 14:29

russelltg added 3 commits March 19, 2024 08:31

apply suggestion

9eff753

hopefully get clang in path

7c20d23

oops this is powershell

3da03c7

NobodyXu requested changes Mar 19, 2024

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

NobodyXu mentioned this pull request Mar 19, 2024

Compiler version doesn't include clang or GCC: "cc" "--version" #958

Closed

use -? to detect cl-style frontends

0571f2a

russelltg added 3 commits March 20, 2024 23:17

use output flag directly for assembler type

4581b26

apply ci suggestion

c5c7562

add windows clang-cl ci

ea91a79

russelltg changed the title ~~fix llvm-ar as archiver for msvc targets~~ fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI Mar 21, 2024

russelltg added 5 commits March 20, 2024 23:20

remove duplicate env

0ec9ca3

make sure output arg is in same arg

d44e733

fix creation of output arg

8b7ed60

use msvc-style args for msvc

e022679

hopefully fix env

4c5260a

russelltg added 2 commits March 21, 2024 00:21

add test env command

9d35dcd

go back to previous env method

ba42c16

maybe?

d69c416

NobodyXu requested changes Mar 21, 2024

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

NobodyXu added 4 commits March 30, 2024 21:35

FIx CI

975a9d7

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Re-enable warnings-as-error when compiling tests

6237e2e

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix create_compile_object_cmd

f573a66

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix is_flag_supported and create_compile_object_cmd

6e72ff5

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

russelltg added 9 commits March 31, 2024 14:12

another ci attempt

ad8d072

generalize NMakefile to allow clang flags

940bc3e

fmt

2c41d81

try re-adding devenv

ff0f7af

fix env vars in commandline tests

fe5ae81

apparently clang-cl doesn't accept -Fo:

fd6b59a

better check for if compiler is clang

80a2f81

fix clang not being detected as clang

006ac81

fix typo, minor cleanup

eed65b4

NobodyXu approved these changes Mar 31, 2024

View reviewed changes

NobodyXu requested a review from thomcc March 31, 2024 23:40

thomcc approved these changes Apr 7, 2024

View reviewed changes

NobodyXu merged commit fd912ec into rust-lang:main Apr 7, 2024
23 checks passed

NobodyXu mentioned this pull request Apr 8, 2024

1.0.91 update breaks cargo-xwin #1026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI #1015

fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI #1015

russelltg commented Mar 19, 2024 •

edited

NobodyXu left a comment

NobodyXu Mar 19, 2024

russelltg Mar 19, 2024 •

edited

NobodyXu Mar 19, 2024

russelltg commented Mar 19, 2024

russelltg commented Mar 19, 2024

russelltg commented Mar 19, 2024

NobodyXu commented Mar 20, 2024

NobodyXu commented Mar 20, 2024

russelltg commented Mar 21, 2024

russelltg commented Mar 21, 2024

russelltg commented Mar 21, 2024 •

edited

russelltg commented Mar 21, 2024

NobodyXu commented Mar 21, 2024

NobodyXu commented Mar 30, 2024

russelltg commented Mar 31, 2024

russelltg commented Mar 31, 2024

NobodyXu commented Mar 31, 2024

NobodyXu left a comment

russelltg commented Mar 31, 2024

thomcc left a comment

thomcc Apr 7, 2024

thomcc Apr 7, 2024

thomcc Apr 7, 2024

thomcc Apr 7, 2024

thomcc Apr 7, 2024

NobodyXu commented Apr 7, 2024

	if target.contains("msvc") && !cmd.to_str().unwrap().contains("llvm-") {
	if target.contains("msvc") && !cmd.to_str_lossy().contains("llvm-") {

	if target.contains("msvc") && !program.to_str().unwrap().contains("llvm-") {
	if target.contains("msvc") && !program.to_str_lossy().contains("llvm-") {

fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI #1015

fix llvm-ar as archiver for msvc targets; fix clang-cl detection; fix assembler output flag detection; add clang/clang-cl windows CI #1015

Conversation

russelltg commented Mar 19, 2024 • edited

NobodyXu left a comment

Choose a reason for hiding this comment

NobodyXu Mar 19, 2024

Choose a reason for hiding this comment

russelltg Mar 19, 2024 • edited

Choose a reason for hiding this comment

NobodyXu Mar 19, 2024

Choose a reason for hiding this comment

russelltg commented Mar 19, 2024

russelltg commented Mar 19, 2024

russelltg commented Mar 19, 2024

NobodyXu commented Mar 20, 2024

NobodyXu commented Mar 20, 2024

russelltg commented Mar 21, 2024

russelltg commented Mar 21, 2024

russelltg commented Mar 21, 2024 • edited

russelltg commented Mar 21, 2024

NobodyXu commented Mar 21, 2024

NobodyXu commented Mar 30, 2024

russelltg commented Mar 31, 2024

russelltg commented Mar 31, 2024

NobodyXu commented Mar 31, 2024

NobodyXu left a comment

Choose a reason for hiding this comment

russelltg commented Mar 31, 2024

thomcc left a comment

Choose a reason for hiding this comment

thomcc Apr 7, 2024

Choose a reason for hiding this comment

thomcc Apr 7, 2024

Choose a reason for hiding this comment

thomcc Apr 7, 2024

Choose a reason for hiding this comment

thomcc Apr 7, 2024

Choose a reason for hiding this comment

thomcc Apr 7, 2024

Choose a reason for hiding this comment

NobodyXu commented Apr 7, 2024

russelltg commented Mar 19, 2024 •

edited

russelltg Mar 19, 2024 •

edited

russelltg commented Mar 21, 2024 •

edited