Skip to content
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

adds [*mut|*const] ptr::set_ptr_value #74774

Closed
wants to merge 4 commits into from
Closed

adds [*mut|*const] ptr::set_ptr_value #74774

wants to merge 4 commits into from

Conversation

oliver-giersch
Copy link
Contributor

I propose the addition of these two functions to *mut T and *const T, respectively. The motivation for this is primarily byte-wise pointer arithmetic on (potentially) fat pointers, i.e. for types with a T: ?Sized bound. A concrete use-case has been discussed in this thread.
TL;DR: Currently, byte-wise pointer arithmetic with potentially fat pointers in not possible in either stable or nightly Rust without making assumptions about the layout of fat pointers, which is currently still an implementation detail and not formally stabilized. This PR adds one function to *mut T and *const T each, allowing to circumvent this restriction without exposing any internal implementation details.
One possible alternative would be to add specific byte-wise pointer arithmetic functions to the two pointer types in addition to the already existing count-wise functions. However, I feel this fairly niche use case does not warrant adding a whole set of new functions like add_bytes, offset_bytes, wrapping_offset_bytes, etc. (times two, one for each pointer type) to libcore.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2020
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

These should go further down in the documentation. Near the offsetting methods would make sense. Maybe in between wrapping_sub and read.

@oliver-giersch
Copy link
Contributor Author

Done. Although I always thought documentation was generated alphabetically, so in-file function order did not matter?

@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

@oliver-giersch
Copy link
Contributor Author

made a complete mess trying to merge the recent directory structure changes, but everything should be ok again

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

LGTM. Could you open a tracking issue and update the issue number in the unstable attributes in the PR?

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Aug 3, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2020

📌 Commit 6c81556 has been approved by dtolnay

@bors bors 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 Aug 3, 2020
@bors
Copy link
Contributor

bors commented Aug 3, 2020

⌛ Testing commit 6c81556 with merge 24265c7b79dcb81fb22b9d4c8e14acb89cb7a736...

@bors
Copy link
Contributor

bors commented Aug 3, 2020

💥 Test timed out

@bors bors 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 Aug 3, 2020
@oliver-giersch
Copy link
Contributor Author

What's the significance of the test time out and what can I do about it?

@ecstatic-morse
Copy link
Contributor

@bors retry

@bors bors 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 Aug 6, 2020
@RalfJung
Copy link
Member

RalfJung commented Aug 6, 2020

I think there is some code in Rc/Arc that could benefit from this... Cc @CAD97

@CAD97
Copy link
Contributor

CAD97 commented Aug 7, 2020

Mild concern: this is fn(*mut T, *mut ()) -> *mut T, but with the name set_ptr_value it could be assumed to mutate in place instead of create a new pointer. At the very minimum these functions should be marked #[must_use] to help prevent that mistake.

The version of this in the A/Rc implementation also does the same thing (fn set_data_ptr<T: ?Sized, U>(mut ptr: *mut T, data: *mut U) -> *mut T), but it also doesn't use method syntax, so it's clear that it returns a new value at the use site (as no &mut is provided), unlike with method syntax.

@CAD97
Copy link
Contributor

CAD97 commented Aug 7, 2020

Mild concern: a main usecase of this is to implement the [wrapping_]offset operation for potentially fat pointers, with a byte offset instead of an element-wise offset. I still think it's worth doing this; it's not that uncommon for optimized raw-pointer-heavy data structures to store a pointer not to the whole allocated chunk but to the "interesting" part of the allocation.

Offsetting a pointer to sized type by a byte offset is (ptr as *mut u8).offset(offset) as *mut T, offsetting a pointer to a potentially unsized type by a byte offset with this PR would be (ptr as *mut T).set_ptr_value((ptr as *mut u8).offset(offset) as *mut ()).

At the least, I think it'd be better for the "pointer to data" pointer for these methods to be *mut u8 instead of *mut () so it's a lot harder to write the shorter and incorrect (ptr as *mut T).set_ptr_value((ptr as *mut ()).offset(offset)). Clippy catches calling offset on a pointer to a ZST now (after I made the mistake and had a nasty bug in a refactor due to it (thankfully a miri-caught one)), but it'd be nice to avoid making this mistake easier to write.

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 7, 2020
adds [*mut|*const] ptr::set_ptr_value

I propose the addition of these two functions to `*mut T` and `*const T`, respectively. The motivation for this is primarily byte-wise pointer arithmetic on (potentially) fat pointers, i.e. for types with a `T: ?Sized` bound. A concrete use-case has been discussed in [this](https://internals.rust-lang.org/t/byte-wise-fat-pointer-arithmetic/12739) thread.
TL;DR: Currently, byte-wise pointer arithmetic with potentially fat pointers in not possible in either stable or nightly Rust without making assumptions about the layout of fat pointers, which is currently still an implementation detail and not formally stabilized. This PR adds one function to `*mut T` and `*const T` each, allowing to circumvent this restriction without exposing any internal implementation details.
One possible alternative would be to add specific byte-wise pointer arithmetic functions to the two pointer types in addition to the already existing count-wise functions. However, I feel this fairly niche use case does not warrant adding a whole set of new functions like `add_bytes`, `offset_bytes`, `wrapping_offset_bytes`, etc. (times two, one for each pointer type) to `libcore`.
@oliver-giersch
Copy link
Contributor Author

There are two identical definitions of a similar set_data_ptr function here and here with four separate usages each.

In the last commit I've added #[must_use] attributes, mentioned provenance in the documentation and changed the type of val to *mut u8.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 7, 2020
…arth

Rollup of 4 pull requests

Successful merges:

 - rust-lang#74774 (adds [*mut|*const] ptr::set_ptr_value)
 - rust-lang#75079 (Disallow linking to items with a mismatched disambiguator)
 - rust-lang#75203 (Make `IntoIterator` lifetime bounds of `&BTreeMap` match with `&HashMap` )
 - rust-lang#75227 (Fix ICE when using asm! on an unsupported architecture)

Failed merges:

r? @ghost
@oliver-giersch
Copy link
Contributor Author

Ok, I am a bit confused by the merge process. So the initial changes have been merged as part of a roll-up, but not that latest commit I've made addressing the latest change requests and neither has the PR itself been merged.
Are there any actions I should be taking in all this or do these things just take time until someone with the right permissions comes about?

@RalfJung
Copy link
Member

@oliver-giersch looks like you pushed something after approval -- in fact even after rollup, so it was not considered.

Can you make a new PR with just those remaining changes?

@oliver-giersch
Copy link
Contributor Author

Got it, I wasn't aware that I wasn't supposed to make changes after approval, will keep that in mind for further contributions. The rollup also occurred only a few minutes before I made the final commit. Should I close this PR now?

@RalfJung
Copy link
Member

Yeah it's better to wait for someone to bors r- before pushing again. Looks like it was a close race.^^

Yes please close this PR and make a new one for what got missed. Great that you noticed this. :)

tmandry added a commit to tmandry/rust that referenced this pull request Aug 11, 2020
…fJung

Requested changes to [*mut T|*const T]::set_ptr_value

This is a follow-up to PR rust-lang#74774 (tracking issue rust-lang#75091), acting on some change requests made after approval:

- adds `#[must_use]` attribute
- changes type of `val` pointer argument from `()` to `u8`
- adjusts documentation mentioning pointer provenance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants