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

Add convenience byte offset/check align functions to pointers #95643

Merged
merged 5 commits into from
May 19, 2022

Conversation

WaffleLapkin
Copy link
Member

This PR adds the following APIs:

impl *const T {
    // feature gates `pointer_byte_offsets` and `const_pointer_byte_offsets
    pub const unsafe fn byte_offset(self, count: isize) -> Self;
    pub const fn wrapping_byte_offset(self, count: isize) -> Self;
    pub const unsafe fn byte_offset_from(self, origin: *const T) -> isize;
    pub const unsafe fn byte_add(self, count: usize) -> Self;
    pub const unsafe fn byte_sub(self, count: usize) -> Self;
    pub const fn wrapping_byte_add(self, count: usize) -> Self;
    pub const fn wrapping_byte_sub(self, count: usize) -> Self;

    // feature gate `pointer_is_aligned`
    pub fn is_aligned(self) -> bool where T: Sized;
    pub fn is_aligned_to(self, align: usize) -> bool;
}
// ... and the same for` *mut T`

Note that all functions except is_aligned do not require T: Sized as their pointee-sized-offset counterparts.

cc @oli-obk (you may want to check that I've correctly placed consts)
cc @RalfJung

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Apr 4, 2022
@Gankra
Copy link
Contributor

Gankra commented Apr 4, 2022

I proposed and drafted the initial code for this PR, so I 100% endorse the concept.

For motivation we need look no farther than #95241, where I poked at most of the ptr<->int casts in the codebase. Patterns that showed up that I left in their "manual" state:

  • debug_asserting alignment
  • using the distance between addresses as byte-wise len/capacity
  • adjusting a pointer for alignment
  • working with *c_void, where yes we implement byte offsets (and this is a common C extension) but morally void* is !Sized (but Thin) so it's nice to be explicit about byte offsets. More generally for actual extern types this is nice to have (which is why it's actually unironically useful that these methods work on ?Sized).

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Apr 5, 2022

Are you sure we need the check & panic in is_aligned_to()? It'll probably often get used with constants so it doesn't matter, but I fell uncomfortable to add a branch that isn't really needed (though if not this branch, we'll have a branch for the modulo 0 check, but it may be better optimized out thanks to NonZeroUsize and friends and also it uses less instructions).

@Gankra
Copy link
Contributor

Gankra commented Apr 5, 2022

I was just mirroring the semantics of the more evil existing alignment-handling function, on the assumption that it was needed for... Reasons.

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Apr 6, 2022

I was just mirroring the semantics of the more evil existing alignment-handling function, on the assumption that it was needed for... Reasons.

Are you talking about align_offset()? Well it looks like it really needs that (TBH, I don't really understand this complicated piece of code, nor I tried to).

@Gankra
Copy link
Contributor

Gankra commented Apr 6, 2022

Yeah, I figured it align_offset insisted it was important I should be consistent with it, and we can always relax the panic to "it's all fine"... right? I guess we would have to say "might panic" or "it's UB" or something? So that people aren't allowed to treat it as an assert they can depend on?

@ChayimFriedman2
Copy link
Contributor

ChayimFriedman2 commented Apr 6, 2022

I guess we would have to say "might panic" or "it's UB" or something? So that people aren't allowed to treat it as an assert they can depend on?

I don't think "it's UB" is a good idea (especially not when the method is safe and I don't think it should be unsafe just because of that), but "might panic" sounds good.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 14, 2022
@Gankra
Copy link
Contributor

Gankra commented Apr 17, 2022

@WaffleLapkin is this ready to land? are there still things to do?

@WaffleLapkin
Copy link
Member Author

@Gankra I believe this is ready to land, yes.

@Gankra
Copy link
Contributor

Gankra commented Apr 21, 2022

Filed

please add these to the unstable decls. insofar as my opinion carries weight anymore, I would be happy to r+ this

@yaahc
Copy link
Member

yaahc commented Apr 21, 2022

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 21, 2022
@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 21, 2022
@yaahc yaahc assigned yaahc and unassigned joshtriplett Apr 21, 2022
@yaahc
Copy link
Member

yaahc commented Apr 29, 2022

I'm reassigning this PR because I'm taking a break from the review rotation for a little while. Thank you for your patience.

r? rust-lang/libs-api

Since they work on byte pointers (by `.cast::<u8>()`ing them), there is
no need to know the size of `T` and so there is no need for `T: Sized`.

The `is_aligned_to` is similar, though it doesn't need the _alignment_
of `T`.
Apparently LLVM is unable to understand that if count_ones() == 1 then self != 0.
Adding `assume(align != 0)` helps generating better asm:
https://rust.godbolt.org/z/ja18YKq91
@WaffleLapkin
Copy link
Member Author

I wander if we also need byte_sub_ptr

@joshtriplett
Copy link
Member

Looks reasonable to me!

We can always bikeshed names later, but I personally think these names look good.

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2022

📌 Commit 03d4569 has been approved by joshtriplett

@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 May 18, 2022
@bors
Copy link
Contributor

bors commented May 18, 2022

⌛ Testing commit 03d4569 with merge d8a3fc4...

@bors
Copy link
Contributor

bors commented May 19, 2022

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing d8a3fc4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2022
@bors bors merged commit d8a3fc4 into rust-lang:master May 19, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 19, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d8a3fc4): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@WaffleLapkin WaffleLapkin deleted the ptr_convenience branch May 19, 2022 14:04
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
1. change `PtrMut::as_ptr(self)` and `OwnedPtr::as_ptr(self)` to take `&self`, otherwise printing the pointer will prevent doing anything else afterwards
2. make all `as_ptr` methods safe. There's nothing unsafe about obtaining a pointer, these kinds of methods are safe in std as well [str::as_ptr](https://doc.rust-lang.org/stable/std/primitive.str.html#method.as_ptr), [Rc::as_ptr](https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr)
3. rename `offset`/`add` to `byte_offset`/`byte_add`. The unprefixed methods in std add in increments of `std::mem::size_of::<T>`, not in bytes. There's a PR for rust to add these byte_ methods rust-lang/rust#95643 and at the call site it makes it much more clear that you need to do `.byte_add(i * layout_size)` instead of `.add(i)`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 6, 2022
…o, r=workingjubilee

Optimize `pointer::as_aligned_to`

This PR replaces `addr % align` with `addr & align - 1`, which is correct due to `align` being a power of two.

Here is a proof that this makes things better: [[godbolt]](https://godbolt.org/z/Wbq3hx6YG).

This PR also removes `assume(align != 0)`, with the new impl it does not improve anything anymore ([[godbolt]](https://rust.godbolt.org/z/zcnrG4777), [[original concern]](rust-lang#95643 (comment))).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 20, 2022
Add pointer masking convenience functions

This PR adds the following public API:
```rust
impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T
```
This is equivalent to `ptr.map_addr(|a| a & mask)` but also uses a cool llvm intrinsic.

Proposed in rust-lang#95643 (comment)

cc `@Gankra` `@scottmcm` `@RalfJung`

r? rust-lang/libs-api
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 20, 2022
Add pointer masking convenience functions

This PR adds the following public API:
```rust
impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T
```
This is equivalent to `ptr.map_addr(|a| a & mask)` but also uses a cool llvm intrinsic.

Proposed in rust-lang#95643 (comment)

cc ``@Gankra`` ``@scottmcm`` ``@RalfJung``

r? rust-lang/libs-api
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2022
Add pointer masking convenience functions

This PR adds the following public API:
```rust
impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T
```
This is equivalent to `ptr.map_addr(|a| a & mask)` but also uses a cool llvm intrinsic.

Proposed in rust-lang#95643 (comment)

cc `@Gankra` `@scottmcm` `@RalfJung`

r? rust-lang/libs-api
antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request Aug 28, 2022
Add pointer masking convenience functions

This PR adds the following public API:
```rust
impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T
```
This is equivalent to `ptr.map_addr(|a| a & mask)` but also uses a cool llvm intrinsic.

Proposed in rust-lang/rust#95643 (comment)

cc `@Gankra` `@scottmcm` `@RalfJung`

r? rust-lang/libs-api
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Aug 30, 2022
Add pointer masking convenience functions

This PR adds the following public API:
```rust
impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T
```
This is equivalent to `ptr.map_addr(|a| a & mask)` but also uses a cool llvm intrinsic.

Proposed in rust-lang/rust#95643 (comment)

cc `@Gankra` `@scottmcm` `@RalfJung`

r? rust-lang/libs-api
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
…ingjubilee

Optimize `pointer::as_aligned_to`

This PR replaces `addr % align` with `addr & align - 1`, which is correct due to `align` being a power of two.

Here is a proof that this makes things better: [[godbolt]](https://godbolt.org/z/Wbq3hx6YG).

This PR also removes `assume(align != 0)`, with the new impl it does not improve anything anymore ([[godbolt]](https://rust.godbolt.org/z/zcnrG4777), [[original concern]](rust-lang/rust#95643 (comment))).
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
Add pointer masking convenience functions

This PR adds the following public API:
```rust
impl<T: ?Sized> *const T {
    fn mask(self, mask: usize) -> *const T;
}

impl<T: ?Sized> *mut T {
    fn mask(self, mask: usize) -> *const T;
}

// mod intrinsics
fn mask<T>(ptr: *const T, mask: usize) -> *const T
```
This is equivalent to `ptr.map_addr(|a| a & mask)` but also uses a cool llvm intrinsic.

Proposed in rust-lang/rust#95643 (comment)

cc `@Gankra` `@scottmcm` `@RalfJung`

r? rust-lang/libs-api
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
1. change `PtrMut::as_ptr(self)` and `OwnedPtr::as_ptr(self)` to take `&self`, otherwise printing the pointer will prevent doing anything else afterwards
2. make all `as_ptr` methods safe. There's nothing unsafe about obtaining a pointer, these kinds of methods are safe in std as well [str::as_ptr](https://doc.rust-lang.org/stable/std/primitive.str.html#method.as_ptr), [Rc::as_ptr](https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr)
3. rename `offset`/`add` to `byte_offset`/`byte_add`. The unprefixed methods in std add in increments of `std::mem::size_of::<T>`, not in bytes. There's a PR for rust to add these byte_ methods rust-lang/rust#95643 and at the call site it makes it much more clear that you need to do `.byte_add(i * layout_size)` instead of `.add(i)`
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-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.