Skip to content

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

Merged
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
scottmcm:fix-159116
Jul 12, 2026
Merged

Fix offset used to read the second part of scalar pairs from a const#159148
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
scottmcm:fix-159116

Conversation

@scottmcm

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

Copy link
Copy Markdown
Member

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 11, 2026
@rustbot

rustbot commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

scottmcm
scottmcm commented Jul 11, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_ssa/src/mir/operand.rs
} => {
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.

scottmcm
scottmcm commented Jul 11, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_ssa/src/mir/operand.rs
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);

@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

RalfJung
RalfJung reviewed Jul 11, 2026
View reviewed changes
Comment thread compiler/rustc_codegen_ssa/src/mir/operand.rs
Comment on lines 235 to 240
let a_val = read_scalar(
offset,
a_size,
a,
bx.scalar_pair_element_backend_type(layout, 0, true),
);

@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".

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee

workingjubilee commented Jul 11, 2026

Copy link
Copy Markdown
Member

@bors r+

@rust-bors

rust-bors Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

📌 Commit fb07057 has been approved by workingjubilee

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2026
@workingjubilee

workingjubilee commented Jul 11, 2026

Copy link
Copy Markdown
Member

@bors p=1 we really need to not miscompile this.

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Jul 11, 2026
@bors
Auto merge of #159148 - scottmcm:fix-159116, r=workingjubilee
8799719 
Fix offset used to read the second part of scalar pairs from a `const`

Fixes #159116

r? @workingjubilee
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-bors rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 12, 2026
@rust-bors

rust-bors Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

💔 Test for 8799719 failed: CI. Failed job:

@workingjubilee

workingjubilee commented Jul 12, 2026

Copy link
Copy Markdown
Member

Spurious.

@bors retry

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Jul 12, 2026
@bors
Auto merge of #159148 - scottmcm:fix-159116, r=workingjubilee
769702c 
Fix offset used to read the second part of scalar pairs from a `const`

Fixes #159116

r? @workingjubilee
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-bors rust-bors Bot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 12, 2026
@rust-bors

rust-bors Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

💔 Test for 769702c failed: CI. Failed job:

@scottmcm
Use smaller scalars in the test and force optimizations on
d693dfe 
Otherwise 32-bit has a different ABI and the `CHECK` fails.
And the noopt job fails without forcing MIR optimization here.
@rust-bors rust-bors Bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2026
@scottmcm

scottmcm commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

@bors try jobs=x86_64-gnu-nopt

@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Jul 12, 2026
@bors
Auto merge of #159148 - scottmcm:fix-159116, r=<try>
ab24c36 
Fix offset used to read the second part of scalar pairs from a `const`


try-job: x86_64-gnu-nopt
@scottmcm

scottmcm commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

Ok, the codegen tests passed so the -O addition was enough.

@bors r=workingjubilee

@rust-bors

rust-bors Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

📌 Commit d693dfe has been approved by workingjubilee

It is now in the queue for this repository.

@rust-bors rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 12, 2026
@workingjubilee

workingjubilee commented Jul 12, 2026

Copy link
Copy Markdown
Member

scribbles a note

codegen PR (review?) checklist:

  • new test? set compile flags explicitly for optimization

(probably should actually add that to the rustc devguide... )

@rust-bors

rust-bors Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: ab24c36 (ab24c36adcef556fbefb53379f052eb3c1317e23)
Base parent: be8e824 (be8e82435eb04fbe75ed5286b52735366e160bed)

@JonathanBrouwer JonathanBrouwer mentioned this pull request Jul 12, 2026
Merged
rust-bors Bot pushed a commit that referenced this pull request Jul 12, 2026
@bors
Auto merge of #159170 - JonathanBrouwer:rollup-UxyQ0Ig, r=JonathanBro…
170e61f 
…uwer

Rollup of 3 pull requests

Successful merges:

 - #159148 (Fix offset used to read the second part of scalar pairs from a `const`)
 - #159167 (c-variadic: Update cfg for 128-bit integers to fix build error on AArch64 ILP32)
 - #159153 (Rename `errors.rs` file to `diagnostics.rs` (12/N))
@rust-bors

rust-bors Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

⌛ Testing commit d693dfe with merge 5d7b931...

Workflow: https://github.com/rust-lang/rust/actions/runs/29187429671

rust-bors Bot pushed a commit that referenced this pull request Jul 12, 2026
@bors
Auto merge of #159148 - scottmcm:fix-159116, r=workingjubilee
5d7b931 
Fix offset used to read the second part of scalar pairs from a `const`



Fixes #159116

r? @workingjubilee
@JonathanBrouwer

JonathanBrouwer commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@bors yield
Yielding to enclosing rollup

@rust-bors

rust-bors Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Auto build was cancelled. Cancelled workflows:

The next pull request likely to be tested is #159170.

rust-bors Bot pushed a commit that referenced this pull request Jul 12, 2026
@bors
Auto merge of #159170 - JonathanBrouwer:rollup-UxyQ0Ig, r=JonathanBro…
babf6ea 
…uwer

Rollup of 3 pull requests

Successful merges:

 - #159148 (Fix offset used to read the second part of scalar pairs from a `const`)
 - #159167 (c-variadic: Update cfg for 128-bit integers to fix build error on AArch64 ILP32)
 - #159153 (Rename `errors.rs` file to `diagnostics.rs` (12/N))
@rust-bors rust-bors Bot merged commit 3ab3767 into rust-lang:main Jul 12, 2026
14 of 15 checks passed
rust-timer added a commit that referenced this pull request Jul 12, 2026
@rust-timer
Unrolled build for #159148
a242f50 
Rollup merge of #159148 - scottmcm:fix-159116, r=workingjubilee

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

Fixes #159116

r? @workingjubilee
@rustbot rustbot added this to the 1.99.0 milestone Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RalfJung RalfJung RalfJung left review comments
@workingjubilee workingjubilee workingjubilee left review comments

Assignees

@workingjubilee workingjubilee

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.99.0

Development

Successfully merging this pull request may close these issues.

memory corruption with nightly-2026-07-10

6 participants

@scottmcm @rustbot @rust-log-analyzer @workingjubilee @JonathanBrouwer @RalfJung