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 a specific type for pointer alignment #108

Closed
scottmcm opened this issue Sep 17, 2022 · 4 comments
Closed

Add a specific type for pointer alignment #108

scottmcm opened this issue Sep 17, 2022 · 4 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@scottmcm
Copy link
Member

Proposal

Problem statement

We often use usize to represent the alignment of a type. However, on a 64-bit machine there are only 64 valid alignments, which is under 0.000_000_000_000_000_35% of the valid values of the type -- and, realistically, there are only ≈10 useful alignments. (Sure, it's legal to write repr(align(1152921504606846976)), but nobody ever would.)

It's also common to use the special property of valid alignments -- that they're powers of two -- to optimize various operations in ways that produce meaningless results for other values. For example, ptr % align is written as ptr & (align - 1), but that has "no meaning" for invalid alignment values.

With a custom type for alignments we can provide APIs that can safely take advantage of these properties.

Motivation, use-cases

Layout::padding_needed_for takes align: usize, and thus needs the note

The return value of this function has no meaning if align is not a power-of-two.

if it took a specific alignment type, that wouldn't be a concern. And needing to call .pading_needed_for(Alignment::of::<T>()) (say) instead of .padding_needed_for(align_of::<T>()) is no hardship for the caller.

Similarly, <*const T>::is_aligned_to also takes align: usize, and its tracking issue (rust-lang/rust#96284) has an open question about how to handle invalid alignments passed to it.

Already today, Layout requires that its alignment be a valid alignment. Having a type with the invariant that it's a valid alignment allowed making a method safe when it would have otherwise required unsafe (rust-lang/rust#99117 (comment)).

Solution sketches

I propose something like the following type:

// in core::ptr

pub struct Alignment();

impl Copy + Clone + Ord + PartialOrd + Eq + PartialEq + Hash + Debug {}

impl Alignment {
    /// Alignment of `T`
    pub const fn of<T>() -> Self;

    /// Checked constructor from `usize`
    pub const fn new(align: usize) -> Option<Self>;

    /// Unchecked constructor from `usize`
    pub const unsafe fn new_unchecked(align: usize) -> Self;

    /// Get the raw value out again
    pub const fn as_usize(self) -> usize;
}

There's a variety of additional things that could go along with that, but aren't essential from the start, like

  • Getting a NonZeroUsize from the alignment
  • Various From/TryFroms to go with the news
  • A log2 to turn alignments into 0, 1, 2, 3, … instead of 1, 2, 4, 8, …
  • *const T: Mod<Alignment, Output = usize>
  • Additional methods on Layout to expose the internal Alignment and create from an Alignment (thus avoiding the align.is_power_of_two() check in its safe constructor).

Links and related work

A type for this already exists internally in core: https://github.com/rust-lang/rust/blob/master/library/core/src/mem/valid_align.rs

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 20, 2022

This seems reasonable.

The only thing I'm wondering is how specific this type should be to alignment of types, versus be more like a more general "PowerOfTwoUsize" (similar to our NonZero types). However, that very quickly explodes into all kind of sets of integers, so we should probably not go that route.

Splitting the 'is power of two'-check off into a separate type seems like a great thing. I like how the value of Alignment::of<T>() would not need any subsequent checking.

@rustbot second

@rustbot
Copy link
Collaborator

rustbot commented Sep 20, 2022

Error: The feature major_change is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@scottmcm
Copy link
Member Author

However, that very quickly explodes into all kind of sets of integers, so we should probably not go that route.

Right. I really didn't want to get into "should there be PowerOfTwoI16?" discussions, just the one domain-specific type.

Thanks for the second! I'll get started on a PR.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 9, 2022
Add `ptr::Alignment` type

Essentially no new code here, just exposing the previously-`pub(crate)` `ValidAlign` type under the name from the ACP.

ACP: rust-lang/libs-team#108
Tracking Issue: rust-lang#102070

r? `@ghost`
JohnTitor added a commit to JohnTitor/rust that referenced this issue Oct 9, 2022
Add `ptr::Alignment` type

Essentially no new code here, just exposing the previously-`pub(crate)` `ValidAlign` type under the name from the ACP.

ACP: rust-lang/libs-team#108
Tracking Issue: rust-lang#102070

r? ``@ghost``
@scottmcm
Copy link
Member Author

This is merged to nightly, so I'll close the ACP.

Please reopen if it should stay around.

thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Add `ptr::Alignment` type

Essentially no new code here, just exposing the previously-`pub(crate)` `ValidAlign` type under the name from the ACP.

ACP: rust-lang/libs-team#108
Tracking Issue: rust-lang/rust#102070

r? ``@ghost``
@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants