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

Tracking Issue for pointer_is_aligned_to #96284

Open
5 of 7 tasks
Gankra opened this issue Apr 21, 2022 · 12 comments
Open
5 of 7 tasks

Tracking Issue for pointer_is_aligned_to #96284

Gankra opened this issue Apr 21, 2022 · 12 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Gankra
Copy link
Contributor

Gankra commented Apr 21, 2022

Feature gate: #![feature(pointer_is_aligned)]
Feature gate: #![feature(pointer_is_aligned_to)]

This is a tracking issue for ptr.is_aligned() and ptr.is_aligned_to(alignment).

Public API

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

impl <T: ?Sized> NonNull<T> {
    pub const fn is_aligned_to(self, align: usize) -> bool;
}

Steps / History

pointer_is_aligned:

pointer_is_aligned_to:

Unresolved Questions

  • Should is_aligned_to require a power of two or should it be more flexible? (currently panics)
@Gankra Gankra added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Apr 21, 2022
@scottmcm
Copy link
Member

For the unresolved question: another option might be to expose https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/core/mem/valid_align/struct.ValidAlign.html and use it as the parameter type.

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Sep 4, 2022

I want to propose to stabilize these methods! (can someone open an fcp?)

Stabilization report

Implementation History

API Summary

impl *const T {
    pub fn is_aligned(self) -> bool where T: Sized;
    pub fn is_aligned_to(self, align: usize) -> bool;
}

// ... and the same for` *mut T`

Experience Report

#100820 and #100823 refactored some std code into using these methods simplifying the code.

These methods were long requested (see for example #56304) and seem to be quite nice to use. They also may have slightly better codegen, compared to naive % align implementation.

@scottmcm
Copy link
Member

scottmcm commented Sep 6, 2022

I think the unresolved question needs an answer, not just a mention.

As I brought up earlier, I think it'd be really nice to have a type for valid alignments. The other two options are both unfortunate, IMHO: needing to assert!(align.is_power_of_two()) in something that would otherwise be a trivial bit of code, or just going 🤷 and returning garbage if the input isn't a power of two.

@WaffleLapkin
Copy link
Member

So, our options for resolving the unresolved question:

  1. Leave as-is, assert!(align.is_power_of_two())
    • Pros: notifies users of about invalid usages
    • Cons: in some cases adds an additional check/panic
  2. Make behavior is case of !align.is_power_of_two() implementation defined (non-sensible result, debug_assert!, etc)
    • Pros: no performance penalty, ever
    • Cons: does not notify users about invalid usages
  3. Introduce a type that holds always valid alignment
    • Pros: impossible to use incorrectly, such type can improve other APIs as well
    • Cons: bigger API surface, users need to move to new APIs (align_of2::<T>() -> Align?...), given an usize (e.g. from an external library) you need a trailing_zeros call to convert back

I'm leaning towards 2, but that's just me 🤷

@tyler274
Copy link

tyler274 commented Sep 14, 2022

Could these methods be made const fns ?

@WaffleLapkin
Copy link
Member

@tyler274 they can't, CTFE engine (compile time function execution) does not (necessarily) have a notion of "address" of a pointer. Since you can't take the address (e.g. it's UB to transmute::<*const (), usize>() in CTFE) you also can't check if it's divisible by something.

@scottmcm
Copy link
Member

scottmcm commented Sep 16, 2022

given an usize (e.g. from an external library) you need a trailing_zeros call to convert back

That's assuming it would be stored as log_2(align), but that's not necessary. In fact, the type that Layout uses internally today stores it as the power of two -- since that's convenient for things like the usual &(align-1) trick. So using a usize from an external library would be a "free" Alignment::new_unchecked call (if you can trust that other library).

EDIT: I opened an ACP to add an Alignment newtype to core: rust-lang/libs-team#108

@m-ou-se m-ou-se added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 20, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Sep 20, 2022

Question from the libs-api meeting: How often would one use is_aligned_to given that is_aligned exists? (Or maybe a is_aligned_for::<T>() might suffice for most cases?)

How often does one want to check a calculated alignment rather than a constant alignment? With e.g. .is_aligned_to(16), the check would be optimized away, making the usize argument fine. Using a separate Alignment type would only add extra verbosity.

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 20, 2022
@WaffleLapkin
Copy link
Member

Status update: I originally planned to use grep.app or an alternative to gather stats about "is aligned" checks in the wild. But... I did not have enough courage to finish the work and now I simply do not have the capacity for that.

If anyone wants to take this from here (and answer t-libs-api questions in the above post) — feel free to.

@MaxVerevkin
Copy link

Or maybe a is_aligned_for::<T>() might suffice for most cases?

Can is_aligned() be stabilized before is_aligned_to()? Especially because it can be used as cast::<T>().is_aligned().

@saethlin
Copy link
Member

Question from the libs-api meeting: How often would one use is_aligned_to given that is_aligned exists? (Or maybe a is_aligned_for::<T>() might suffice for most cases?)

The standard library currently only uses is_aligned_to to implement is_aligned.

I searched grep.app as Waffle suggests above, and I only found these callers that could not easily port to .is_aligned() or .cast().is_aligned().

https://github.com/jamwaffles/linuxcnc-hal-rs/blob/0aac97869278fd90a7e3857226cb8ebd4ce89123/linuxcnc-hal/src/hal_pin/hal_pin.rs#L33-L45
https://github.com/SFBdragon/talc/blob/deb93cf518da25f9eb61cd7154c68f0e3f6be90f/src/talck.rs#L132-L138
https://github.com/twizzler-operating-system/twizzler/blob/4ec5a30d1348d0af395dc018d951658a2c00434a/src/kernel/src/memory/pagetables/table.rs#L47-L50

But also, those 3 projects are not even using the unstable is_aligned_to.

At this time, I do not think that the is_aligned_to method is suitably motivated.

Gankra added a commit to Gankra/rust that referenced this issue Mar 3, 2024
Per rust-lang#96284 (comment)
this API is a mess and is regarded as insufficiently motivated. Removing
it from the public API is potentially the only blocker on stabilizing
its pleasant brother .is_aligned().

I initially just deleted it completely but it's since become Heavily Used
for the purposes of assert_unsafe_precondition, and there's clearly
Some Stuff going on with that API that I have no interest in poking.
Gankra added a commit to Gankra/rust that referenced this issue Mar 3, 2024
Per rust-lang#96284 (comment)
this API is a mess and is regarded as insufficiently motivated. Removing
it from the public API is potentially the only blocker on stabilizing
its pleasant brother .is_aligned().

I initially just deleted it completely but it's since become Heavily Used
for the purposes of assert_unsafe_precondition, and there's clearly
Some Stuff going on with that API that I have no interest in poking.
Gankra added a commit to Gankra/rust that referenced this issue Mar 3, 2024
Per rust-lang#96284 (comment)
this API is a mess and is regarded as insufficiently motivated. Removing
it from the public API is potentially the only blocker on stabilizing
its pleasant brother .is_aligned().

I initially just deleted it completely but it's since become Heavily Used
for the purposes of assert_unsafe_precondition, and there's clearly
Some Stuff going on with that API that I have no interest in poking.
Gankra added a commit to Gankra/rust that referenced this issue Mar 3, 2024
Per rust-lang#96284 (comment)
this API is a mess and is regarded as insufficiently motivated. Removing
it from the public API is potentially the only blocker on stabilizing
its pleasant brother .is_aligned().

I initially just deleted it completely but it's since become Heavily Used
for the purposes of assert_unsafe_precondition, and there's clearly
Some Stuff going on with that API that I have no interest in poking.
Gankra added a commit to Gankra/rust that referenced this issue Mar 3, 2024
Per rust-lang#96284 (comment)
this API is a mess and is regarded as insufficiently motivated. Removing
it from the public API is potentially the only blocker on stabilizing
its pleasant brother .is_aligned().

I initially just deleted it completely but it's since become Heavily Used
for the purposes of assert_unsafe_precondition, and there's clearly
Some Stuff going on with that API that I have no interest in poking.
@Gankra
Copy link
Contributor Author

Gankra commented Mar 3, 2024

I propose we split the feature into two and stabilize the perfectly inoffensive ptr.is_aligned(), which handles 95% of usecases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants