Critical Generic comparison bug found #123
Conversation
|
The old checking code truncated the checked values causing many comparisons to become |
|
I can confirm that this fixes panic in such rtic idle loop: let mut t = monotonics::now();
loop {
let now = monotonics::now();
if now - t >= Milliseconds(500).into() {
t = monotonics::now();
let _ = ctx.local.led.toggle();
}
}Without this it panics in It is critical to get it merged. |
|
Hmm, while this works the 3 multiplications can easily overflow. |
|
Wow, yeah. This was poorly implemented on my part. Unless I'm mistaken, you're simply finding and comparing the numerators that would result from a cross-multiplication. I agree with you about the easy overflow on the multiplication. I am going to add the GCD reductions using the existing Stein's Algorithm GCD implementation in the |
|
I'm still poking around a bit. I think an initial implementation should follow the patterns present in the named Duration/Rate implementations. They convert the value with the larger scaling factor into one with the smaller scaling factory (e.g. Seconds -> Milliseconds). If the conversion fails, than that shows us it's greater. The step would be implementing a |
…inate overrun failures
|
@korken89 Please take a look at my changes. It follows virtual the same pattern as the named duration |
| assert!( | ||
| Generic::new(200u32, Fraction::new(1, 1000)) >= Generic::new(10u32, Fraction::new(1, 100)) | ||
| ); | ||
| assert!( | ||
| Generic::new(200u32, Fraction::new(1, 1000)) >= Generic::new(10u32, Fraction::new(1, 100)) | ||
| ); |
There was a problem hiding this comment.
These two test cases are identical. These should either be de-duplicated, one of these changed to cover a different part of the comparison space, or a comment added as to why the duplicate is present.
There was a problem hiding this comment.
I have stopped contributing here and created fugit instead.
|
|
||
| /// Moves an integer into a comparable base for checking | ||
| fn checked_same_base(&self, fraction: &Fraction, rhs_fraction: &Fraction) -> Option<Self> { | ||
| let a_n = *fraction.numerator(); | ||
| let b_d = *rhs_fraction.denominator(); | ||
|
|
||
| self.checked_mul(&(b_d.into()))?.checked_mul(&(a_n.into())) | ||
| } |
There was a problem hiding this comment.
This is now unused in the PR after the refactor:
| /// Moves an integer into a comparable base for checking | |
| fn checked_same_base(&self, fraction: &Fraction, rhs_fraction: &Fraction) -> Option<Self> { | |
| let a_n = *fraction.numerator(); | |
| let b_d = *rhs_fraction.denominator(); | |
| self.checked_mul(&(b_d.into()))?.checked_mul(&(a_n.into())) | |
| } |
There was a problem hiding this comment.
I have stopped contributing here and created fugit instead.
| /* Generic comparison stress test: | ||
| loop { | ||
| let mut rands: [u32; 6] = [0; 6]; | ||
| for rand in rands.iter_mut() { | ||
| unsafe { | ||
| loop { | ||
| core::arch::x86_64::_rdrand32_step(rand); | ||
| if *rand != 0 { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| println!("rands: {:#?}", rands); | ||
| assert!( | ||
| Generic::new(rands[0], Fraction::new(rands[1], rands[2])) | ||
| != Generic::new(rands[3], Fraction::new(rands[4], rands[5])) | ||
| ); | ||
| } */ |
There was a problem hiding this comment.
It might be useful to rewrite this using the proptest crate, so you retain the "random checks" testing, while also having bounded runtimes for tests, and reductions of failures to simplified cases.
There was a problem hiding this comment.
I have stopped contributing here and created fugit instead.
| @@ -443,14 +443,42 @@ pub struct Generic<T> { | |||
| scaling_factor: Fraction, | |||
| } | |||
|
|
|||
| impl<T: TimeInt> Generic<T> { | |||
| /// Try to create a new, equivalent `Generic` with the given _scaling factor_ | |||
There was a problem hiding this comment.
Just to help make what is going on here a little clearer:
| /// Try to create a new, equivalent `Generic` with the given _scaling factor_ | |
| /// Try to create a new, equivalent `Generic` with the given _scaling factor_. | |
| /// | |
| /// To minimise errors due to truncation rounding, the new integer component is | |
| /// calculated as `self.integer * (self.scaling_factor / scaling_factor)`. |
There was a problem hiding this comment.
I have stopped contributing here and created fugit instead.
| Some(self.integer.cmp(&rhs.integer)) | ||
| } else if self.scaling_factor < rhs.scaling_factor { | ||
| // convert to the smaller scaling factor (rhs -> self) | ||
| // if conversion fails, we know self is less than rhs |
There was a problem hiding this comment.
It feels like this will be true most of the time, but I'm not entirely convinced that there aren't edge cases where rhs.scaling_factor / self.scaling_factor < self.integer / rhs.integer (so self > rhs), and rhs is close enough to the maximum bound on T that it will overflow. In particular, TimeInt::checked_mul_fraction is being called with rhs.integer and a fraction that is greater than 1, and is implemented by first multiplying by the numerator (which could overflow) and then dividing by the denominator (which if not for the overflow could bring the result back in range and below self.integer). It would be great to have tests that exercise these near-bounds edge cases.
There was a problem hiding this comment.
I have stopped contributing here and created fugit instead.
I found a
Genericcomparison bug that is quite critical.