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 const alloc::Layout #67521

Open
lukaslueg opened this issue Dec 22, 2019 · 12 comments
Open

Tracking issue for const alloc::Layout #67521

lukaslueg opened this issue Dec 22, 2019 · 12 comments
Labels
A-allocators Area: Custom and system allocators A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@lukaslueg
Copy link
Contributor

lukaslueg commented Dec 22, 2019

Feature const_alloc_layout makes these methods on Layout const:

impl Layout {
  fn for_value<T>(t: &T) -> Layout
  fn align_to(&self, align: usize) -> Result<Layout, LayoutError>
  fn pad_to_align(&self) -> Layout
  fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutError>
  fn array<T>(n: usize) -> Result<Layout, LayoutError>
}

Making more methods of alloc::Layout const allows computing alignment/size information for arbitrary (sized) types at compile-time. While mem::size_of and mem::align_of are already const and Layout is solely based on those, there is no guarantee that a const derived from these functions will be exactly the same as what is used in a call to alloc::alloc. Constifying Layout makes this possible.

Implemented in #67494

Blocked on #46571 (needed for for_value)

@jonas-schievink jonas-schievink added A-allocators Area: Custom and system allocators A-const-fn C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 22, 2019
lukaslueg added a commit to lukaslueg/rust that referenced this issue Jan 11, 2020
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Jul 30, 2020
@rustonaut
Copy link

Shouldn't most methods of Layout be const?

This would allow certain usages where a custom derive + const is used to calculate field offsets of repr(C) structs at compiler time. Which can be useful to create pointers to fields without ever having a reference to the struct. Which should be useful if working with unaligned structs or creation of custom DST (once we have a way to create fat-pointers by hand). There are probably other usages, too I think.

So following additional methods should become const too in the future in my opinion:

method hindered by can be resolved from >1.47.0 on note
align_to cmp::max::<usize> yes why was it crossed out above?
pad_to_align Result.unwrap() yes The unsafe Layout constructor should be usable but I need to double check this
repeat usize.checked_mul + Option.ok_or + ? yes unsatable
extend cmp::max::<usize> + usize.checked_add + Option.ok_or + ? yes -
repeat_packed usize.checked_mul + Option.ok_or + ? yes unstable
extend_packed checked_add + Option.ok_or + ? yes unstable
array debug_assert_eq! + repeat + ? + pad_to_align ? -

The problems can be resolved as following

problem solutions note
cmp::max::<usize> internal helper const fn align_max(align1: usize, algin2: usize) { ... } max is generic and I'm not sure if we can have something like const specialization but a internal const helper function does the job, too
usize.checked_mul stable on beta of 1.47 -
usize.checked_add stable on beta of 1.47 -
Result.unwrap() in pad_to_align we probably can use the unsafe constructor of Layout safely
Option.ok_or method can be const implemented in the beta of 1.47, alternative a temporary helper method/explicit match could be used to stabilize const here independent of const methods on Option
? at least from 1.47 one we can use a manual match + early return see below
debug_assert_eq! isn't really needed in array tbh do we have a "debug_assert if we don't run const evaluation method?"

The main hindrance I see is that not using ? would make the code more verbose.
All places where it's used do not do any error conversion so all of them can easily
be done in const (at least from 1.47 onward, maybe earlier). So it mostly boils down to
a match + early return still in extend half of the code lines use ?. We could temporary
have a const_try macro or similar. Best probably would be to allow ? usage in const use-cases where no type conversion is done e.g. option.ok_or(LayoutErr { private: {} })?. Most usages of ? are a checked add/mull followed by a ok_or mapping and a ?. Given that we maybe can temporary be ok with a helper macro like bail_on_overflow!{ }. E.g. for extend:

pub fn extend(&self, next: Self) -> Result<(Self, usize), LayoutErr> {
        let new_align = max_align(self.align(), next.align());
        let pad = self.padding_needed_for(next.align());

        let offset = bail_on_overflow!{ self.size().checked_add(pad) };
        let new_size = bail_on_overflow!{ offset.checked_add(next.size()) };

        match Layout::from_size_align(new_size, new_align) {
            Ok(layout) => Ok((layout, offset)),
            other => other
        }
}
///------------- temporary helpers to be used by `extend` and other `Layout` methods
macro_rules! bail_on_overflow!{
    ($expr:expr) => ({
        match $expr {
            None => return Err(LayoutErr { private: () }),
            Some(x) => x
        }
    });
}

const fn max_align(align1: usize, align2: usize) -> usize {
    if align1 < align2 { align2 }
    else { align1 } 
}

Not super nice but it would do for now I think.


Lastly let's try to reason why pad_to_algin should be able to use the unsafe constructor instead of unwrap() (and with that can be made const). Constraints for Layout::from_size_align_unchecked:

  • align != 0
  • align power of 2
  • size when rounded up to the nearest multiple of align must not overflow

And:

pub fn pad_to_align(&self) -> Layout {
        let pad = self.padding_needed_for(self.align());
        let new_size = self.size() + pad;
        Layout::from_size_align(new_size, self.align()).unwrap()
}

As the original source code already mentions new_size can not overflow as
pad is just "rounding" it up the the next multiple of self.align(). But if that is
the case then new_size will always be valid afterward as it will be a multiple of
it's alignment and as such the "nearest multiple of align" is it self. Furthermore
algin is not touched and was valid before.

So following should be fine:

pub const fn pad_to_align(&self) -> Layout {
        let pad = self.padding_needed_for(self.align());
        // [...] 
        let new_size = self.size() + pad;
        // Safe: [...]
        unsafe { Layout::from_size_align_unchecked(new_size, align) }
}

@rustonaut
Copy link

Open questions:

  • Did I overlook anything?
  • which solution for ?/Option.ok_or?
  • how to handle the debug_assert_eq! in array?

m-ou-se added a commit to m-ou-se/rust that referenced this issue Nov 22, 2020
Stabilize alloc::Layout const functions

Stabilizes rust-lang#67521. In particular the following stable methods are stabilized as `const fn`:

* `size`
* `align`
* `from_size_align`

Stabilizing `size` and `align` should not be controversial as they are simple (usize and NonZeroUsize) fields and I don't think there's any reason to make them not const compatible in the future. That being true, the other methods are trivially `const`. The only other issue being returning a `Result` from a `const fn` but this has been made more usable by recent stabilizations.
@joshtriplett

This comment has been minimized.

Manishearth added a commit to Manishearth/rust that referenced this issue Nov 18, 2022
Constify remaining `Layout` methods

Makes the methods on `Layout` that aren't yet unstably const, under the same feature and issue, rust-lang#67521. Most of them required no changes, only non-trivial change is probably constifying `ValidAlignment` which may affect rust-lang#102072
Manishearth added a commit to Manishearth/rust that referenced this issue Nov 18, 2022
Constify remaining `Layout` methods

Makes the methods on `Layout` that aren't yet unstably const, under the same feature and issue, rust-lang#67521. Most of them required no changes, only non-trivial change is probably constifying `ValidAlignment` which may affect rust-lang#102072
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Nov 19, 2022
Constify remaining `Layout` methods

Makes the methods on `Layout` that aren't yet unstably const, under the same feature and issue, rust-lang#67521. Most of them required no changes, only non-trivial change is probably constifying `ValidAlignment` which may affect rust-lang#102072
Manishearth added a commit to Manishearth/rust that referenced this issue Nov 22, 2022
Constify remaining `Layout` methods

Makes the methods on `Layout` that aren't yet unstably const, under the same feature and issue, rust-lang#67521. Most of them required no changes, only non-trivial change is probably constifying `ValidAlignment` which may affect rust-lang#102072
@ChrisDenton
Copy link
Member

ChrisDenton commented Feb 17, 2023

The following stable functions are unstably const under this issues:

fn for_value<T>(t: &T) -> Layout
fn align_to(&self, align: usize) -> Result<Layout, LayoutError>
fn pad_to_align(&self) -> Layout
fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutError>
fn array<T>(n: usize) -> Result<Layout, LayoutError>

The following are unstable functions that are also unstably const:

unsafe fn for_value_raw<T>(t: *const T) -> Layout
fn padding_needed_for(&self, align: usize) -> usize
fn repeat(&self, n: usize) -> Result<(Layout, usize), LayoutError>
fn repeat_packed(&self, n: usize) -> Result<Layout, LayoutError>
fn extend_packed(&self, next: Layout) -> Result<Layout, LayoutError>

I think the unstable functions should simply be const without a separate const unstable issue? const fn is now in a good enough shape that I believe a decision on whether or not to constify a Layout function can be made a the same time as stabilizing the function itself. There's no need to track it separately.

@matthieu-m

This comment has been minimized.

@RalfJung

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented Nov 1, 2024

I think the unstable functions should simply be const without a separate const unstable issue? const fn is now in a good enough shape that I believe a decision on whether or not to constify a Layout function can be made a the same time as stabilizing the function itself. There's no need to track it separately.

Yes, good point. I am implementing that in #132455.

jieyouxu added a commit to jieyouxu/rust that referenced this issue Nov 1, 2024
…lnay

make const_alloc_layout feature gate only about functions that are already stable

The const_alloc_layout feature gate has two kinds of functions: those that are stable, but not yet const-stable, and those that are fully unstable.

I think we should split that up. So this PR makes const_alloc_layout just about functions that are already stable but waiting for const-stability; all the other functions now have their constness guarded by the gate that also guards their regular stability.

Cc rust-lang#67521
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 1, 2024
…lnay

make const_alloc_layout feature gate only about functions that are already stable

The const_alloc_layout feature gate has two kinds of functions: those that are stable, but not yet const-stable, and those that are fully unstable.

I think we should split that up. So this PR makes const_alloc_layout just about functions that are already stable but waiting for const-stability; all the other functions now have their constness guarded by the gate that also guards their regular stability.

Cc rust-lang#67521
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 2, 2024
Rollup merge of rust-lang#132455 - RalfJung:const_alloc_layout, r=dtolnay

make const_alloc_layout feature gate only about functions that are already stable

The const_alloc_layout feature gate has two kinds of functions: those that are stable, but not yet const-stable, and those that are fully unstable.

I think we should split that up. So this PR makes const_alloc_layout just about functions that are already stable but waiting for const-stability; all the other functions now have their constness guarded by the gate that also guards their regular stability.

Cc rust-lang#67521
@RalfJung
Copy link
Member

@rust-lang/libs-api the remaining const fn tracked here are all already stable. So I suggest we const-stabilize them as well:

impl Layout {
  fn for_value<T>(t: &T) -> Layout
  fn align_to(&self, align: usize) -> Result<Layout, LayoutError>
  fn pad_to_align(&self) -> Layout
  fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutError>
  fn array<T>(n: usize) -> Result<Layout, LayoutError>
}

for_value is blocked on size/align_of_val, which passed FCP in #46571. I'm waiting for confirmation that this won't interact poorly with other ongoing efforts; worst-case we can split that function out and stabilize the rest. So I don't think this has to block FCP here.

@RalfJung RalfJung added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 27, 2024
@dtolnay
Copy link
Member

dtolnay commented Nov 27, 2024

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 27, 2024

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 27, 2024
@dtolnay dtolnay removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Nov 27, 2024
@RalfJung RalfJung added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) and removed A-const-fn labels Dec 1, 2024
@RalfJung
Copy link
Member

@BurntSushi @joshtriplett @m-ou-se FCP checkbox reminder. :) These are all simple stable fn becoming const fn.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 14, 2024
@rfcbot
Copy link

rfcbot commented Dec 14, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. Libs-Tracked Libs issues that are tracked on the team's project board. 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

10 participants