Refactor fd read/write

[tiif]

This PR passed the responsibility of reading to user supplied buffer and dest place to each implementation of FileDescription::read/write/pread/pwrite.

This is part of #3665.

[tiif]

Thanks for the feedback, I will slowly address them. So currently read_byte_helper is writing bytes to the user supplied buffer pointer, and write the total number of bytes written to the dest place. Right now, each FileDescription::read is responsible to writing to the buffer by calling read_byte_helper. A lot of error handling doesn't make sense yet, but feel free to suggest improvement while I am working on it.

Currently, I still need to:

1. let FileDescription::write to do the actual writing
2. get FileDescription::pread to do the actual reading by calling read_byte_helper
3. Simplify error handling, there are a few InterpResult that I want to get rid of

[tiif]

Wow, the only difference between 7721683 and dc74101 is just one inlined variable, but dc74101 somehow failed ./miri test fs but 7721683 passed? That's pretty unexpected.

EDIT: After a closer look, this should be caused by the variable bytes, when it is inlined, bytes.to_vec() gets evaluated before (&mut &self.file).read(bytes). Since to_vec creates a clone, so even though bytes is updated afterwards, the clone won't be updated.

I reverted the entire inline variable commit.

[RalfJung]

You changed evaluation order:

// Runs `bytes.to_vec()` after `Read::read.
let result = Read::read(&mut { self }, bytes);
ecx.read_byte_helper(ptr, bytes.to_vec(), result, dest)?;

// Runs `bytes.to_vec()` before `Read::read.
ecx.read_byte_helper(ptr, bytes.to_vec(), Read::read(&mut { self }, bytes), dest)?;

So maybe that explains the difference.

[tiif]

Is there any nice way to deref Pointer type to a place? So for eventfd, I perhaps can directly write to the user supplied buffer using write_int:

    fn read_byte_helper_ev(
        &mut self,
        buf_place: &MPlaceTy<'tcx>,
        read_val: u64,
        result: io::Result<usize>,
        dest: &MPlaceTy<'tcx>,
    ) -> InterpResult<'tcx> {
        let this = self.eval_context_mut();
        // `File::read` never returns a value larger than `count`, so this cannot fail.
        match result.map(|c| i64::try_from(c).unwrap()) {
            // try to pass this the write_ptr inside write
            // Pass the pointer inside the write function.
            Ok(read_bytes) => {
                // Write to the user supplied buffer.
                this.write_int(read_val, buf_place)?;  // <-- Here!!
                // Write to the function return value place.
                this.write_int(read_bytes, dest)?;
                return Ok(());
            }
            Err(e) => {
                this.set_last_error_from_io_error(e)?;
                this.write_int(-1, dest)?;
                return Ok(());
            }
        }
    }

Unfortunately I can't just use deref_pointer_as for the Pointer type because it doesn't implement Readable:

    |
71  |         let buf_place = ecx.deref_pointer_as(&ptr, ecx.machine.layouts.u64);
    |                             ---------------- ^^^^ the trait `rustc_const_eval::interpret::Readable<'_, machine::Provenance>` is not implemented for `rustc_const_eval::interpret::Pointer<std::option::Option<machine::Provenance>>`
    |                             |
    |                             required by a bound introduced by this call
    |
    = help: the following other types implement trait `rustc_const_eval::interpret::Readable<'tcx, Prov>`:
              rustc_const_eval::interpret::ImmTy<'tcx, Prov>
              rustc_const_eval::interpret::MPlaceTy<'tcx, Prov>
              rustc_const_eval::interpret::OpTy<'tcx, Prov>

We might also need to keep it as a Pointer type (instead of using OpTy) because we need to check pointer access in EvalContextExt::read

        // Check that the *entire* buffer is actually valid memory.
        this.check_ptr_access(buf, Size::from_bytes(count), CheckInAllocMsg::MemoryAccessTest)?;

[RalfJung]

Yes, ptr_to_mplace takes a Pointer and a Layout and makes it an MPlace.

Instead of passing it as a pointer, you could also try passing it as an ImmTy. That implements Readable. But I think passing as Pointer is probably the better choice.

[tiif]

Another idea here:

These are all the file description that implements FileDescription::read, and what they did is

1. fs.rs:
   • File::read takes in &mut [u8] and writes the bytes to it
2. fd.rs:
   • Read::read takes in &mut [u8] and writes the bytes to it
3. unnamed_socket.rs:
   • VecDeque::read take in &mut [u8] writes to it
4. eventfd.rs:
   • Turning u64 to &mut [u8]

So for the first three read, I perhaps can turn the user supplied buffer into &mut [u8] and let all these different read apis directly write to it, but I am still not sure how to handle eventfd. It is slightly different because currently what it is doing is turning u64 to &mut [u8] to match the implementation of all the other file description above.

[RalfJung]

I think they should all just take a Pointer. We already have a helper function (I think) to write a bunch of bytes to a pointer, so it should not be hard for fs and socket to use that.

[bors]

☔ The latest upstream changes (presumably #3867) made this pull request unmergeable. Please resolve the merge conflicts.

[tiif]

This is partially blocked by rust-lang/rust#130215 as I'd need the ptr_to_mplace_unalignedto pass the tests.

[tiif]

@rustbot ready

[oli-obk]

Please squash the commits, then r=me

@bors delegate+

[bors]

✌️ @tiif, you can now approve this pull request!

If @oli-obk told you to "r=me" after making some further change, please make that change, then do @bors r=@oli-obk

[tiif]

@bors r=@oli-obk

[bors]

📌 Commit de515da has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit de515da with merge 8311557...

[RalfJung]

@bors r-
Since since affects Miri globally, I'm going to have some comments as well, sorry.

[RalfJung]

Please do this after the size check.

[tiif]

Unfortunately inside the size check, return return_read_bytes_and_count_ev(&buf_place, None, result, dest, ecx); is called, so we need buf_place. I probably should separate these two code path in return_read_bytes_and_count into different function.

        match result {
            Ok(read_bytes) => {
                // If reading to `bytes` did not fail, we write those bytes to the buffer.
                // Crucially, if fewer than `bytes.len()` bytes were read, only write
                // that much into the output buffer!
                this.write_bytes_ptr(buf, bytes[..read_bytes].iter().copied())?;
                // The actual read size is always less than what got originally requested so this cannot fail.
                this.write_int(u64::try_from(read_bytes).unwrap(), dest)?;
                return Ok(());
            }
            Err(e) => {
                this.set_last_error_from_io_error(e)?;
                this.write_int(-1, dest)?;
                return Ok(());
            }
        }

This is the same issue as #3852 (comment).

[RalfJung]

Why is return_written_byte_count_or_error a helper function? Every single caller fixes either Err or Ok so this is funneling through a Result without a good reason, it just makes the code harder to follow.

If there is any helper we want here, it is a new very generic helper that sets the last errors and also writes -1 to dest. But that's probably better done in a future PR, for now you can just copy those 2 lines here I would say, like we do everywhere else.

[RalfJung]

Why is return_read_bytes_and_count_ev a helper function? Every single caller fixes either Err or Ok so this is funneling through a Result without a good reason, it just makes the code harder to follow.

If there is any helper we want here, it is a new very generic helper that sets the last errors and also writes -1 to dest. But that's probably better done in a future PR, for now you can just copy those 2 lines here I would say, like we do everywhere else.

[bors]

☀️ Try build successful - checks-actions
Build commit: 8311557 (8311557d994abbd31c9d5b75a4a3f89be3a40697)

[tiif]

Instead of separating the helper with read and write, it would be better to separate it according to this two code path:

        match result {
            Ok(read_bytes) => {
                // If reading to `bytes` did not fail, we write those bytes to the buffer.
                // Crucially, if fewer than `bytes.len()` bytes were read, only write
                // that much into the output buffer!
                this.write_bytes_ptr(buf, bytes[..read_bytes].iter().copied())?;
                // The actual read size is always less than what got originally requested so this cannot fail.
                this.write_int(u64::try_from(read_bytes).unwrap(), dest)?;
                return Ok(());
            }
            Err(e) => {
                this.set_last_error_from_io_error(e)?;
                this.write_int(-1, dest)?;
                return Ok(());
            }
        }

or use try_unwrap_io_result.

As this could be done in a new PR,

@rustbot ready

[RalfJung]

Thanks! I've done some further minor tweaks, mostly to remove a bunch of to_owned() calls that were completely unnecessary. Please think a bit about the involved lifetimes to understand whether getting full ownership is needed or not -- or at least try it without to_owned before adding a call to that function. :)

Also I re-did eventfd a bit, see the last commit. I didn't think that helper function was helpful so I removed it.

Further improvements are possible I think (specifically some sort of helper for the quite common case of "set errno and return -1"), but that can be a separate PR.

@bors r+

[bors]

📌 Commit d7741a9 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit d7741a9 with merge 7ded26a...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 7ded26a to master...