Skip to content

Commit

Permalink
Auto merge of rust-lang#3940 - rust-lang:refutable_slice, r=RalfJung
Browse files Browse the repository at this point in the history
Prefer refutable slice patterns over len check + index op

Just something I noticed while reviewing other PRs

We do it for shim arguments almost everywhere, but when the size is not statically known, we didn't use the helpers as they rely on array ops. But we can avoid a len check followed by several index ops by just using a refutable slice pattern with `let else`.

The pattern is so common, it seems almost worth a dedicated macro
  • Loading branch information
bors committed Oct 5, 2024
2 parents eb97047 + f46e162 commit 3b418b1
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 33 deletions.
5 changes: 3 additions & 2 deletions src/tools/miri/src/borrow_tracker/tree_borrows/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -859,14 +859,15 @@ impl Tree {
) -> Option<UniIndex> {
let node = self.nodes.get(idx).unwrap();

let [child_idx] = node.children[..] else { return None };

// We never want to replace the root node, as it is also kept in `root_ptr_tags`.
if node.children.len() != 1 || live.contains(&node.tag) || node.parent.is_none() {
if live.contains(&node.tag) || node.parent.is_none() {
return None;
}
// Since protected nodes are never GC'd (see `borrow_tracker::FrameExtra::visit_provenance`),
// we know that `node` is not protected because otherwise `live` would
// have contained `node.tag`.
let child_idx = node.children[0];
let child = self.nodes.get(child_idx).unwrap();
// Check that for that one child, `can_be_replaced_by_child` holds for the permission
// on all locations.
Expand Down
14 changes: 7 additions & 7 deletions src/tools/miri/src/shims/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -481,14 +481,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
fn fcntl(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
let this = self.eval_context_mut();

if args.len() < 2 {
let [fd_num, cmd, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for fcntl: got {}, expected at least 2",
args.len()
);
}
let fd_num = this.read_scalar(&args[0])?.to_i32()?;
let cmd = this.read_scalar(&args[1])?.to_i32()?;
};
let fd_num = this.read_scalar(fd_num)?.to_i32()?;
let cmd = this.read_scalar(cmd)?.to_i32()?;

// We only support getting the flags for a descriptor.
if cmd == this.eval_libc_i32("F_GETFD") {
Expand All @@ -508,13 +508,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// because exec() isn't supported. The F_DUPFD and F_DUPFD_CLOEXEC commands only
// differ in whether the FD_CLOEXEC flag is pre-set on the new file descriptor,
// thus they can share the same implementation here.
if args.len() < 3 {
let [_, _, start, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for fcntl with cmd=`F_DUPFD`/`F_DUPFD_CLOEXEC`: got {}, expected at least 3",
args.len()
);
}
let start = this.read_scalar(&args[2])?.to_i32()?;
};
let start = this.read_scalar(start)?.to_i32()?;

match this.machine.fds.get(fd_num) {
Some(fd) =>
Expand Down
8 changes: 4 additions & 4 deletions src/tools/miri/src/shims/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,18 +433,18 @@ fn maybe_sync_file(
impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
fn open(&mut self, args: &[OpTy<'tcx>]) -> InterpResult<'tcx, Scalar> {
if args.len() < 2 {
let [path_raw, flag, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for `open`: got {}, expected at least 2",
args.len()
);
}
};

let this = self.eval_context_mut();

let path_raw = this.read_pointer(&args[0])?;
let path_raw = this.read_pointer(path_raw)?;
let path = this.read_path_from_c_str(path_raw)?;
let flag = this.read_scalar(&args[1])?.to_i32()?;
let flag = this.read_scalar(flag)?.to_i32()?;

let mut options = OpenOptions::new();

Expand Down
10 changes: 5 additions & 5 deletions src/tools/miri/src/shims/unix/linux/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,19 +122,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
id if id == sys_getrandom => {
// Used by getrandom 0.1
// The first argument is the syscall id, so skip over it.
if args.len() < 4 {
let [_, ptr, len, flags, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for `getrandom` syscall: got {}, expected at least 4",
args.len()
);
}
};

let ptr = this.read_pointer(&args[1])?;
let len = this.read_target_usize(&args[2])?;
let ptr = this.read_pointer(ptr)?;
let len = this.read_target_usize(len)?;
// The only supported flags are GRND_RANDOM and GRND_NONBLOCK,
// neither of which have any effect on our current PRNG.
// See <https://github.com/rust-lang/rust/pull/79196> for a discussion of argument sizes.
let _flags = this.read_scalar(&args[3])?.to_i32()?;
let _flags = this.read_scalar(flags)?.to_i32()?;

this.gen_random(ptr, len)?;
this.write_scalar(Scalar::from_target_usize(len, this), dest)?;
Expand Down
30 changes: 15 additions & 15 deletions src/tools/miri/src/shims/unix/linux/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@ pub fn futex<'tcx>(
// may or may not be left out from the `syscall()` call.
// Therefore we don't use `check_arg_count` here, but only check for the
// number of arguments to fall within a range.
if args.len() < 3 {
let [addr, op, val, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for `futex` syscall: got {}, expected at least 3",
args.len()
);
}
};

// The first three arguments (after the syscall number itself) are the same to all futex operations:
// (int *addr, int op, int val).
// We checked above that these definitely exist.
let addr = this.read_pointer(&args[0])?;
let op = this.read_scalar(&args[1])?.to_i32()?;
let val = this.read_scalar(&args[2])?.to_i32()?;
let addr = this.read_pointer(addr)?;
let op = this.read_scalar(op)?.to_i32()?;
let val = this.read_scalar(val)?.to_i32()?;

// This is a vararg function so we have to bring our own type for this pointer.
let addr = this.ptr_to_mplace(addr, this.machine.layouts.i32);
Expand Down Expand Up @@ -55,15 +55,15 @@ pub fn futex<'tcx>(
let wait_bitset = op & !futex_realtime == futex_wait_bitset;

let bitset = if wait_bitset {
if args.len() < 6 {
let [_, _, _, timeout, uaddr2, bitset, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for `futex` syscall with `op=FUTEX_WAIT_BITSET`: got {}, expected at least 6",
args.len()
);
}
let _timeout = this.read_pointer(&args[3])?;
let _uaddr2 = this.read_pointer(&args[4])?;
this.read_scalar(&args[5])?.to_u32()?
};
let _timeout = this.read_pointer(timeout)?;
let _uaddr2 = this.read_pointer(uaddr2)?;
this.read_scalar(bitset)?.to_u32()?
} else {
if args.len() < 4 {
throw_ub_format!(
Expand Down Expand Up @@ -183,15 +183,15 @@ pub fn futex<'tcx>(
// Same as FUTEX_WAKE, but allows you to specify a bitset to select which threads to wake up.
op if op == futex_wake || op == futex_wake_bitset => {
let bitset = if op == futex_wake_bitset {
if args.len() < 6 {
let [_, _, _, timeout, uaddr2, bitset, ..] = args else {
throw_ub_format!(
"incorrect number of arguments for `futex` syscall with `op=FUTEX_WAKE_BITSET`: got {}, expected at least 6",
args.len()
);
}
let _timeout = this.read_pointer(&args[3])?;
let _uaddr2 = this.read_pointer(&args[4])?;
this.read_scalar(&args[5])?.to_u32()?
};
let _timeout = this.read_pointer(timeout)?;
let _uaddr2 = this.read_pointer(uaddr2)?;
this.read_scalar(bitset)?.to_u32()?
} else {
u32::MAX
};
Expand Down

0 comments on commit 3b418b1

Please sign in to comment.