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

ptr::swap_nonoverlapping can no longer copy pointers bytewise in latest miri #2118

Closed
tower120 opened this issue May 13, 2022 · 15 comments
Closed

Comments

@tower120
Copy link

tower120 commented May 13, 2022

After updating miri from pre-1.60 to 1.62, I start getting those errors:
https://github.com/tower120/any_vec/runs/6425015915?check_suite_focus=true
(that branch runs OK with older MIRI)

Previously, miri had troubles with any raw pointer deference operations, for me.
But I "fixed" that with ptr::copy_nonoverlapping - and it worked for miri.
It looks like there was some support for std::ptr::* operations on miri side... Which is now gone...

@RalfJung
Copy link
Member

Looks like you are using the 2022-04-04 nightly, which is quite old. We had some trouble with copy_nonoverlapping a few weeks ago, but that has since been fixed.

Can you try again with the latest nightly?

@RalfJung
Copy link
Member

RalfJung commented May 13, 2022

Or maybe I looked into the wrong parts of the log? But this looks a lot like rust-lang/rust#94371...

Do you have a self-contained example to reproduce the problem? That would help with debugging.

copy_nonoverlapping is definitely still supported, but you cannot use it to copy a part of a pointer -- you have to copy the entire thing in one go. But that has always been like that, so not sure how the upgrade could have changed anything.

@tower120
Copy link
Author

Can you try again with the latest nightly?

I'm not very good with rustup, but aren't it says 2022-05-11?

nightly-x86_64-unknown-linux-gnu installed - rustc 1.62.0-nightly (6dd68402c 2022-05-11)

https://github.com/tower120/any_vec/runs/6425015915?check_suite_focus=true#step:3:35

On my local Windows machine I can't install latest nightly, because it missing MIRI.

@tower120
Copy link
Author

copy_nonoverlapping is definitely still supported, but you cannot use it to copy a part of a pointer -- you have to copy the entire thing in one go...

It does copy entire element with alignment, but represented as u8 byte array.

@tower120
Copy link
Author

Here is working branch https://github.com/tower120/any_vec/blob/exp_drop_after_remove

MIRI problem occurs here:
https://github.com/tower120/any_vec/blob/exp_drop_after_remove/src/lib.rs#L462

I don't have smaller example at hand now.

@RalfJung
Copy link
Member

That's a lot of code, I won't really have time to dig into that I am afraid. :/

(FWIW, it is a bad idea to represent pointers as a u8 array. u8 as an integer type does not carry provenance. The recommended type to represent data of arbitrary type is MaybeUninit<_>. But that will probably not make a difference here.)

@tower120
Copy link
Author

tower120 commented May 13, 2022

Hmm, I tried to replace swap with 3 ptr::copy_nonoverlapping and intermediate vec - and it worked!

// same as copy_bytes but for swap_nonoverlapping.
#[inline]
unsafe fn swap_bytes(src: *mut u8, dst: *mut u8, count: usize){
    // MIRI hack
    if cfg!(miri) {
        let mut tmp = Vec::<u8>::new();
        tmp.resize(count, 0);
    
        // src -> tmp
        ptr::copy_nonoverlapping(src, tmp.as_mut_ptr(), count);
        // dst -> src
        ptr::copy_nonoverlapping(dst, src, count);
        // tmp -> dst
        ptr::copy_nonoverlapping(tmp.as_ptr(), dst, count);
    
        return;
    }

    // this does not work with MIRI
    // "unable to turn pointer into raw bytes"
    for i in 0..count{
        let src_pos = src.add(i);
        let dst_pos = dst.add(i);

        let tmp = *src_pos;
        *src_pos = *dst_pos;
        *dst_pos = tmp;
    }
}

The thing is

ptr::swap_nonoverlapping(src, dst, count);

does not work...

@RalfJung
Copy link
Member

Btw:

// This is faster then ptr::copy_nonoverlapping,
// when count is runtime value, and count is small.

That sounds like a bug worth reporting against rustc; a naive manual impl should not be faster than the stdlib method.

@tower120
Copy link
Author

tower120 commented May 13, 2022

The recommended type to represent data of arbitrary type is MaybeUninit<_>.

What about type-erased data?

@RalfJung
Copy link
Member

That's what I mean -- the correct type to store type-erased data in Rust is an array of MaybeUninit<u8>.

A u8 (a) has to be initialized, and (b) cannot be a part of a pointer ("byte with provenance").

@RalfJung
Copy link
Member

Looking at the implementation of swap_nonoverlapping, this is indeed not surprising: it just copies the data one T at a time. Your T is u8, so this copies the pointers byte-per-byte, and Miri does not support this.

swap_nonoverlapping used to be implemented in a different way, that's why you didn't see these problems before.

I wonder if this is even a correct implementation, since it means this is a typed copy...

@RalfJung RalfJung changed the title ptr::copy_nonoverlapping become "unsupported operation" in latest miri ptr::swap_nonoverlapping become "unsupported operation" in latest miri May 13, 2022
@tower120
Copy link
Author

If you found the problem - you don't need that branch anymore?

@RalfJung
Copy link
Member

I added a comment in rust-lang/rust#63159 (comment) to further discuss this.

For now, you can work around the problem by using swap rather than swap_nonoverlapping.

@RalfJung
Copy link
Member

Oh, but swap doesn't take a parameter to do this N times... your work-around with 3 ptr::copy_nonoverlapping is probably the best option for now then.

@RalfJung RalfJung changed the title ptr::swap_nonoverlapping become "unsupported operation" in latest miri ptr::swap_nonoverlapping can no longer copy pointers bytewise in latest miri May 13, 2022
@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2022

Closing this in favor of #2181.

@RalfJung RalfJung closed this as completed Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants