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

oli-obk · 2024-07-30T16:05:12Z

Instead of having to repeat the conversion to Scalar everywhere.

Instead of having to repeat the conversion to `Scalar`

RalfJung · 2024-07-30T16:15:13Z

src/helpers.rs

@@ -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>(


The doc comment above is outdated now.

RalfJung · 2024-07-30T16:15:19Z

src/helpers.rs

@@ -1434,3 +1447,35 @@ pub(crate) fn windows_check_buffer_size((success, len): (bool, u64)) -> u32 {
        u32::try_from(len).unwrap()
    }
 }
+
+pub trait WriteIoResult {


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.

RalfJung · 2024-07-30T16:17:12Z

src/shims/unix/fd.rs

+            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))


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

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

RalfJung · 2024-07-30T16:18:34Z

src/shims/unix/fs.rs

-        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> {


Why does this one still return Scalar?

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

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.

RalfJung · 2024-07-30T16:19:18Z

src/shims/unix/foreign_items.rs

@@ -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)?;


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.

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.

RalfJung · 2024-07-30T16:22:22Z

src/shims/unix/fs.rs

        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()));


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.

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

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.

oli-obk · 2024-07-31T15:14:29Z

I think the other scheme will allow us to better simplify things, so I'm closing this PR and will cherry-pick the parts that are useful

oli-obk added 3 commits July 30, 2024 13:50

Remove unnecessary method call

fe82f5a

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

c87ca20

Instead of having to repeat the conversion to `Scalar`

Generalize write_io_result to arbitrary Ok payloads

7860557

RalfJung reviewed Jul 30, 2024

View reviewed changes

oli-obk mentioned this pull request Jul 31, 2024

WIP: handle writing to dest within the foreign item handler #3779

Draft

oli-obk closed this Jul 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oli-obk commented Jul 30, 2024

RalfJung Jul 30, 2024

RalfJung Jul 30, 2024

RalfJung Jul 30, 2024

oli-obk Jul 30, 2024

RalfJung Jul 30, 2024

oli-obk Jul 30, 2024

RalfJung Jul 30, 2024

RalfJung Jul 30, 2024 •

edited

Loading

oli-obk Jul 30, 2024

RalfJung Jul 30, 2024

oli-obk Jul 30, 2024

RalfJung Jul 30, 2024

oli-obk commented Jul 31, 2024

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

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

Conversation

oli-obk commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jul 31, 2024

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

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

RalfJung Jul 30, 2024 •

edited

Loading