Fix #61793 #62011
Closed
Fix #61793 #62011
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
Centril
reviewed
Jun 21, 2019
eddyb
reviewed
Jun 21, 2019
src/librustc/ty/layout.rs
Outdated
| let mut inverse_index = (0..offsets.len() as u32).collect::<Vec<_>>(); | ||
| inverse_index.sort_unstable_by_key(|i| { | ||
| // Place ZSTs before other fields at the same offset. | ||
| // Codegen expects this. |
There was a problem hiding this comment.
I'm confused, I don't see why this would be guaranteed for even a plain struct?
There was a problem hiding this comment.
Nevermind, I think what you mean is simply that you can't put fields out of order - a non-ZST can't be followed by a field of the same offset, because the non-ZST had advanced the offset.
I'll try a much simpler fix, to confirm my suspicions.
EDIT: okay, using a stable sort isn't enough. I think my basic assumptions were wrong when I suggested sorting by offsets - I'll try to come up with a way that doesn't involve sorts.
There was a problem hiding this comment.
Yep, that's what I mean.
eddyb
reviewed
Jun 21, 2019
src/librustc/ty/layout.rs
Outdated
|
|
||
| fn recompute_memory_index(offsets: &[Size], fields: &[TyLayout<'_>]) -> Vec<u32> { |
There was a problem hiding this comment.
I wouldn't have moved this out unless it was used for anything other than the generator code.
There was a problem hiding this comment.
oh yeah it was originally being turned into a generic fn, I'll put it back
There was a problem hiding this comment.
(you can have generic fns inside other functions anyway, FWIW)
eddyb
reviewed
Jun 21, 2019
| // beinning of generator field layouts, causing an assertion error downstream. | ||
|
|
||
| // compile-pass | ||
| // compile-flags:--edition 2018 |
There was a problem hiding this comment.
You should use // edition:2018 in tests.
bors
added a commit
that referenced
this pull request
Jun 26, 2019
rustc: correctly transform memory_index mappings for generators. Fixes #61793, closes #62011 (previous attempt at fixing #61793). During #60187, I made the mistake of suggesting that the (re-)computation of `memory_index` in `ty::layout`, after generator-specific logic split/recombined fields, be done off of the `offsets` of those fields (which needed to be computed anyway), as opposed to the `memory_index`. `memory_index` maps each field to its in-memory order index, which ranges over the same `0..n` values as the fields themselves, making it a bijective mapping, and more specifically a permutation (indeed, it's the permutation resulting from field reordering optimizations). Each field has an unique "memory index", meaning a sort based on them, even an unstable one, will not put them in the wrong order. But offsets don't have that property, because of ZSTs (which do not increase the offset), so sorting based on the offset of fields alone can (and did) result in wrong orders. Instead of going back to sorting based on (slices/subsets of) `memory_index`, or special-casing ZSTs to make sorting based on offsets produce the right results (presumably), as #62011 does, I opted to drop sorting altogether and focus on `O(n)` operations involving *permutations*: * a permutation is easily inverted (see the `invert_mapping` `fn`) * an `inverse_memory_index` was already employed in other parts of the `ty::layout` code (that is, a mapping from memory order to field indices) * inverting twice produces the original permutation, so you can invert, modify, and invert again, if it's easier to modify the inverse mapping than the direct one * you can modify/remove elements in a permutation, as long as the result remains dense (i.e. using every integer in `0..len`, without gaps) * for splitting a `0..n` permutation into disjoint `0..x` and `x..n` ranges, you can pick the elements based on a `i < x` / `i >= x` predicate, and for the latter, also subtract `x` to compact the range to `0..n-x` * in the general case, for taking an arbitrary subset of the permutation, you need a renumbering from that subset to a dense `0..subset.len()` - but notably, this is still `O(n)`! * you can merge permutations, as long as the result remains disjoint (i.e. each element is unique) * for concatenating two `0..n` and `0..m` permutations, you can renumber the elements in the latter to `n..n+m` * some of these operations can be combined, and an inverse mapping (be it a permutation or not) can still be used instead of a forward one by changing the "domain" of the loop performing the operation I wish I had a nicer / more mathematical description of the recombinations involved, but my focus was to fix the bug (in a way which preserves information more directly than sorting would), so I may have missed potential changes in the surrounding generator layout code, that would make this all more straight-forward. r? @tmandry
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the generator layout computation we were recomputing the memory index of each field in the layout based on the offset of the field. This sometimes caused ZST fields to go in the middle of the layout rather at the beginning, which caused an assertion error in codegen.
r? @eddyb
cc @cramertj