Fix offset used to read the second part of scalar pairs from a const#159148
Fix offset used to read the second part of scalar pairs from a const#159148scottmcm wants to merge 3 commits into
const#159148Conversation
rustbot
added
S-waiting-on-review
|
|
| } => { | ||
| let (a_size, b_size) = (a.size(bx), b.size(bx)); | ||
| assert!(b_offset.bytes() > 0); | ||
| let alloc_b_offset = offset + local_b_offset; |
There was a problem hiding this comment.
Ahh yeah, that was easy to miss unfortunately. Needed to look closer I guess, if I wanted to make sure I got it. But yeah, important to just not make things look the same if they aren't.
There was a problem hiding this comment.
Yeah. Super easy when scanning to do the "well obviously this b_offset is exactly the same as the literally dozens of other b_offset variables I'm touching".
Especially when I'm not trying to change anything so am more inclined to trust tests passing that it's right -- but apparently we didn't have any coverage for this block with offset > 0.
| let (a_size, b_size) = (a.size(bx), b.size(bx)); | ||
| assert!(b_offset.bytes() > 0); | ||
| let alloc_b_offset = offset + local_b_offset; | ||
| assert!(alloc_b_offset.bytes() > 0); |
There was a problem hiding this comment.
It's not obvious to me what the intent of this assert! is, but this is what it was checking before #158666
rust/compiler/rustc_codegen_ssa/src/mir/operand.rs
Lines 230 to 231 in b0cbd79
| let a_val = read_scalar( | ||
| offset, | ||
| a_size, | ||
| a, | ||
| bx.scalar_pair_element_backend_type(layout, 0, true), | ||
| ); |
There was a problem hiding this comment.
We do have much nicer APIs for reading stuff from a const Allocation... but they need an InterpCx. Those are not very expensive to set up though. Maybe codegen should have one and then use the nicer APIs?
There was a problem hiding this comment.
Can you start a zulip thread and ping me there, Ralf? I started #t-compiler/const-eval > InterpCx in codegen @ 💬
It absolutely sounds interesting, but I'd not want to do it in this PR since I'd rather get back to correct with a simple PR, then do the more-churn-for-better-maintainability separately.
There was a problem hiding this comment.
Oh yeah definitely not for this PR, this was more of a "in case you are interested to clean this up further".
This comment has been minimized.
This comment has been minimized.
Otherwise 32-bit has a different ABI and the `CHECK` fails.
|
@bors r+ |
rust-bors
Bot
added
S-waiting-on-bors
|
@bors p=1 we really need to not miscompile this. |
5 participants
Fixes #159116
r? @workingjubilee