Fix find_last_non_zero_bit, and align metadata address before converting to data address. #1188
Conversation
| fn find_last_non_zero_bit_u8(value: u8, start: u8, end: u8) -> Option<u8> { | ||
| // TODO: Ideally we implement the function as a generic function. | ||
| // However Rust does not have a generic trait for integers yet. We can use | ||
| // third party traits or our own trait (such as MetadataValue), but it is cumbersome, | ||
| // and does not worth the efforts. | ||
| impl_find_last_non_zero_bit!(u8, value, start, end) | ||
| } | ||
|
|
||
| fn find_last_non_zero_bit_usize(value: usize, start: u8, end: u8) -> Option<u8> { | ||
| // TODO: Ideally we implement the function as a generic function. | ||
| // However Rust does not have a generic trait for integers yet. We can use | ||
| // third party traits or our own trait (such as MetadataValue), but it is cumbersome, | ||
| // and does not worth the efforts. | ||
| impl_find_last_non_zero_bit!(usize, value, start, end) |
There was a problem hiding this comment.
Actually we already depend on the num-traits crate, and it has the needed traits for bit operations, including CheckedShl. Macros perform fewer compile-time checks, and may surprise us in unexpected ways (such as missing type casts and unexpected type coercion). Writing a generic function is safer and more readable. It is a bit harder to write, but not that bad. Here is one version that works. (Note that I replaced leading_zeroes >= <$int_ty>::BITS with a simple comparison against zero (.is_zero()))
fn find_last_non_zero_bit_in_t<T>(value: T, start: u8, end: u8) -> Option<u8>
where
T: PrimInt + CheckedShl,
{
let mask = match T::one().checked_shl((end - start) as u32) {
Some(shl) => (shl - T::one()) << (start as u32),
None => T::max_value() << (start as u32),
};
let masked = value & mask;
if masked.is_zero() {
None
} else {
let leading_zeroes = masked.leading_zeros();
let total_bits = std::mem::size_of::<T>() * u8::BITS as usize;
Some(total_bits as u8 - leading_zeroes as u8 - 1)
}
}If it is not worth writing a generic function, a simpler solution is using find_last_non_zero_bit_usize for both u8 and usize. We just cast the u8 to usize and set start and end to 0 and 8. It involves a zero extension from u8 to usize, but is usually a no-op for most architectures because compilers usually use a whole 32-bit or 64-bit register to hold a u8 variable anyway. And because it is only done in the first or last incomplete byte, the cost is negligible.
There was a problem hiding this comment.
Done. I used your implemenation. Thanks.
| None => <$int_ty>::MAX << $start, | ||
| }; | ||
| let leading_zeroes = ($value & mask).leading_zeros(); | ||
| if leading_zeroes >= <$int_ty>::BITS { |
There was a problem hiding this comment.
We can just compare $value & mask with zero. If not zero, there must be at least one non-zero bit.
us to check metadata value that is not in the given range.
| let step = if cur.is_aligned_to(BYTES_IN_ADDRESS) | ||
| && cur.align_down(BYTES_IN_ADDRESS) >= meta_start | ||
| { | ||
| let step = if cur.is_aligned_to(BYTES_IN_ADDRESS) && cur - BYTES_IN_ADDRESS >= meta_start { |
There was a problem hiding this comment.
This is the other fix that is needed. The old code may check an address that is below meta_start.
2 participants
This PR fixes some bugs in finding base references for internal references. Prominently two bugs:
trailing_zeroesand compare the result with the given range of the starting and ending bits. This was simply wrong. Now we do a mask first to extract the value between the starting and the ending bits, and then useleading_zeroes. Performance-wise, the new code has similar performnace.align_metadata_address.This PR skips some tests for LOS in plans that do not use LOS.