Skip to content

Commit

Permalink
Auto merge of rust-lang#3776 - oli-obk:duplicator, r=oli-obk
Browse files Browse the repository at this point in the history
Use Scalar consistently in foreign item emulation

Step 0 of rust-lang#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.
  • Loading branch information
bors committed Jul 30, 2024
2 parents cf63c16 + 6fc1b69 commit f1e8c54
Show file tree
Hide file tree
Showing 10 changed files with 239 additions and 245 deletions.
16 changes: 10 additions & 6 deletions src/tools/miri/src/shims/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(Scalar::from_i32(0))
}

fn gettimeofday(&mut self, tv_op: &OpTy<'tcx>, tz_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
fn gettimeofday(
&mut self,
tv_op: &OpTy<'tcx>,
tz_op: &OpTy<'tcx>,
) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

this.assert_target_os_is_unix("gettimeofday");
Expand All @@ -106,7 +110,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
if !this.ptr_is_null(tz)? {
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
return Ok(-1);
return Ok(Scalar::from_i32(-1));
}

let duration = system_time_to_duration(&SystemTime::now())?;
Expand All @@ -115,7 +119,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

this.write_int_fields(&[tv_sec.into(), tv_usec.into()], &tv)?;

Ok(0)
Ok(Scalar::from_i32(0))
}

// The localtime() function shall convert the time in seconds since the Epoch pointed to by
Expand Down Expand Up @@ -308,7 +312,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
&mut self,
req_op: &OpTy<'tcx>,
_rem: &OpTy<'tcx>, // Signal handlers are not supported, so rem will never be written to.
) -> InterpResult<'tcx, i32> {
) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

this.assert_target_os_is_unix("nanosleep");
Expand All @@ -320,7 +324,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
None => {
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
return Ok(-1);
return Ok(Scalar::from_i32(-1));
}
};

Expand All @@ -333,7 +337,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
@timeout = |_this| { Ok(()) }
),
);
Ok(0)
Ok(Scalar::from_i32(0))
}

#[allow(non_snake_case)]
Expand Down
40 changes: 19 additions & 21 deletions src/tools/miri/src/shims/unix/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(var_ptr.unwrap_or_else(Pointer::null))
}

fn setenv(&mut self, name_op: &OpTy<'tcx>, value_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
fn setenv(
&mut self,
name_op: &OpTy<'tcx>,
value_op: &OpTy<'tcx>,
) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();
this.assert_target_os_is_unix("setenv");

Expand All @@ -169,16 +173,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?;
}
this.update_environ()?;
Ok(0) // return zero on success
Ok(Scalar::from_i32(0)) // return zero on success
} else {
// name argument is a null pointer, points to an empty string, or points to a string containing an '=' character.
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
Ok(-1)
Ok(Scalar::from_i32(-1))
}
}

fn unsetenv(&mut self, name_op: &OpTy<'tcx>) -> InterpResult<'tcx, i32> {
fn unsetenv(&mut self, name_op: &OpTy<'tcx>) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();
this.assert_target_os_is_unix("unsetenv");

Expand All @@ -195,12 +199,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?;
}
this.update_environ()?;
Ok(0)
Ok(Scalar::from_i32(0))
} else {
// name argument is a null pointer, points to an empty string, or points to a string containing an '=' character.
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
Ok(-1)
Ok(Scalar::from_i32(-1))
}
}

Expand Down Expand Up @@ -232,7 +236,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(Pointer::null())
}

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

Expand All @@ -242,16 +246,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.reject_in_isolation("`chdir`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;

return Ok(-1);
return Ok(Scalar::from_i32(-1));
}

match env::set_current_dir(path) {
Ok(()) => Ok(0),
Err(e) => {
this.set_last_error_from_io_error(e)?;
Ok(-1)
}
}
let result = env::set_current_dir(path).map(|()| 0);
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
}

/// Updates the `environ` static.
Expand All @@ -270,18 +269,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(())
}

fn getpid(&mut self) -> InterpResult<'tcx, i32> {
fn getpid(&mut self) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();
this.assert_target_os_is_unix("getpid");

// The reason we need to do this wacky of a conversion is because
// `libc::getpid` returns an i32, however, `std::process::id()` return an u32.
// So we un-do the conversion that stdlib does and turn it back into an i32.
#[allow(clippy::cast_possible_wrap)]
Ok(this.get_pid() as i32)
// In `Scalar` representation, these are the same, so we don't need to anything else.
Ok(Scalar::from_u32(this.get_pid()))
}

fn linux_gettid(&mut self) -> InterpResult<'tcx, i32> {
fn linux_gettid(&mut self) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_ref();
this.assert_target_os("linux", "gettid");

Expand All @@ -290,7 +289,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// Compute a TID for this thread, ensuring that the main thread has PID == TID.
let tid = this.get_pid().strict_add(index);

#[allow(clippy::cast_possible_wrap)]
Ok(tid as i32)
Ok(Scalar::from_u32(tid))
}
}
47 changes: 24 additions & 23 deletions src/tools/miri/src/shims/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,20 +310,20 @@ impl FdTable {

impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, i32> {
fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

let Some(dup_fd) = this.machine.fds.dup(old_fd) else {
return this.fd_not_found();
return Ok(Scalar::from_i32(this.fd_not_found()?));
};
Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0))
Ok(Scalar::from_i32(this.machine.fds.insert_fd_with_min_fd(dup_fd, 0)))
}

fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, i32> {
fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

let Some(dup_fd) = this.machine.fds.dup(old_fd) else {
return this.fd_not_found();
return Ok(Scalar::from_i32(this.fd_not_found()?));
};
if new_fd != old_fd {
// Close new_fd if it is previously opened.
Expand All @@ -333,7 +333,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
file_description.close(this.machine.communicate())?.ok();
}
}
Ok(new_fd)
Ok(Scalar::from_i32(new_fd))
}

fn flock(&mut self, fd: i32, op: i32) -> InterpResult<'tcx, Scalar> {
Expand Down Expand Up @@ -370,7 +370,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
Ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))
}

fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, i32> {
fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

if args.len() < 2 {
Expand All @@ -388,11 +388,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// `FD_CLOEXEC` value without checking if the flag is set for the file because `std`
// always sets this flag when opening a file. However we still need to check that the
// file itself is open.
if this.machine.fds.is_fd(fd) {
Ok(this.eval_libc_i32("FD_CLOEXEC"))
Ok(Scalar::from_i32(if this.machine.fds.is_fd(fd) {
this.eval_libc_i32("FD_CLOEXEC")
} else {
this.fd_not_found()
}
this.fd_not_found()?
}))
} else if cmd == this.eval_libc_i32("F_DUPFD")
|| cmd == this.eval_libc_i32("F_DUPFD_CLOEXEC")
{
Expand All @@ -409,15 +409,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let start = this.read_scalar(&args[2])?.to_i32()?;

match this.machine.fds.dup(fd) {
Some(dup_fd) => Ok(this.machine.fds.insert_fd_with_min_fd(dup_fd, start)),
None => this.fd_not_found(),
Some(dup_fd) =>
Ok(Scalar::from_i32(this.machine.fds.insert_fd_with_min_fd(dup_fd, start))),
None => Ok(Scalar::from_i32(this.fd_not_found()?)),
}
} else if this.tcx.sess.target.os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC") {
// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`fcntl`", reject_with)?;
this.set_last_error_from_io_error(ErrorKind::PermissionDenied.into())?;
return Ok(-1);
return Ok(Scalar::from_i32(-1));
}

this.ffullsync_fd(fd)
Expand Down Expand Up @@ -462,7 +463,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
buf: Pointer,
count: u64,
offset: Option<i128>,
) -> InterpResult<'tcx, i64> {
) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

// Isolation check is done via `FileDescriptor` trait.
Expand All @@ -482,7 +483,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// We temporarily dup the FD to be able to retain mutable access to `this`.
let Some(fd) = this.machine.fds.dup(fd) else {
trace!("read: FD not found");
return this.fd_not_found();
return Ok(Scalar::from_target_isize(this.fd_not_found()?, this));
};

trace!("read: FD mapped to {fd:?}");
Expand All @@ -496,7 +497,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let Ok(offset) = u64::try_from(offset) else {
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
return Ok(-1);
return Ok(Scalar::from_target_isize(-1, this));
};
fd.borrow_mut().pread(communicate, &mut bytes, offset, this)
}
Expand All @@ -513,11 +514,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
buf,
bytes[..usize::try_from(read_bytes).unwrap()].iter().copied(),
)?;
Ok(read_bytes)
Ok(Scalar::from_target_isize(read_bytes, this))
}
Err(e) => {
this.set_last_error_from_io_error(e)?;
Ok(-1)
Ok(Scalar::from_target_isize(-1, this))
}
}
}
Expand All @@ -528,7 +529,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
buf: Pointer,
count: u64,
offset: Option<i128>,
) -> InterpResult<'tcx, i64> {
) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

// Isolation check is done via `FileDescriptor` trait.
Expand All @@ -546,7 +547,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let bytes = this.read_bytes_ptr_strip_provenance(buf, Size::from_bytes(count))?.to_owned();
// We temporarily dup the FD to be able to retain mutable access to `this`.
let Some(fd) = this.machine.fds.dup(fd) else {
return this.fd_not_found();
return Ok(Scalar::from_target_isize(this.fd_not_found()?, this));
};

let result = match offset {
Expand All @@ -555,15 +556,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let Ok(offset) = u64::try_from(offset) else {
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
return Ok(-1);
return Ok(Scalar::from_target_isize(-1, this));
};
fd.borrow_mut().pwrite(communicate, &bytes, offset, this)
}
};
drop(fd);

let result = result?.map(|c| i64::try_from(c).unwrap());
this.try_unwrap_io_result(result)
Ok(Scalar::from_target_isize(this.try_unwrap_io_result(result)?, this))
}
}

Expand Down
Loading

0 comments on commit f1e8c54

Please sign in to comment.