Miscompilation with incr. comp. #82920
Assignees
Labels
A-incr-comp
Area: Incremental compilation
C-bug
Category: This is a bug.
I-unsound
Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
P-high
High priority
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Comments
jonas-schievink
added
T-compiler
rustbot
added
the
I-prioritize
|
Does |
|
Yes: |
wesleywiser
added
the
E-needs-bisection
|
Assigning @rustbot label -I-prioritize +P-high |
rustbot
added
P-high
|
(Oh, I told wesley to tag with needs-bisection yesterday, but then I realize now that we don't know that this is a regression. Its only been reported as being reproducible in a recent stable; we do not yet know about older stables. Will investigate.) |
|
I modified my local rustc to print out the current value when the fingerprint changes: |
|
The value from the original (non-crashing) run is: |
|
It looks like the predicates in |
|
Epic work @Aaron1011 |
|
This is caused by the use of rust/compiler/rustc_typeck/src/astconv/mod.rs Line 945 in 066f01d A Using a Nightly rustc, 190 tests fail, most with panic messages like this (I didn't get a segfault): After changing that line to sort by the Given the consequences of getting this kind of thing wrong, I think we should set |
|
Unfortunately, another unstable hash erorr showed up after I fixed the first one: I strongly suspect that this is #80691 |
This was referenced Mar 11, 2021
Aaron1011
added a commit
to Aaron1011/rust
that referenced
this issue
Mar 11, 2021
cc rust-lang#82920 The value of a `DefId` is not stable across compilation sessions. If we sort by `DefIds`, and then include the sorted list as part of a query result, then the query will no longer be a pure function of its input (e.g. the `DefId` can change while the `DefPathHash` remains the same). For reasons that are not yet clear, this issue lead to segfaults and garbage slice index values when the project in the linked issue was built with a particular incremental cache.
|
Ouch, that must have been a nasty bug to run into. Thanks for reporting it, @jonas-schievink! |
|
It would be good to try to figure out exactly why this caused a miscompilation. I'm pretty sure that the order of the bounds can't cause any issues by itself, so I suspect there's some subtle issue with the query system when the hash value is unstable. |
|
I've determined the cause of the segfault:
This can be seen in the following backtrace: The issue occurs here: We attempt to call the method However, we end up invoking the macro-generated method This is clearly the wrong method - |
|
I am now convinced that we need some form of #83007 (hopefully with better performance). It's not sufficient for a query to be a pure function of the data it accesses (e.g. This is a very subtle requirement. I'm certain that we'll run into this issue again, unless the query system verifies that re-running a query actually produces the expected result (via hashing the new result). |
Aaron1011
added a commit
to Aaron1011/rust
that referenced
this issue
Mar 13, 2021
Issue rust-lang#82920 showed that the kind of bugs caught by this flag have soundness implications. This causes performance regressions of up to 15.2% during incremental compilation, but this is necessary to catch miscompilations caused by bugs in query implementations.
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Mar 13, 2021
…-Simulacrum Turn `-Z incremental-verify-ich` on by default Issue rust-lang#82920 showed that the kind of bugs caught by this flag have soundness implications.
|
I've managed to minimize this to a short test: // revisions: rpass1 rpass2
trait MyTrait: One + Two {}
impl<T> One for T {
fn method_one(&self) -> usize {
1
}
}
impl<T> Two for T {
fn method_two(&self) -> usize {
2
}
}
impl<T: One + Two> MyTrait for T {}
fn main() {
let a: &dyn MyTrait = &true;
assert_eq!(a.method_one(), 1);
assert_eq!(a.method_two(), 2);
}
// Re-order traits 'One' and 'Two' between compilation
// sessions
#[cfg(rpass1)]
trait One { fn method_one(&self) -> usize; }
trait Two { fn method_two(&self) -> usize; }
#[cfg(rpass2)]
trait One { fn method_one(&self) -> usize; } |
Aaron1011
added a commit
to Aaron1011/rust
that referenced
this issue
Mar 13, 2021
Fixes issue rust-lang#82920 Even if an item does not change between compilation sessions, it may end up with a different `DefId`, since inserting/deleting an item affects the `DefId`s of all subsequent items. Therefore, we use a `DefPathHash` in the incremental compilation system, which is stable in the face of changes to unrelated items. In particular, the query system will consider the inputs to a query to be unchanged if any `DefId`s in the inputs have their `DefPathHash`es unchanged. Queries are pure functions, so the query result should be unchanged if the query inputs are unchanged. Unfortunately, it's possible to inadvertantly make a query result incorrectly change across compilations, by relying on the specific value of a `DefId`. Specifically, if the query result is a slice that gets sorted by `DefId`, the precise order will depend on how the `DefId`s got assigned in a particular compilation session. If some definitions end up with different `DefId`s (but the same `DefPathHash`es) in a subsequent compilation session, we will end up re-computing a *different* value for the query, even though the query system expects the result to unchanged due to the unchanged inputs. It turns out that we have been sorting the predicates computed during `astconv` by their `DefId`. These predicates make their way into the `super_predicates_that_define_assoc_type`, which ends up getting used to compute the vtables of trait objects. This, re-ordering these predicates between compilation sessions can lead to undefined behavior at runtime - the query system will re-use code built with a *differently ordered* vtable, resulting in the wrong method being invoked at runtime. This PR avoids sorting by `DefId` in `astconv`, fixing the miscompilation. However, it's possible that other instances of this issue exist - they could also be easily introduced in the future. To fully fix this issue, we should 1. Turn on `-Z incremental-verify-ich` by default. This will cause the compiler to ICE whenver an 'unchanged' query result changes between compilation sessions, instead of causing a miscompilation. 2. Remove the `Ord` impls for `CrateNum` and `DefId`. This will make it difficult to introduce ICEs in the first place.
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this issue
Mar 13, 2021
Avoid sorting predicates by `DefId` Fixes issue rust-lang#82920 Even if an item does not change between compilation sessions, it may end up with a different `DefId`, since inserting/deleting an item affects the `DefId`s of all subsequent items. Therefore, we use a `DefPathHash` in the incremental compilation system, which is stable in the face of changes to unrelated items. In particular, the query system will consider the inputs to a query to be unchanged if any `DefId`s in the inputs have their `DefPathHash`es unchanged. Queries are pure functions, so the query result should be unchanged if the query inputs are unchanged. Unfortunately, it's possible to inadvertantly make a query result incorrectly change across compilations, by relying on the specific value of a `DefId`. Specifically, if the query result is a slice that gets sorted by `DefId`, the precise order will depend on how the `DefId`s got assigned in a particular compilation session. If some definitions end up with different `DefId`s (but the same `DefPathHash`es) in a subsequent compilation session, we will end up re-computing a *different* value for the query, even though the query system expects the result to unchanged due to the unchanged inputs. It turns out that we have been sorting the predicates computed during `astconv` by their `DefId`. These predicates make their way into the `super_predicates_that_define_assoc_type`, which ends up getting used to compute the vtables of trait objects. This, re-ordering these predicates between compilation sessions can lead to undefined behavior at runtime - the query system will re-use code built with a *differently ordered* vtable, resulting in the wrong method being invoked at runtime. This PR avoids sorting by `DefId` in `astconv`, fixing the miscompilation. However, it's possible that other instances of this issue exist - they could also be easily introduced in the future. To fully fix this issue, we should 1. Turn on `-Z incremental-verify-ich` by default. This will cause the compiler to ICE whenver an 'unchanged' query result changes between compilation sessions, instead of causing a miscompilation. 2. Remove the `Ord` impls for `CrateNum` and `DefId`. This will make it difficult to introduce ICEs in the first place.
Aaron1011
removed
E-needs-bisection
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Mar 15, 2021
Avoid sorting predicates by `DefId` Fixes issue rust-lang#82920 Even if an item does not change between compilation sessions, it may end up with a different `DefId`, since inserting/deleting an item affects the `DefId`s of all subsequent items. Therefore, we use a `DefPathHash` in the incremental compilation system, which is stable in the face of changes to unrelated items. In particular, the query system will consider the inputs to a query to be unchanged if any `DefId`s in the inputs have their `DefPathHash`es unchanged. Queries are pure functions, so the query result should be unchanged if the query inputs are unchanged. Unfortunately, it's possible to inadvertantly make a query result incorrectly change across compilations, by relying on the specific value of a `DefId`. Specifically, if the query result is a slice that gets sorted by `DefId`, the precise order will depend on how the `DefId`s got assigned in a particular compilation session. If some definitions end up with different `DefId`s (but the same `DefPathHash`es) in a subsequent compilation session, we will end up re-computing a *different* value for the query, even though the query system expects the result to unchanged due to the unchanged inputs. It turns out that we have been sorting the predicates computed during `astconv` by their `DefId`. These predicates make their way into the `super_predicates_that_define_assoc_type`, which ends up getting used to compute the vtables of trait objects. This, re-ordering these predicates between compilation sessions can lead to undefined behavior at runtime - the query system will re-use code built with a *differently ordered* vtable, resulting in the wrong method being invoked at runtime. This PR avoids sorting by `DefId` in `astconv`, fixing the miscompilation. However, it's possible that other instances of this issue exist - they could also be easily introduced in the future. To fully fix this issue, we should 1. Turn on `-Z incremental-verify-ich` by default. This will cause the compiler to ICE whenver an 'unchanged' query result changes between compilation sessions, instead of causing a miscompilation. 2. Remove the `Ord` impls for `CrateNum` and `DefId`. This will make it difficult to introduce ICEs in the first place.
|
Fixed in #83074 (not sure why this wasn't auto-closed) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I have pushed 2 commits that reproduce an incremental compilation bug that leads to a segmentation fault or countless test failures due to out-of-bounds panics:
Steps to reproduce:
First commit, then second:
Second commit, then first:
This reproduces both on stable 1.50.0, as well as rustc 1.52.0-nightly (234781a 2021-03-07).
Apologies in advance for the nightmare that reducing this bug will probably be.
The text was updated successfully, but these errors were encountered: