ICE: stage2 builds of rustc itself see metadata serialized by a different rustc

[arora-aman]

Error happened while working on updating closure capture analysis in rustc which is part of rustc_typeck.

Code

https://github.com/rust-lang/rust/compare/master...sexxi-goose:use_places_new?expand=1#diff-a7b1a49c68b1a1ce388558eee4f3514aR267

The highlighted line is what changed between the build that broke and the build before it.

Meta

Version reported in build log

rustc 1.48.0-dev running on x86_64-unknown-linux-gnu

Error output

error: internal compiler error: compiler/rustc_middle/src/ty/query/on_disk_cache.rs:422:23: could not decode cached query result: read_option: expected 0 for None or 1 for Some

thread 'rustc' panicked at 'assertion failed: `(left == right)

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `245`,
 right: `0`', 

and similarly one or two more

Complete build log: http://csclub.uwaterloo.ca/~a52arora/ice/log_75e93202-df10-4ea2-bff9-167ff9b968c2
Build log with RUST_BACKTRACE=1: http://csclub.uwaterloo.ca/~a52arora/ice/log_b7ef389e-c9a9-495e-92c1-5f55468705ec

Build directory: http://csclub.uwaterloo.ca/~a52arora/ice/build.tar (Heads up, this is 29G tar, 39G extracted)

[RalfJung]

What was the exact build command that caused the ICE?

If this is with x.py, then I also see this issue every now and then. Locally built rustc all have the "dev" version string, which means it doesn't have to rebuild everything just because you do git commit, but it also means it cannot detect that the ABI changed, so it might try to load old files with the new rustc and of course that will fail. I am not sure if much can be done about this.

[RalfJung]

Some potential duplicates: #74581, #74258, #69639, #60228.

[arora-aman]

What was the exact build command that caused the ICE?

If this is with x.py, then I also see this issue every now and then. Locally built rustc all have the "dev" version string, which means it doesn't have to rebuild everything just because you do git commit, but it also means it cannot detect that the ABI changed, so it might try to load old files with the new rustc and of course that will fail. I am not sure if much can be done about this.

@RalfJung I do x.py build -j50 -i --stage 1.
This happened to me again last night: http://csclub.uwaterloo.ca/~a52arora/rust-builds/log_30d4a7ae-dc48-41c6-a2c3-39043eee22bc

The assertion is fired in the not the same line but the same file, I haven't gotten around looking at the query code to see what failed.

Only thing I worked on since the last x.py clean (maybe 10-15ish builds in, though I could be wrong) was change closure/generator pretty printing (in rustc_middle/src/ty/print/pretty), haven't rebased since.

Though I can see that rustc builds are themselves are more prone incr compile errors, also didn't think git commit had anything to do with the build process, otherthan maybe some version strings.

[RalfJung]

Cc @Mark-Simulacrum

didn't think git commit had anything to do with the build process, otherthan maybe some version strings.

I mentioned git commit because one way to avoid this problem might be to embed the git commit in the version string used for rebuild detection -- but that would be rather annoying in many cases.

[Mark-Simulacrum]

You can set ignore-git to false in config.toml and that'll embed git commit hashes. But I think that must only patch over bugs - it can't be necessary. I doubt it will fully fix this bug.

I suspect the problem is that Cargo doesn't delete incremental state on dependency changes - which means that rustc must be prepared, currently, to see incremental state serialized by a different rustc... ideally, handling that in a way that doesn't just throw everything out, but even just not panicking would probably be enough.

[jyn514]

See also #75849.

[jyn514]

The proper way to fix this is to wipe the build cache whenever the stage1 compiler changes. That will make builds a lot slower, but it's better than ICEs and having to wipe the cache anyway.

I guess we could add a --keep-stage1-cache flag if you're sure you haven't touched metadata - not sure how useful it would be in practice.

@RalfJung said on Zulip:

I think that's a reasonable default.
FWIW, I rarely ever build the stage 1 compiler, and when I do it's to work with external miri and I usually build it once (where I want the cache to be wiped since it's horribly out of date), and then use --keep-stage 0 on all subsequent builds. so with that workflow, wiping the cache on stage1 compiler changes would never make anything slower.

[jyn514]

Wait, I'm confused again. Why does the metadata matter other than the incremental cache? Cargo already invalidates the cache when stage1/bin/rustc is modified, which is why libstd gets rebuilt when you modify the compiler.

Is there anyone not using incremental = true that's been hitting this issue?

[jyn514]

You can set ignore-git to false in config.toml and that'll embed git commit hashes. But I think that must only patch over bugs - it can't be necessary. I doubt it will fully fix this bug.

The only time it won't fully fix the issue is when you're modifying rustc_metadata or similar and don't make a commit between builds. I'm ok with not fixing that case, because anyone working on metadata is much more likely to understand what went happened and how to work around it than random contributors stumbling into this for the first time.

[jyn514]

Is there anyone not using incremental = true that's been hitting this issue?

@BoxyUwU hit this issue this morning with incremental disabled, so it's relevant to all metadata loading, not just incremental.

[whtahy]

Thanks for pointing me to the right issue. Following #60228 (comment)

./x clean
git checkout a
./x build
git checkout b
./x build

Tried some commits from the past few days

a       -> b        ice/no ice
32f6e7a -> 1ee189c  no ice
1ee189c -> d6af602  no ice
32f6e7a -> d6af602  no ice
d6af602 -> 7037ff9  ice
7037ff9 -> dc73052  ice

Would it be useful to keep going with this approach?

Oh, and this is with incremental = true.

[jyn514]

That's helpful, thanks! No need to find more commits, having a reproducer at all is already very useful.

[RalfJung]

This fundamental issue doesn't just apply to bootstrap, right? If one uses a locally built toolchain to build some crates, and then updates the toolchain, the same ICEs can arise (because the dependency rlibs will now be in the wrong format).

Bootstrap is just where people run into it most often. But a solution inside bootstrap won't fix the fundamental issue. (E.g., it won't fix the Miri version of this problem, which got reported at rust-lang/miri#2847.)

[jyn514]

Yes, that's right, any two toolchains that were built from different sources but have the same version string will have this problem (this is why we set a custom description by default in the source tarballs, so they won't reuse metadata from the official toolchain). Removing ignore-git or defaulting it to false would work for miri though, or at least make it so that you have to try a lot harder to run into the bug (e.g. x install twice with different local changes that haven't been committed).

[jyn514]

A more "principled" fix would be to have something like a metadata version number embedded in the .rmeta/.rlib files that contributors are responsible for updating when the format changes. But I worry people will forget to update it, the same way people often forget to bump download-ci-llvm-stamp and that causes caching issues: #110474

[jyn514]

oh right, and @Mark-Simulacrum suggested adding incremental to dep-info: #86641 (comment)

but given that this bug is also showing up for metadata, not just incremental, I suspect that's not a complete fix: #76720 (comment)

[RalfJung]

Yeah, if we could somehow ensure that such a version number is always correct it would be a proper fix but manual bumping probably won't work. Unless we find a way to at least have CI fail when one forgets to bump...

[jyn514]

@saethlin reported seeing these crashes for x test, not just for tools and stage 1 rustc. I think he's running into this line being buggy:

rust/src/bootstrap/builder.rs

Lines 1558 to 1567 in 32e27cc

	// Clear the output directory if the real rustc we're using has changed;
	// Cargo cannot detect this as it thinks rustc is bootstrap/debug/rustc.
	//
	// Avoid doing this during dry run as that usually means the relevant
	// compiler is not yet linked/copied properly.
	//
	// Only clear out the directory if we're compiling std; otherwise, we
	// should let Cargo take care of things for us (via depdep info)
	if !self.config.dry_run() && mode == Mode::Std && cmd == "build" {
	self.clear_if_dirty(&out_dir, &self.rustc(compiler));

that shouldn't filter only to build, nearly all the commands read metadata and need the same treatment.

Not making a PR because I don't have a test case yet, but if anyone comes up with an MCVE that makes x test --stage 1 ui crash, I'd be interested to see if this diff fixes it:

diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs
index 1267c0be719..d64c4fa16c6 100644
--- a/src/bootstrap/builder.rs
+++ b/src/bootstrap/builder.rs
@@ -1563,7 +1563,7 @@ pub fn cargo(
         //
         // Only clear out the directory if we're compiling std; otherwise, we
         // should let Cargo take care of things for us (via depdep info)
-        if !self.config.dry_run() && mode == Mode::Std && cmd == "build" {
+        if !self.config.dry_run() && mode == Mode::Std {
             self.clear_if_dirty(&out_dir, &self.rustc(compiler));
         }

also, I wonder if we could fix the original problem with metadata/incremental in stage 1 by applying clear_if_dirty to rustc too:

diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs
index 1267c0be719..875bd09dc9e 100644
--- a/src/bootstrap/builder.rs
+++ b/src/bootstrap/builder.rs
@@ -1560,10 +1560,7 @@ pub fn cargo(
         //
         // Avoid doing this during dry run as that usually means the relevant
         // compiler is not yet linked/copied properly.
-        //
-        // Only clear out the directory if we're compiling std; otherwise, we
-        // should let Cargo take care of things for us (via depdep info)
-        if !self.config.dry_run() && mode == Mode::Std && cmd == "build" {
+        if !self.config.dry_run() {
             self.clear_if_dirty(&out_dir, &self.rustc(compiler));
         }

[jyn514]

@whtahy's reproducer unfortunately gives me a different ICE than the one in the linked issue; it's for stage0, not stage1. So I can't use it to test my diff.

Building tool rustdoc (stage0 -> stage1)
   Compiling rustdoc v0.0.0 (/home/jyn/src/rust/src/librustdoc)
thread 'rustc' panicked at 'no entry found for key', compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs:639:9
note: rustc 1.69.0-beta.1 (b955c8271 2023-03-06) running on x86_64-unknown-linux-gnu

[jyn514]

@BoxyUwU a while ago found that

git checkout 6874f4e3fc2a16be7c78e702d068bbc1daa90e16
x clean
x build
git checkout 999ac5
x build

reproduced the issue; if someone could run that both with and without my diff applied to see if it makes a difference that would be helpful; I'm out of time for today.