Simplify LLVM bitcode linker in bootstrap #142357
Draft
+69
−52
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
jieyouxu
reviewed
Jun 11, 2025
Comment on lines
2037
to
2039
| 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).
Comment on lines
+1318
to
+1319
| /// 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.
Comment on lines
+1238
to
+1241
| /// 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 |
|
Reminder, once the PR becomes ready for a review, use |
|
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. |
tgross35
added a commit
to tgross35/rust
that referenced
this pull request
Jun 16, 2025
…=jieyouxu Add initial version of snapshot tests to bootstrap When making any changes to bootstrap (steps), it is very difficult to realize how does it affect various common bootstrap commands, and if everything still works as we expect it to. We are far away from having actual end-to-end tests, but what we could at least do is have a way of testing what steps does bootstrap execute in dry run mode. Now, we already have something like this in `src/bootstrap/src/core/builder/tests.rs`, however that is quite limited, because it only checks executed steps for a specific impl of `Step` and it does not consider step order. Recently, when working on what I thought was one of the simplest possible step untanglings in bootstrap (rust-lang#142357), I ran into errors in tests that were quite hard to debug. Partly also because the current staging test diffs are multiline and use `Debug` output, so it's quite difficult for me to make sense of them. In this PR, I introduce `insta`, which allows writing snapshot tests in a very simple way. With it, I want to allow writing tests that will clearly show us what is going on during bootstrap execution, and then write golden tests for `build/check/test` stage `0/1/2` for compiler/std/tools etc., to make sure that we don't regress something, and also to help with [#t-infra/bootstrap > Proposal to cleanup stages and steps after the redesign](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Proposal.20to.20cleanup.20stages.20and.20steps.20after.20the.20redesign/with/523488806), to help avoid a situation where we would (again) have to make a flurry of staging changes because of unexpected consequences. In the snapshot tests, we currently render the build of rustc, std and LLVM. Currently I render the executed steps using downcasting, which is not super pretty, but it allows us to make the test rendering localized in one place, and it's IMO enough for now. I implemented only a single test using the new machinery. Maybe if you take a look at it, you will understand why 😆 Bootstrap currently does some peculiar things, such as running a stage 0 std step (even though stage 0 std no longer exists) and running the Rustc stage 0 -> 1 step twice, once with a single crates, once with all rustc crates. So I think that even with this single step, there will be a bunch of things to fix in the near future... The way we currently prepare the Config test fixtures is far from ideal, this is something I think `@Shourya742` could work on as a part of their GSoC project (remove as much command execution from Config construction as possible, actually run bootstrap on a temporary directory instead of running it on the rustc checkout, create a Builder-like API for creating the Config test fixtures). r? `@jieyouxu`
Kobzol
added a commit
to Kobzol/rust
that referenced
this pull request
Jun 16, 2025
…=jieyouxu Add initial version of snapshot tests to bootstrap When making any changes to bootstrap (steps), it is very difficult to realize how does it affect various common bootstrap commands, and if everything still works as we expect it to. We are far away from having actual end-to-end tests, but what we could at least do is have a way of testing what steps does bootstrap execute in dry run mode. Now, we already have something like this in `src/bootstrap/src/core/builder/tests.rs`, however that is quite limited, because it only checks executed steps for a specific impl of `Step` and it does not consider step order. Recently, when working on what I thought was one of the simplest possible step untanglings in bootstrap (rust-lang#142357), I ran into errors in tests that were quite hard to debug. Partly also because the current staging test diffs are multiline and use `Debug` output, so it's quite difficult for me to make sense of them. In this PR, I introduce `insta`, which allows writing snapshot tests in a very simple way. With it, I want to allow writing tests that will clearly show us what is going on during bootstrap execution, and then write golden tests for `build/check/test` stage `0/1/2` for compiler/std/tools etc., to make sure that we don't regress something, and also to help with [#t-infra/bootstrap > Proposal to cleanup stages and steps after the redesign](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Proposal.20to.20cleanup.20stages.20and.20steps.20after.20the.20redesign/with/523488806), to help avoid a situation where we would (again) have to make a flurry of staging changes because of unexpected consequences. In the snapshot tests, we currently render the build of rustc, std and LLVM. Currently I render the executed steps using downcasting, which is not super pretty, but it allows us to make the test rendering localized in one place, and it's IMO enough for now. I implemented only a single test using the new machinery. Maybe if you take a look at it, you will understand why 😆 Bootstrap currently does some peculiar things, such as running a stage 0 std step (even though stage 0 std no longer exists) and running the Rustc stage 0 -> 1 step twice, once with a single crates, once with all rustc crates. So I think that even with this single step, there will be a bunch of things to fix in the near future... The way we currently prepare the Config test fixtures is far from ideal, this is something I think ``@Shourya742`` could work on as a part of their GSoC project (remove as much command execution from Config construction as possible, actually run bootstrap on a temporary directory instead of running it on the rustc checkout, create a Builder-like API for creating the Config test fixtures). r? ``@jieyouxu``
rust-timer
added a commit
that referenced
this pull request
Jun 16, 2025
Rollup merge of #142431 - Kobzol:bootstrap-snapshot-tests, r=jieyouxu Add initial version of snapshot tests to bootstrap When making any changes to bootstrap (steps), it is very difficult to realize how does it affect various common bootstrap commands, and if everything still works as we expect it to. We are far away from having actual end-to-end tests, but what we could at least do is have a way of testing what steps does bootstrap execute in dry run mode. Now, we already have something like this in `src/bootstrap/src/core/builder/tests.rs`, however that is quite limited, because it only checks executed steps for a specific impl of `Step` and it does not consider step order. Recently, when working on what I thought was one of the simplest possible step untanglings in bootstrap (#142357), I ran into errors in tests that were quite hard to debug. Partly also because the current staging test diffs are multiline and use `Debug` output, so it's quite difficult for me to make sense of them. In this PR, I introduce `insta`, which allows writing snapshot tests in a very simple way. With it, I want to allow writing tests that will clearly show us what is going on during bootstrap execution, and then write golden tests for `build/check/test` stage `0/1/2` for compiler/std/tools etc., to make sure that we don't regress something, and also to help with [#t-infra/bootstrap > Proposal to cleanup stages and steps after the redesign](https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Proposal.20to.20cleanup.20stages.20and.20steps.20after.20the.20redesign/with/523488806), to help avoid a situation where we would (again) have to make a flurry of staging changes because of unexpected consequences. In the snapshot tests, we currently render the build of rustc, std and LLVM. Currently I render the executed steps using downcasting, which is not super pretty, but it allows us to make the test rendering localized in one place, and it's IMO enough for now. I implemented only a single test using the new machinery. Maybe if you take a look at it, you will understand why 😆 Bootstrap currently does some peculiar things, such as running a stage 0 std step (even though stage 0 std no longer exists) and running the Rustc stage 0 -> 1 step twice, once with a single crates, once with all rustc crates. So I think that even with this single step, there will be a bunch of things to fix in the near future... The way we currently prepare the Config test fixtures is far from ideal, this is something I think ``@Shourya742`` could work on as a part of their GSoC project (remove as much command execution from Config construction as possible, actually run bootstrap on a temporary directory instead of running it on the rustc checkout, create a Builder-like API for creating the Config test fixtures). r? ``@jieyouxu``
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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