Skip to content

Fix wasm32-unknown-unknown by passing -c #1424

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

NobodyXu
Copy link
Collaborator

Fix #1423

@NobodyXu NobodyXu requested a review from madsmtm February 27, 2025 13:37
@NobodyXu NobodyXu enabled auto-merge (squash) February 27, 2025 13:38
@NobodyXu NobodyXu disabled auto-merge February 27, 2025 13:38
@NobodyXu
Copy link
Collaborator Author

I don't know why it's removed, cc @madsmtm do you remember why?

@madsmtm
Copy link
Collaborator

madsmtm commented Feb 28, 2025

Traced this back to #1322.

@madsmtm
Copy link
Collaborator

madsmtm commented Feb 28, 2025

Perhaps we need to add an (internal) flag needs_linker to is_flag_supported? And set it to true for -fembed-bitcode=... (and maybe others)?

I generally agree that we should not attempt to link if we can avoid it, so we really should pass -c unless absolutely necessary that we don't.

@madsmtm
Copy link
Collaborator

madsmtm commented Feb 28, 2025

Hmm, actually, I think #1322 is wrong, and the real solution was #1379.

CC @clubby789, can you check if this PR still works for you?

@madsmtm madsmtm added the bug label Feb 28, 2025
Co-authored-by: Mads Marquart <[email protected]>
@clubby789
Copy link
Contributor

Hmm, actually, I think 1322 is wrong, and the real solution was 1379.

CC @clubby789, can you check if this PR still works for you?

1379 does indeed fix the specific case, but 1322 is about preventing the general case (of linker-specific flags being incorrectly reported as supported). The needs_linker idea sounds like a sensible fix to me

@madsmtm
Copy link
Collaborator

madsmtm commented Mar 1, 2025

But cc generally doesn't link, and thus we shouldn't be passing flags that affect the linker in the first place?

@dot-asm
Copy link
Contributor

dot-asm commented Apr 28, 2025

But cc generally doesn't link, and thus we shouldn't be passing flags that affect the linker in the first place?

Nor does it have a say in which linker will be called. Since the controversy seems to revolve around -fembed-bitcode, one should recognize that in order to utilize the embedded bitcode user has to explicitly instruct rustc to link the binary with suitable clang command, sufficiently recent version, and with necessary supplemental flags. And the point is that it's external to cc-rs. Also note that it's on the user to instruct both cc-rs and rustc to use clang, preferably the same version. With this in mind there is a case to be made that probing for -fembed-bitcode without -c flag adds no value(*) to the user. Such a probe won't help solving any related problems, probing with -c is more than sufficient.

(*) As a matter of fact binary code compiled with -fembed-bitcode alone is the same as one compiled without. It takes additional flags to utilize the bitcode even when you use clang by itself.

@dot-asm
Copy link
Contributor

dot-asm commented Apr 28, 2025

On a vaguely related note. cc-rs appears to parse RUSTFLAGS and map -Clto rustc option to clang's -flto=full. Note that the -Clto is required to be complemented with -Cembed-bitcode. Which is not mapped. However, if it was mapped, it wouldn't make difference, because -flto makes clang generate raw bitcode regardless. Since rustc's preferred object format is embedded bitcode, it makes more sense to map -Cembed-bitcode and let -Clto go unmapped. How is it related to the issue at hand? This is just an example of a potential problem that lack of -c in the probe won't help solving.

@NobodyXu
Copy link
Collaborator Author

Thanks for explanation, in that case cc @madsmtm we probably should merge it?

@NobodyXu
Copy link
Collaborator Author

Since rustc's preferred object format is embedded bitcode, it makes more sense to map -Cembed-bitcode and let -Clto go unmapped.

Well IIRC it doesn't matter unless cross lang LTO is turned on, otherwise rustc just invokes linker for the rust and external C crate separately?

@dot-asm
Copy link
Contributor

dot-asm commented Apr 29, 2025

otherwise rustc just invokes linker for the rust and external C crate separately?

I don't understand what "separately" means in the question. Rust links the final binary module by calling a linker, which doesn't care where its inputs come from, Rust or C. You can tell compilers to complement the object modules with LTO data, but if you don't tell rustc to use the linker that can make use of it, then LTO simply doesn't happen.

@NobodyXu
Copy link
Collaborator Author

if you don't tell rustc to use the linker that can make use of it, then LTO simply doesn't happen.

Yeah this was what I was trying to say, I was pretty sure most people don't enable cross lang LTO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_flag_supported returns false negatives
4 participants