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

Add "prefer to use clang instead of MSVC" API that can automatically find Microsoft-provided clang #882

Open
briansmith opened this issue Oct 3, 2023 · 15 comments

Comments

@briansmith
Copy link

briansmith commented Oct 3, 2023

I would love to be able in my build.rs to instead just say b.prefer_clang_over_msvc() to get cc-rs to automatically discover Microsoft-provided clang's and use it if/when it is available, when it would otherwise choose MSVC.

Some projects, such as ring, require clang when building for Windows AArch64.
Some projects, such as ring would prefer to build with clang if it were available, when building for Windows X86-64, because the C code is optimized for clang in various ways (because Google uses clang for everything, and the C code we have comes from Google, and Google's preferences are unlikely to change any time soon).

Currently I am recommending people to do this:

$env:Path += ";C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC\Tools\Llvm\x64\bin"

to get ring to build for aarch64-pc-windows-msvc using the clang that Microsoft provides with the build tools. Unfortunately, this advice is problematic:

  • It is one extra step that shouldn't be necessary.
  • "2022" varies depending on the version of MS build tools that is released
  • "BuildTools" varies depending on whether the user installed the "Build Tools," "Community," "Enterprise," etc. edition of Visual Studio.
  • "x64" varies depending on the host: ARM64 vs. x64, IIRC.
@ChrisDenton
Copy link
Contributor

This sounds like a job for vswhere.exe. If Visual Studio is installed then it's always located at %ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe and can discover paths.

@thomcc
Copy link
Member

thomcc commented Oct 3, 2023

I'd take patches to support this, but have no strong feeling on the implementation strategy (calling out to something like vswhere is certainly acceptable if it works well though).

The API name prefer_clang_over_msvc() is a little unwieldy but I don't think it's likely to be commonly needed so it probably doesn't matter.

@briansmith
Copy link
Author

I'd take patches to support this, but have no strong feeling on the implementation strategy (calling out to something like vswhere is certainly acceptable if it works well though).

How does using vswhere.exe differ from what cc-rs currently does to find MSVC itself? I wonder it might be better for cc-rs to adapt what it currently does for finding MSVC to find clang directly without shelling out to vswhere?

@ChrisDenton
Copy link
Contributor

Sure. If we did this though I'd personally prefer we adapt cc::windows_registry::find (and find_tool) to work for finding llvm rather than adding an unwieldy function.

@briansmith
Copy link
Author

briansmith commented Oct 3, 2023

This would be awesome. I don't have time to write the implementation myself, but I am happy to commit to helping in every other way though, especially testing. I can also help review the design and code though I'm not authoritative in any way regarding cc-rs.

@thomcc
Copy link
Member

thomcc commented Oct 3, 2023

Hm, the hard part of this does tend to be testing. I no longer have access to a windows machine I can use for development though, so I can't help either.

@ChrisDenton
Copy link
Contributor

Unfortunately I don't have much time myself atm (I'm already not keeping up with a lot of stuff). But if someone wants to give it a try I can offer help if they get stuck or need questions answered. If nobody picks this up I'll be happy to work on it when I have more time but that might be awhile.

You'd possibly want to start by looking at how msbuild is handled in https://github.com/rust-lang/cc-rs/blob/main/src/windows_registry.rs. Fair warning that quite a bit of the code is quite old and could really do with a cleanup/refactoring. But that's beyond the scope of this issue.

@briansmith
Copy link
Author

@awakecoding did a lot of work in this area and shared the link to microsoft/vswhere#242 where he tried to get vswhere.exe to add this functionality. There and elsewhere he also shared his logic, which I'll copy here (I hope you don't mind; PLMK if you do):

$VSINSTALLDIR = $(vswhere.exe -latest -requires Microsoft.VisualStudio.Component.VC.Llvm.Clang -property installationPath)
$VCINSTALLDIR = Join-Path $VSINSTALLDIR "VC"
$LLVM_ROOT = Join-Path $VCINSTALLDIR "Tools\Llvm\x64"
$Env:PATH += ";$LLVM_ROOT\bin"

image

@ChrisDenton
Copy link
Contributor

ChrisDenton commented Oct 3, 2023

So it should just need to probe the visual studio directories for e.g. VC\Tools\Llvm\{host_arch}\bin\clang.exe. Which should not be too difficult. I think llvm only started being distributed via VS in version 2019 so we only need to support 2019+ (aka Vs16+).

@briansmith
Copy link
Author

I think that's right. And I think if you had to limit it to 2022+, it would not be bad.

@dot-asm
Copy link
Contributor

dot-asm commented Oct 7, 2023

My question is does it have to be that "selfish"? What I'm referring to is suggestion to make it exclusively about msvc. Why not .compiler_if_available() as a complement to the .compiler() method? Or .try_compiler()? As a last resort why not just platform-neutral .prefer_clang()?

On a tangential note. Using clang in *-msvc context is actually [linking-]error-prone, clang-cl would be a much better choice. With this in mind, if .prefer_clang_over_msvc() is chosen, I would suggest that it switched to clang-cl, not clang.

@briansmith
Copy link
Author

@dot-asm I am so happy to hear from you. Note that I need this just for aarch64-pc-windows-msvc where I'm inheriting code from BoringSSL that is NOT written using PerlAsm but rather just straight gas syntax .S files. Also, in this build configuration, LINK.EXE still does the linking.

@dot-asm
Copy link
Contributor

dot-asm commented Oct 9, 2023

straight gas syntax .S files.

Well, formally speaking it doesn't qualify for making things less usable for everybody. I mean no overly specific and non-common case should. Well, at least not in my book, as I don't speak for maintainers and don't know what they will choose to do :-)

Also, in this build configuration, LINK.EXE still does the linking.

I'm aware of that. Linking problems stem from the fact that clang [unlike clang-cl!], doesn't leave linking directives that *-mscv targets rely upon. Well, this can be misinterpreted. Rust-only programs don't actually rely on the said directives, nor would lack of them affect your build with .S files, but C++ users are virtually guaranteed to fail to link their C++ libraries if they are compiled with clang. And C users would run a risk depending on what their code does. This is why I advocate for clang-cl over clang. As for the .S files, I would argue that it's sufficiently uncommon to say that it's not unreasonable to suggest to call .compiler("clang") in the build script. Granted, it would be a big help if cc-rs would discover if clang is installed and adjust the search %PATH%, so maybe this should be the suggestion, not .prefer_clang_over_msvc()...

@dot-asm
Copy link
Contributor

dot-asm commented Oct 9, 2023

ring ... straight gas syntax .S files.

But all the straight .S files you have don't target Windows, let alone Windows on ARM. Wouldn't it be more reasonable to simply not attempt to compile them? Or what am I missing? OrOn a somewhat related note, arm-xlate.pl [as well as its x86_64 counterpart actually] can emit code suitable for consumption by msvc assembler now. [Though to be honest, I haven't actually tested 32-bit code "in the field," only 64-bit one.]

@dot-asm
Copy link
Contributor

dot-asm commented Oct 10, 2023

Alternatively one can put is as following. If user tries to compile .S file in msvc environment, look up clang and use it. Irregardless which compiler is chosen. If lookup fails, compilation fails.

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

No branches or pull requests

4 participants