Simplify LLVM bitcode linker in bootstrap #142357
Conversation
It wasn't used anywhere.
That step should be responsible for building the tool, not performing side-effects. Also, only copy the tool to the `self-contained` directory, not to the `rustlib/<target>/bin` directory.
This tool can be built with any compiler that can produce code for the host target of the compiler for which the linker should be installed. This change saves one rebuild of the tool in stage 2 build.
rustbot
added
S-waiting-on-review
| let bindir_self_contained = builder | ||
| .sysroot(compiler) | ||
| .join(format!("lib/rustlib/{}/bin/self-contained", compiler.host)); |
There was a problem hiding this comment.
Suggestion (follow-up, not for this PR): maybe pull out a method to do the "path to sysroot target self-contained" logic.
There was a problem hiding this comment.
I actually had a commit for that, but it's super tricky (lol). Because:
- There are like 5 places in bootstrap that access the self-contained directory. Some of them need a relative path, some of them absolute, some of them work with a compiler, some don't, it's a mess.
- The functions that we have in
Builderfor getting similar paths (invoked through theLibdirstep) actually delete the directory before returning it to you 🤦 I tried to use it here and it broke everything 😂 I need to clean all of this up eventually, and move the directory clearing to a single step, so that you can get a path to something without bootstrap falling over, lol.
There was a problem hiding this comment.
Yeah, this is uhhh tricky. I'll open a issue about this follow-up (and that it's tricky).
There was a problem hiding this comment.
#142361, not in scope for this PR (keeping this unresolved to make it easier to find).
| /// Return the lowest stage compiler that can compile code for the given `target`. | ||
| pub fn compiler_for_target(&self, target: TargetSelection) -> Compiler { |
There was a problem hiding this comment.
Discussion: maybe tool_compiler_for_target? I ask because this should also say something like "it must not be used to build stuff that depends on staged rustc/std", right?
There was a problem hiding this comment.
I don't think there's any requirement like this. It's really just "give me a compiler that can build code for target T". If stage0 could cross-compile for any target, we could use it for this always.
We actually have a bunch of situations like this in bootstrap already, but it was always done sort of implicitly. Here I wanted to make it explicit.
In other words, I think that this could be useful also for other things than just tools. But right now it's only for a single tool, ofc, so happy to rename if you want.
There was a problem hiding this comment.
Hm right. The reason I asked is that
this could be useful also for other things than just tools
is probably true, just that the "other things" must not use this method if they do "depend on staged compiler/std" (be it through rustc_private or directly or somehow).
For now, I think it's safer to name this as sth like tool_compiler_for_target to make it clear that you should not use it for sth beyond its purpose (because we also have Builder::{compiler,compiler_for}), and then consider its naming if we do use it for not-just-tools, but it's not really a blocking concern.
| /// Check that during a non-cross-compiling stage 2 build, we only compile rustc host tools | ||
| /// (such as llvm-bitcode-linker) only once. | ||
| #[test] | ||
| fn llvm_bitcode_linker_compile_once() { |
There was a problem hiding this comment.
Question: should we have another test that does check the cross-compiling case, that we do build a stage 1 (rustc, std) pair that can produce artifacts for target, and that llvm-bitcode-linker is produced by that pair for the target?
There was a problem hiding this comment.
Sure, happy to add it. Although from my local experiments, I'm almost sure that bootstrap is actually broken for these situations somehow. Like, ./x build --host i686-unknown-linux-gnu, which is like the simplest cross-compilation scenario on x64 Linux, fails for me on master.
|
@rustbot author |
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #142392) made this pull request unmergeable. Please resolve the merge conflicts. |
We have several linker tools in bootstrap that act as "host tools", i.e. they will be executed by rustc on the host machine where it runs. Funnily, we use three different modes for compiling them:
ToolRustc(LlvmBitcodeLinker)ToolStd(LldWrapper)ToolBootstrap(WasmComponentLd)Clearly, the existing bootstrap modes can't accurately describe how should these tools be built (more on this here). This also means that right now, when we do a stage 2 build, this tool is unnecessarily built twice, wasting CI time.
This PR tries to simplify the whole setup for
LlvmBitcodeLinker. Notably, I removed the side effect of modifying the toolchain of a compiler from the LLVM bitcode linker step (I don't like these side effects and I think that we should localize them to the fewest possible amount of places in bootstrap - this will take a lot of work), and changed it so that it is built only once per target.The tool is now also copied only into the
rustlib/<target>/bin/self-containeddirectory, before it was also copied torustlib/<target>/bin. However, I tested withrustc +stage1 src/main.rs -Zunstable-options -Clinker-flavor=llbc --target nvptx64-nvidia-cudathat rustc can get the binary from both places just fine, regardless of the value of-Clink-self-contained=[+-]linker. And the dist component of this tool only puts the binary into theself-containeddirectory, so it clearly works for people using the tool through rustup.As a follow-up, I want to simplify LldWrapper and WasmComponentLd in a similar ways, and then introduce the new "host tool" mode for them.
r? @jieyouxu