Skip to content

Insert FileDescription instead of FileDescriptor in insert_fd #3763

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Insert FileDescription instead of FileDescriptor in insert_fd #3763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ impl<'tcx> MiriMachine<'tcx> {
tls: TlsData::default(),
isolated_op: config.isolated_op,
validate: config.validate,
fds: shims::FdTable::new(config.mute_stdout_stderr),
fds: shims::FdTable::init(config.mute_stdout_stderr),
dirs: Default::default(),
layouts,
threads,
Expand Down
27 changes: 14 additions & 13 deletions src/shims/unix/fd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,6 @@ impl FileDescription for NullOutput {
pub struct FileDescriptor(Rc<RefCell<Box<dyn FileDescription>>>);

impl FileDescriptor {
pub fn new<T: FileDescription>(fd: T) -> Self {
FileDescriptor(Rc::new(RefCell::new(Box::new(fd))))
}

pub fn borrow(&self) -> Ref<'_, dyn FileDescription> {
Ref::map(self.0.borrow(), |fd| fd.as_ref())
}
Expand Down Expand Up @@ -227,20 +223,25 @@ impl VisitProvenance for FdTable {
}

impl FdTable {
pub(crate) fn new(mute_stdout_stderr: bool) -> FdTable {
let mut fds: BTreeMap<_, FileDescriptor> = BTreeMap::new();
fds.insert(0i32, FileDescriptor::new(io::stdin()));
fn new() -> Self {
FdTable { fds: BTreeMap::new() }
}
pub(crate) fn init(mute_stdout_stderr: bool) -> FdTable {
let mut fds = FdTable::new();
fds.insert_fd(io::stdin());
if mute_stdout_stderr {
fds.insert(1i32, FileDescriptor::new(NullOutput));
fds.insert(2i32, FileDescriptor::new(NullOutput));
assert_eq!(fds.insert_fd(NullOutput), 1);
assert_eq!(fds.insert_fd(NullOutput), 2);
} else {
fds.insert(1i32, FileDescriptor::new(io::stdout()));
fds.insert(2i32, FileDescriptor::new(io::stderr()));
assert_eq!(fds.insert_fd(io::stdout()), 1);
assert_eq!(fds.insert_fd(io::stderr()), 2);
}
FdTable { fds }
fds
}

pub fn insert_fd(&mut self, file_handle: FileDescriptor) -> i32 {
/// Insert a file descriptor to the FdTable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment seems to contradict the signature? This is a file description, isn't it?

pub fn insert_fd<T: FileDescription>(&mut self, fd: T) -> i32 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the motivation is for this API change, but it seems odd to me. Note that insert_fd_with_min_fd wasn't changed, so now we have two equally-named APIs doing very different things. And insert_fd_with_min_fd can't be changed since indeed it is used to insert duplicates of existing file descriptions that shouldn't be wrapped in a new Rc.

This change is needed by #3712.

I don't understand how it is possible for this change to be needed. This change means that the API is now strictly less powerful than it was before. The only argument I can currently imagine for this change is ergonomics, and I don't agree that's worth it given the API inconsistencies this introduces.

IMO this should be reverted, both insert_fd and insert_fd_with_min_fd should take a FileDescriptionRef.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API didn't need to be as powerful as it was, so this seems good (restricting to the use case we need). It's also the only sane design I see for having each FileDescriptionrRef carry a unique id that we can use instead of its address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We considered do it in FileDescriptionRef::new, but then that needs access to the InterpCx or at least the FdTable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pure coincidence that we have no operation which creates a 2nd file descriptor for an existing file description, and calls this function. In fact arguably this here should just call insert_fd.

And having two insert_fd functions that don't even take the same kind of argument seems like an obviously bad API to me, way worse than an API that is slightly more general than what we happen to need right now.

Creating a FileDescriptionRef from a FileDescription should be a completely independent operation from inserting a FileDescriptionRef into the FD table.

Copy link
Member

@RalfJung RalfJung Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also the only sane design I see for having each FileDescriptionrRef carry a unique id that we can use instead of its address.

Just have FileDescriptionRef::new take a reference like ecx: &mut MiriInterpCx<'tcx> and then it can do the ID assignment.

Then we can maybe have fd.insert_new_fd for the common pattern of "create new ref and insert it".

Copy link
Member

@RalfJung RalfJung Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could call the functions insert_new_fd and insert_existing_fd where only the latter supports a "minimum file description" and then we don't even have to change their implementation. (Though i think factoring FileDescriptionRef::new into a separate operation, even if it remains private, would still help to make the conceptual structure more clear.)

The PR description should have mentioned the point about FileDescriptionRef::new needing access to global state in #3712, that would have avoided some confusion. :)

let file_handle = FileDescriptor(Rc::new(RefCell::new(Box::new(fd))));
self.insert_fd_with_min_fd(file_handle, 0)
}

Expand Down
13 changes: 4 additions & 9 deletions src/shims/unix/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ use crate::shims::unix::*;
use crate::*;
use shims::time::system_time_to_duration;

use self::fd::FileDescriptor;

#[derive(Debug)]
struct FileHandle {
file: File,
Expand Down Expand Up @@ -452,10 +450,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
return Ok(-1);
}

let fd = options.open(path).map(|file| {
let fh = &mut this.machine.fds;
fh.insert_fd(FileDescriptor::new(FileHandle { file, writable }))
});
let fd = options
.open(path)
.map(|file| this.machine.fds.insert_fd(FileHandle { file, writable }));

this.try_unwrap_io_result(fd)
}
Expand Down Expand Up @@ -1547,9 +1544,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

match file {
Ok(f) => {
let fh = &mut this.machine.fds;
let fd =
fh.insert_fd(FileDescriptor::new(FileHandle { file: f, writable: true }));
let fd = this.machine.fds.insert_fd(FileHandle { file: f, writable: true });
return Ok(fd);
}
Err(e) =>
Expand Down
4 changes: 1 addition & 3 deletions src/shims/unix/linux/epoll.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ use rustc_data_structures::fx::FxHashMap;
use crate::shims::unix::*;
use crate::*;

use self::shims::unix::fd::FileDescriptor;

/// An `Epoll` file descriptor connects file handles and epoll events
#[derive(Clone, Debug, Default)]
struct Epoll {
Expand Down Expand Up @@ -66,7 +64,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
);
}

let fd = this.machine.fds.insert_fd(FileDescriptor::new(Epoll::default()));
let fd = this.machine.fds.insert_fd(Epoll::default());
Ok(Scalar::from_i32(fd))
}

Expand Down
6 changes: 2 additions & 4 deletions src/shims/unix/linux/eventfd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ use rustc_target::abi::Endian;
use crate::shims::unix::*;
use crate::{concurrency::VClock, *};

use self::shims::unix::fd::FileDescriptor;

// We'll only do reads and writes in chunks of size u64.
const U64_ARRAY_SIZE: usize = mem::size_of::<u64>();

Expand Down Expand Up @@ -180,11 +178,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
throw_unsup_format!("eventfd: encountered unknown unsupported flags {:#x}", flags);
}

let fd = this.machine.fds.insert_fd(FileDescriptor::new(Event {
let fd = this.machine.fds.insert_fd(Event {
counter: val.into(),
is_nonblock,
clock: VClock::default(),
}));
});
Ok(Scalar::from_i32(fd))
}
}
6 changes: 2 additions & 4 deletions src/shims/unix/socket.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ use std::rc::{Rc, Weak};
use crate::shims::unix::*;
use crate::{concurrency::VClock, *};

use self::fd::FileDescriptor;

/// The maximum capacity of the socketpair buffer in bytes.
/// This number is arbitrary as the value can always
/// be configured in the real system.
Expand Down Expand Up @@ -221,9 +219,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
};

let fds = &mut this.machine.fds;
let sv0 = fds.insert_fd(FileDescriptor::new(socketpair_0));
let sv0 = fds.insert_fd(socketpair_0);
let sv1 = fds.insert_fd(socketpair_1);
let sv0 = Scalar::from_int(sv0, sv.layout.size);
let sv1 = fds.insert_fd(FileDescriptor::new(socketpair_1));
let sv1 = Scalar::from_int(sv1, sv.layout.size);

this.write_scalar(sv0, &sv)?;
Expand Down