Fix clang-cl target when cross-compiling by ArchangelX360 · Pull Request #1670 · rust-lang/cc-rs (original) (raw)

Otherwise it forces the Arch of the host upon the compilation, producing for example coff arm64 on Darwin aarch64 host for target x86_64-pc-windows-msvc

Otherwise it forces the Arch of the host upon the compilation, producing for example coff arm64 on Darwin aarch64 host for target x86_64-pc-windows-msvc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, are you sure? target here should be the target we're compiling for, this seems more like it'd regress cross-compilation? E.g. compiling from x86_64-pc-windows-msvc to i686-pc-windows-msvc, we'd no longer be passing -m32?

Yes I'm sure that that code was producing a coff arm64 on Darwin aarch64 host for target x86_64-pc-windows-msvc, and that this patch fixed it. I can make a reproducer for you if you need.

How is the original code supposed to understand it will compile for Windows when ran on a Darwin host? It does not set the --target flag AFAIU due to the implementation of that if statement, so the behaviour I observed made sense: the compilation is producing a file for the host instead of for the target.

E.g. compiling from x86_64-pc-windows-msvc to i686-pc-windows-msvc, we'd no longer be passing -m32?

We'd be passing --target=i686-pc-windows-msvc, I'm unfamiliar but would not this be sufficient for clang-cl? It was for my case for example, but again please advise if I misunderstood, I'm not an expert here 😄 .

Right, I didn't read the else branch 🤦.

Frankly, I'm leaning towards maybe just removing the branching here, and instead always pass --target? Alternatively, maybe the check we want is not !is_cross, but maybe !host.target.contains("windows")

instead always pass --target?

I was about to ask you the same thing.
I think it makes more sense to me too, it's straightforward and I expect clang-cl to do the right thing.
At the time of the introduction of of --target in 6229121 maybe it was not considered to remove the accretion of flags that happened over time.

What do you think, should I go that way?

EDIT: probably we should maintain the -arch:SSE2 branch, something like:

if clang_cl { cmd.push_cc_arg( format!( "--target={}", target.llvm_target(&self.get_raw_target()?, None) ) .into(), );

  if target.arch == "x86" {
      // See
      // <https://learn.microsoft.com/en-us/cpp/build/reference/arch-x86?view=msvc-170>.
      //
      // NOTE: Rust officially supported Windows targets all require SSE2 as part
      // of baseline target features.
      //
      // NOTE: The same applies for STL. See: -
      // <https://github.com/microsoft/STL/issues/3922>, and -
      // <https://github.com/microsoft/STL/pull/4741>.
      cmd.push_cc_arg("-arch:SSE2".into());
  }

}

I'd be in favor of that, especially if you can test that it works on a Windows machine (if not, I can do it, but it'll take me a little while to get it set up on a VM).

Ok, I'll go that way then, cool thanks for the quick answers, really appreciate it!
Don't worry I'll test it on our Rust workspace on Windows too, and I will come back to you with a branch update 😄

Some issues observed:

• macOS ARM host, compiling for x86_64-pc-windows-msvc target, producing unexpectidly coff arm64.

I tested that new approach in the JetBrains monorepo (Fleet/Air is using Rust extensively under the hood with some non-trivial crates) and it worked fine there.

I also tested it on ArchangelX360/toolchains_llvm_testbench too, just so that you can have a look at a usual setup I would be testing against, it's important to read that section to understand what is out of the ordinary in my setups. It won't work if you try to run it yourself, because it requires an MSVC sysroot archive, which I cannot provide you for legal reasons (thanks Microsoft ☹️).

Please re-review when you have time, and we could move to a merge if you feel like it 😄

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, thanks! 👍

(CI is gonna make a release on Friday IIRC)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})