On Wed, Mar 19, 2025 at 12:06?PM Miguel Ojeda <miguel.ojeda.sandonis at gmail.com> wrote:> > On Wed, Mar 19, 2025 at 4:59?PM Tamir Duberstein <tamird at gmail.com> wrote: > > > > If we're talking about the same thing then I think we're both wrong > > and the correct phrasing would have been: "you can avoid underflow > > checking when CONFIG_RUST_OVERFLOW_CHECKS=y by using `checked_sub`". I > > was referring to the underflow check implicit in `new_len - > > self.len()`. > > `checked_sub` always checks (if not optimized away). The config option > is about the implicit one. > > Do you mean avoiding panics?No, I meant avoiding the check. The existing code already explicitly checks `new_len > self.len()` before evaluating `new_len - self.len()`. This means the check occurs twice. `checked_sub` reduces the number of checks by 1. Perhaps my wording could have been clearer ("avoid *an* underflow check"). Tamir
On Wed, Mar 19, 2025 at 5:13?PM Tamir Duberstein <tamird at gmail.com> wrote:> > No, I meant avoiding the check. The existing code already explicitly > checks `new_len > self.len()` before evaluating `new_len - > self.len()`. This means the check occurs twice. `checked_sub` reduces > the number of checks by 1. Perhaps my wording could have been clearer > ("avoid *an* underflow check").Ah, you mean in the function you suggested, I see. I think it they all may compile down to the same thing, whether overflows checks are enabled or not, and whether the version in the patch or `checked_sub` is used or not. At least in a quick Compiler Explorer test it seems so, but I didn't check in an actual kernel build. The implicit check is gated behind the other one, so that one can be removed, even if values are unknown -- we always have optimizations enabled, even under "debug" builds (assuming "debug" means overflow checking enabled). Cheers, Miguel
On Wed, Mar 19, 2025 at 12:43?PM Miguel Ojeda <miguel.ojeda.sandonis at gmail.com> wrote:> > On Wed, Mar 19, 2025 at 5:13?PM Tamir Duberstein <tamird at gmail.com> wrote: > > > > No, I meant avoiding the check. The existing code already explicitly > > checks `new_len > self.len()` before evaluating `new_len - > > self.len()`. This means the check occurs twice. `checked_sub` reduces > > the number of checks by 1. Perhaps my wording could have been clearer > > ("avoid *an* underflow check"). > > Ah, you mean in the function you suggested, I see. > > I think it they all may compile down to the same thing, whether > overflows checks are enabled or not, and whether the version in the > patch or `checked_sub` is used or not. At least in a quick Compiler > Explorer test it seems so, but I didn't check in an actual kernel > build. > > The implicit check is gated behind the other one, so that one can be > removed, even if values are unknown -- we always have optimizations > enabled, even under "debug" builds (assuming "debug" means overflow > checking enabled).Yeah, this elision is what I was trying to allude to in the original wording. I still think the suggested code is simpler, but gave my RB either way.