Skip to content

Don't use Immediate::offset to transmute pointers to integers #131068

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.

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
merged 2 commits into from
Oct 7, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 30, 2024

This applies the relatively new assert_matches_abi check in the offset operation on immediates, which makes sure that if offsets are used to alter the layout (which is possible because the field layout is arbitrarily picked by the caller), this is not done in a way that breaks the invariant of the Immediate type.

This leads to ICEs in a GVN mir-opt test, so the second commit fixes GVN.

Fixes #131064.

@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Sep 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Oct 3, 2024

☔ The latest upstream changes (presumably #131006) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung RalfJung force-pushed the immediate-offset-sanity-check branch from fafb277 to a8f9a32 Compare October 3, 2024 06:26
@rust-log-analyzer

This comment has been minimized.

@saethlin saethlin added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 3, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung RalfJung removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 5, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2024

This is unblocked now, I added a second commit that fixes the ICE.

Cc @cjgillot

@RalfJung RalfJung changed the title interpret: Immediate::offset: use shared sanity-check function to ensure invariant Don't use Immediate::offset to transmute pointers to integers Oct 5, 2024
@RalfJung RalfJung force-pushed the immediate-offset-sanity-check branch from 35fa9ca to d329d94 Compare October 5, 2024 15:10
@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the immediate-offset-sanity-check branch from d329d94 to 7d63efd Compare October 5, 2024 15:55
@rustbot
Copy link
Collaborator

rustbot commented Oct 5, 2024

Some changes occurred in coverage tests.

cc @Zalathar

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2024

This changes something in the coverage output. I don't know what that output means or why it changes now, but if it relied on bogus optimizations then it's probably better to have it fixed now...

@Zalathar
Copy link
Contributor

Zalathar commented Oct 6, 2024

Looks like an artifact of some MIR node no longer being optimized away.

As you say, if a bogus optimization is no longer happening, then that's probably for the best.

@chenyukang
Copy link
Member

r? compiler

@rustbot rustbot assigned nnethercote and unassigned chenyukang Oct 6, 2024
@@ -334,7 +353,10 @@ impl<'tcx, Prov: Provenance> ImmTy<'tcx, Prov> {
/// given layout.
// Not called `offset` to avoid confusion with the trait method.
fn offset_(&self, offset: Size, layout: TyAndLayout<'tcx>, cx: &impl HasDataLayout) -> Self {
debug_assert!(layout.is_sized(), "unsized immediates are not a thing");
// Verify that the input matches its type.
if cfg!(debug_assertions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding a debug_assert_matches_abi method?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO no, I'd rather not duplicate this logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it look like this, with no logic duplication?

pub fn debug_assert_matches_abi(self, abi: Abi, cx: &impl HasDataLayout) {
    if cfg!(debug_assertions) {
        self.assert_matches_abi(abi, cx);
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that's what you mean.

Didn't seem worth it to me. 🤷

@nnethercote
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 6, 2024

📌 Commit 7d63efd has been approved by nnethercote

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 6, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 6, 2024
@bors
Copy link
Collaborator

bors commented Oct 7, 2024

⌛ Testing commit 7d63efd with merge a964a92...

@bors
Copy link
Collaborator

bors commented Oct 7, 2024

☀️ Test successful - checks-actions
Approved by: nnethercote
Pushing a964a92 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 7, 2024
@bors bors merged commit a964a92 into rust-lang:master Oct 7, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 7, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a964a92): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 2.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.1% [2.0%, 2.1%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [2.0%, 2.1%] 2

Cycles

Results (secondary -0.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
6.4% [2.3%, 9.2%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-7.5% [-8.8%, -6.8%] 6
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 774.632s -> 773.837s (-0.10%)
Artifact size: 329.53 MiB -> 329.55 MiB (0.01%)

@RalfJung RalfJung deleted the immediate-offset-sanity-check branch October 8, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

ICE: assertion failed: matches!(scalar, Scalar::Int(..))
9 participants