make sure ScalarPair enums have ScalarPair variants; add some layout sanity checks #96872
Conversation
rustbot
added
the
T-compiler
|
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| // Would result in ICE see the issue #92464 for more info | ||
| if tag.size(dl) == size || variants.iter().all(|layout| layout.is_empty()) { | ||
| if layout_variants.iter().all(|v| v.abi.is_uninhabited()) { | ||
| abi = Abi::Uninhabited; |
There was a problem hiding this comment.
I reordered this a bit so it better fits the pattern of these three branches here computing the abi.
Rewriting this without mutable variables would be cleaner but that is an undertaking for someone else. ;)
| assert!(layout.abi.is_uninhabited()); | ||
| } | ||
|
|
||
| if cfg!(debug_assertions) { |
There was a problem hiding this comment.
We should at some point measure the cost of having this on by default.
| Union { | ||
| value: Int( | ||
| I8, | ||
| false, |
There was a problem hiding this comment.
Looking at this makes me wish there was an enum Signedness { Unsigned, Signed } instead of a bool, heh.
There was a problem hiding this comment.
Yeah... and also nicer debug-printing of Scalar layouts. Another time.^^
| Abi::ScalarPair(scalar1, scalar2) => { | ||
| // Sanity-check scalar pair size. | ||
| let field2_offset = scalar1.size(&tcx).align_to(scalar2.align(&tcx).abi); | ||
| let total = field2_offset + scalar2.size(&tcx); | ||
| assert!( | ||
| layout.size() >= total, | ||
| "size mismatch between ABI and layout in {layout:#?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Hmm, shouldn't this also have an alignment check?
Honestly, reusing LayoutCx::scalar_pair would probably make this really straight-forward (and maybe some miri/codegen code as well, tho in that case it should be a Layout constructor rather than on LayoutCx) - except for the fact that it allocates two Vecs (if only they were SmallVec<[_; 2]>... - more things that aren't immediately actionable sigh)
There was a problem hiding this comment.
I wasn't sure what the alignment check would be, so this felt like better than nothing. 🤷
I don't see how this could reuse scalar_pair; things with ScalarPair ABI can have any number of fields since there can be newtypes around the underlying pair.
There was a problem hiding this comment.
Oh yeah, the fields would need to be ignored wrt validation (but e.g. codegen could use the offset of the second field to avoid computing a_size.align_to(b_align) itself).
Anyway, not that viable today with the allocations.
|
That codegen test failure is odd. For some reason |
That would make sense, I think, IIRC only |
|
@eddyb I think I took care of all your comments, but it might be good for you to have another look. :) |
This comment has been minimized.
This comment has been minimized.
|
r=me once fixing the vector check, just noticed that's failing (there's a IIUC, this is what causes the weirdness where, say, 3 (or 5-7) element vectors end up with the same size as vectors of 4 (or 8) elements (on some targets, for some element types). |
|
@bors r=eddyb |
|
📌 Commit 02eca34 has been approved by |
bors
added
S-waiting-on-bors
Rollup of 5 pull requests Successful merges: - rust-lang#93661 (Add missing rustc arg docs) - rust-lang#96674 (docs: add link explaining variance to NonNull docs) - rust-lang#96812 (Do not lint on explicit outlives requirements from external macros.) - rust-lang#96823 (Properly fix rust-lang#96638) - rust-lang#96872 (make sure ScalarPair enums have ScalarPair variants; add some layout sanity checks) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
@eddyb suggested that it might be reasonable for
ScalarPairenums to simply adjust the ABI of their variants accordingly, such that the layout invariant Miri expects actually holds. This PR implements that. I should note though that I don't know much about this layout computation code and what non-Miri consumers expect from it, so tread with caution!I also added a function to sanity-check that computed layouts are internally consistent. This helped a lot in figuring out the final shape of this PR, though I am also not 100% sure that these sanity checks are the right ones.
Cc @oli-obk
Fixes #96221