Stream: general

Topic: Does Miri support `repr(align)`?


Jake Goulding (Aug 01 2019 at 16:37, on Zulip):
use std::mem;

#[derive(Debug, Default)]
#[repr(align(8))]
struct AlignToU64<T>(T);

const BYTE_LEN: usize = mem::size_of::<[u64; 4]>();
type Data = AlignToU64<[u8; BYTE_LEN]>;

fn example(data: &Data) {
    let (head, u64_arrays, tail) = unsafe { data.0.align_to::<[u64; 4]>() };

    assert!(head.is_empty(), "buffer was not aligned for 64-bit numbers");
    assert_eq!(u64_arrays.len(), 1, "buffer was not long enough");
    assert!(tail.is_empty(), "buffer was too long");
}

fn main() {
    example(&Data::default());
}

This code passes outside of Miri, but fails inside:

error[E0080]: Miri evaluation error: the evaluated program panicked at 'buffer was not aligned for 64-bit numbers', src/main.rs:13:5
  --> src/main.rs:13:5
   |
13 |     assert!(head.is_empty(), "buffer was not aligned for 64-bit numbers");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: the evaluated program panicked at 'buffer was not aligned for 64-bit numbers', src/main.rs:13:5
   |
Jake Goulding (Aug 01 2019 at 16:38, on Zulip):

/cc @oli @RalfJ

Jake Goulding (Aug 01 2019 at 16:38, on Zulip):

I see repr(packed) in Miri testcases, but didn't see repr(align). No issues immediately popped out, either.

RalfJ (Aug 01 2019 at 16:45, on Zulip):

I dont think miri needs any extra handling for repr(align), that just affects layout computation. but let me digest that example.

RalfJ (Aug 01 2019 at 16:46, on Zulip):

align_to in Miri always puts everything into the first component

RalfJ (Aug 01 2019 at 16:46, on Zulip):

so Miri does support align but Miri also exploits the freedom that the align_to spec grants

RalfJ (Aug 01 2019 at 16:46, on Zulip):

which is to say "oh Im sorry you cannot align this"

Jake Goulding (Aug 01 2019 at 16:47, on Zulip):

...

RalfJ (Aug 01 2019 at 16:48, on Zulip):

now, we could do better. and by this I mean, even considering that I am not sure we want an implementation that is equivalent to what rustc does, because that impedes our ability to find mis-aligned accesses, we could still do better.

RalfJ (Aug 01 2019 at 16:48, on Zulip):

I am not sure if that's enough for your example though.

RalfJ (Aug 01 2019 at 16:49, on Zulip):

oh and just to complete the lawyering, the spec says:

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.

Jake Goulding (Aug 01 2019 at 16:49, on Zulip):

I see what you are saying, but I feel like it's veering into the camp of

Jake Goulding (Aug 01 2019 at 16:49, on Zulip):

yeah, rules lawyering

RalfJ (Aug 01 2019 at 16:49, on Zulip):

well the thing is these rules are useful

Jake Goulding (Aug 01 2019 at 16:50, on Zulip):

Except that the quoted rules taken to the letter effectively make the function useless.

Jake Goulding (Aug 01 2019 at 16:50, on Zulip):

"Here is a thing guaranteed by the language to be aligned" ... "oh, that's not aligned"

RalfJ (Aug 01 2019 at 16:50, on Zulip):

that is incorrect

RalfJ (Aug 01 2019 at 16:50, on Zulip):

the part in the middle is guaranteed to be aligned

RalfJ (Aug 01 2019 at 16:50, on Zulip):

libstd correctly uses this function to great effect

RalfJ (Aug 01 2019 at 16:51, on Zulip):

just because you used it wrong and without reading the spec (or ignoring it) doesnt mean it cant usefully be used correctly ;)

Jake Goulding (Aug 01 2019 at 16:52, on Zulip):

Ok, what is the appropriate function to use?

RalfJ (Aug 01 2019 at 16:52, on Zulip):

the typical usecase is to process the parts on the 1st and 3rd slice with a "slow path" and use an alignment-optimized "fast path" for the middle

RalfJ (Aug 01 2019 at 16:52, on Zulip):

memchr and memrchr do that, for example. both work fine in Miri.

RalfJ (Aug 01 2019 at 16:52, on Zulip):

what you are doing with this function is weird... there's no point in using it if you already know everything is aligned

Jake Goulding (Aug 01 2019 at 16:53, on Zulip):

Do I need to (as @oli said... somewhere) use ptr::alignand do it myself?

RalfJ (Aug 01 2019 at 16:53, on Zulip):

just cast your pointers

RalfJ (Aug 01 2019 at 16:53, on Zulip):

or check alignment

RalfJ (Aug 01 2019 at 16:53, on Zulip):

aign_to is for when you want to handle any-aligned data

RalfJ (Aug 01 2019 at 16:53, on Zulip):

you clearly dont want to do that, sicne you are asserting the first and last component to be empty

Jake Goulding (Aug 01 2019 at 16:54, on Zulip):

Which I did, until that tanked my performance

RalfJ (Aug 01 2019 at 16:54, on Zulip):

so just do assert!(data as *const _ as usize % mem::align_of::<u64>() == 0) or so

RalfJ (Aug 01 2019 at 16:54, on Zulip):

I dont see how making your code more complicated by calling align_to will make performance better?

RalfJ (Aug 01 2019 at 16:55, on Zulip):

the recommendation to sue align_to as to use it the way it is intended to be used

RalfJ (Aug 01 2019 at 16:55, on Zulip):

not to shoehorn it into your existing code in ways that make little sense

RalfJ (Aug 01 2019 at 16:56, on Zulip):

but I also dont know anything about how to make the optimizer happy^^

Jake Goulding (Aug 01 2019 at 16:57, on Zulip):

No, I mean that my original code could operate on unaligned data, so I wanted to use align_toto handle it. The algorithm itself doesn't handle it well, so I'm having to rewrite that. Now I have to rewrite this as well.

RalfJ (Aug 01 2019 at 16:57, on Zulip):

all I can help you with is correctness and clarity, and in terms of that, I'd say it is quite clear: align_to is for when you dont know if your input is aligned, but it is a big slice of things that has an "aligned middle part".
your case seems to be that it already has to be aligned, so you can just assert that and go on with it. (you might run into Miri limitations at some point as well but then it'll really be Miri's fault. ;)

RalfJ (Aug 01 2019 at 16:58, on Zulip):

IIRC you said the algorithm didnt handle unaligned "pre"-data or so?

RalfJ (Aug 01 2019 at 16:58, on Zulip):

I sense an XY problem somewhere near us^^

Jake Goulding (Aug 01 2019 at 16:58, on Zulip):

No

Jake Goulding (Aug 01 2019 at 16:58, on Zulip):

Basically align_to has zero use whatsoever in this code.

RalfJ (Aug 01 2019 at 16:58, on Zulip):

I dont see how "using align_to to handle it" implies "using align_to in a case where it literally makes no sense because you assume data is already aligned"

RalfJ (Aug 01 2019 at 16:59, on Zulip):

Basically align_to has zero use whatsoever in this code.

that may well be the case

RalfJ (Aug 01 2019 at 16:59, on Zulip):

if the algorithm cannot work with an unaligned prefix followed by an aligned central part, indeed align_to does not help

Jake Goulding (Aug 01 2019 at 16:59, on Zulip):

Originally I used align_to to handle unaligned data, and I wanted to reuse the same code for a buffer I knew to be aligned

RalfJ (Aug 01 2019 at 17:00, on Zulip):

if there is some mroe context around the minimal example you gave me above, sure maybe the code makes sense

RalfJ (Aug 01 2019 at 17:00, on Zulip):

I dont know that context, obviously

Jake Goulding (Aug 01 2019 at 17:00, on Zulip):

Well, we've talked about it in the other thread.

RalfJ (Aug 01 2019 at 17:00, on Zulip):

you would have to lobby for strengthening the align_to spec then though

RalfJ (Aug 01 2019 at 17:00, on Zulip):

and maybe Miri should support align_to better (and certainly it has to if the spec gets strengthened)

Jake Goulding (Aug 01 2019 at 17:01, on Zulip):

This boils down to I'm stupid and can't read the documentation or implement an algorithm.

Jake Goulding (Aug 01 2019 at 17:01, on Zulip):

So I probably shouldn't be writing this code in the first place.

RalfJ (Aug 01 2019 at 17:01, on Zulip):

Well, we've talked about it in the other thread.

I barely remember anything from that, it's not like I was heavily involved or didnt have two dozen other Rust discussions since then^^

RalfJ (Aug 01 2019 at 17:01, on Zulip):

I dont think you are stupid :)
and I am sorry if I came across too negative above

Jake Goulding (Aug 01 2019 at 17:01, on Zulip):

My only feedback is that Miri should put at least one thing into the aligned chunk, otherwise that code will never be exercised by Miri.

RalfJ (Aug 01 2019 at 17:02, on Zulip):

My only feedback is that Miri should put at least one thing into the aligned chunk, otherwise that code will never be exercised by Miri.

that is indeed a concern I had. but there's a big downside to doing that, which is that we can no longer reliably detect unaligned accesses.

RalfJ (Aug 01 2019 at 17:03, on Zulip):

accesses might be aligned "by chance" (because of what the allocator picked) and we could not distinguish that from properly checked aligned accesses

RalfJ (Aug 01 2019 at 17:03, on Zulip):

we had the thought of emitting a warning when that happens (we can detect that case) and encouraging the user to run Miri with lots of different seeds

RalfJ (Aug 01 2019 at 17:03, on Zulip):

but if we then make align_to actually put anything into the middle, that warning would basically fire for 100% of the code

RalfJ (Aug 01 2019 at 17:04, on Zulip):

(like, any code using align_to on a sufficiently big slice)

RalfJ (Aug 01 2019 at 17:04, on Zulip):

so we might as well not have the warning and just generally encourage using different seeds

RalfJ (Aug 01 2019 at 17:04, on Zulip):

and maybe that's indeed the best we can do

RalfJ (Aug 01 2019 at 17:20, on Zulip):

ah btw, if we ever want to make align_to a const fn we also have to keep its spec the way it is.

nagisa (Aug 01 2019 at 18:41, on Zulip):

align_to was made specifically with the intention that miri would never put anything in the middle.

nagisa (Aug 01 2019 at 18:41, on Zulip):

well, would be allowed to not.

oli (Aug 01 2019 at 19:39, on Zulip):

Note that miri the tool may still use the nondeterministic mode to arbitrarily overalign for fuzzing reasons

oli (Aug 01 2019 at 19:40, on Zulip):

Const eval needs to do the deterministic non-aligning

oli (Aug 01 2019 at 19:41, on Zulip):

There's also no point in using ptr::align because it also does the fallback where it returns usize::max_value or sth

RalfJ (Aug 01 2019 at 20:58, on Zulip):

we could decide to change the spec of align_to and the underlying align to disallow spurious failures when run outside const context. that would be implementable at least. it would however restrict miri's ability to do alignment checks. I would be fine if t-libs concluded that this trade-off is worth it.

nagisa (Aug 01 2019 at 23:11, on Zulip):

What’s the point though? The whole point of this function is that you would have to write code that has no observable differences (other than performance) regardless of how inputs are split

Thom Chiovoloni (Aug 02 2019 at 00:43, on Zulip):

I've written code (nothing outside of throwaway demos, thankfully) that assumed that using align_to to go from &[u8] to &[T] wouldn't return more than align_of::<T>() items in the first slice. I think I just asserted on this, but hearing that miri takes advantage of this to put everything in the first slice is fairly surprising

RalfJ (Aug 02 2019 at 06:05, on Zulip):

@Jake Goulding Ah, finally found the rustc issue for this: https://github.com/rust-lang/rust/issues/62420

RalfJ (Aug 02 2019 at 06:06, on Zulip):

@Thom Chiovoloni given that we explicitly document that code may not rely on getting the best possible splitting, is there anything else that could be done to make this less surprising?

Thom Chiovoloni (Aug 02 2019 at 06:14, on Zulip):

@RalfJ Err, why can't it always return slices such that the middle is the largest possible? I agree it's documented, but it seems like a needless restriction that will only trip people up.

Thom Chiovoloni (Aug 02 2019 at 06:18, on Zulip):

I'm probably missing something though, since I've mostly thought about this for the &[u8] input case

oli (Aug 02 2019 at 06:46, on Zulip):

The reason this exists is that const eval cannot return a middle slice because it doesn't know the alignment of an allocation

oli (Aug 02 2019 at 06:46, on Zulip):

Like the real alignment

oli (Aug 02 2019 at 06:48, on Zulip):

@nagisa returning anything is for miri to be able to check the code that should be the same, but the user may have screwed up

RalfJ (Aug 02 2019 at 06:58, on Zulip):

We don't tend to put needless restrictions into our language spec. We are not the adversary. ;)

RalfJ (Aug 02 2019 at 06:58, on Zulip):

@Thom Chiovoloni https://github.com/rust-lang/rust/issues/62420 mentions some reasons

RalfJ (Aug 02 2019 at 07:00, on Zulip):

@oli hm, I just realized that with dropping alignment checks from CTFE, the argument that we need this to make align_to a const fn becomes much weaker

RalfJ (Aug 02 2019 at 07:01, on Zulip):

OTOH, we still don't have an alignment to actually use

RalfJ (Aug 02 2019 at 07:04, on Zulip):

Yeah I think CTFE is a strong argument here.
@Thom Chiovoloni so imagine running this during CTFE. When the program wants a 1-aligned allocation, such as for Vec<u8>, we create an allocation that we remember to have alignment 1. We don't actually position this anywhere in the virtual address space -- after all we cannot know (in case this allocation end up in the output of CTFE and is used by the final program) where in the virtual address space this allocation will lie. So even for a 1-aligned allocation of size 256, where in the final program there will be some 8-aligned indices in that allocation, we cannot know which are 8-aligned during CTFE. So conservatively, we have to treat none of them as 8-aligned.

RalfJ (Aug 02 2019 at 07:05, on Zulip):

However, in @Jake Goulding's original test case here, the allocation actually is 8-aligned. So we could do better here.

RalfJ (Aug 02 2019 at 07:06, on Zulip):

I am still not convinced that align_to actually helps for what @Jake Goulding wants to achieve, but we might be able to get one obstacle out of the way.

oli (Aug 02 2019 at 07:25, on Zulip):

Oh, that makes sense. We can make align_to respect the allocation's alignment. I think we even have a FIXME for that somewhere

RalfJ (Aug 02 2019 at 07:25, on Zulip):

we do

RalfJ (Aug 02 2019 at 07:25, on Zulip):

this is not easy in general though

RalfJ (Aug 02 2019 at 07:26, on Zulip):

hm well maybe it's not so bad

RalfJ (Aug 02 2019 at 07:26, on Zulip):

if requested alignment is <= alloc.align: round up offset to next multiple of requested align

RalfJ (Aug 02 2019 at 07:26, on Zulip):

does that seem reasonable?

oli (Aug 02 2019 at 07:41, on Zulip):

Jup, or offset by zero if already aligned

RalfJ (Aug 02 2019 at 07:47, on Zulip):

that should be an instance of "round up to next multiple"

RalfJ (Aug 02 2019 at 07:48, on Zulip):

the one thing I am worried about here is that the code iterating over the middle part (if non-empty) might use SSE or so...^^

RalfJ (Aug 02 2019 at 07:56, on Zulip):

ah it gets more complicated becuase align_offset returns an offset it units of size_of::<T>()

RalfJ (Aug 02 2019 at 07:56, on Zulip):

so the best thing probably is to just run the libcore code...^^

RalfJ (Aug 02 2019 at 07:58, on Zulip):

see https://github.com/rust-lang/miri/issues/873

oli (Aug 02 2019 at 08:33, on Zulip):

Oh right

oli (Aug 02 2019 at 08:33, on Zulip):

OK, we are doing the right thing here

oli (Aug 02 2019 at 08:34, on Zulip):

We can't detect SSE vs non-sse

oli (Aug 02 2019 at 08:34, on Zulip):

Or lol, we can

oli (Aug 02 2019 at 08:35, on Zulip):

Make a snapshot, wait for failure, if it happens, revert and run normally

oli (Aug 02 2019 at 08:35, on Zulip):

Kill snapshot on return from frame that did snapshot

oli (Aug 02 2019 at 08:36, on Zulip):

:smiling_devil:

RalfJ (Aug 02 2019 at 09:12, on Zulip):

:skull:

RalfJ (Aug 02 2019 at 09:12, on Zulip):

maybe they are not using complex SSE instructions ;) from what I gathered, there's a small amount of them that should cover most code

Thom Chiovoloni (Aug 02 2019 at 20:00, on Zulip):

We don't tend to put needless restrictions into our language spec. We are not the adversary. ;)

Sure, but it's often in a language designer's best interest to be vague where possible, whereas it's in the programmer's best interest for those vagueness's to be concrete.

RalfJ (Aug 02 2019 at 20:12, on Zulip):

I like proving things about programs, so I don't usually like vague. ;)

Last update: Nov 22 2019 at 01:00UTC