Skip to content

Fix offset used to read the second part of scalar pairs from a const #159148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rust-bors merged 3 commits into from
Jul 12, 2026
+21 −3
Merged

Fix offset used to read the second part of scalar pairs from a const #159148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73e0280
Add regression test for 159116
scottmcm Jul 11, 2026
9fddc69
Include the subobject offset, not just the field offset, when reading…
scottmcm Jul 11, 2026
d693dfe
Use smaller scalars in the test and force optimizations on
scottmcm Jul 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions compiler/rustc_codegen_ssa/src/mir/operand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,18 +227,19 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
BackendRepr::ScalarPair {
a: a @ abi::Scalar::Initialized { .. },
b: b @ abi::Scalar::Initialized { .. },
b_offset,
b_offset: local_b_offset,
} => {
let (a_size, b_size) = (a.size(bx), b.size(bx));
assert!(b_offset.bytes() > 0);
let alloc_b_offset = offset + local_b_offset;

@scottmcm scottmcm Jul 11, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooray for over 20 year old techniques.

View changes since the review

@workingjubilee workingjubilee Jul 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops.

@workingjubilee workingjubilee Jul 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@scottmcm scottmcm Jul 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert!(alloc_b_offset.bytes() > 0);

@scottmcm scottmcm Jul 11, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 b_offset = (offset + a_size).align_to(b.default_align(bx).abi);
assert!(b_offset.bytes() > 0);

View changes since the review

let a_val = read_scalar(
offset,
a_size,
a,
bx.scalar_pair_element_backend_type(layout, 0, true),
);
Comment on lines 235 to 240

@RalfJung RalfJung Jul 11, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

View changes since the review

@scottmcm scottmcm Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@RalfJung RalfJung Jul 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah definitely not for this PR, this was more of a "in case you are interested to clean this up further".

let b_val = read_scalar(
b_offset,
alloc_b_offset,
b_size,
b,
bx.scalar_pair_element_backend_type(layout, 1, true),
Expand Down
17 changes: 17 additions & 0 deletions tests/codegen-llvm/const-array-of-pairs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//@ compile-flags: -O -C no-prepopulate-passes

#![crate_type = "lib"]

// Regression test for https://github.com/rust-lang/rust/issues/159116
// where a change had started reading at the wrong offset into a const
// when the sub-object had BackendRepr::ScalarPair

const PAIRS: [(u16, u16); 3] = [(1, 2), (3, 4), (5, 6)];

// CHECK-LABEL: @read_not_first_pair
#[no_mangle]
pub fn read_not_first_pair() -> (u16, u16) {
// CHECK: start:
// CHECK-NEXT: ret { i16, i16 } { i16 3, i16 4 }
PAIRS[1]
}
Loading