Clear incremental cache when stage1 compiler is rebuilt

[jyn514]

Previously, this would cause ICEs because the stage1 compiler would see incremental state from a different compiler with a different ABI.

As a performance optimization, it might be possible to turn incremental off by default for stage1 builds, which would speed them up in the common case where the compiler is rebuilt. But that seems complicated and I don't have time to work on it at the moment.

Closes #76720.

r? @Mark-Simulacrum

cc @rust-lang/compiler @rust-lang/compiler-contributors: this should allow you to see incremental = true unconditionally in config.toml, modulo any remaining bugs in incremental.

[RalfJung]

this should allow you to see incremental = true unconditionally

FWIW I already did that years ago and it (mostly) works fine.^^
I fairly regularly find myself using ./x.py build --keep-stage 0 compiler/rustc -- when I need a stage 2 rustc and keep doing edits. I think incremental stage 1 is very useful in that case since stage 0 does not change, so your premise "it's a new compiler each time" does not hold. Maybe such a use of --keep-stage should overwrite your "force incremental off"? OTOH, the first build of course does not use that flag so we'd turn on incremental on a rebuild which does not make that much sense...

[jyn514]

@RalfJung how's this - x.py has the info itself to know if it did a rebuild or not. If it rebuilds the stage 1 compiler, it can force off incremental when building with it; otherwise, incremental is treated like normal.

[RalfJung]

Doesn't that still have the problem that the first build of stage 1 will be without incremental, and then rebuilds will be with incremental but there's no incremental cache (and I have no idea what that does)?

I think there's two different problems here, and they probably have different solutions:

• Problem: We get ICEs and whatnot on stage 1 builds because it uses an old incremental cache.
  Solution: make x.py clean the incremental cache when it builds the compiler that created that cache (so every time stage 0 compiler artifacts changed and are "propagated" to prepare the sysroot for stage 1 builds, delete the stage 1 incremental cache).
• Problem: Incremental builds are significantly slower than non-incremental builds, and for most workflows (with the exception of things like build --keep-stage 0 compiler/rustc), we get no benefit from the stage 1 incremental cache.
  Solution: Let the user configure something like incremental = 0 to indicate that only stage 0 should use incremental, higher stages should not. Passing -i should overwrite that so that I can use build --keep-stage 0 compiler/rustc -i for cases where I do want an incremental cache in my stage 1 build.

Then people can set incremental = 0 or incremental = true and both will behave correctly (but they won't behave the same, the latter does use incremental builds on all stages).

Maybe = 0 is a bad idea as false and 0 would then have different meaning which could surprise some people... but we can bikeshed the exact way to configure this.

[jyn514]

@RalfJung I like the idea of wiping the cache instead of forcing incremental to off, that seems more correct.

I don't see why incremental = "stage0" is necessary in that case. Shouldn't it work automatically for all stages now that the cache isn't invalid? It might be slightly slower to do a full build without incremental, but x.py can't know in advance if you'll want to rebuild later, and it fixes the main issue of ICEs.

[RalfJung]

Last time I tried (which is to say more than a year ago), the initial incremental build was significantly slower than a non-incremental build -- certainly >10%. So that seems like time well saved when you usually can't make use of that incremental cache anyway.

[jyn514]

Hmm, ok. I'm not necessarily opposed to that but I don't think the change should happen here. The UI I imagine is we add new stage0 and stage1 options to incremental, which seems less confusing than 0 and 1.

[RalfJung]

If the point of this PR is to fix the ICE (the first item on my list), then I agree that this is orthogonal.

[Mark-Simulacrum]

Disabling incremental seems like a patch that would work, but it doesn't seem like the right approach. Rustbuild should already be clearing the output directory on changes to the rustc binary (and that should include incremental state), and if we're not we would expect to see other bugs crop up (e.g., reuse unrelated to incremental is also wrong).

I think the actual bug is that

rust/src/bootstrap/builder.rs

Lines 1080 to 1081 in 17ea490

	if !self.config.dry_run && mode == Mode::Std && cmd == "build" {
	self.clear_if_dirty(&out_dir, &self.rustc(compiler));

only clears out the out dir for std, relying on Cargo to do the clearing of rustc due to -Zbinary-dep-dep being passed (and all rustc crates would depend on the std/core sysroot).

It's pretty likely that there's not a dependency edge in Cargo from the incremental state to std/core (for "obvious" reasons; this is desirable in the general case as you want to avoid clearing out incremental state when rebuilding due to changes in dependencies as you might be able to save work in theory). So what we probably need to do in rustbuild is to clear out the incremental directory ourselves on changes to rustc, via a line similar to the one linked above.

I think this patch as-is would likely help with the majority of the ICEs as it'll disable incremental in stage 1, but it won't help when we bump the bootstrap compiler with the incremental state in stage 0, for example. Probably what we want is:

• Fix rustbuild to clear out incremental directories in all stages when rustc binary being run changes
• Adjust rustbuild to default to not running with incremental enabled in all but the first stage (we can say just zero for now but in theory it may be advantageous to use keep-stage to guide this). This is basically this patch.

[jyn514]

I think this patch as-is would likely help with the majority of the ICEs as it'll disable incremental in stage 1, but it won't help when we bump the bootstrap compiler with the incremental state in stage 0, for example.

Bumping the bootstrap compiler will give us different hashes, because even with git = false, the version string includes the version number:

rustc 1.54.0-dev

Fix rustbuild to clear out incremental directories in all stages when rustc binary being run changes

👍

[Mark-Simulacrum]

Hm, it's certainly true that we've bumped bootstrap compilers within a cycle before (where the version number wouldn't change; beta releases are separately numbered IIRC with a -beta.X). I'm not sure if that's enough to force separate caching; it seems like a good idea to add a test to cargo ensuring it's enough to change -vV output without changing the binary being run (when rustc is replaced by a wrapper).

[jyn514]

Oh - I don't think git = false should matter here at all, beta is built by CI with git = true:

> ./build/x86_64-unknown-linux-gnu/stage0/bin/rustc --version
rustc 1.53.0-beta.3 (82b862164 2021-05-22)

I'm not sure if that's enough to force separate caching; it seems like a good idea to add a test to cargo ensuring it's enough to change -vV output without changing the binary being run (when rustc is replaced by a wrapper).

Sure, I can test this, but why -vV specifically?

[jyn514]

Changing the version while keeping the name of the wrapper the same does indeed rebuild.

> cat rustc.sh 
exec rustc +beta "$@"
> RUSTC=./rustc.sh cargo build
   Compiling hello v0.1.0 (/home/joshua/hello)
    Finished dev [unoptimized + debuginfo] target(s) in 0.20s
> RUSTC=./rustc.sh cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
> cat rustc.sh 
exec rustc +stable "$@"
> RUSTC=./rustc.sh cargo build
   Compiling hello v0.1.0 (/home/joshua/hello)
    Finished dev [unoptimized + debuginfo] target(s) in 0.25s

[jyn514]

Probably what we want is:

1. Fix rustbuild to clear out incremental directories in all stages when rustc binary being run changes
2. Adjust rustbuild to default to not running with incremental enabled in all but the first stage (we can say just zero for now but in theory it may be advantageous to use keep-stage to guide this). This is basically this patch.

I changed this to implement 1, which should fix the ICEs (and works well with --keep-stage 1). I don't expect to have time to make 2. work in the near future, and AFAIK it's only a performance optimization anyway.

[RalfJung]

Sounds reasonable to me. Could you update the PR title and description?

[Mark-Simulacrum]

Please update this comment, too.

I am a little wary of making this change -- IIRC, and based on description of the commit that added this, it was quite intentional to limit it so harshly in #67760... but presuming we don't get a scattering of bug reports, it seems like we could do this.

I think one thing we can do, in theory, is not do this if we're not using incremental, but that seems worse to me right now. If we start getting bug reports due to extra rebuilds that users don't expect, then we can revisit this decision and figure out a more targeted fix. AFAICT, it should only be necessary to clear out the incremental state for rustc, while this targets all output directories (in build/check invocations). My guess is that will lead to some problems, but we'll see.

[jyn514]

Please update this comment, too.

Sure, done.

I think one thing we can do, in theory, is not do this if we're not using incremental, but that seems worse to me right now.

Hmm, that makes sense to me. Do you want me to do that now? I think the only case this can cause bugs is when using incremental; or I guess if cargo assumes that output artifacts are the same. Not sure when that could happen though.

[Mark-Simulacrum]

I was going to suggest a re-write of the comment in this code, but I actually now think that this is possibly the wrong fix. Currently, it looks like rustc does not emit incremental files as outputs into the depinfo it gives Cargo. I think that's actually the reason compiler builds break here -- Cargo isn't deleting those output files prior to running rustc, because it doesn't know about them. If we fix the code here to include incremental output files in the output paths list, I think that should fix this without needing changes to bootstrap, which seems preferable.

I'm not sure if this suggestion would cause other problems, but it seems loosely like a better idea than just deleting the whole compiler/ directory. This would, I believe, also avoid rebuilding the whole compiler on changes to libtest (with keep-stage to limit to a single stage build) since there isn't a dependency edge between libtest and most compiler crates. That seems like a good feature to keep.

We still need to delete std as there's not a dependency edge from std crates to the real compiler file known to Cargo, but that is already the behavior on master.

[jyn514]

FYI: I am not planning to work on this until I have a way to replicate the build failures, since I don't trust my understanding of incremental enough to just "eyeball" it like I was doing before. If someone can come up with a series of git + x.py commands that reproduces a stage1 ICE, that would be helpful.

[jyn514]

I won't have time to work on this in the near future.