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_pointer_is_aligned #104203

Closed
2 of 8 tasks
lukas-code opened this issue Nov 9, 2022 · 7 comments · Fixed by #132423
Closed
2 of 8 tasks

Tracking Issue for const_pointer_is_aligned #104203

lukas-code opened this issue Nov 9, 2022 · 7 comments · Fixed by #132423
Labels
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.

Comments

@lukas-code
Copy link
Member

lukas-code commented Nov 9, 2022

Feature gate: #![feature(const_pointer_is_aligned)]

This is a tracking issue for using ptr.is_aligned() and ptr.is_aligned_to(alignment) in const contexts.

Public API

impl<T: Sized> *(const|mut) T {
    pub const fn is_aligned(self) -> bool;
}

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

impl <T> NonNull<T> {
    pub const fn is_aligned(self) -> bool;
}

Steps / History

Unresolved Questions

  • Do we want this at all or should alignment be runtime only / should users be using a stable const_eval_select instead?
  • Should these be separate functions guaranteed_aligned{,to} instead?
  • What behavior should these functions have during const eval? Some options include:
    1. Return true if the pointer is guaranteed to be aligned at runtime and false if we don't know whether the pointer is aligned at runtime. This is the current implementation.
    2. Same as above, but only document that the function may spuriously fail (during const eval), similar to align_offset. This allows changes to const alignment in the future.
    3. Assign a dummy address to all variables during const eval. Each allocation, including stack variables, would get its own "address space", which overlaps with other const eval address spaces and the runtime address space. This would make const eval alignment behavior similar to runtime, but a runtime pointer derived from a const eval pointer is still differently aligned. Pointer-to-integer casts are still not supported. The implementation for this would just remove this check: https://github.com/rust-lang/rust/blob/005f92d24acde37a02e0d084ce2ca8f40d2ee95a/compiler/rustc_const_eval/src/const_eval/machine.rs#L237
    4. Same as above, but we also allow pointer-to-integer casts, but not integer-to-pointer casts. Any program that assumes that different variables have different addresses bursts up in flame.

Related Links

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html

@lukas-code lukas-code added 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 Nov 9, 2022
@the8472
Copy link
Member

the8472 commented Nov 18, 2022

Do we want this at all or should alignment be runtime only?

What would const code need this for?

@lukas-code
Copy link
Member Author

lukas-code commented Nov 19, 2022

What would const code need this for?

Const alignment in general

Const code doesn't need to check for alignment. The are, however, algorithms that depend on alignment at runtime and also make sense in const. In the standard library we have str::from_utf8 and [u8]::is_ascii for example.

To make these functions work in const we have two options:

  • Split the function into "runtime" and "comptime" versions and then const_eval_select between them.
  • Create a good enough approximation of alignment for const contexts that allows the function to run without modification.

This feature (const_pointer_is_aligned) together with const_align_offset tries to address the second bullet point.

ptr.is_aligned() specifically

The main motivation for const is_aligned is catching errors early via assertions. Currently, the standard library uses the internal macro assert_unsafe_precondition! in many places to assert a condition when debug assertions are enabled and at runtime only. Most of these conditions are something like ptr.is_aligned() && !ptr.is_null() and can therefore not be checked at compiletime.

This feature allows to replace almost all of the assert_unsafe_precondition! with something like debug_or_const_assert! in the future, which can produce better error messages.

Consider this program for example: (playground)

const U16_SLICE: &[u16] = {
    let u8_slice: &[u8] = &[1, 2, 3, 4];
    let u16_slice = unsafe { core::slice::from_raw_parts(u8_slice.as_ptr().cast::<u16>(), 2) };

    // ... lots of other code here ...

    u16_slice
};

The current error message looks like this:

error[E0080]: it is undefined behavior to use this value
 --> src/lib.rs:1:1
  |
1 | const U16_SLICE: &[u16] = {
  | ^^^^^^^^^^^^^^^^^^^^^^^ constructing invalid value: encountered an unaligned reference (required 2 byte alignment but found 1)
  |
  = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
  = note: the raw bytes of the constant (size: 16, align: 8) {
              ╾───────alloc3────────╼ 02 00 00 00 00 00 00 00 │ ╾──────╼........
          }

In the future it might look like this instead:

error[E0080]: evaluation of constant value failed
 --> src/lib.rs:3:30
  |
3 |     let u16_slice = unsafe { core::slice::from_raw_parts(u8_slice.as_ptr().cast::<u16>(), 2) };
  |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'pointer must be aligned', src/lib.rs:3:30

Edit: I realize now, that this particular example is better solved by #104616, but the idea still applies for downstream crates that want to do a debug_assert!(ptr.is_aligned()) or are required to do "runtime" alignment checks, like safe_transmute for example.

bors added a commit to rust-lang-ci/rust that referenced this issue Nov 19, 2022
…lign-offset, r=oli-obk

Constify `is_aligned` via `align_offset`

Alternative to rust-lang#102753

Make `align_offset` work in const eval (and not always return `usize::MAX`) and then use that to constify `is_aligned{_to}`.

Tracking Issue: rust-lang#104203
RalfJung pushed a commit to RalfJung/miri that referenced this issue Nov 20, 2022
…et, r=oli-obk

Constify `is_aligned` via `align_offset`

Alternative to rust-lang/rust#102753

Make `align_offset` work in const eval (and not always return `usize::MAX`) and then use that to constify `is_aligned{_to}`.

Tracking Issue: rust-lang/rust#104203
@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2022

Also see the discussion here -- having is_aligned suddenly and silently "misbehave" when called in a const fn seems like a pretty bug footgun and we should probably not do that.

I think something like a new const fn is_guaranteed_aligned makes a lot more sense.

@lukas-code
Copy link
Member Author

If we add is_guaranteed_aligned, should that return Option<bool>, like guaranteed_eq? I can't think of a situation where it would be useful to check if a pointer is guaranteed to be unaligned.

If it just returns bool, then we'd have two basically identical functions in the standard library, but it might still be worth for documenting "this function might behave unexpectedly at compiletime".

As for potential footguns, the only thing I can think of is relying on

ptr.is_aligned_to(2) != ptr.wrapping_byte_add(1).is_aligned_to(2)

Or the same thing extended for larger alignments. For example in this buggy align_to implementation. After #104616, this will result in a loud compiler error instead of a silent bug. (This error should probably add a hint why the memory has alignment 1, even after offsetting the pointer.)

buggy program & error message
#![feature(const_slice_split_at_not_mut)]
#![feature(const_pointer_is_aligned)]
#![feature(pointer_is_aligned)]

use core::slice;

const fn bad_align_to(slice: &[u8]) -> (&[u8], &[u16], &[u8]) {
    if slice.is_empty() {
        return (&[], &[], &[]);
    }

    let (front, aligned) = if slice.as_ptr().is_aligned_to(2) {
        ([].as_slice(), slice)
    } else {
        slice.split_at(1)
    };
    
    let (middle, back) = aligned.split_at(aligned.len() / 2 * 2);
    
    // SAFETY: It's safe to transmute between `[u8; N * 2]` and `[u16; N]`.
    // We ensure that `middle` is aligned to 2 above.
    let middle_transmute = unsafe {
        slice::from_raw_parts(middle.as_ptr().cast::<u16>(), middle.len() / 2)
    };
    
    (front, middle_transmute, back)
}

fn main() {
    let a = [1, 2, 3, 4, 5, 6];
    for start in 0..a.len() {
        for end in start..a.len() {
            let slice = &a[start..end];
            println!("{:X?} -> {:X?}", slice, bad_align_to(slice));
        }
    }
    
    const _FOO: &[u16] = bad_align_to(&[1, 2, 3]).1; // ERROR
    const _BAR: u16 = bad_align_to(&[1, 2, 3]).1[0]; // ERROR after #104616
}
error[E0080]: evaluation of constant value failed
   --> /home/lukas/code/rust/library/core/src/slice/raw.rs:100:9
    |
100 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         accessing memory with alignment 1, but alignment 2 is required
    |         inside `std::slice::from_raw_parts::<'_, u16>` at /home/lukas/code/rust/library/core/src/slice/raw.rs:100:9
    |
   ::: src/main.rs:23:9
    |
23  |         slice::from_raw_parts(middle.as_ptr().cast::<u16>(), middle.len() / 2)
    |         ---------------------------------------------------------------------- inside `bad_align_to` at src/main.rs:23:9
...
38  |     const _FOO: &[u16] = bad_align_to(&[1, 2, 3]).1; //~ ERROR: it is undefined behavior to use this value
    |                          ------------------------ inside `_FOO` at src/main.rs:38:26

error[E0080]: evaluation of constant value failed
   --> /home/lukas/code/rust/library/core/src/slice/raw.rs:100:9
    |
100 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         accessing memory with alignment 1, but alignment 2 is required
    |         inside `std::slice::from_raw_parts::<'_, u16>` at /home/lukas/code/rust/library/core/src/slice/raw.rs:100:9
    |
   ::: src/main.rs:23:9
    |
23  |         slice::from_raw_parts(middle.as_ptr().cast::<u16>(), middle.len() / 2)
    |         ---------------------------------------------------------------------- inside `bad_align_to` at src/main.rs:23:9
...
39  |     const _BAR: u16 = bad_align_to(&[1, 2, 3]).1[0];
    |                       ------------------------ inside `_BAR` at src/main.rs:39:23

For more information about this error, try `rustc --explain E0080`.
error: could not compile `const_align_ub` due to 2 previous errors

In my opinion, every program that uses is_aligned for anything other than "if this isn't aligned, return an error/panic" should probably be using align_offset or {read/write}_unaligned instead.

@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2022

If it just returns bool, then we'd have two basically identical functions in the standard library, but it might still be worth for documenting "this function might behave unexpectedly at compiletime".

Yeah I was thinking the function name would help serve as a warning sign.

Cc @thomcc who brought this up in Zulip.

@the8472
Copy link
Member

the8472 commented Dec 3, 2022

Split the function into "runtime" and "comptime" versions and then const_eval_select between them.

Imo this is preferable. The alignment checks are only needed for performance optimizations that aren't relevant at comptime. The const version can be very naive code.

The main motivation for const is_aligned is catching errors early via assertions. Currently, the standard library uses the internal macro assert_unsafe_precondition! in many places to assert a condition when debug assertions are enabled and at runtime only. Most of these conditions are something like ptr.is_aligned() && !ptr.is_null() and can therefore not be checked at compiletime.

Note that we don't even ship the standard library with debug asserts enabled. So for now these are mostly CI aids. It doesn't seem right to extend the public API surface to enable this at const time, especially since alignment at const time is underdetermined, unlike runtime alignment.

Edit: I realize now, that this particular example is better solved by #104616, but the idea still applies for downstream crates that want to do a debug_assert!(ptr.is_aligned()) or are required to do "runtime" alignment checks, like safe_transmute for example.

Wouldn't a debug_assert_rt!() do the job too?

thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
…et, r=oli-obk

Constify `is_aligned` via `align_offset`

Alternative to rust-lang/rust#102753

Make `align_offset` work in const eval (and not always return `usize::MAX`) and then use that to constify `is_aligned{_to}`.

Tracking Issue: rust-lang/rust#104203
@RalfJung
Copy link
Member

#132423 will remove this feature gate and de-constify these functions. Their current implementation is not something we want to stabilize, so a blank restart is required here.

@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: 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants