Messages in this thread | | | Subject | Re: [PATCH 2/2] rust: time: Use wrapping_sub() for Ktime::sub() | From | Philipp Stanner <> | Date | Fri, 12 Apr 2024 09:43:24 +0200 |
| |
On Fri, 2024-04-12 at 09:14 +0200, Miguel Ojeda wrote: > On Fri, Apr 12, 2024 at 1:08 AM Boqun Feng <boqun.feng@gmail.com> > wrote: > > > > Currently since Rust code is compiled with "-Coverflow-checks=y", > > so a > > Nit: it is enabled by default, but configurable > (`CONFIG_RUST_OVERFLOW_CHECKS`).
Is that going to remain enabled by default or what was the plan here?
P.
> > > although overflow detection is nice to have, however this makes > > `Ktime::sub()` behave differently than `ktime_sub()`, moreover it's > > not > > clear that the overflow checking is helpful, since for example, the > > current binder usage[1] doesn't have the checking. > > > > Therefore make `Ktime::sub()` have the same semantics as > > `ktime_sub()`: > > overflow behaves like 2s-complement wrapping sub. > > If `ktime_sub()`'s callers rely on wrapping in some cases, then an > alternative we should consider is having a method for explicitly > wrapping, like the integers. This would allow callers to decide and > it > would make the expected semantics clear since the beginning (which is > the easiest time to add this kind of thing) for Rust code. > > Otherwise, I agree we should at least document the preconditions > clearly. > > Having said that, I see a `ktime_add_unsafe()` too, which was added > due to a UBSAN report for `ktime_add()` in commit 979515c56458 > ("time: > Avoid undefined behaviour in ktime_add_safe()"). There is also a > private `ktime_add_safe()` too, which is a saturating one. > > So, given that, can callers actually rely on wrapping for these > functions, or not? The documentation on the C side could perhaps be > clarified here (including the mention of UB in `ktime_add_unsafe()` - > - > we use `-fno-strict-overflow`) and perhaps using the `wrapping_*()` C > functions too. > > In addition, Binder calls `ktime_ms_delta()`, not `ktime_sub()`, > right? In that case the arguments are called `later` and `earlier`, > perhaps those have a different expectation even if `ktime_sub()` is > allowed to overflow and thus it would make sense to check in that > function only instead? (and document accordingly) > > Thanks! > > Cheers, > Miguel >
| |