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

Project idea: strongly typed file descriptor integers and deduplication of various checks around them #3772

Open
oli-obk opened this issue Jul 29, 2024 · 4 comments
Labels
A-files Area: related to files, paths, sockets, file descriptors, or handles C-cleanup Category: cleaning up our code E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Jul 29, 2024

This is less of a fundamental/large-in-behavior change and more a very bitrotty change with possibly not-really-deduplicating-that-much changes that hopefully still make the code more manageable.

Problem statement

Currently we represent file descriptors just as i32 within miri. We mix them with various negative integer values for signalling errors, just like libc does. We also duplicate a lot of error handling logic like

let Some(fd) = this.machine.fds.some_getter(fd) else {
   return this.fd_not_found();
};

and

Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))

Solution parts

  1. consistently use Scalar instead of i32 as the return type of foreign function handlers (groundwork for the following, as we inconsistently use i32 and Scalar. Even using Scalar when i32 would definitely suffice, not just when it could be a pointer).
  2. wrap all i32 known-to-be-file-descriptors in a newtype struct together with some convenience functions for reading them and getting the integer back out (into a scalar)
  3. return io::Result<FileDescriptionRef> (or the reference versions) from the various getters, instead of Option<FileDescriptionRef>. Forward that where useful, avoiding explicit -1 returns paired with set_last_error, instead handling it with try_unwrap_io_result
  4. add get_mut, get, and dup methods to FdTable that return io::Result instead of Option, even though the only io error they ever return is NotFound, allowing us to ? away their result
@RalfJung
Copy link
Member

RalfJung commented Jul 29, 2024

It would also be good to be able to write something like return this.set_last_error_and_return_neg1(...); which would set the last error and then return -1, as that's such a common pattern.

EDIT: Oh, that's what try_unwrap_io_result does. I don't think we are using it everywhere we could, and maybe the name could be improved. It also requires an io::Result<T> but often we want to set EINVAL or some other named constant from libc.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 29, 2024

These constants (almost?) always have a corresponding ErrKind, so I think we can transform them to io::Result usage. I mean, that is how we convert io::Error to an error value to set, we eval the corresponding constant

@RalfJung
Copy link
Member

RalfJung commented Jul 29, 2024 via email

@oli-obk
Copy link
Contributor Author

oli-obk commented Jul 29, 2024

Separate idea: instead of hardcoding the io::Result to integer conversion logic, we add a new enum around io::Error that has variants for the extra cases we need. Then we use that enum as the error type everywhere

bors added a commit that referenced this issue Jul 30, 2024
Use Scalar consistently in foreign item emulation

Step 0 of #3772

This just makes the code consistent. While we could also go for consistency the other way, that would only allow us to use integers in some cases where we use `Scalar` right now, because `Scalar` can also hold pointers where applicable.

There's also no danger in messing up (even though we do lose some compile-time checks), as `Scalar` knows the size of the integer stored within, so it will check that against the destination when it is written. In fact, this makes the checks much stronger compared with `write_int`, which just checks that the integer fits into the destination size.
@oli-obk oli-obk added the E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available label Jul 31, 2024
@RalfJung RalfJung added C-cleanup Category: cleaning up our code A-files Area: related to files, paths, sockets, file descriptors, or handles labels Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-files Area: related to files, paths, sockets, file descriptors, or handles C-cleanup Category: cleaning up our code E-good-second-issue A good issue to pick up if you've already seen some parts of Miri, mentoring is available
Projects
None yet
Development

No branches or pull requests

2 participants