On Thu, Mar 07, 2019 at 09:28:45PM +0100, Rasmus Villemoes wrote: > On 07/03/2019 18.12, Kees Cook wrote: > > On Thu, Mar 7, 2019 at 9:02 AM Leon Romanovsky <leonro@xxxxxxxxxxxx> wrote: > >> > >> On Thu, Mar 07, 2019 at 08:52:51AM -0800, Kees Cook wrote: > >>> On Thu, Mar 7, 2019 at 7:40 AM Leon Romanovsky <leonro@xxxxxxxxxxxx> wrote: > >>>> > >>>> On Thu, Mar 07, 2019 at 06:53:54AM -0800, Bart Van Assche wrote: > >>>>> On 3/6/19 11:24 PM, Leon Romanovsky wrote: > >>>>>> My simple patch passes too :). > >>>>> > >>>>> Can you repost your patch? > >>>> > >>>> https://patchwork.kernel.org/patch/10841079/ > >>>> > >>>> As Rasmus wrote, the thing is to avoid a < 0 check. In my patch, > >>>> I converted a <= 0 to !(a > 0 || a == 0) expression. > >>> > >>> I'd be happy either way. Is there a larger benefit to having a safe > >>> "is_non_negative()" helper, or should we go with the minimal change to > >>> the shl macro? > >> > >> I personally prefer simplest possible solution. > > So, I played around with a few variants on godbolt.org, and it seems > that gcc is smart enough to combine (a > 0 || a == 0) into (a >= 0) - in > all the cases I tried Leon's patch resulted in the exact same generated > code as the current version. Conversely, and rather surprising to me, > Bart's patch seemed to cause worse code generation. So now I've changed > my mind and also support Leon's version - however, I would _strongly_ > prefer if it introduced > > #define is_non_negative(a) (a > 0 || a == 0) > #define is_negative(a) (!(is_non_negative(a)) > > with appropriate comments and used that. check_shl_overflow is hard > enough to read already. What about if we call them is_normal(a) and is_negative(a)? Thanks
Attachment:
signature.asc
Description: PGP signature
