Skip to content

Commit

Permalink
Auto merge of #3694 - southball:fix/use_strict_ops_instead_of_checked…
Browse files Browse the repository at this point in the history
…_ops, r=RalfJung

Use strict ops instead of checked ops

## What

Replace `checked_add(...).unwrap()` with `strict_add(...)`, etc.

Resolves #3668.
  • Loading branch information
bors committed Jun 21, 2024
2 parents 10daab1 + ef22eb1 commit dca0151
Show file tree
Hide file tree
Showing 14 changed files with 65 additions and 75 deletions.
2 changes: 1 addition & 1 deletion src/alloc_addresses/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl GlobalStateInner {
fn align_addr(addr: u64, align: u64) -> u64 {
match addr % align {
0 => addr,
rem => addr.checked_add(align).unwrap() - rem,
rem => addr.strict_add(align) - rem,
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ pub fn create_ecx<'tcx>(
let mut argvs = Vec::<Immediate<Provenance>>::with_capacity(config.args.len());
for arg in config.args.iter() {
// Make space for `0` terminator.
let size = u64::try_from(arg.len()).unwrap().checked_add(1).unwrap();
let size = u64::try_from(arg.len()).unwrap().strict_add(1);
let arg_type = Ty::new_array(tcx, tcx.types.u8, size);
let arg_place =
ecx.allocate(ecx.layout_of(arg_type)?, MiriMemoryKind::Machine.into())?;
Expand Down
6 changes: 3 additions & 3 deletions src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -973,7 +973,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null
// terminator to memory using the `ptr` pointer would cause an out-of-bounds access.
let string_length = u64::try_from(c_str.len()).unwrap();
let string_length = string_length.checked_add(1).unwrap();
let string_length = string_length.strict_add(1);
if size < string_length {
return Ok((false, string_length));
}
Expand Down Expand Up @@ -1037,7 +1037,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required
// 0x0000 terminator to memory would cause an out-of-bounds access.
let string_length = u64::try_from(wide_str.len()).unwrap();
let string_length = string_length.checked_add(1).unwrap();
let string_length = string_length.strict_add(1);
if size < string_length {
return Ok((false, string_length));
}
Expand Down Expand Up @@ -1406,7 +1406,7 @@ pub(crate) fn windows_check_buffer_size((success, len): (bool, u64)) -> u32 {
if success {
// If the function succeeds, the return value is the number of characters stored in the target buffer,
// not including the terminating null character.
u32::try_from(len.checked_sub(1).unwrap()).unwrap()
u32::try_from(len.strict_sub(1)).unwrap()
} else {
// If the target buffer was not large enough to hold the data, the return value is the buffer size, in characters,
// required to hold the string and its terminating null character.
Expand Down
4 changes: 2 additions & 2 deletions src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
});
let (_, addr) = ptr.into_parts(); // we know the offset is absolute
// Cannot panic since `align` is a power of 2 and hence non-zero.
if addr.bytes().checked_rem(align.bytes()).unwrap() != 0 {
if addr.bytes().strict_rem(align.bytes()) != 0 {
throw_unsup_format!(
"`miri_promise_symbolic_alignment`: pointer is not actually aligned"
);
Expand Down Expand Up @@ -714,7 +714,7 @@ trait EvalContextExtPriv<'tcx>: crate::MiriInterpCxExt<'tcx> {
// That is probably overly cautious, but there also is no fundamental
// reason to have `strcpy` destroy pointer provenance.
// This reads at least 1 byte, so we are already enforcing that this is a valid pointer.
let n = this.read_c_str(ptr_src)?.len().checked_add(1).unwrap();
let n = this.read_c_str(ptr_src)?.len().strict_add(1);
this.mem_copy(ptr_src, ptr_dest, Size::from_bytes(n), true)?;
this.write_pointer(ptr_dest, dest)?;
}
Expand Down
2 changes: 1 addition & 1 deletion src/shims/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
("tm_hour", dt.hour().into()),
("tm_mday", dt.day().into()),
("tm_mon", dt.month0().into()),
("tm_year", dt.year().checked_sub(1900).unwrap().into()),
("tm_year", dt.year().strict_sub(1900).into()),
("tm_wday", dt.weekday().num_days_from_sunday().into()),
("tm_yday", dt.ordinal0().into()),
("tm_isdst", tm_isdst),
Expand Down
6 changes: 2 additions & 4 deletions src/shims/unix/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,8 @@ impl<'tcx> UnixEnvVars<'tcx> {
return Ok(None);
};
// The offset is used to strip the "{name}=" part of the string.
let var_ptr = var_ptr.offset(
Size::from_bytes(u64::try_from(name.len()).unwrap().checked_add(1).unwrap()),
ecx,
)?;
let var_ptr = var_ptr
.offset(Size::from_bytes(u64::try_from(name.len()).unwrap().strict_add(1)), ecx)?;
Ok(Some(var_ptr))
}

Expand Down
2 changes: 1 addition & 1 deletion src/shims/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ impl FdTable {
let new_fd = candidate_new_fd.unwrap_or_else(|| {
// find_map ran out of BTreeMap entries before finding a free fd, use one plus the
// maximum fd in the map
self.fds.last_key_value().map(|(fd, _)| fd.checked_add(1).unwrap()).unwrap_or(min_fd)
self.fds.last_key_value().map(|(fd, _)| fd.strict_add(1)).unwrap_or(min_fd)
});

self.fds.try_insert(new_fd, file_handle).unwrap();
Expand Down
2 changes: 1 addition & 1 deletion src/shims/unix/socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl FileDescription for SocketPair {
};
let mut writebuf = writebuf.borrow_mut();
let data_size = writebuf.buf.len();
let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.checked_sub(data_size).unwrap();
let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size);
if available_space == 0 {
if self.is_nonblock {
// Non-blocking socketpair with a full buffer.
Expand Down
4 changes: 2 additions & 2 deletions src/shims/windows/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// If the function succeeds, the return value is the length of the string that
// is copied to the buffer, in characters, not including the terminating null
// character.
this.write_int(size_needed.checked_sub(1).unwrap(), dest)?;
this.write_int(size_needed.strict_sub(1), dest)?;
} else {
// If the buffer is too small to hold the module name, the string is truncated
// to nSize characters including the terminating null character, the function
Expand Down Expand Up @@ -689,7 +689,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
throw_unsup_format!("FormatMessageW: buffer not big enough");
}
// The return value is the number of characters stored *excluding* the null terminator.
this.write_int(length.checked_sub(1).unwrap(), dest)?;
this.write_int(length.strict_sub(1), dest)?;
}

// Incomplete shims that we "stub out" just to get pre-main initialization code to work.
Expand Down
4 changes: 2 additions & 2 deletions src/shims/windows/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ impl Handle {
/// None of this layout is guaranteed to applications by Windows or Miri.
fn to_packed(self) -> u32 {
let disc_size = Self::packed_disc_size();
let data_size = u32::BITS.checked_sub(disc_size).unwrap();
let data_size = u32::BITS.strict_sub(disc_size);

let discriminant = self.discriminant();
let data = self.data();
Expand Down Expand Up @@ -103,7 +103,7 @@ impl Handle {
/// see docs for `to_packed`
fn from_packed(handle: u32) -> Option<Self> {
let disc_size = Self::packed_disc_size();
let data_size = u32::BITS.checked_sub(disc_size).unwrap();
let data_size = u32::BITS.strict_sub(disc_size);

// the lower `data_size` bits of this mask are 1
#[allow(clippy::arithmetic_side_effects)] // cannot overflow
Expand Down
35 changes: 17 additions & 18 deletions src/shims/x86/avx2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
assert_eq!(dest_len, mask_len);

let mask_item_size = mask.layout.field(this, 0).size;
let high_bit_offset = mask_item_size.bits().checked_sub(1).unwrap();
let high_bit_offset = mask_item_size.bits().strict_sub(1);

let scale = this.read_scalar(scale)?.to_i8()?;
if !matches!(scale, 1 | 2 | 4 | 8) {
Expand All @@ -93,8 +93,7 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let offset =
i64::try_from(this.read_scalar(&offset)?.to_int(offset.layout.size)?)
.unwrap();
let ptr = slice
.wrapping_signed_offset(offset.checked_mul(scale).unwrap(), &this.tcx);
let ptr = slice.wrapping_signed_offset(offset.strict_mul(scale), &this.tcx);
// Unaligned copy, which is what we want.
this.mem_copy(
ptr,
Expand Down Expand Up @@ -124,22 +123,22 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let (dest, dest_len) = this.mplace_to_simd(dest)?;

assert_eq!(left_len, right_len);
assert_eq!(dest_len.checked_mul(2).unwrap(), left_len);
assert_eq!(dest_len.strict_mul(2), left_len);

for i in 0..dest_len {
let j1 = i.checked_mul(2).unwrap();
let j1 = i.strict_mul(2);
let left1 = this.read_scalar(&this.project_index(&left, j1)?)?.to_i16()?;
let right1 = this.read_scalar(&this.project_index(&right, j1)?)?.to_i16()?;

let j2 = j1.checked_add(1).unwrap();
let j2 = j1.strict_add(1);
let left2 = this.read_scalar(&this.project_index(&left, j2)?)?.to_i16()?;
let right2 = this.read_scalar(&this.project_index(&right, j2)?)?.to_i16()?;

let dest = this.project_index(&dest, i)?;

// Multiplications are i16*i16->i32, which will not overflow.
let mul1 = i32::from(left1).checked_mul(right1.into()).unwrap();
let mul2 = i32::from(left2).checked_mul(right2.into()).unwrap();
let mul1 = i32::from(left1).strict_mul(right1.into());
let mul2 = i32::from(left2).strict_mul(right2.into());
// However, this addition can overflow in the most extreme case
// (-0x8000)*(-0x8000)+(-0x8000)*(-0x8000) = 0x80000000
let res = mul1.wrapping_add(mul2);
Expand All @@ -161,22 +160,22 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let (dest, dest_len) = this.mplace_to_simd(dest)?;

assert_eq!(left_len, right_len);
assert_eq!(dest_len.checked_mul(2).unwrap(), left_len);
assert_eq!(dest_len.strict_mul(2), left_len);

for i in 0..dest_len {
let j1 = i.checked_mul(2).unwrap();
let j1 = i.strict_mul(2);
let left1 = this.read_scalar(&this.project_index(&left, j1)?)?.to_u8()?;
let right1 = this.read_scalar(&this.project_index(&right, j1)?)?.to_i8()?;

let j2 = j1.checked_add(1).unwrap();
let j2 = j1.strict_add(1);
let left2 = this.read_scalar(&this.project_index(&left, j2)?)?.to_u8()?;
let right2 = this.read_scalar(&this.project_index(&right, j2)?)?.to_i8()?;

let dest = this.project_index(&dest, i)?;

// Multiplication of a u8 and an i8 into an i16 cannot overflow.
let mul1 = i16::from(left1).checked_mul(right1.into()).unwrap();
let mul2 = i16::from(left2).checked_mul(right2.into()).unwrap();
let mul1 = i16::from(left1).strict_mul(right1.into());
let mul2 = i16::from(left2).strict_mul(right2.into());
let res = mul1.saturating_add(mul2);

this.write_scalar(Scalar::from_i16(res), &dest)?;
Expand Down Expand Up @@ -309,7 +308,7 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

for i in 0..2 {
let dest = this.project_index(&dest, i)?;
let src = match (imm >> i.checked_mul(4).unwrap()) & 0b11 {
let src = match (imm >> i.strict_mul(4)) & 0b11 {
0 => this.project_index(&left, 0)?,
1 => this.project_index(&left, 1)?,
2 => this.project_index(&right, 0)?,
Expand All @@ -336,22 +335,22 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let (dest, dest_len) = this.mplace_to_simd(dest)?;

assert_eq!(left_len, right_len);
assert_eq!(left_len, dest_len.checked_mul(8).unwrap());
assert_eq!(left_len, dest_len.strict_mul(8));

for i in 0..dest_len {
let dest = this.project_index(&dest, i)?;

let mut acc: u16 = 0;
for j in 0..8 {
let src_index = i.checked_mul(8).unwrap().checked_add(j).unwrap();
let src_index = i.strict_mul(8).strict_add(j);

let left = this.project_index(&left, src_index)?;
let left = this.read_scalar(&left)?.to_u8()?;

let right = this.project_index(&right, src_index)?;
let right = this.read_scalar(&right)?.to_u8()?;

acc = acc.checked_add(left.abs_diff(right).into()).unwrap();
acc = acc.strict_add(left.abs_diff(right).into());
}

this.write_scalar(Scalar::from_u64(acc.into()), &dest)?;
Expand All @@ -377,7 +376,7 @@ pub(super) trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

let res = if right & 0x80 == 0 {
// Shuffle each 128-bit (16-byte) block independently.
let j = u64::from(right % 16).checked_add(i & !15).unwrap();
let j = u64::from(right % 16).strict_add(i & !15);
this.read_scalar(&this.project_index(&left, j)?)?
} else {
// If the highest bit in `right` is 1, write zero.
Expand Down
Loading

0 comments on commit dca0151

Please sign in to comment.