pointee_info_at: fix logic for recursing into enums #132745
Merged
+61
−20
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
DianQK
reviewed
Nov 8, 2024
| // the pointer's alignment. | ||
| Some(this.for_variant(cx, *untagged_variant)) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
IIUC, this align will always be 1. I adjusted align at #131739, and I think it's sound.
There was a problem hiding this comment.
How about adding a dereferenceable field to PointeeInfo? Or something similar?
There was a problem hiding this comment.
No, align here can be larger than 1. This is checked by the existing test:
// CHECK: @option_borrow(ptr noalias noundef readonly align 4 dereferenceable_or_null(4) %_x)
#[no_mangle]
pub fn option_borrow(_x: Option<&i32>) {}
// CHECK: @option_borrow_mut(ptr noalias noundef align 4 dereferenceable_or_null(4) %_x)
#[no_mangle]
pub fn option_borrow_mut(_x: Option<&mut i32>) {}
Notice the align 4. Not sure why you are saying align will be 1 here.
I don't see why adding a "dereferenceable" field would help. The entire PointeeInfo struct is the "derefernceable-or-null" flag; a non-dereferenceable type must return None. The "or-null" part is easy to deal with later since we can use the scalar range to check whether null is allowed or not; it would be redundant to also compute that here.
There was a problem hiding this comment.
Perhaps my comment was a bit misplaced. See https://github.com/rust-lang/rust/pull/131739/files#diff-a93329b2efc9dabaf147bfdb806fb52501ed7889eb08e26e1a2714bb43862531R22-R39. In this case, we need to drop dereferenceable, but we can keep align 8.
There was a problem hiding this comment.
I don't think that's worth the effort. We shouldn't implement complicated logic like that unless it actually has real-world benefit.
In particular, I think it is actually impossible for stable code to even potentially benefit from this.
DianQK
reviewed
Nov 8, 2024
RalfJung
force-pushed
the
pointee-info-inside-enum
branch
from
f52a0d9
to
6296849
Compare
RalfJung
force-pushed
the
pointee-info-inside-enum
branch
from
6296849
to
35a913b
Compare
DianQK
approved these changes
Nov 8, 2024
|
Thanks! |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 9, 2024
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#132552 (Add v9, v8plus, and leoncasa target feature to sparc and use v8plus in create_object_file) - rust-lang#132745 (pointee_info_at: fix logic for recursing into enums) - rust-lang#132777 (try_question_mark_nop: update test for LLVM 20) - rust-lang#132785 (rustc_target: more target string fixes for LLVM 20) - rust-lang#132794 (Use a separate dir for r-a builds consistently in helix config) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 9, 2024
Rollup merge of rust-lang#132745 - RalfJung:pointee-info-inside-enum, r=DianQK pointee_info_at: fix logic for recursing into enums Fixes rust-lang#131834 The logic in `pointee_info_at` was likely written at a time when the null pointer optimization was the *only* enum layout optimization -- and as `Variant::Multiple` kept getting expanded, nobody noticed that the logic is now unsound. The job of this function is to figure out whether there is a dereferenceable-or-null and aligned pointer at a given offset inside a type. So when we recurse into a multi-variant enum, we better make sure that all the other enum variants must be null! This is the part that was forgotten, and this PR adds it. The reason this didn't explode in many ways so far is that our references only have 1 niche value (null), so it's not possible on stable to have a multi-variant enum with a dereferenceable pointer and other enum variants that are not null. But with `rustc_layout_scalar_valid_range` attributes one can force such a layout, and if `@the8472's` work on alignment niches ever lands, that will make this possible on stable.
mati865
pushed a commit
to mati865/rust
that referenced
this pull request
Nov 12, 2024
… r=DianQK pointee_info_at: fix logic for recursing into enums Fixes rust-lang#131834 The logic in `pointee_info_at` was likely written at a time when the null pointer optimization was the *only* enum layout optimization -- and as `Variant::Multiple` kept getting expanded, nobody noticed that the logic is now unsound. The job of this function is to figure out whether there is a dereferenceable-or-null and aligned pointer at a given offset inside a type. So when we recurse into a multi-variant enum, we better make sure that all the other enum variants must be null! This is the part that was forgotten, and this PR adds it. The reason this didn't explode in many ways so far is that our references only have 1 niche value (null), so it's not possible on stable to have a multi-variant enum with a dereferenceable pointer and other enum variants that are not null. But with `rustc_layout_scalar_valid_range` attributes one can force such a layout, and if `@the8472's` work on alignment niches ever lands, that will make this possible on stable.
mati865
pushed a commit
to mati865/rust
that referenced
this pull request
Nov 12, 2024
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#132552 (Add v9, v8plus, and leoncasa target feature to sparc and use v8plus in create_object_file) - rust-lang#132745 (pointee_info_at: fix logic for recursing into enums) - rust-lang#132777 (try_question_mark_nop: update test for LLVM 20) - rust-lang#132785 (rustc_target: more target string fixes for LLVM 20) - rust-lang#132794 (Use a separate dir for r-a builds consistently in helix config) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #131834
The logic in
pointee_info_atwas likely written at a time when the null pointer optimization was the only enum layout optimization -- and asVariant::Multiplekept getting expanded, nobody noticed that the logic is now unsound.The job of this function is to figure out whether there is a dereferenceable-or-null and aligned pointer at a given offset inside a type. So when we recurse into a multi-variant enum, we better make sure that all the other enum variants must be null! This is the part that was forgotten, and this PR adds it.
The reason this didn't explode in many ways so far is that our references only have 1 niche value (null), so it's not possible on stable to have a multi-variant enum with a dereferenceable pointer and other enum variants that are not null. But with
rustc_layout_scalar_valid_rangeattributes one can force such a layout, and if @the8472's work on alignment niches ever lands, that will make this possible on stable.