Temporary fix for the layout of aligned enums #92932
Conversation
rustbot
added
the
T-compiler
|
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
ouz-a
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
updated the tittle and main comment, to explain why changes were made |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
r? @jackh726 |
This comment has been minimized.
This comment has been minimized.
|
There's probably a better reviewer than me for this. r? rust-lang/compiler |
|
I'm not familiar with this code so I'll reassign. You'll need remove the merge commits though - we don't land pull requests with merge commits, you should rebase instead. r? @oli-obk |
|
@bors r+ |
|
📌 Commit fd5be23 has been approved by |
bors
added
S-waiting-on-bors
|
@bors rolllup=never (in case of perf impact) |
|
@bors rollup=never |
|
⌛ Testing commit fd5be23 with merge 85305239569a792a47cc31c47fa937f807f5d5ea... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (8b7853f): comparison url. Summary: This benchmark run did not return any relevant results. 5 results were found to be statistically significant but too small to be relevant. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression |
|
FWIW this fix was wrong and introduced another bug elsewhere, see #96185. |
|
I knew this one was going to bite me later, IIRC the original issue this PR tried to fix was convoluted one and this was the best I could do back then 😅 |
Fix repr(align) enum handling `enum`, for better or worse, supports `repr(align)`. That has already caused a bug in rust-lang#92464, which was "fixed" in rust-lang#92932, but it turns out that that fix is wrong and caused rust-lang#96185. So this reverts rust-lang#92932 (which fixes rust-lang#96185), and attempts another strategy for fixing rust-lang#92464: special-case enums when doing a cast, re-using the code to load the discriminant rather than assuming that the enum has scalar layout. This works fine for the interpreter. However, rust-lang#92464 contained another testcase that was previously not in the test suite -- and after adding it, it ICEs again. This is not surprising; codegen needs the same patch that I did in the interpreter. Probably this has to happen [around here](https://github.com/rust-lang/rust/blob/d32ce37a171663048a4c4a536803434e40f52bd6/compiler/rustc_codegen_ssa/src/mir/rvalue.rs#L276). Unfortunately I don't know how to do that -- the interpreter can load a discriminant from an operand, but codegen can only do that from a place. `@oli-obk` `@eddyb` `@bjorn3` any idea?
Fix for the issue #92464
I was after this issue for quite some time now, I have a temporary fix for it.I think the current problem is here created
tagvalue might be wrong, because when I checkedminandmaxvalues it's always between 0..1, which results in wrong size comparison in a few lines down below.I think
minandmaxvalues don't take#[repr(aligned(8))]into consideration and just act from base values assigned inside the enum. If what I am saying is true, aligned enums were created with the wrong layout for some time.As stated in the title this is only a temporary fix and I think this needs further investigation, if someone wants to mentor it I would like to work on that too.😸Edit: Weird some tests fail now going to close this for now...
Edit2: I made it work again.
I think I figured out the main problem of the issue, layout types of aligned enums with custom discriminant types were not handled, which resulted in confusing(such as this issue) behavior down the line, this is a kinda hacky fix for the issue.