-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
Conversation
r? @chenyukang rustbot has assigned @chenyukang. Use |
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #131006) made this pull request unmergeable. Please resolve the merge conflicts. |
fafb277
to
a8f9a32
Compare
This comment has been minimized.
This comment has been minimized.
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This is unblocked now, I added a second commit that fixes the ICE. Cc @cjgillot |
35fa9ca
to
d329d94
Compare
This comment has been minimized.
This comment has been minimized.
d329d94
to
7d63efd
Compare
Some changes occurred in coverage tests. cc @Zalathar |
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... |
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. |
r? compiler |
@@ -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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
}
}
There was a problem hiding this comment.
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. 🤷
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (a964a92): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
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.
CyclesResults (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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 774.632s -> 773.837s (-0.10%) |
This applies the relatively new
assert_matches_abi
check in theoffset
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 theImmediate
type.This leads to ICEs in a GVN mir-opt test, so the second commit fixes GVN.
Fixes #131064.