rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.38k stars 339 forks source link

Make file descriptors into refcount references #3533

Closed Luv-Ray closed 5 months ago

Luv-Ray commented 5 months ago

fixes #3525

Remove fn dup in trait FileDescription, define struct FileDescriptor(Rc<RefCell<dyn FileDescription>>), and use BTreeMap<i32, FileDescriptor> in FdTable.


There are some refactors similar to the following form:

{  // origin:
    if let Some(file_descriptor) = this.machine.fds.get_mut(fd) {
        // write file_descriptor
        this.try_unwrap_io_result(result)
    } else {
        this.fd_not_found()
    }
}
{  // now:
    let Some(mut file_descriptor) = this.machine.fds.get_mut(fd) else {
        return this.fd_not_found();
    };
    // write file_descriptor
    drop(file_descriptor);
    this.try_unwrap_io_result(result)
}

The origin form can't compile because as using RefCell to get interior mutability, fn get_mut return Option<std::cell::RefMut<'_, dyn FileDescription>> instead of Option<&mut dyn FileDescription> now, and the deref_mut on file_descriptor: RefMut will cause borrow this as mutable more than once at a time. So this form of refactors and manual drops are are implemented to avoid borrowing this at the same time.

RalfJung commented 5 months ago

Thanks for the PR! That was quick. :)

define struct FileDescription(Rc<RefCell>)

This sounds the wrong way around. Many file descriptors map to fewer file descriptions, not the other way around.

Luv-Ray commented 5 months ago

Sorry for the bad naming. It has been rectified.

RalfJung commented 5 months ago

Awesome, I think we're done. :)

Please squash the commits together.

RalfJung commented 5 months ago

Thanks a lot!

@bors r+

bors commented 5 months ago

:pushpin: Commit 41b5a1b4e8294fc620bb3f4450aa01859325add0 has been approved by RalfJung

It is now in the queue for this repository.

bors commented 5 months ago

:hourglass: Testing commit 41b5a1b4e8294fc620bb3f4450aa01859325add0 with merge be0e91ebec475b6c7bf94a81b41db29ceb99c16a...

RalfJung commented 5 months ago

@Luv-Ray a possible next step, if you want, would be to give the read and write methods of FileDescriptor access to ecx: &mut InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>. This will be needed in the future to support blocking on these operations.

To make that work out in terms of borrowing, I think what we will have to do is clone the Rc in the read/write functions in unix/fd.rs, so that we have a fully owned FileDescriptor next to a uniquely borrowed InterpCx.

bors commented 5 months ago

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing be0e91ebec475b6c7bf94a81b41db29ceb99c16a to master...

Luv-Ray commented 5 months ago

Thanks for review! I think I can give it a try on the next step.