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

bootstrap: do not invoke global lld binary #116197

Closed
wants to merge 1 commit into from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Sep 27, 2023

#102101 has introduced a check for passing some thread flags to lld. However, it was invoking a (globally available) binary called lld, which I'm pretty sure we never want to do in bootstrap. If you enable use-lld and don't have a lld binary available, this will crash the build.

This PR fixes that by invoking the lld coming from the snapshot compiler instead.

@rustbot
Copy link
Collaborator

rustbot commented Sep 27, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 27, 2023
@petrochenkov petrochenkov self-assigned this Sep 27, 2023
@petrochenkov
Copy link
Contributor

the lld coming from the snapshot compiler instead.

I'm sufficiently sure that it's not the lld that is used through the build, or at least not always used.
#102101 was actually tested, in the sense that rustc build didn't work before it, and worked after it.
Let me check, there were some changes in this area.

@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 28, 2023

I went through bootstrap and I'm relatively sure that just setting use-lld will use LLD from the snapshot compiler, and not external lld. For that you would also have to set the linker flag of the given target. Here, initial_lld is used, and that is taken from the snapshot compiler here.

@petrochenkov
Copy link
Contributor

petrochenkov commented Sep 28, 2023

So yeah, global lld is indeed used - #116182 (comment).
For the shipped lld we don't actually need this check, it's always new enough to support --threads.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 28, 2023

Hmm, this complicates things. I don't really like that bootstrap somewhat arbitrarily uses an externally provided lld, especially when it claims this:

# Indicates whether LLD will be used to link Rust crates during bootstrap on
# supported platforms. The LLD from the bootstrap distribution will be used
# and not the LLD compiled during the bootstrap.

and when it also uses (or at least tries to use) the snapshot compiler's lld in some places. This is a mess.

Assuming that there even is a practical way of passing absolute linker paths to all the tools that bootstrap configures with -fuse-ld=lld, would you accept changing the logic so that we always try to use the snapshot compiler's lld? And perhaps in the future, as a generalization, allow users to select which lld should be used (snapshot, locally built, external), while keeping lld's usage consistent across all stages of compilation.

@Kobzol Kobzol marked this pull request as draft September 28, 2023 08:35
@petrochenkov
Copy link
Contributor

Assuming that there even is a practical way of passing absolute linker paths to all the tools that bootstrap configures with -fuse-ld=lld, would you accept changing the logic so that we always try to use the snapshot compiler's lld?

Sure, as I said in #116182 (comment) that would even be more convenient.

@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 28, 2023

So I looked a bit more into this and it seems to me that bootstrap is reimplementing a lot of what rustc already does, in regard to choosing the linker flavor and "self-containedness" of the linker.

I wonder, what if we could remove some/most of this stuff from bootstrap, and just directly use the -C linker-flavor, -C self-contained and -C linker flags from rustc? Instead of directly setting -Clink-arg=-fuse-ld and similar messy things? I know that some of it is still unstable, but I guess that's not really a showstopper for bootstrap. Allowing people to choose linker-flavor and "self-containedness" in config.toml, and then using these flags directly in boostrap could hopefully simplify things.

One thing I'm not sure about though is why does bootstrap combine the usage of -Clink-arg and CARGO_TARGET_{target}_LINKER (and also RUSTC_HOST_LINKER, which is forwarded in the rustc shim). I assume that it's because of the difference between linker flags for the host and the target. But hopefully this should be orthogonal to using the -C linker-flavor/link-self-contained flags.

@petrochenkov
Copy link
Contributor

So I looked a bit more into this and it seems to me that bootstrap is reimplementing a lot of what rustc already does, in regard to choosing the linker flavor and "self-containedness" of the linker.

That's not surprising, the compiler logic was only implemented recently, and the bootstrap logic is much older.
It would be good to try dogfooding what the compiler has now in bootstrap, maybe just as an implementation detail for a start.

@petrochenkov
Copy link
Contributor

One thing I'm not sure about though is why does bootstrap combine the usage of -Clink-arg and CARGO_TARGET_{target}_LINKER (and also RUSTC_HOST_LINKER, which is forwarded in the rustc shim). I assume that it's because of the difference between linker flags for the host and the target. But hopefully this should be orthogonal to using the -C linker-flavor/link-self-contained flags.

Not sure, what is the alternative?
We probably could pass raw -Clinker, but CARGO_TARGET_{target}_LINKER seems to be the standard method provided by cargo.
CARGO_TARGET_{target}_LINKER is the linker (typically a C/C++ compiler), and -Clink-arg is used for passing additional arguments for that linker.

@Kobzol
Copy link
Contributor Author

Kobzol commented Sep 29, 2023

Closing in favour of #116278.

@Kobzol Kobzol closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants