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

align_offset guarantees #62420

Closed
RalfJung opened this issue Jul 5, 2019 · 37 comments · Fixed by #121201
Closed

align_offset guarantees #62420

RalfJung opened this issue Jul 5, 2019 · 37 comments · Fixed by #121201
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-miri Area: The miri tool T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 5, 2019

The documentation for align_offset says

If it is not possible to align the pointer, the implementation returns usize::max_value().

It does not give any details of when it might not be possible to perform the alignment. My reading of that is that the user must always be prepared for failure to happen. In accordance with that, a while ago I adjusted the docs for align_to (the preferred way to use align_offset) to say

This method splits the slice into three distinct slices: prefix, correctly aligned middle slice of a new type, and the suffix slice. The method does a best effort to make the middle slice the greatest length possible for a given type and input slice, but only your algorithm's performance should depend on that, not its correctness.

In practice, returning max_value happens when p as usize & (gcd - 1) != 0 (whatever exactly this means, this is taken from the implementation) -- and when running in Miri, which will always return max_value.

Historically, Miri did this because it had no notion of the "integer address" of an allocation, so there literally was no way to offset a pointer to get a particular alignment. This is changing now, Miri is getting the support for this. So maybe we should note some conditions under which align_offset will definitely succeed. A motivation for this is #61339, which implicitly made the assumption that aligning with size_of::<T>() == 1 will always succeed.

On the other hand, the current contract for align_offset lets Miri do more reliable alignment checking. This is off-by-default but can be enabled with -Zmiri-symbolic-alignment-check: when checking whether some pointer p that points to offset o inside an allocation with alignment a, we have the option to consider only a and o and not the integer value of p. This allows us to reliably detect alignment problems in code such as:

fn main() {
    let x = &mut [0u8; 3];
    let base_addr = x as *mut _ as usize;
    let u16_ref = &mut *(base_addr as *mut u16);
    *u16_ref = 2;
    println!("{:?}", x);
}

If we were to take the actual integer value of p into account, the program might get "lucky" and actually run successfully in Miri because base_addr happens to be even. In contrast, by not doing this, Miri can offer a flag where the bug in the program above is definitely caught. With this flag, the user can be sure that none of the accesses in the program are aligned "by chance".

However, this also means that when this flag is set, the following code will not pass:

fn main() {
    let x = &mut [0u8; 3];
    let base_addr = x as *mut _ as usize;
    let u16_ref = unsafe { if base_addr % 2 == 0 {
        &mut *(base_addr as *mut u16)
    } else {
        &mut *((base_addr+1) as *mut u16)
    } };
    *u16_ref = 2;
    println!("{:?}", x);
}

Miri cannot know that you actually did your homework and checked the integer address. This program is basically indistinguishable from the bad program above.

Currently there does not seem to be much code that operates like the last example above -- code will instead use align_to, which will (when run in Miri with symbolic alignment checking) make the middle part empty, and thus the "higher-aligned" accesses just don't happen. This means the vast majority of code works fine in the better-alignment-checking mode. If we force align_offset to not fail like #61339 expects, then suddenly align_to will return non-empty middle parts in Miri as well, and -Zmiri-symbolic-alignment-check will basically be useless. There will be false positives when any method using align_to is called, which includes a few fundamental methods in libcore.

So, here's the trade-off: either Miri has a mode that can reliably detect alignment problems, or align_offset guarantees success under certain conditions. I don't think we can have both. Which one do we pick?

(The particular PR #61339 that triggered this might be fixable to work with the contract Miri needs, I don't know. But this discussion will probably come up again.)

Cc @rust-lang/libs @rust-lang/wg-unsafe-code-guidelines

@RalfJung
Copy link
Member Author

RalfJung commented Jul 5, 2019

Turns out that fixing #61339 to work fine with Miri does not even change the generated assembly; looks like LLVM can just optimize away the extra check for max_value.

That takes the time pressure of this discussion, that PR can land without having to reach any conclusion here. The discussion still seems to be worth having though!

@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2019

Another reason we might want to keep the spec as-is is to be able to eventually make align_to a const fn. CTFE does not, cannot and should not pick an actual "base address" for an allocation, so there is no way for any byte in a 1-aligned allocation (say, a Vec<u8>) to have an alignment higher than 1. After all, depending on the actual base address, any of these bytes could be just 1-aligned. Sure, some of them will have higher alignment, but we cannot know which -- and deciding which during CTFE would have to be made consistent with the base address this data gets later at run-time.

@nagisa
Copy link
Member

nagisa commented Aug 2, 2019

In practice, returning max_value happens when p as usize & (gcd - 1) != 0 (whatever exactly this means, this is taken from the implementation)

This essentially checks that it is indeed possible to align p, which might not be the case for some specific values of size_of::<T>, p and a. For example size_of::<T> = 2, p = 3 and a = 2.

Or, alternatively, that a solution for the $$ p + so ≡ 0 mod a $$ linear congruence equation exists.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2019

What is a in your text? For me it was the alignment of the allocation but when considering the physical machine that doesn't seem to make sense? I guess you mean the align parameter?

I see. If your pointer points to an odd address and you can only move even-sized steps, you cannot ever get an even address.

@nagisa
Copy link
Member

nagisa commented Aug 2, 2019

Its the align parameter, yes. This portion of the comment elaborates.

@RalfJung RalfJung added A-const-eval Area: Constant evaluation (MIR interpretation) A-miri Area: The miri tool T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 4, 2019
@That3Percent
Copy link

The changes made to the documentation of align_offset, and in particular the present stance that

Only your algorithm's performance can depend on getting a usable offset here, not its correctness.

is not very pragmatic. The need to get an aligned reference inside a Vec is a very mundane and common one. Eg: bumpalo.

The way that bumpalo seems to be working around this limitation is just to cast to a usize, do the rounding there, and then cast back to a pointer. See: try_alloc_layout_fast.

Is round tripping a pointer through a usize really what we want to say is the best practice? That seems to me kind of bizarre. If some operation like aligning a *u8 is fundamentally unsound, why would converting to usize, aligning, and converting back be any more sound? Does altering code that would normally deal with *u8 so that it deals with usize instead lose many of the benefits that Miri is meant to provide? Perhaps code dealing with *u8 does have a soundness problem which Miri would catch, but then the code is altered to use usize instead and Miri can no longer catch the problem.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 13, 2019

Quick remark (will come back to the rest of your comment when I have more time):

The changes made to the documentation of align_offset, and in particular the present stance that

Note that this is not a change. Already the very first stable doc for align_to says

[...] but only your algorithm's performance should depend on that, not its correctness.

Guaranteeing more than that would be a change.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 16, 2019

If some operation like aligning a *u8 is fundamentally unsound

It is not fundamentally unsound. Where are you getting that idea from? The issue above explains in great detail what the issue is with guaranteeing more for align_offset; "unsound" is not a word that is used there. Do you have a concrete question about what the problem is, something I should clarify?

Does altering code that would normally deal with *u8 so that it deals with usize instead lose many of the benefits that Miri is meant to provide?

I do not understand the question.

What bumpalo does will indeed not work properly in Miri right now, and when Miri gains support for such code it will come at the expense of test coverage. For code like bumpalo that really needs to successfully get a higher-aligned pointer out of a lower-aligned allocation, that's fine, there is nothing else we can do -- but most code using align_offset is not of that kind, and it would be sad to lose Miri test coverage for all that code.

@tdelabro
Copy link
Contributor

tdelabro commented Feb 7, 2021

Hello,

I'm using #![feature(const_ptr_offset)] in one project and I find myself in the need of align_offset being const.
I never contributed to the rust language before, but is there something I can do to help ?

@RalfJung
Copy link
Member Author

RalfJung commented Feb 7, 2021

@tdelabro what exactly do you need from it at const-time? The full functionality of align_offset is impossible to implement in const, since it is impossible to determine the actual integer address that an allocation will be placed at.

@tdelabro
Copy link
Contributor

tdelabro commented Feb 7, 2021

@RalfJung I define symbols in a linker script, then get them in my rust code as external symbols. And want to use them in a static objects.

In linker.ld:

kernel_end = .;

In rust:

extern "C" {
    pub fn kernel_end();
}

const unsafe fn get_ext_symb_add(f: unsafe extern "C" fn()) -> *const usize {
    f as *const usize
 }
 
 pub const fn get_kernel_end() -> *const usize {
    unsafe { get_ext_symb_add(kernel_end) }
}
 
 pub static KERNEL_HEAP: Locked<KernelHeap> = Locked::new(KernelHeap::new(get_kernel_end()));

So this works. But I need my struct to contain a 4k page aligned value, so I tried the following:

match get_kernel_end() {
    v if v as usize & 0xFFF == 0 => v,
    v => (v as usize & !0xFFF + 0x1000) as *const usize,
}

But this is not allowed because the cast to usize would allow things that would lead to non constant results, which I understand. But I was expecting that this will be allowed as it will always produce the same output:

let add_of_first_page_after_kernel = get_kernel_end.offset(get_kernel_end.align_offset(0x1000) as isize)

@comex
Copy link
Contributor

comex commented Feb 8, 2021

This is impossible. Const evaluation is done by the compiler, while the final address is determined by the linker, which runs later. The compiler doesn't know the address; all it can do is emit a relocation to ask the linker to insert the address in a specified place in the code or data. A relocation can request adding or subtracting an arbitrary offset to the address, but not more complicated math than that.

That said, you can do the math in the linker script. Something like kernel_end_aligned = ALIGN(0x1000) should work.

@tdelabro
Copy link
Contributor

tdelabro commented Feb 8, 2021

@comex Ok yeah, it seems obvious when you explain it. Dunno why I didn't realized it myself.

Thanks for for linker align tip, I got the same idea last night when I went to sleep. That's what I will do. Ty.

@taylordotfish
Copy link

Now that Miri's align_offset succeeds when align is less than or equal to the alignment of the allocation, would it be possible to strengthen align_offset's documented guarantees accordingly? Or are there still reasons we want to allow it to fail in all cases?

@RalfJung
Copy link
Member Author

There's still the question of, at some point, allowing align_offset to be called during CTFE -- which will only be possible with the weak semantics.

@taylordotfish
Copy link

For reference, the main reason I'm interested in having stronger guarantees is that it would enable the use of tagged pointers without platform- or implementation-specific assumptions.

For example:

#[derive(Clone, Copy)]
pub struct TaggedPtr(*const u8);

impl TaggedPtr {
    /// # Safety
    ///
    /// * `ptr` must be properly aligned (i.e., have an alignment of at least 2).
    /// * `ptr` must be “dereferencable” in the sense defined by [`std::ptr`](std::ptr#safety).
    pub unsafe fn new(ptr: *const u16, tag: bool) -> Self {
        Self(unsafe { (ptr as *const u8).add(tag as usize) })
    }

    /// Returns the stored pointer and tag.
    pub fn get(self) -> (*const u16, bool) {
        let offset = self.0.align_offset(2);
        assert!(offset != usize::MAX); // Ideally, this would be guaranteed.

        let tag = offset & 1;
        (unsafe { self.0.sub(tag) } as *const u16, tag != 0)
    }
}

fn main() {
    let i1 = 0_u16;
    let i2 = 1_u16;

    let p1 = &i1 as *const u16;
    let p2 = &i2 as *const u16;

    let t1 = unsafe { TaggedPtr::new(p1, true) };
    let t2 = unsafe { TaggedPtr::new(p2, false) };

    assert_eq!(t1.get(), (p1, true));
    assert_eq!(t2.get(), (p2, false));
}

The “standard” way of doing this is to cast the pointer to a usize and use the lower bits, which works in practice but, as far as I know, relies on assumptions about pointer-to-integer conversions that aren't guaranteed by the language.

With stronger align_offset guarantees, even if they applied only to non-CTFE contexts, the example above could be guaranteed not to panic.

@RalfJung
Copy link
Member Author

The “standard” way of doing this is to cast the pointer to a usize and use the lower bits, which works in practice but, as far as I know, relies on assumptions about pointer-to-integer conversions that aren't guaranteed by the language.

The implementation of align_offset is relying on those same guarantees... but I guess that is a libstd implementation detail and none of your concern. ;)
I'd say we should probably make those guarantees, except I have no idea what happens on platforms like CHERI that have hardware ptr tagging -- but then, there are many open questions for Rust on those platforms.

Btw, is there a reason you are not using wrapping_add and wrapping_sub on pointers? That would avoid having to make inbounds assumptions (in particular, it would make your scheme work even for pointers to zero-sized objects).

@taylordotfish
Copy link

The implementation of align_offset is relying on those same guarantees... but I guess that is a libstd implementation detail and none of your concern. ;)

Indeed, and while searching around, I came across someone who noted that the implementation of std::sync::Once uses a tagged pointer with the standard method of converting to a usize and using the lower bits.

Making additional guarantees about pointer-to-integer conversions would definitely work for this purpose, but I figured they might be more controversial than strengthening align_offset's guarantees. There are theoretically platforms, as you note, that wouldn't allow reusing the lower bits of an aligned pointer, but would work with the align_offset implementation. Supporting those platforms, of course, would require changes to libstd.

is there a reason you are not using wrapping_add and wrapping_sub on pointers

Ah, good point—that would indeed be better. As silly as it sounds, I hadn't actually realized that wrapping_add and wrapping_sub aren't unsafe (I should've read the documentation more carefully!).

@comex
Copy link
Contributor

comex commented Mar 14, 2021

I must say, as someone who has to align pointers from time to time, align_offset strikes me as a very strange API.

To start with, why does it express failure as a sentinel value instead of returning Option? Perhaps as a micro-optimization, but I don't see why a sentinel would be any faster. (If it returned an Option, in a typical case I'd expect the optimizer to inline align_offset and then be able to show that the discriminant is always Some.)

Beyond that, at first I didn't even understand the use case for an alignment operation that can fail. After reading up on align_to I see what it is: algorithms that can run on arbitrary aligned buffers, but have a fast path for well-aligned buffers, and try to extract the well-aligned subset of the input buffer to run the fast path on. Fair enough, such algorithms can often tolerate spurious failure – but even then, some might rely on the assumption that the prefix and suffix each have a size smaller than the alignment.

Regardless, there are many other use cases for align_offset that can't tolerate failure.

One common one is to allocate an aligned buffer within a non-aligned slice of unused memory, like bumpalo does.

Some in-memory and file formats have a similar principle: they contain a series of entries which are implicitly padded to multiples of N bytes. For example, if N is 8 and you saw a 5 byte entry, you would have to skip an additional 3 bytes to find the next entry. If you use align_offset for this and it fails, you won't know where to find the next entry.

Admittedly, even in C, these formats are usually implemented by checking the alignment of integer sizes or offsets, not pointers. But not always: here is an example of Rust code using align_offset to compute the correct offset in an in-memory format. And even when it comes to code that does operate on integers… while align_offset is only provided for pointers, I found two examples of code that casts a size value to a *const u8 for the sole purpose of calling align_offset on it!

Another use case is testing p.align_offset(alignment) == 0 in order to check whether p is already aligned – sometimes for functional purposes, though more often for asserts.

(Pointless sidenote about C) It was nonintuitive to me that people would use align_offset for this purpose. I'm more used to C where there's no standard helper function equivalent to align_offset, so you have to write out the math explicitly. To compute the bytes needed to round up, i.e. what align_offset is, you write -foo & (align - 1), whereas to compute the bytes needed to round *down*, you write foo & (align - 1). If you want to check whether something is already aligned, either will work, but the latter is less nonintuitive (what is negating a pointer supposed to accomplish?) and one character shorter, so you would usually pick that: (foo & (align - 1)) == 0. Of course, there's nothing wrong with using align_offset. If it's behind a helper function, intuitiveness doesn't matter, and the performance is likely identical since the optimizer will likely optimize away the negation. That said, I'm surprised that we have align_offset, but don't have a complementary helper function to compute the number of bytes needed to round down.

Honestly, I originally thought this didn't matter very much either way, but researching this comment has changed my mind. Each of the five links above points to real-world code that uses align_offset and assumes it will never fail, despite what the docs say. Some of the examples have an assert that it doesn't return usize::MAX, while others will just silently misbehave in that case. The examples weren't exactly hard to find: I used grep.app to search for uses of align_offset, and it seemed like most of them assumed it wouldn't fail. (There were also surprisingly many uses overall, to me; I had thought it was a little-used function.) Heck, even align_to seems to be misused more often than not: [1] [2] [3].

Given that, I am strongly in favor of strengthening align_offset's semantics to guarantee what people already expect of it, even though it means giving up on const compatibility.

If we want a const-compatible version, it can be added as a new function. The new function could have a longer name that somehow indicates it can spuriously fail, though I'm not sure how to word this. And it could have a proper Option return type, so that if people do misuse it, at least it will only result in panics when they try to unwrap it, not silent miscalculation.

@taylordotfish
Copy link

It might be worth pointing out C++'s std::align, just as a matter of seeing what other languages do. It also rounds up and also has a weird API (although differently so), but the most important difference is that it isn't allowed to spuriously fail.

@RalfJung
Copy link
Member Author

@comex just to be sure, those uses you found that assume that align_offset cannot fail, they use it with a pointer type of size 1? For the more general case of arbitrary pointer type and arbitrary alignment, it is not always possible to align the pointer properly, even disregarding const-compatibility. IOW, the awkwardness of the API (sentinel value instead of Option) is not solved by ditching const-compatibility.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 17, 2021

The advantage of the sentinel value is that the normal usage on normal platforms has one less branch, as you don't need to match on an option first and then do an inbound check. Instead the inbound check alone suffices

@comex
Copy link
Contributor

comex commented Apr 19, 2021

@comex just to be sure, those uses you found that assume that align_offset cannot fail, they use it with a pointer type of size 1? For the more general case of arbitrary pointer type and arbitrary alignment, it is not always possible to align the pointer properly, even disregarding const-compatibility. IOW, the awkwardness of the API (sentinel value instead of Option) is not solved by ditching const-compatibility.

Oops, I missed this when you originally posted it.

I re-checked my examples and they do all use it with u8 pointers. That seems to be by far the most common use case.

Perhaps worth noting: for any type whose size equals its alignment, align_offset will never fail for a well-aligned input pointer. However, the set of types for which size equals alignment is platform-dependent. I tend to assume it's true for all basic integer types, but it's false for u64 on Windows 32-bit x86.

@talchas
Copy link

talchas commented Jul 18, 2023

Is there actually any fundamental reason align_offset cannot work sanely in CTFE so long as ptr2int doesn't exist (which aiui is not expected to change)? The algorithm I'm thinking of is for p.align_offset(n) to just increase the alignment for the allocation of p to be n (if n is larger than the current alignment). Then CTFE has all of the information needed to compute align_offset: the offset mod n of the allocation base (now guaranteed to be 0), and the offset of p from the allocation base (pointer::offset_from is stable const).

So long as the compiler is guaranteed to be able to recognize every CTFE alignment check (by the lack of ptr2int), it can claim that the allocation is aligned for every checked value, and then if the allocation is promoted to a static, it can require that alignment for the static. This does mean that it would still fail during const eval (in some way, panic, linker error, return failure) for "too large" alignments that still fit in a usize, but that really doesn't sound like a problem tbh. (And slice::align_to/as_simd would never fail)

@RalfJung
Copy link
Member Author

I think it would be very surprising if calling align_offset actually affected the alignment of an allocation.

Also, I don't think this can actually work: the const to which p points might be in another crate whose bitcode has already been generated.

@talchas
Copy link

talchas commented Jul 18, 2023

I mean, less surprising than the current definition (and more definitely, results in less obnoxious downstream APIs). The fact that the allocation might be defined in another crate is technically not a problem at the moment with the current definition of const that I hate (that they're substituted at every use and any dedup anywhere is entirely best-effort, and you cannot even refer to any statics at all), though it's certainly a bit more ugly as a potential source of .(ro)data size, and the restriction preventing access to Freeze statics is otherwise unnecessary iirc. In practice it could certainly be avoided by increasing the default align of statics to their size up to an arch-specific maximum, though that doesn't prevent the general case problem if we permit const references to any actual static FOO.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 18, 2023

I mean, less surprising than the current definition (and more definitely, results in less obnoxious downstream APIs).

I strongly disagree. Currently we just have an API that returns an "unknown" result. Your proposal means non-local side-effects of what should be a read-only operation. I can already see people trying to debug why Rust puts such huge amounts of padding into their binary, just because some other crate internally somewhere used align_offset with a high value. Any crate with access to a given const can now suddenly change the way that const is compiled!

The fact that the allocation might be defined in another crate is technically not a problem at the moment

I think it very much is a problem at least with the current implementation in rustc. With const C = { let x = &0; x };, the allocation containing 0 is part of the original crate and will not be duplicated (it has an AllocId and that is used consistently by all downstream crates). It is impossible for downstream crates to alter this allocation. yet, downstream crates might call align_offset on C.

The fact that, just looking at this crate, we cannot tell the alignment constraint that will be generated for that 0, is both IMA a big red flag semantically, and also completely incompatible with the rustc architecture. (Not that rustc architecture should necessarily drive our design. But it just shows that such effects of downstream crates on upstream data is so out-of-the-ordinary that rustc was designed in a way that it cannot even be achieved.)

@talchas
Copy link

talchas commented Jul 18, 2023

I mean, less surprising than the current definition (and more definitely, results in less obnoxious downstream APIs).

I strongly disagree. Currently we just have an API that returns an "unknown" result.

We have APIs (align_to/as_simd) that return aggressively useless results that make everyone who sees them go wtf. And once they understand, it's still annoying and warps the code to not be able to do simple things that would require the head/tail to be less than the lane count. And you can't force the head to be empty by forcing the alignment elsewhere (and just use these functions as a safe and correct transmute).

Your proposal means non-local side-effects of what should be a read-only operation. I can already see people trying to debug why Rust puts such huge amounts of padding into their binary, just because some other crate internally somewhere used align_offset with a high value. Any crate with access to a given const can now suddenly change the way that const is compiled!

Yes, this is not ideal, but let's be realistic here; people don't randomly call these APIs, generally don't call them in const in the first place (the higher-level APIs aren't even const, despite having the screwed up design that comes from align_offset), and don't generally call them with large values in the first place (much less values larger than the size).

The fact that the allocation might be defined in another crate is technically not a problem at the moment

I think it very much is a problem at least with the current implementation in rustc. With const C = { let x = &0; x };, the allocation containing 0 is part of the original crate and will not be duplicated (it has an AllocId and that is used consistently by all downstream crates). It is impossible for downstream crates to alter this allocation. yet, downstream crates might call align_offset on C.

Isn't that precisely the one case where dedup failure can be detected (and comes up via associated consts being generic)? It still does make it far more ugly, though the silly align=size heuristic is always more likely to remove the issue in practice.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2023

These APIs are not useful for your use case, because they are not meant for it. I created #105296 that adds targetted functions for your use cases.

My preference is still to have precise functions instead of a universal function, similar to how the proposal for strict provenance argues against as as a tool that does 50 different things, depending on the runtime input values.

@talchas
Copy link

talchas commented Jul 18, 2023

You mean as_simd isn't intended to be useful for SIMD algorithms (and align_to isn't intended to be useful for them without portable-simd)? The extra "force alignment other ways" part sure, though requiring separate APIs for that is silly.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2023

We can bounce arguments back and forth, but I don't understand why align_to is useless, considering it has libstd usages that don't even need SIMD ops but instead use integer types on the middle slice for measurable speedups.

Why do you think having separate APIs is silly? You are throwing away the start and end slice on every call, woundn't you rather just have a function that gives you what you actually want?

@RalfJung
Copy link
Member Author

as_simd is perfectly useful; claiming it produces "aggressively useless results" is useless hyperbole. It returns three slices that join to the intended slice. It's not even a const fn so I don't see the relation to your const-related suggestions.

Calling as_simd with the intent of ignoring the first and last component is clearly an API misuse, as is strongly suggested by both the docs and the type of that function. However, since as_simd is not a const fn, it would be totally possible to change its contract to always require the middle part to be maximal. wg-const-eval isn't even involved in that discussion; you'll have to take that up with t-libs-api (probably by writing an ACP).

@talchas
Copy link

talchas commented Jul 18, 2023

The entire reason it has issues is because of align_offset's API, so it is clearly relevant if align_offset's API can be fixed (for which "external crates consts" is a vaguely reasonable counterargument even if it's somewhat workaroundable).

And when people are ignoring as_simd/align_to to make their own versions because of the warning text (probably before the update to align_to's warning text, to be fair) I think it's worth calling them useless. I agree that if align_to/as_simd aren't going to go const that they should just be fixed via docs and it's probably worth leaving align_offset alone .

@scottmcm
Copy link
Member

I would suggest mostly ignoring as_simd for this discussion. It's under the portable_simd tracking issue, so isn't particularly close to stabilization at all, and can still be changed however we want -- including potentially removing it entirely.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 18, 2023

And when people are ignoring as_simd/align_to to make their own versions because of the warning text (probably before the update to align_to's warning text, to be fair) I think it's worth calling them useless.

No, it just means that this function does not cover those people's needs. It says nothing about the usefulness of the function for its actually intended usecase. We are not looking to turn align_to into a kitchen sink that can cover everything; if there are needs it doesn't cover, the best answer is new APIs that cover them.

When you claim "useless" you are claiming "nobody can have a use for this function". That claim is obviously false, and also non-productive.


The public API of as_simd is no no way tied to its internal implementation via align_offset. If you can convince t-libs-api that as_simd should have a different API contract, we'll change the implementation to make that work -- that is not and has never been the blocking issue. We could even change align_offset's contract to say that at runtime, the offset computation is guaranteed to not fail "spuriously". This has been talked about for years, but nobody seriously tried to actually make such a change. That would require a proper motivation and explanation of usecases, and why changing align_to to be suitable for such cases is a better option than adding a new function.

So if you want to truly move this discussion forward, I suggest you prepare such a document. I don't think there is much to discuss in this issue here in the meantime.

@talchas
Copy link

talchas commented Jul 18, 2023

When people reimplement the exact same function only without the unnecessary caveat, saying "does not cover those people's needs" like it's a special case is silly.

And claiming that the public API limitations that are taken straight from its internal implementation and have no other excuse are not related is also wtf. I agree that it sounds like this is not the place to actually solve it.

@RalfJung
Copy link
Member Author

Given the semantic and architectural issues with "have align_offset change the alignment of a compile-time allocation", I can only repeat what I have been saying for years: the most promising way to change align_offset is to declare that at runtime, it will always produce the best possible result, but at compiletime it can fail. This situation is blocked on someone who cares sufficiently about this problem to write up that proposal with motivating examples and argue why those examples are not better handled with a different API. So far I haven't seen the proponents of this change seriously engage with the idea of alternative APIs.

A consequence of this proposal is that either, align_offset will never be stably const, or we have const functions that behave in ways that are never permitted at runtime. I think we will have the latter anyway with an eventual safe const_eval_select, so I don't think that is in itself a big deal.

@bors bors closed this as completed in 948d32d Mar 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 8, 2024
Rollup merge of rust-lang#121201 - RalfJung:align_offset_contract, r=cuviper

align_offset, align_to: no longer allow implementations to spuriously fail to align

For a long time, we have allowed `align_offset` to fail to compute a properly aligned offset, and `align_to` to return a smaller-than-maximal "middle slice". This was done to cover the implementation of `align_offset` in const-eval and Miri. See rust-lang#62420 for more background. For about the same amount of time, this has caused confusion and surprise, where people didn't realize they have to write their code to be defensive against `align_offset` failures.

Another way to put this is: the specification is effectively non-deterministic, and non-determinism is hard to test for -- in particular if the implementation everyone uses to test always produces the same reliable result, and nobody expects it to be non-deterministic to begin with.

With rust-lang#117840, Miri has stopped making use of this liberty in the spec; it now always behaves like rustc. That only leaves const-eval as potential motivation for this behavior. I do not think this is sufficient motivation. Currently, none of the relevant functions are stably const: `align_offset` is unstably const, `align_to` is not const at all. I propose that if we ever want to make these const-stable, we just accept the fact that they can behave differently at compile-time vs at run-time. This is not the end of the world, and it seems to be much less surprising to programmers than unexpected non-determinism. (Related: rust-lang/rfcs#3352.)

`@thomcc` has repeatedly made it clear that they strongly dislike the non-determinism in align_offset, so I expect they will support this. `@oli-obk,` what do you think? Also, whom else should we involve? The primary team responsible is clearly libs-api, so I will nominate this for them. However, allowing const-evaluated code to behave different from run-time code is t-lang territory. The thing is, this is not stabilizing anything t-lang-worthy immediately, but it still does make a decision we will be bound to: if we accept this change, then
- either `align_offset`/`align_to` can never be called in const fn,
- or we allow compile-time behavior to differ from run-time behavior.

So I will nominate for t-lang as well, with the question being: are you okay with accepting either of these outcomes (without committing to which one, just accepting that it has to be one of them)? This closes the door to "have `align_offset` and `align_to` at compile-time and also always have compile-time behavior match run-time behavior".

Closes rust-lang#62420
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Mar 9, 2024
align_offset, align_to: no longer allow implementations to spuriously fail to align

For a long time, we have allowed `align_offset` to fail to compute a properly aligned offset, and `align_to` to return a smaller-than-maximal "middle slice". This was done to cover the implementation of `align_offset` in const-eval and Miri. See rust-lang/rust#62420 for more background. For about the same amount of time, this has caused confusion and surprise, where people didn't realize they have to write their code to be defensive against `align_offset` failures.

Another way to put this is: the specification is effectively non-deterministic, and non-determinism is hard to test for -- in particular if the implementation everyone uses to test always produces the same reliable result, and nobody expects it to be non-deterministic to begin with.

With rust-lang/rust#117840, Miri has stopped making use of this liberty in the spec; it now always behaves like rustc. That only leaves const-eval as potential motivation for this behavior. I do not think this is sufficient motivation. Currently, none of the relevant functions are stably const: `align_offset` is unstably const, `align_to` is not const at all. I propose that if we ever want to make these const-stable, we just accept the fact that they can behave differently at compile-time vs at run-time. This is not the end of the world, and it seems to be much less surprising to programmers than unexpected non-determinism. (Related: rust-lang/rfcs#3352.)

`@thomcc` has repeatedly made it clear that they strongly dislike the non-determinism in align_offset, so I expect they will support this. `@oli-obk,` what do you think? Also, whom else should we involve? The primary team responsible is clearly libs-api, so I will nominate this for them. However, allowing const-evaluated code to behave different from run-time code is t-lang territory. The thing is, this is not stabilizing anything t-lang-worthy immediately, but it still does make a decision we will be bound to: if we accept this change, then
- either `align_offset`/`align_to` can never be called in const fn,
- or we allow compile-time behavior to differ from run-time behavior.

So I will nominate for t-lang as well, with the question being: are you okay with accepting either of these outcomes (without committing to which one, just accepting that it has to be one of them)? This closes the door to "have `align_offset` and `align_to` at compile-time and also always have compile-time behavior match run-time behavior".

Closes rust-lang/rust#62420
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-miri Area: The miri tool T-lang Relevant to the language team, which will review and decide on the PR/issue. 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.

9 participants