Avoid sorting predicates by `DefId` by Aaron1011 · Pull Request #83074 · rust-lang/rust

Aaron1011 · 2021-03-13T03:20:33Z

Fixes issue #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 DefIds 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 DefIds in the inputs have their DefPathHashes
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 DefIds got
assigned in a particular compilation session. If some definitions end up
with different DefIds (but the same DefPathHashes) 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

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.
Remove the Ord impls for CrateNum and DefId. This will make it
difficult to introduce ICEs in the first place.

rust-highfive · 2021-03-13T03:20:35Z

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

Aaron1011 · 2021-03-13T04:51:38Z

This code appears to date back to 2014 from PR #15985:
rust/src/librustc/middle/typeck/collect.rs
Line 1221 in 08f033b
param_bounds.trait_bounds.sort_by(|a,b| a.def_id.cmp(&b.def_id));

oli-obk · 2021-03-13T13:34:30Z

Thanks for the detailed analysis. I think we should merge this PR quickly, without addressing the two additional protection mechanisms you mentioned. These can then get added to the issue and be addressed independently

jackh726 · 2021-03-13T15:58:31Z

I'm a little surprised that removing the sorting of auto traits does anything, given that we sort the predicates later: v.sort_by(|a, b| a.skip_binder().stable_cmp(tcx, &b.skip_binder()));. For the bounds, I wonder if it's worth doing a stable sort still, anyways?

CI looks good, so r=me assuming my comments above don't spark any thoughts/concerns.

Aaron1011 · 2021-03-13T16:32:44Z

I'm a little surprised that removing the sorting of auto traits does anything, given that we sort the predicates later: v.sort_by(|a, b| a.skip_binder().stable_cmp(tcx, &b.skip_binder()));

Yeah, I'm not quite sure what's going on there. However, this fixes the current miscompilation - once we start verifying the hashes, we'll find out if that sorting has any impact.

For the bounds, I wonder if it's worth doing a stable sort still, anyways?

I tried this in #83005, but it ended up making the UI test output dependent on whether debuginfo/debug-assertions were enabled in the config.toml.

Aaron1011 · 2021-03-13T16:32:56Z

@bors r=jackh726

bors · 2021-03-13T16:32:58Z

📌 Commit 4c0ba8cc0f9f76a95706d6bd5f7dd34ba3cf583d has been approved by jackh726

Aaron1011 · 2021-03-13T18:31:43Z

I've added a regression test.

@bors r=jackh726

bors · 2021-03-13T18:31:44Z

📌 Commit 50bd1e9cb1dff6985330b6a24f6b542d771134ac has been approved by jackh726

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 · 2021-03-13T18:45:31Z

@bors r=jackh726

bors · 2021-03-13T18:45:32Z

📌 Commit 06546d4 has been approved by jackh726

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.

JohnTitor · 2021-03-14T06:58:02Z

Heads up: This rollup failure (#83096 (comment)) may be related to this PR. I'm uncertain and not going to r- but it'd be great if someone could check it.

Aaron1011 · 2021-03-14T07:34:59Z

@bors rollup=iffy

bors · 2021-03-14T17:26:48Z

💔 Test failed - checks-actions

Aaron1011 · 2021-03-14T18:12:50Z

@bors r=jackh726

bors · 2021-03-14T18:12:51Z

📌 Commit 94604706122cb3b4ae21b79a66d5e3a5fbe105f8 has been approved by jackh726

bors · 2021-03-14T20:05:57Z

⌛ Testing commit 94604706122cb3b4ae21b79a66d5e3a5fbe105f8 with merge 9b3d7d5d8658f518007dd1c485a63d8d14aa1517...

bors · 2021-03-14T20:44:52Z

💔 Test failed - checks-actions

This is needed to get rustdoc to succeed on `dist-x86_64-linux-alt`

Aaron1011 · 2021-03-15T03:02:57Z

@bors r=jackh726 rollup=never

bors · 2021-03-15T03:02:58Z

📌 Commit 18f8979 has been approved by jackh726

bors · 2021-03-15T06:20:33Z

⌛ Testing commit 18f8979 with merge 3963c3d...

bors · 2021-03-15T08:49:20Z

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 3963c3d to master...

Avoid sorting by DefId for `necessary_variants()` Follow-up to rust-lang#83074. Originally I tried removing `impl Ord for DefId` but that hit *lots* of errors 😅 so I thought I would start with easy things. I am not sure whether this could actually cause invalid query results, but this is used from `MarkSymbolVisitor::visit_arm` so it's at least feasible. r? `@Aaron1011`

Avoid sorting by DefId for `necessary_variants()` Follow-up to rust-lang#83074. Originally I tried removing `impl Ord for DefId` but that hit *lots* of errors 😅 so I thought I would start with easy things. I am not sure whether this could actually cause invalid query results, but this is used from `MarkSymbolVisitor::visit_arm` so it's at least feasible. r? ``@Aaron1011``

Avoid sorting by DefId for `necessary_variants()` Follow-up to rust-lang#83074. Originally I tried removing `impl Ord for DefId` but that hit *lots* of errors 😅 so I thought I would start with easy things. I am not sure whether this could actually cause invalid query results, but this is used from `MarkSymbolVisitor::visit_arm` so it's at least feasible. r? `@Aaron1011`

rust-highfive assigned oli-obk Mar 13, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021

Aaron1011 mentioned this pull request Mar 13, 2021
Sort bounds by DefPathHash during astconv #83005
Closed

This comment has been minimized.
Sign in to view

Aaron1011 force-pushed the new-sort-fix branch from 18b1998 to d2b2819Compare March 13, 2021 04:40

Aaron1011 commented Mar 13, 2021
View reviewed changes

bjorn3 reviewed Mar 13, 2021
View reviewed changes

Comment threadcompiler/rustc_typeck/src/astconv/mod.rs Outdated

jackh726 approved these changes Mar 13, 2021
View reviewed changes

Aaron1011 force-pushed the new-sort-fix branch from d2b2819 to 4c0ba8cCompare March 13, 2021 16:28

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2021

Aaron1011 force-pushed the new-sort-fix branch from 4c0ba8c to 50bd1e9Compare March 13, 2021 18:31

This comment has been minimized.
Sign in to view

Aaron1011 force-pushed the new-sort-fix branch from 50bd1e9 to 06546d4Compare March 13, 2021 18:45

Dylan-DPC-zz mentioned this pull request Mar 13, 2021
Rollup of 8 pull requests #83096
Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 14, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2021

This comment has been minimized.
Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 14, 2021

Bump recursion_limit in a few places
18f8979
This is needed to get rustdoc to succeed on `dist-x86_64-linux-alt`

Aaron1011 force-pushed the new-sort-fix branch from 9460470 to 18f8979Compare March 15, 2021 03:02

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 15, 2021

bors merged commit 3963c3d into rust-lang:masterMar 15, 2021

rustbot added this to the 1.52.0 milestone Mar 15, 2021

bors mentioned this pull request Mar 15, 2021
Stabilize or_patterns (RFC 2535, 2530, 2175) #79278
Merged
5 tasks

Aaron1011 mentioned this pull request Mar 15, 2021
Miscompilation with incr. comp. #82920
Closed

jyn514 mentioned this pull request Mar 28, 2021
Avoid sorting by DefId for necessary_variants()#83599
Merged

Uh oh!

Uh oh!

Conversation

Aaron1011 commented Mar 13, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 13, 2021

Uh oh!

This comment has been minimized.

Aaron1011Mar 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oli-obk commented Mar 13, 2021

Uh oh!

jackh726 commented Mar 13, 2021

Uh oh!

Aaron1011 commented Mar 13, 2021

Uh oh!

Aaron1011 commented Mar 13, 2021

Uh oh!

bors commented Mar 13, 2021

Uh oh!

Aaron1011 commented Mar 13, 2021

Uh oh!

bors commented Mar 13, 2021

Uh oh!

This comment has been minimized.

Aaron1011 commented Mar 13, 2021

Uh oh!

bors commented Mar 13, 2021

Uh oh!

JohnTitor commented Mar 14, 2021

Uh oh!

Aaron1011 commented Mar 14, 2021

Uh oh!

bors commented Mar 14, 2021

Uh oh!

Aaron1011 commented Mar 14, 2021

Uh oh!

bors commented Mar 14, 2021

Uh oh!

bors commented Mar 14, 2021

Uh oh!

This comment has been minimized.

bors commented Mar 14, 2021

Uh oh!

Aaron1011 commented Mar 15, 2021

Uh oh!

bors commented Mar 15, 2021

Uh oh!

bors commented Mar 15, 2021

Uh oh!

bors commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Aaron1011 commented Mar 13, 2021•
edited
Loading