Use libcore's align_offset

[pvdrz]

Related issue: #873

[bjorn3]

I think it should be possible to remove that whole if block, as that would fallback to std impl, which now works.

[pvdrz]

I think it should be possible to remove that whole if block, as that would fallback to std impl, which now works.

which if block? do you mean this one?:

if this.tcx.lang_items().align_offset_fn() == Some(instance.def.def_id()) { ... }

Edit: Ohhh... I think I understand now, so if we remove that block, miri will just load the same code from libcore... So I guess that makes this PR pointless :P

[pvdrz]

@bjorn3 I removed the whole block and ran the test suite. However, one of the tests failed

error[E0080]: Miri evaluation error: tried to access memory with alignment 1, but alignment 8 is required
    --> /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libcore/slice/mod.rs:5302:5
     |
5302 |     &*ptr::slice_from_raw_parts(data, len)
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Miri evaluation error: tried to access memory with alignment 1, but alignment 8 is required
     |
     = note: inside call to `std::slice::from_raw_parts::<(usize, usize)>` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libcore/slice/mod.rs:2367:14
     = note: inside call to `core::slice::<impl [u8]>::align_to::<(usize, usize)>` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libcore/slice/memchr.rs:98:44
note: inside call to `core::slice::memchr::memrchr` at $DIR/memchr.rs:43:25
    --> $DIR/memchr.rs:43:25
     |
43   |     assert_eq!(Some(0), memrchr(b'z', b"zaaaa"));
     |                         ^^^^^^^^^^^^^^^^^^^^^^^
note: inside call to `matches_end_reversed` at $DIR/memchr.rs:83:5
    --> $DIR/memchr.rs:83:5
     |
83   |     matches_end_reversed();
     |     ^^^^^^^^^^^^^^^^^^^^^^
     = note: inside call to `main` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:64:34
     = note: inside call to closure at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:52:53
     = note: inside call to closure at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/panicking.rs:296:40
     = note: inside call to `std::panicking::try::do_call::<[closure@DefId(1:5902 ~ std[3899]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/panicking.rs:292:5
     = note: inside call to `std::panicking::try::<i32, [closure@DefId(1:5902 ~ std[3899]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/panic.rs:394:9
     = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:5902 ~ std[3899]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:52:25
     = note: inside call to `std::rt::lang_start_internal` at /home/christian/.rustup/toolchains/miri/lib/rustlib/src/rust/src/libstd/rt.rs:64:5
     = note: inside call to `std::rt::lang_start::<()>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.

This didn't happen with the changes i did in this branch (and tbh I'm not sure why).

Edit: In fact all the tests pass in local with the current branch. I'm not sure why this std error is showing in CI but I think it has something to do with using intrinsics.

[RalfJung]

I don't have time to review this (and likely won't until I am done traveling, which is in about a month), but I think this needs careful testing before landing. In particular, I think https://github.com/RalfJung/miri-test-libstd should be run with this before r+'ing. I am worried that making align_offset in Miri too smart will mean alignment checks will fail (because, unsurprisingly, code will actually exploit the better alignment provided by align_offset, but Miri does not actually take into account the integer address of a pointer when doing alignment checks), and while that is fixable, it comes at a cost of precision. See rust-lang/rust#62420 for a detailed discussion. (And I think that is what you are running into here.)

EDIT: Ah I see, the code tries to handle this by only doing something smart for sufficiently aligned allocations. That should work. It needs careful testing though.

[pvdrz]

I don't have time to review this (and likely won't until I am done traveling, which is in about a month), but I think this needs careful testing before landing. In particular, I think https://github.com/RalfJung/miri-test-libstd should be run with this before r+'ing. I am worried that making align_offset in Miri too smart will mean alignment checks will fail (because, unsurprisingly, code will actually exploit the better alignment provided by align_offset, but Miri does not actually take into account the integer address of a pointer when doing alignment checks), and while that is fixable, it comes at a cost of precision. See rust-lang/rust#62420 for a detailed discussion. (And I think that is what you are running into here.)

EDIT: Ah I see, the code tries to handle this by only doing something smart for sufficiently aligned allocations. That should work. It needs careful testing though.

So basically this happens because we're running the libcore code without checking that the current alignment is at least the required one?

EDIT: I think this is ready for testing, I ran the all the miri tests I had in local (it's still failing in travis and at this point I'm not even sure why) and I think we can test this against stdlib

[oli-obk]

CI is failing on master, I'll investigate

[pvdrz]

After rebasing to use the most recent rustc-version CI finally passed, I ran Ralf's tests locally and there were no errors.

[oli-obk]

@bors r+

[bors]

📌 Commit 55863cb has been approved by oli-obk

[bors]

⌛ Testing commit 55863cb with merge d0a1050...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing d0a1050 to master...