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_align_offset #90962

Closed
2 of 5 tasks
WaffleLapkin opened this issue Nov 16, 2021 · 20 comments · Fixed by #132423
Closed
2 of 5 tasks

Tracking Issue for const_align_offset #90962

WaffleLapkin opened this issue Nov 16, 2021 · 20 comments · Fixed by #132423
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

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Nov 16, 2021

Feature gate: #![feature(const_align_offset)]

This is a tracking issue for <*const _>::align_offset and <*mut _>::align_offset functions marked as const fn

Public API

impl<T> *const T {
    pub const fn align_offset(self, align: usize) -> usize;
}

impl<T> *mut T {
    pub const fn align_offset(self, align: usize) -> usize;
}

impl <T> NonNull<T> {
    pub const fn align_offset(self, align: usize) -> usize;
}

Steps / History

Unresolved Questions

@WaffleLapkin WaffleLapkin added 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. labels Nov 16, 2021
@RalfJung
Copy link
Member

This allows to distinguish compiletime and runtime function execution, see #90607

I agree this is a problem, but sadly the link seems to just go to the PR (links to collapsed discussions don't seem to work) so I have no idea what this has to do with the UTF8 methods -- could someone explain the connection? Is it just because align_offset is indirectly being called there? Or can the UTF8 methods themselves be used to do a CTFE/runtime distinction?

@WaffleLapkin
Copy link
Member Author

@RalfJung I've unresolved the discussion, now the link should be working (it's the first discussion in the PR).

TL;DR: originally in #90607 I've used const_eval_select in the methods that check utf8 validity to make them available in const fn. const_eval_select just replaced call to align_offset to usize::MAX there, this is valid per the documentation of align_offset - it can spuriously return usize::MAX. In the discussion, it was argued that the same reasoning can be applied to any correct use of align_offset.

@RalfJung
Copy link
Member

any correct use of align_offset

Emphasis mine. :) Safe code can easily use align_offset incorrectly. So this is not something we can rely on for safety concerns.

@phip1611
Copy link

phip1611 commented Mar 10, 2022

@WaffleLapkin Can you please explain why the const implementation always returns usize::MAX? Please look at the following example that does not work because of this limitation:

#![feature(const_align_offset)]
#![feature(const_mut_refs)]

#[repr(align(4096))]
struct PageAligned<T>(T);
static mut MEMORY: PageAligned<[u8; 1]> = PageAligned([0; 1]);

struct Foo;
impl Foo {
    const fn new(mem: &[u8]) -> Self {
        assert!(mem.as_ptr().align_offset(4096) == 0, "memory must be page aligned!");
        Self
    }
}
static FOO: Foo = unsafe {Foo::new(&mut MEMORY.0)};


fn main() {}

This currently prevents me from doing an alignment check inside the constructor. Is this intended? What is a possible solution?

@WaffleLapkin
Copy link
Member Author

@phip1611 during CTFE (compile time function execution) memory is organized differently, than while runtime execution Specifically it doesn't really have addresses or alignment, so there is no way to implement align_offset in a sensible way. (PS I'm not an expert in CTFE, so correct me if I'm wrong)

Also, see the documentation:

If it is not possible to align the pointer, the implementation returns usize::MAX. It is permissible for the implementation to always return usize::MAX. Only your algorithm’s performance can depend on getting a usable offset here, not its correctness.

align_offset is expected to be used in cases where you can process adjacent elements of an array at the same time, if they are aligned, but you can also process them one-by-one otherwise. I don't think that your use case fits here.

Also note that this function is unlikely to be stabilized in it's current form as a const fn since it allows to distinguish runtime and compile time execution.

@phip1611
Copy link

Thanks for the quick reply @WaffleLapkin. What do you think about the use case shown in my example? Shouldn't this be possible in the future? Do you think it is worth it to open a dedicated ticket for this?

@WaffleLapkin
Copy link
Member Author

@phip1611 I don't think this can be ever possible, addresses of static variables are not known at compile time and are even randomized at runtime:

; bat t.rs
───────┬────────────────────────────────────────────────────────────────────────
       │ File: t.rs
───────┼────────────────────────────────────────────────────────────────────────
   1   │ fn main() {
   2   │     static A: i32 = 0;
   3   │     let r: &'static _ = &A;
   4   │     dbg!(r as *const _ as usize);
   5   │ }
───────┴────────────────────────────────────────────────────────────────────────
; rustc t.rs
; ./t
[t.rs:4] r as *const _ as usize = 94587865219336
; ./t
[t.rs:4] r as *const _ as usize = 94617522643208
; ./t
[t.rs:4] r as *const _ as usize = 93992536973576

(screenshot)

@phip1611
Copy link

Rust could deduce the alignment somehow from a #[repr(align(4096))] attribute perhaps?

@RalfJung
Copy link
Member

It is true that in this specific case, the CTFE implementation of align_offset could be made smarter to take into account the alignment of the static.

However, align_offset is also not meant for doing alignment checks, as the docs clearly indicate. If const-compatible alignment checks are something we want to provide, IMO it should be a separate function (and one that clearly documents that during CTFE, this is a conservative check since the actual address of allocations is not known yet). Being able to distinguish compile-time and run-time behavior is not necessarily a blocker, though it should be carefully considered.

Cc @rust-lang/wg-const-eval

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Mar 10, 2022

@phip1611 aligning everything to the minimum alignment wouldn't work either 😄

Consider this example:

static A: [u8; 2] = [0, 0];

assert!(
    addr_of!(A[0]).align_offset(2) == 0
    || addr_of!(A[1]).align_offset(2) == 0
);

This will pass in runtime, because either &A[0] or &A[1] must have an even address. However, if we think that every object is aligned to the minimum possible alignment (align_of::<T>()), then both &A[0] or &A[1] will have alignment = 1, so .align_offset(2) will return 1 for both (which is very unhelpful, because normally addr_of!(A[0]).align_offset(2) == 1 would tell us that A[0 + 1] is 2-aligned).

You can suggest aligning outer-most objects at minimum ([u8; 2] in this case) and then calculating alignment of other objects inside accordingly (&A[0] 1-aligned, &A[1] 2-aligned, etc), but this will, again, make runtime and compile time behaviour different, since we don't know which of &A[0] and &A[1] will get 2-aligned at runtime.

Upd: didn't see Ralfs comment when writing all of this, but similar problems would arise with "const-compatible alignment checks"

@RalfJung
Copy link
Member

@WaffleLapkin definitely, not all cases will work. A [u8; N] static will never have a member that is const-time-aligned to more than 1. That's why I said such a check would be conservative.

The specific case @phip1611 asked about, however, would work. In fact, ptr.align_offset(X) == 0 under the smarter align_offset would behave exactly the same as the conservative const-compatible alignment check. However, a dedicated is_aligned or so might still be better since

  • it is more efficient at runtime as it has to do a lot less work
  • it avoids using align_offset in a way that is clearly warned against in the docs

@RalfJung
Copy link
Member

I wonder... is there even a good usecase for align_offset in const? We've had a lot of complaints over the years about the strange API contract of align_offset (and, by extension, align_to); making them work in const is a major motivation for that. (There are other motivations, but I think we can resolve those differently.)

If we are okay with saying that align_offset will not be const (and one should use some version of const_eval_select instead), then we could clean up a bunch of hacks from the CTFE engine and from the align_offset/align_to docs.

(I'm aware that const is_aligned is currently implemented via align_offset, but that could just become an intrinsic instead.)

Cc @rust-lang/wg-const-eval

@WaffleLapkin
Copy link
Member Author

Not sure what constitutes "good", but the usecase for align_offset in const is pretty clear imo — doing simd-style (not actually simd) checks when possible at runtime, while still supporting doing the slow path in const. There are a few examples in std:

In my opinion what we should do is explicitly say that align_offset and align_to behave well at runtime and weirdly in CTFE. I think this should solve the complaints @RalfJung mentions, while not invalidating const use-cases.

Alternatively we could indeed provide some more powerful way of doing runtime-only optimizations in const...

Also, it's surprising for me to learn about hacks, the initial implementation just always returned usize::MAX (which is perfectly fine for all usecases I mentioned above), did this change since then?...

@RalfJung
Copy link
Member

RalfJung commented Nov 12, 2023

Also, it's surprising for me to learn about hacks, the initial implementation just always returned usize::MAX (which is perfectly fine for all usecases I mentioned above), did this change since then?...

Yes: #102795

In my opinion what we should do is explicitly say that align_offset and align_to behave well at runtime and weirdly in CTFE. I think this should solve the complaints @RalfJung mentions, while not invalidating const use-cases.

The evidence indicates that people expect align_to to return non-empty middle parts in a number of situations, no matter what the documentation says. I don't see why that would change when they do it inside const.

@RalfJung
Copy link
Member

There are a few examples in std:

It's interesting that these examples all use align_offset, not align_to. In fact align_to is not even unstably const so far.

Would those examples use align_to if it was const, or is there some reason why they can't?

@WaffleLapkin
Copy link
Member Author

The evidence indicates that people expect align_to to return non-empty middle parts in a number of situations, no matter what the documentation says. I don't see why that would change when they do it inside const.

Interesting, I thought the issue was that people wanted more guarantees, not necessarily that it's confusing...

Would those examples use align_to if it was const, or is there some reason why they can't?

I don't know. I can't see why they couldn't use it, but I might be missing something.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 13, 2023

people wanted more guarantees,

I think that's it, and created #105296 a while back

@RalfJung
Copy link
Member

Will people be happy if they get those guarantees but not when the function is called in const?

@RalfJung
Copy link
Member

RalfJung commented Oct 2, 2024

@m-ou-se wrote in a different discussion

In general, I think it's perfectly reasonable for there to be (uncommon) situations where const eval results in a panic and runtime eval doesn't. (Unlike the other way around, or giving different return values, etc.)

align_offset pretty inevitably returns a different result in const eval in some cases. I wonder if it wouldn't be better, for now, to just remove this from const-eval, and use const_eval_select instead in the places that call it from a const fn?

@RalfJung
Copy link
Member

#132423 de-constifies these functions.

@bors bors closed this as completed in 3313e76 Nov 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 4, 2024
Rollup merge of rust-lang#132423 - RalfJung:const-eval-align-offset, r=dtolnay

remove const-support for align_offset and is_aligned

As part of the recent discussion to stabilize `ptr.is_null()` in const context, the general vibe was that it's okay for a const function to panic when the same operation would work at runtime (that's just a case of "dynamically detecting that something is not supported as a const operation"), but it is *not* okay for a const function to just return a different result.

Following that, `is_aligned` and `is_aligned_to` have their const status revoked in this PR, since they do return actively wrong results at const time. In the future we can consider having a new intrinsic or so that can check whether a pointer is "guaranteed to be aligned", but the current implementation based on `align_offset` does not have the behavior we want.

In fact `align_offset` itself behaves quite strangely in const, and that support needs a bunch of special hacks. That doesn't seem worth it. Instead, the users that can fall back to a different implementation should just use const_eval_select directly, and everything else should not be made const-callable. So this PR does exactly that, and entirely removes const support for align_offset.

Closes some tracking issues by removing the associated features:
Closes rust-lang#90962
Closes rust-lang#104203

Cc `@rust-lang/wg-const-eval` `@rust-lang/libs-api`
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

Successfully merging a pull request may close this issue.

4 participants