Stream: t-compiler/wg-incr-comp

Topic: Enabling `-Z incremental-verify-ich`


view this post on Zulip Aaron Hill (Mar 13 2021 at 02:54):

As a result of the miscompilation induced by incremental compilation in https://github.com/rust-lang/rust/issues/82920, I opened PR https://github.com/rust-lang/rust/pull/83007 to turn on -Z incremental-verify-ch. Having this option on by default would have turned the miscompilation into an ICE.

Unfortunately, the performance impact is awful (https://perf.rust-lang.org/compare.html?start=61365c06250e2ba6e0a578ae990f055ac5339107&end=213521923e2df5e849eee21b917477d429078204) - regressions were anywhere from 1% to 320% (though the 320% was for a stress test benchmark).

view this post on Zulip Aaron Hill (Mar 13 2021 at 02:57):

it looks like incremental_verify_ich gets called for both re-computed results and re-computed results. I think we could safely skip recomputing the hashes for results loaded from disk - if the hash were to change, it would be caused by a completely different kind of issue (the HashStable implementation would have to be non-deterministic, I think)

view this post on Zulip Aaron Hill (Mar 13 2021 at 02:57):

I'll see if that makes the performance any better.

view this post on Zulip Aaron Hill (Mar 13 2021 at 03:01):

I'm not 100% certain, but I think a non-deterministic HashStable impl would only lead to spuriouly marking a query as red (assuming that it didn't produce hash collisions), and could not lead to the unsoundness that results from the query result actually changing

view this post on Zulip simulacrum (Mar 13 2021 at 03:04):

Commented on the PR but I'll say it here too -- I think performance hit is acceptable for mitigating this soundness hole.

view this post on Zulip simulacrum (Mar 13 2021 at 03:07):

Looking at wall-times and disabling full and incr-full (both of which are not reusing results so don't really see impacts) you get at most a 50% hit on the stress test and only 5-15% wall time hits on most benchmarks. That's bad, but it's not horrible. IMO, it makes sense to land it on nightly at least - I'd rather ship a slower but correct compiler. Ultimately the 15% hits are also in the faster benchmarks; unsurprisingly opt and other builds which end up rerunning e.g. LLVM this is not as visible.

view this post on Zulip cjgillot (Mar 13 2021 at 12:53):

Aaron Hill said:

it looks like incremental_verify_ich gets called for both re-computed results and re-computed results. I think we could safely skip recomputing the hashes for results loaded from disk - if the hash were to change, it would be caused by a completely different kind of issue (the HashStable implementation would have to be non-deterministic, I think)

I would have said exactly the opposite. The re-computed queries are already hashed to determine the red/green color of the dep-node. Only the results loaded from disk are implicitly assumed to be green.

view this post on Zulip Aaron Hill (Mar 13 2021 at 16:44):

The re-computed queries are already hashed to determine the red/green color of the dep-node.

That's only the case when at least one of the dependencies could not be marked green. In that case, try_mark_previous_green returns None, so we will never call load_from_disk_and_cache_in_memory

Instead, we will end up calling force_query_with_job, which does hash the result to determine the red/green color

view this post on Zulip Aaron Hill (Mar 13 2021 at 16:45):

My change only affects the case where all the inputs are green - the query system currentl assumes that the query output will be unchanged, without actually checking it

view this post on Zulip Aaron Hill (Mar 13 2021 at 16:51):

More generally, we can end up with miscompilations if the query system assumes that a query result hasn't changed when it actually has. I think this is the only case where that can happen - all other cases are either:

  1. Running a query for the first time, so nothing could already be expecting a different result
  2. We're loading something from disk - unless the incremental cache got corrupted, then we're getting back whatever we serialized (assuming no bugs in the serialization/deserialization impl)
  3. We're re-running a query because some of its inputs are not green. We already re-hash the result as part of the red/green algorithm, so the worst that can happen is that we spuriously throw mark a query as red, leading to unencessary work

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:02):

We're loading something from disk - unless the incremental cache got corrupted, then we're getting back whatever we serialized (assuming no bugs in the serialization/deserialization impl)

shouldn't the compiler handle these sorts of errors, though? having unsoundness because of bugs in deserialize seems not great

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:03):

and theoretically someone could mess with with the incremental cache; the threat model is probably not super realistic, but it seems better to ICE than to generate broken code

view this post on Zulip cjgillot (Mar 13 2021 at 17:05):

Actually, incremental-verify-ich is already restricted to loading from the on-disk cache.

view this post on Zulip cjgillot (Mar 13 2021 at 17:07):

The incremental serialization and deserialization are not straightforward at all. For instance, the path for a DefId is DefId -> DefPathHash -> DefId, so the input and output DefIds are not guaranteed to be the same. This is the source of the issues with sorting by DefId.

view this post on Zulip cjgillot (Mar 13 2021 at 17:09):

Computing a query always computes its hash, inside DepGraph::with_task_impl (except for no-hash queries).

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:09):

and theoretically someone could mess with with the incremental cache; the threat model is probably not super realistic, but it seems better to ICE than to generate broken code

I think a better way of detecting that kind of issue would be to store hash the entire incremental file, store that somewhere, and verify it when we load it

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:09):

(not sure if we already do something like that)

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:09):

someone could mess with that hash too though

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:10):

if you don't trust part of the file system, you can't trust any of it

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:12):

I was thinking of guarding against corruption

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:12):

not deliberate modification

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:13):

hmm, maybe a better example is that bootstrap reuses the same cache between different nightly compilers

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:13):

it would be cool if the compiler could catch that somehow - I don't know if that's what verify-ich is actually for though

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:14):

Computing a query always computes its hash, inside DepGraph::with_task_impl (except for no-hash queries).

Maybe you mean something specific by 'compute', but the underlying query provider can get run without hashing the result. The query provider gets directly invoked via compute inside load_from_disk_and_cache_in_memory, which is the case my PR addresses

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:14):

https://github.com/rust-lang/rust/blob/32dce353dec5f9069c941d4cd0c059ecc67b7c2b/compiler/rustc_query_impl/src/plumbing.rs#L391-L401

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:16):

hmm, maybe a better example is that bootstrap reuses the same cache between different nightly compilers

Do you mean between different beta compilers, or re-using the stage0 incr cache with the stage1 compiler?

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:17):

re-using the stage0 incr cache with the stage1 compiler?

^ this one

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:17):

also, I'm pretty sure cargo throws away everything when the compiler version changes (which includes switching nightly versions) - the only time that doesn't work is when using a stage1 compiler, which just has -dev in the name

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:17):

Aaron Hill said:

also, I'm pretty sure cargo throws away everything when the compiler version changes (which includes switching nightly versions) - the only time that doesn't work is when using a stage1 compiler, which just has -dev in the name

yup that's exactly what I mean

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:17):

I think any issue with the stage0/stage1 cache should be done in bootstrap

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:17):

bootstrap can't do anything other than completely throw away the cache though :/

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:17):

since that's a really weird use-case that's only relevant for the compiler

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:18):

there's an option today to do that but no one uses it

view this post on Zulip Aaron Hill (Mar 13 2021 at 17:18):

what would you want it to do?

view this post on Zulip Joshua Nelson (Mar 13 2021 at 17:18):

Aaron Hill said:

since that's a really weird use-case that's only relevant for the compiler

yeah that's fair, it's probably off topic for this thread

view this post on Zulip Aaron Hill (Mar 14 2021 at 21:28):

This change has already found another query implementation bug: https://github.com/rust-lang/rust/issues/83126

view this post on Zulip Aaron Hill (Mar 14 2021 at 21:28):

extern_mod_stmt_cnum uses global untracked state, but wasn't marked eval_always


Last updated: Oct 21 2021 at 20:03 UTC