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.

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

Jump to bottom

Add a helper for directly writing io::Result to places #3777

Closed
wants to merge 3 commits into from
Closed

Add a helper for directly writing io::Result to places #3777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe82f5a
Remove unnecessary method call
oli-obk Jul 30, 2024
c87ca20
Add a helper for directly writing `io::Result` to places
oli-obk Jul 30, 2024
7860557
Generalize `write_io_result` to arbitrary `Ok` payloads
oli-obk Jul 30, 2024
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
57 changes: 51 additions & 6 deletions src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use std::cmp;
use std::collections::BTreeSet;
use std::iter;
use std::num::NonZero;
use std::sync::Mutex;
use std::time::Duration;
use std::{cmp, io};

use rand::RngCore;

Expand Down Expand Up @@ -857,19 +857,32 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
///
/// This function uses `T: From<i32>` instead of `i32` directly because some IO related
/// functions return different integer types (like `read`, that returns an `i64`).
fn try_unwrap_io_result<T: From<i32>>(
fn try_unwrap_io_result<T: WriteIoResult>(
Copy link
Member

Choose a reason for hiding this comment

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

The doc comment above is outdated now.

&mut self,
result: std::io::Result<T>,
) -> InterpResult<'tcx, T> {
) -> InterpResult<'tcx, Scalar> {
match result {
Ok(ok) => Ok(ok),
Ok(ok) => Ok(ok.into_scalar()),
Err(e) => {
self.eval_context_mut().set_last_error_from_io_error(e)?;
Ok((-1).into())
self.set_last_error_from_io_error(e)?;
Ok(T::neg_one())
}
}
}

/// Write an io result to a place.
/// No error means writing via `WriteIoResult`
/// Otherwise write `-1` and set the last error.
#[inline(always)]
fn write_io_result<T: WriteIoResult>(
&mut self,
val: io::Result<T>,
dest: &impl Writeable<'tcx, Provenance>,
) -> InterpResult<'tcx> {
let result = self.try_unwrap_io_result(val)?;
self.eval_context_mut().write_scalar(result, dest)
}

/// Dereference a pointer operand to a place using `layout` instead of the pointer's declared type
fn deref_pointer_as(
&self,
Expand Down Expand Up @@ -1434,3 +1447,35 @@ pub(crate) fn windows_check_buffer_size((success, len): (bool, u64)) -> u32 {
u32::try_from(len).unwrap()
}
}

pub trait WriteIoResult {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a doc comment. Also the trait name is kind of confusing, maybe a doc comment will help coming up with a better one.

fn into_scalar(self) -> Scalar;
fn neg_one() -> Scalar;
}

impl WriteIoResult for () {
fn into_scalar(self) -> Scalar {
Scalar::from_i32(0)
}
fn neg_one() -> Scalar {
Scalar::from_i32(-1)
}
}

impl WriteIoResult for i32 {
fn into_scalar(self) -> Scalar {
Scalar::from_i32(self)
}
fn neg_one() -> Scalar {
Scalar::from_i32(-1)
}
}

impl WriteIoResult for i64 {
fn into_scalar(self) -> Scalar {
Scalar::from_i64(self)
}
fn neg_one() -> Scalar {
Scalar::from_i64(-1)
}
}
11 changes: 4 additions & 7 deletions src/shims/unix/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::env;
use std::ffi::{OsStr, OsString};
use std::io::ErrorKind;
use std::mem;
use std::{env, io};

use rustc_data_structures::fx::FxHashMap;
use rustc_middle::ty::layout::LayoutOf;
Expand Down Expand Up @@ -236,21 +236,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(Pointer::null())
}

fn chdir(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
fn chdir(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, io::Result<()>> {
let this = self.eval_context_mut();
this.assert_target_os_is_unix("chdir");

let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?;

if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`chdir`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;

return Ok(Scalar::from_i32(-1));
return Ok(Err(ErrorKind::PermissionDenied.into()));
}

let result = env::set_current_dir(path).map(|()| 0);
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
Ok(env::set_current_dir(path))
}

/// Updates the `environ` static.
Expand Down
13 changes: 9 additions & 4 deletions src/shims/unix/fd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let result = file_descriptor.flock(this.machine.communicate(), parsed_op)?;
drop(file_descriptor);
// return `0` if flock is successful
let result = result.map(|()| 0i32);
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
this.try_unwrap_io_result(result)
}

fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
Expand Down Expand Up @@ -438,7 +437,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let result = file_description.close(this.machine.communicate())?;
// return `0` if close is successful
let result = result.map(|()| 0i32);
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
this.try_unwrap_io_result(result)
}

/// Function used when a file descriptor does not exist. It returns `Ok(-1)`and sets
Expand Down Expand Up @@ -564,7 +563,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
drop(fd);

let result = result?.map(|c| i64::try_from(c).unwrap());
Ok(Scalar::from_target_isize(this.try_unwrap_io_result(result)?, this))
match result {
Ok(written_bytes) => Ok(Scalar::from_target_isize(written_bytes, this)),
Err(e) => {
this.set_last_error_from_io_error(e)?;
Ok(Scalar::from_target_isize(-1, this))
Copy link
Member

Choose a reason for hiding this comment

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

This code got worse than it used to be, what's happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are writing target sized integers, so i'll need to add a helper type to wrap the i64s in. The read method already could not be ported before and was written out fully. I want to fix both at the same time

}
}
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/shims/unix/foreign_items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
"chdir" => {
let [path] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let result = this.chdir(path)?;
this.write_scalar(result, dest)?;
this.write_io_result(result, dest)?;
Copy link
Member

@RalfJung RalfJung Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it really such a big win to move the io::Result handling out of the shim?

A priori I think I'd prefer the version that returns a Scalar, as that matches the C function we're implementing and since it is a pattern we can use for all functions, not just IO functions. We can have more helpers inside those functions that make them easier to write and avoid hard-coding -1 anywhere, that seems like a more promising approach to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially wanted to add such helpers, but then it got annoying for handling i64, i32, and i64 but is actually target usize, as that would require explicit type annotations and reliance on runtime checks (Scalar size vs destination size). I then thought it would be nicer to keep some semblence of result handling around, as now all the return types must match up again, like before moving to scalars. I'll finish this PR to its conclusion and open a separate PR for just using helpers everywhere so you can compare.

}
"getpid" => {
let [] = this.check_shim(abi, Abi::C { unwind: false}, link_name, args)?;
Expand Down Expand Up @@ -188,12 +188,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
"unlink" => {
let [path] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let result = this.unlink(path)?;
this.write_scalar(result, dest)?;
this.write_io_result(result, dest)?;
}
"symlink" => {
let [target, linkpath] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let result = this.symlink(target, linkpath)?;
this.write_scalar(result, dest)?;
this.write_io_result(result, dest)?;
}
"rename" => {
let [oldpath, newpath] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand All @@ -203,12 +203,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
"mkdir" => {
let [path, mode] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let result = this.mkdir(path, mode)?;
this.write_scalar(result, dest)?;
this.write_io_result(result, dest)?;
}
"rmdir" => {
let [path] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let result = this.rmdir(path)?;
this.write_scalar(result, dest)?;
this.write_io_result(result, dest)?;
}
"opendir" => {
let [name] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand Down
54 changes: 23 additions & 31 deletions src/shims/unix/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
.open(path)
.map(|file| this.machine.fds.insert_fd(FileHandle { file, writable }));

Ok(Scalar::from_i32(this.try_unwrap_io_result(fd)?))
this.try_unwrap_io_result(fd)
}

fn lseek64(&mut self, fd: i32, offset: i128, whence: i32) -> InterpResult<'tcx, Scalar> {
Expand Down Expand Up @@ -584,31 +584,28 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
.map(|offset| i64::try_from(offset).unwrap());
drop(file_description);

let result = this.try_unwrap_io_result(result)?;
Ok(Scalar::from_i64(result))
this.try_unwrap_io_result(result)
}

fn unlink(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
fn unlink(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, io::Result<()>> {
let this = self.eval_context_mut();

let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?;

// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`unlink`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
return Ok(Scalar::from_i32(-1));
return Ok(Err(ErrorKind::PermissionDenied.into()));
}

let result = remove_file(path).map(|_| 0);
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
Ok(remove_file(path))
}

fn symlink(
&mut self,
target_op: &OpTy<'tcx>,
linkpath_op: &OpTy<'tcx>,
) -> InterpResult<'tcx, Scalar> {
) -> InterpResult<'tcx, io::Result<()>> {
#[cfg(unix)]
fn create_link(src: &Path, dst: &Path) -> std::io::Result<()> {
std::os::unix::fs::symlink(src, dst)
Expand All @@ -627,12 +624,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`symlink`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
return Ok(Scalar::from_i32(-1));
return Ok(Err(ErrorKind::PermissionDenied.into()));
}

let result = create_link(&target, &linkpath).map(|_| 0);
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
Ok(create_link(&target, &linkpath))
}

fn macos_fbsd_stat(
Expand Down Expand Up @@ -934,10 +929,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

let result = rename(oldpath, newpath).map(|_| 0);

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

fn mkdir(&mut self, path_op: &OpTy<'tcx>, mode_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
fn mkdir(
&mut self,
path_op: &OpTy<'tcx>,
mode_op: &OpTy<'tcx>,
) -> InterpResult<'tcx, io::Result<()>> {
let this = self.eval_context_mut();

#[cfg_attr(not(unix), allow(unused_variables))]
Expand All @@ -952,8 +951,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`mkdir`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
return Ok(Scalar::from_i32(-1));
return Ok(Err(ErrorKind::PermissionDenied.into()));
}

#[cfg_attr(not(unix), allow(unused_mut))]
Expand All @@ -967,26 +965,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
builder.mode(mode);
}

let result = builder.create(path).map(|_| 0i32);

Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
Ok(builder.create(path))
}

fn rmdir(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
fn rmdir(&mut self, path_op: &OpTy<'tcx>) -> InterpResult<'tcx, io::Result<()>> {
let this = self.eval_context_mut();

let path = this.read_path_from_c_str(this.read_pointer(path_op)?)?;

// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`rmdir`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
return Ok(Scalar::from_i32(-1));
return Ok(Err(ErrorKind::PermissionDenied.into()));
Copy link
Member

Choose a reason for hiding this comment

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

For instance, this could become return Ok(this.return_io_error(ErrorKind::PermissionDenied.into())) where return_io_error sets the errno and then returns -1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, but I'd still need to say how big the scalar is supposed to be. Not the end of the world, but also not nice

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, the type...

Another option might be to pass dest to these functions and not have them return anything. A function that blocks usually already needs to do that anyway. Then they can use your new write_io_result internally for when they naturally have an io::Result, and some write_io_error for cases like this where we hard-code an error.

}

let result = remove_dir(path).map(|_| 0i32);

Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
Ok(remove_dir(path))
}

fn opendir(&mut self, name_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this one still return Scalar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It (and others) return error codes via this.eval_libc("EACCES"); without a corresponding ErrorKind existing in libcore. I plan to use an error enum that wraps io errors and has variants for all the eval_libc errors we use in miri. May turn out overkill, but I wanna see how it looks before discarding it

Copy link
Member

Choose a reason for hiding this comment

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

Seems easier to use Scalar as the universal type for what to store in errno -- we can already convert both io::Error and libc constants into Scalar.

Expand Down Expand Up @@ -1280,8 +1273,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
if let Ok(length) = length.try_into() {
let result = file.set_len(length);
drop(file_description);
let result = this.try_unwrap_io_result(result.map(|_| 0i32))?;
Ok(Scalar::from_i32(result))
this.try_unwrap_io_result(result)
} else {
drop(file_description);
let einval = this.eval_libc("EINVAL");
Expand Down Expand Up @@ -1329,7 +1321,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
})?;
let io_result = maybe_sync_file(file, *writable, File::sync_all);
drop(file_description);
Ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?))
this.try_unwrap_io_result(io_result)
}

fn fdatasync(&mut self, fd_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
Expand All @@ -1354,7 +1346,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
})?;
let io_result = maybe_sync_file(file, *writable, File::sync_data);
drop(file_description);
Ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?))
this.try_unwrap_io_result(io_result)
}

fn sync_file_range(
Expand Down Expand Up @@ -1404,7 +1396,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
})?;
let io_result = maybe_sync_file(file, *writable, File::sync_data);
drop(file_description);
Ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?))
this.try_unwrap_io_result(io_result)
}

fn readlink(
Expand Down