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: llvm-bitcode-linker adds unnecessary recompilation #122491

Closed
Kobzol opened this issue Mar 14, 2024 · 8 comments · Fixed by #122558
Closed

Bootstrap: llvm-bitcode-linker adds unnecessary recompilation #122491

Kobzol opened this issue Mar 14, 2024 · 8 comments · Fixed by #122558
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@Kobzol
Copy link
Contributor

Kobzol commented Mar 14, 2024

The llvm-bitcode-linker, which is enabled by default in the compiler config, is recompiled everytime I try to run tests:

$ python3 x.py test tests/run-make/hir-tree/
Creating a sysroot for stage1 compiler (use `rustup toolchain link 'name' build/host/stage1`)
Building tool llvm-bitcode-linker (stage0 -> stage1, x86_64-unknown-linux-gnu)
   Compiling once_cell v1.19.0
   Compiling cfg-if v1.0.0
...
Building tool rustdoc (stage0 -> stage1, x86_64-unknown-linux-gnu)
   Compiling cfg-if v1.0.0
   Compiling once_cell v1.19.0
...
$ python3 x.py test tests/run-make/hir-tree/
Creating a sysroot for stage1 compiler (use `rustup toolchain link 'name' build/host/stage1`)
Building tool llvm-bitcode-linker (stage0 -> stage1, x86_64-unknown-linux-gnu)
   Compiling once_cell v1.19.0
   Compiling cfg-if v1.0.0
...
Building tool rustdoc (stage0 -> stage1, x86_64-unknown-linux-gnu)
   Compiling cfg-if v1.0.0
   Compiling once_cell v1.19.0
...

The recompilation also causes further recompilation of rustdoc (if I disable the bitcode linker, rustdoc is not recompiled).

@Kobzol Kobzol added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. labels Mar 14, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 14, 2024
@onur-ozkan onur-ozkan removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 14, 2024
@workingjubilee
Copy link
Member

cc @kjetilkjeka

@kjetilkjeka
Copy link
Contributor

Oh, that's not good. Thanks for reporting. I will look at it tomorrow.

@kjetilkjeka
Copy link
Contributor

I have tracked it down to this ensure. If I comment out that code it wont trigger the recompilation.

if builder.config.llvm_bitcode_linker_enabled {
let src_path = builder.ensure(crate::core::build_steps::tool::LlvmBitcodeLinker {
compiler: build_compiler,
target: target_compiler.host,
extra_features: vec![],
});
let tool_exe = exe("llvm-bitcode-linker", target_compiler.host);
builder.copy(&src_path, &libdir_bin.join(&tool_exe));
}

From the documentation of ensure it say

This will cache the step, so it is safe (and good!) to call this as often as needed to ensure that all dependencies are built.

Also the definition of LlvmBitcodeLinker is done with the tool_extended macro. That makes me think that it's related to something like the build_compiler invalidating the caching. I'm currently exploring that a bit further.

@kjetilkjeka
Copy link
Contributor

The stuff above seems to work fine as building the llvm-bitcode-linker (without rustdoc) works fine.

The problem seems to be that rustdoc and llvm-bitcode-linker breaks the cache for each other. I suspect that means something is off in feature resolution between those modules.

@kjetilkjeka
Copy link
Contributor

I now know what the problem is. I will need some help to figure out the correct way to solve it.

The problem

impl Step for Rustdoc uses a very specifc cargo invocation which manually sets rustflags if a parallell compilation can be used.

if builder.config.rustc_parallel {
cargo.rustflag("--cfg=parallel_compiler");
}

This causes common dependencies between the LLVM bitcode linker and Rustdoc to be rebuilt when one is called after the other. Since Rustdoc builds the bitcode linker (presumably through the Assemble step) this will always happen when Rustdoc is built.

The questions I need resolved to fix it

Is it correct/ethical of rustdoc to set rustflags like this? Or should rustflags be handled more centrally so every cargo invocation will end up with the same set of rustflags and this cache invalidation cannot occur?

If it is important for rustdoc to use its own set of rustflags, should it then use a different build directory to other tools/steps using cargo?

@kjetilkjeka
Copy link
Contributor

I see that @m-ou-se added this in c7f443a6e3320b4cb70da5af08810f1dd60e146c. Maybe she knows more about this?

@onur-ozkan
Copy link
Member

Is it correct/ethical of rustdoc to set rustflags like this? Or should rustflags be handled more centrally so every cargo invocation will end up with the same set of rustflags and this cache invalidation cannot occur?

We can't have same set of rustflags for every Step, in most cases we want the opposite; setting step specific rustflags. What about adding --cfg=parallel_compiler to llvm-bitcode-linker tool?

@kjetilkjeka
Copy link
Contributor

kjetilkjeka commented Mar 15, 2024

That seems to work well in the scenario described above and in other scenarios I tested for. I think I just imagined that this was more frequently occurring problem if all tools share build folder and are able to set rustflags separately and adding this to llvm-bitcode-linker would cause a conflict with something else (which it doesn't seem to do).

Anyway, as this at least is fixing the apparent issue I'm creating a MR with this solution now

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 17, 2024
…_trigger_rebuild, r=onur-ozkan

LLVM bitcode linker: use --cfg=parallel_compiler to avoid invalidating the build cache of rustdoc

fixes rust-lang#122491

`@rustbot` ready
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 18, 2024
…rigger_rebuild, r=onur-ozkan

LLVM bitcode linker: use --cfg=parallel_compiler to avoid invalidating the build cache of rustdoc

fixes rust-lang#122491

`@rustbot` ready
@bors bors closed this as completed in 13abc0a Mar 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 16, 2024
…t, r=onur-ozkan

Disable `llvm-bitcode-linker` in the default bootstrap profiles

I don't think that we really need to enable `llvm-bitcode-linker` in the default bootstrap profiles, since it seems that it is only useful for running `nvptx` tests. It should be enabled on CI, which it is, and that should be enough. People can enable it easily locally, if they want.

The linker causes occasionally some rebuild issues (rust-lang#122491, rust-lang#126464), but more importantly it is just needless work to build it locally.

I kept it enabled for `dist`, because it is distributed as a `rustup` component (for some reason it's not included in `extended`? not sure).

Fixes: rust-lang#126464
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jun 17, 2024
…-ozkan

Disable `llvm-bitcode-linker` in the default bootstrap profiles

I don't think that we really need to enable `llvm-bitcode-linker` in the default bootstrap profiles, since it seems that it is only useful for running `nvptx` tests. It should be enabled on CI, which it is, and that should be enough. People can enable it easily locally, if they want.

The linker causes occasionally some rebuild issues (rust-lang/rust#122491, rust-lang/rust#126464), but more importantly it is just needless work to build it locally.

I kept it enabled for `dist`, because it is distributed as a `rustup` component (for some reason it's not included in `extended`? not sure).

Fixes: rust-lang/rust#126464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
5 participants