rust icon indicating copy to clipboard operation
rust copied to clipboard

Tracking Issue for pointer_is_aligned

Open Gankra opened this issue 3 years ago • 7 comments

Feature gate: #![feature(pointer_is_aligned)]

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`
    pub fn is_aligned(self) -> bool where T: Sized;
    pub fn is_aligned_to(self, align: usize) -> bool;
}
// ... and the same for` *mut T`

Steps / History

  • [ ] Implementation: #95643
  • [ ] Final comment period (FCP)
  • [ ] Stabilization PR

Unresolved Questions

  • Should is_aligned_to require a power of two or should it be more flexible? (currently panics)

Gankra avatar Apr 21 '22 13:04 Gankra

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.

scottmcm avatar Aug 21 '22 00:08 scottmcm

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

Stabilization report

Implementation History

  • Initially these methods were implemented in https://github.com/rust-lang/rust/pull/95643
  • Then the implementation was improved in https://github.com/rust-lang/rust/pull/100169
  • The only unresolved question raised since then is whatever panicking in is_aligned_to on non-power of two align is acceptable.

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

https://github.com/rust-lang/rust/pull/100820 and #100823 refactored some std code into using these methods simplifying the code.

These methods were long requested (see for example https://github.com/rust-lang/rust/issues/56304) and seem to be quite nice to use. They also may have slightly better codegen, compared to naive % align implementation.

WaffleLapkin avatar Sep 04 '22 18:09 WaffleLapkin

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.

scottmcm avatar Sep 06 '22 00:09 scottmcm

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 :shrug:

WaffleLapkin avatar Sep 11 '22 16:09 WaffleLapkin

Could these methods be made const fns ?

tyler274 avatar Sep 14 '22 01:09 tyler274

@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.

WaffleLapkin avatar Sep 14 '22 20:09 WaffleLapkin

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: https://github.com/rust-lang/libs-team/issues/108

scottmcm avatar Sep 16 '22 00:09 scottmcm

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 avatar Sep 20 '22 15:09 m-ou-se

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.

WaffleLapkin avatar Sep 18 '23 17:09 WaffleLapkin

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().

MaxVerevkin avatar Jan 22 '24 14:01 MaxVerevkin

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.

saethlin avatar Feb 12 '24 00:02 saethlin

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

Gankra avatar Mar 03 '24 19:03 Gankra