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

Disambiguate Windows run-times when using clang to compile. #825

Merged
merged 2 commits into from
Jul 21, 2023

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented Jul 18, 2023

This is with reference to #819 (comment). In the comment I'm suggesting to take it as far as rejecting the use of gcc in the msvc compilation, while this PR only issues a warning.

@thomcc
Copy link
Member

thomcc commented Jul 21, 2023

Yeah, my impression is it's pretty much always wrong to use gcc for a -msvc target. @ChrisDenton any objections to merging this?

@ChrisDenton
Copy link
Member

Yes the current gnu toolchains, unlike llvm, do not aim to be compatible with msvc.

src/lib.rs Outdated Show resolved Hide resolved
// Disambiguate mingw and msvc on Windows. Problem is that
// depending on the origin clang can default to a mismatchig
// run-time.
cmd.push_cc_arg(format!("--target={}", target).into());
Copy link
Member

Choose a reason for hiding this comment

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

Does this still work if there's a mismatch between the clang target name and rust target name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To my knowledge, there is no mismatch between Rust and clang. Which shouldn't really come as surprise, both are llvm-based.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Maybe you refer to aarch64-pc-windows-llvm, or what was it called? Fair enough, I'll double-check...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Maybe you refer to aarch64-pc-windows-llvm,

It's *-pc-windows-gnullvm, i.e. not just aarch64, and there is "gnu" in the name. Surprisingly enough clang appears to accept gnullvm and even do arguably right thing. It appears to be due to a pattern matching, because it also accepts gnufoobar, gnu-foobar, and even msvcfoobar and msvc-foobar. And it generates code targeting mingw runtime if there is gnu somewhere in the target name, and vc runtime otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, clang accepts far more target triples than rustc, which only accepts a finite set (clang, due to compatibility with GCC, does quite a bit of massaging of the value).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To summarize, the answer to the original question is "yes, it still works."

Copy link
Member

Choose a reason for hiding this comment

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

We can presumably check that it continues to work with official targets. My only concern would be third party rustc targets (which can be named anything). But we can either say they aren't supported or require setting the target to a llvm compatible one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can presumably check that it continues to work with official targets.

That's what "yes, it still works" effectively mean. It still works with official targets.

Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

I mean, I'm ok with this if @thomcc is. Though we might want to keep an eye out in case anyone is tripped up by the change.

@thomcc
Copy link
Member

thomcc commented Jul 21, 2023

I'm okay with it too.

@thomcc thomcc merged commit e84367b into rust-lang:main Jul 21, 2023
16 checks passed
@dot-asm
Copy link
Contributor Author

dot-asm commented Jul 23, 2023

With cross-reference to #811, a case can be made that it would be appropriate to issue "unsupported" warning even for clang++ [not to be confused with clang-cl] being used with msvc target.

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