| Message ID | 20190307010153.81157-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Avoid that check_shl_overflow() triggers a compiler warning when building with W=1 | expand |
On Wed, Mar 06, 2019 at 05:01:53PM -0800, Bart Van Assche wrote: > This patch avoids that the following warning is reported when building > the mlx5 driver with W=1: > > drivers/infiniband/hw/mlx5/qp.c: In function set_user_rq_size: > ./include/linux/overflow.h:230:6: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] > _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \ > ^ > drivers/infiniband/hw/mlx5/qp.c:5820:6: note: in expansion of macro check_shl_overflow > if (check_shl_overflow(rwq->wqe_count, rwq->wqe_shift, &rwq->buf_size)) > ^~~~~~~~~~~~~~~~~~ > > Cc: Jason Gunthorpe <jgg@mellanox.com> > Cc: Leon Romanovsky <leonro@mellanox.com> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Fixes: 0c66847793d1 ("overflow.h: Add arithmetic shift helper") # v4.19 > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > include/linux/overflow.h | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > index 40b48e2133cb..8afe0c0ada6f 100644 > +++ b/include/linux/overflow.h > @@ -202,6 +202,24 @@ > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > > +/* > + * Evaluate a >= 0 without triggering a compiler warning if the type of a > + * is an unsigned type. > + */ > +#define is_positive(a) ({ \ > + typeof(a) _minus_one = -1LL; \ > + typeof((a) + 0U) _sign_mask = _minus_one > 0 ? 0 : \ This is probably just is_signed_type(a) > + 1ULL << (8 * sizeof(a) - 1); \ > + \ > + ((a) & _sign_mask) == 0; \ This is the same sort of obfuscation that Leon was building, do you think the & is better than his ==, > version? Will gcc shortcircuit the warning if we write it as (is_signed_type(a) && a < 0) ? Jason
On 3/6/19 5:24 PM, Jason Gunthorpe wrote: > On Wed, Mar 06, 2019 at 05:01:53PM -0800, Bart Van Assche wrote: >> This patch avoids that the following warning is reported when building >> the mlx5 driver with W=1: >> >> drivers/infiniband/hw/mlx5/qp.c: In function set_user_rq_size: >> ./include/linux/overflow.h:230:6: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] >> _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \ >> ^ >> drivers/infiniband/hw/mlx5/qp.c:5820:6: note: in expansion of macro check_shl_overflow >> if (check_shl_overflow(rwq->wqe_count, rwq->wqe_shift, &rwq->buf_size)) >> ^~~~~~~~~~~~~~~~~~ >> >> Cc: Jason Gunthorpe <jgg@mellanox.com> >> Cc: Leon Romanovsky <leonro@mellanox.com> >> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> Fixes: 0c66847793d1 ("overflow.h: Add arithmetic shift helper") # v4.19 >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> include/linux/overflow.h | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >> index 40b48e2133cb..8afe0c0ada6f 100644 >> +++ b/include/linux/overflow.h >> @@ -202,6 +202,24 @@ >> >> #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ >> >> +/* >> + * Evaluate a >= 0 without triggering a compiler warning if the type of a >> + * is an unsigned type. >> + */ >> +#define is_positive(a) ({ \ >> + typeof(a) _minus_one = -1LL; \ >> + typeof((a) + 0U) _sign_mask = _minus_one > 0 ? 0 : \ > > This is probably just is_signed_type(a) Hi Jason, I don't think that gcc accepts something like is_signed_type(typeof(a)) so I'm not sure that the is_signed_type() macro is useful in this context. >> + 1ULL << (8 * sizeof(a) - 1); \ >> + \ >> + ((a) & _sign_mask) == 0; \ > > This is the same sort of obfuscation that Leon was building, do you > think the & is better than his ==, > version? > > Will gcc shortcircuit the warning if we write it as > > (is_signed_type(a) && a < 0) > > ? I have tested this patch. With this patch applied no warnings are reported while building the mlx5 driver and the tests in lib/test_overflow.c pass. Thanks, Bart.
On 07/03/2019 03.14, Bart Van Assche wrote: > On 3/6/19 5:24 PM, Jason Gunthorpe wrote: >>> >>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>> index 40b48e2133cb..8afe0c0ada6f 100644 >>> +++ b/include/linux/overflow.h >>> @@ -202,6 +202,24 @@ >>> #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ >>> +/* >>> + * Evaluate a >= 0 without triggering a compiler warning if the type >>> of a >>> + * is an unsigned type. >>> + */ >>> +#define is_positive(a) ({ \ is_non_negative, please! positive means > 0. And perhaps it's better to move these utility macros closer to the top of the file, together with the other type/range helpers. >>> + typeof(a) _minus_one = -1LL; \ >>> + typeof((a) + 0U) _sign_mask = _minus_one > 0 ? 0 : \ >>>> This is probably just is_signed_type(a) > > Hi Jason, > > I don't think that gcc accepts something like is_signed_type(typeof(a)) > so I'm not sure that the is_signed_type() macro is useful in this context. Of course it does, it is even already used exactly that way in overflow.h. Nice hack, I can't come up with anything better ATM. So if you fix the name and reuse is_signed_type instead of opencoding it you can add my ack. >>> + 1ULL << (8 * sizeof(a) - 1); \ >>> + \ >>> + ((a) & _sign_mask) == 0; \ >> >> This is the same sort of obfuscation that Leon was building, do you >> think the & is better than his ==, > version? >> >> Will gcc shortcircuit the warning if we write it as >> >> (is_signed_type(a) && a < 0) >> >> ? Unlikely, the Wtype-limits warning trigger at a very early stage of parsing, so it's the mere presence of the a < 0 subexpression that tickles it. So no amount of hiding it behind short-circuiting logic or if() statements will help. See also the comment above __signed_mul_overflow, where even code in the the untaken branch of a __builtin_choose_expr() can trigger Wtype-limit. > I have tested this patch. With this patch applied no warnings are > reported while building the mlx5 driver and the tests in > lib/test_overflow.c pass. In cases like this it's good to be more explicit, i.e. "I've tested this patch with gcc 6.5.4 and...", and even better of course if one does it with several compiler versions. Please include the above paragraph + compiler version info in the commit log. Rasmus
On Wed, Mar 06, 2019 at 06:14:09PM -0800, Bart Van Assche wrote: > On 3/6/19 5:24 PM, Jason Gunthorpe wrote: > > On Wed, Mar 06, 2019 at 05:01:53PM -0800, Bart Van Assche wrote: > > > This patch avoids that the following warning is reported when building > > > the mlx5 driver with W=1: > > > > > > drivers/infiniband/hw/mlx5/qp.c: In function set_user_rq_size: > > > ./include/linux/overflow.h:230:6: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] > > > _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \ > > > ^ > > > drivers/infiniband/hw/mlx5/qp.c:5820:6: note: in expansion of macro check_shl_overflow > > > if (check_shl_overflow(rwq->wqe_count, rwq->wqe_shift, &rwq->buf_size)) > > > ^~~~~~~~~~~~~~~~~~ > > > > > > Cc: Jason Gunthorpe <jgg@mellanox.com> > > > Cc: Leon Romanovsky <leonro@mellanox.com> > > > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > > Fixes: 0c66847793d1 ("overflow.h: Add arithmetic shift helper") # v4.19 > > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > > include/linux/overflow.h | 22 ++++++++++++++++++++-- > > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > > index 40b48e2133cb..8afe0c0ada6f 100644 > > > +++ b/include/linux/overflow.h > > > @@ -202,6 +202,24 @@ > > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > > > +/* > > > + * Evaluate a >= 0 without triggering a compiler warning if the type of a > > > + * is an unsigned type. > > > + */ > > > +#define is_positive(a) ({ \ > > > + typeof(a) _minus_one = -1LL; \ > > > + typeof((a) + 0U) _sign_mask = _minus_one > 0 ? 0 : \ > > > > This is probably just is_signed_type(a) > > Hi Jason, > > I don't think that gcc accepts something like is_signed_type(typeof(a)) so > I'm not sure that the is_signed_type() macro is useful in this context. > > > > + 1ULL << (8 * sizeof(a) - 1); \ > > > + \ > > > + ((a) & _sign_mask) == 0; \ > > This is the same sort of obfuscation that Leon was building, do you > > think the & is better than his ==, > version? > > > > Will gcc shortcircuit the warning if we write it as > > > > (is_signed_type(a) && a < 0) > > > > ? > > I have tested this patch. With this patch applied no warnings are reported > while building the mlx5 driver and the tests in lib/test_overflow.c pass. Bart, My simple patch passes too :). > > Thanks, > > Bart.
On 3/6/19 11:24 PM, Leon Romanovsky wrote:
> My simple patch passes too :).
Can you repost your patch?
Thanks,
Bart.
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. Thanks > > Thanks, > > Bart.
On Thu, Mar 7, 2019 at 7:40 AM Leon Romanovsky <leonro@mellanox.com> 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? -Kees
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@mellanox.com> 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. > > -Kees > > -- > Kees Cook
On Thu, Mar 7, 2019 at 9:02 AM Leon Romanovsky <leonro@mellanox.com> 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@mellanox.com> 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. Acked-by: Kees Cook <keescook@chromium.org> Can this go via the rdma tree? -Kees > > > > > -Kees > > > > -- > > Kees Cook > -----BEGIN PGP SIGNATURE----- > > iQIcBAEBAgAGBQJcgU6mAAoJEORje4g2clinE94P/0pHFmUgwzRrVLxjqmnynNPC > e+azQISKrZ4EBI5Is7VwFJuxtiZvsTveCxX0NpRxk3TLfHbA4V9jz4meJ6smp4UQ > Z1uRnPbj2z5iucFN/8SelQvNTmqfvbuRSKpZ08XLxBB4XIAjFaNBbmD+REe7iSGD > xiYNp96oHvKnzGZq/eViqz0rogewsTLHoEBwDkfgyDIqwO0/3qVElNhW7Z6g/v/7 > 2D4yZiB82wIBf+00taEQNnpI/3naVvqdfl34iYGuq51Fd2S36lfmMZ1DUffd/Eq+ > jRq8PiNisFK+0A/96hwi2npVN0LS4tA5at6PHhqOfVxMOt/XAmeKu3cCaxHhjbfb > Oi2+X9/EBDdgVmylssQFwjNaLuXB00109IVDcQGgzTsN8xoTNiwla8gt3fVhDWt+ > X0jQuSnqtANt75/0mucirBoUppCB59aZ9ygolWe4UwBpVV0ZGH/0MFwcOhlpglGB > PbrKaTxP3qQeil8wGXQsJyPGOCLBGh1Qj0C6NG1wsJSX/Zq8awEoz+JlYCXezaq6 > 4R0jSHu50BGp7gt5iePRGeUhjPFVGHucJZ2b6fuDZ3ARN8MtQYmrYDyRqnFJZsCE > UZFd4SZ8UzfIETd17IowOmOs62HwXyIi1WzoWjiHsNjH2dxwiB6Lh1JBvAFQgzJ1 > 0wRa0DMnyLzmIoOdyvQm > =NFtk > -----END PGP SIGNATURE-----
On Thu, Mar 07, 2019 at 09:12:40AM -0800, Kees Cook wrote: > On Thu, Mar 7, 2019 at 9:02 AM Leon Romanovsky <leonro@mellanox.com> 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@mellanox.com> 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. > > Acked-by: Kees Cook <keescook@chromium.org> Thanks > > Can this go via the rdma tree? I think so, Jason advertised that we will have second PR to Linus this merge window. Thanks > > -Kees > > > > > > > > > -Kees > > > > > > -- > > > Kees Cook > > -----BEGIN PGP SIGNATURE----- > > > > iQIcBAEBAgAGBQJcgU6mAAoJEORje4g2clinE94P/0pHFmUgwzRrVLxjqmnynNPC > > e+azQISKrZ4EBI5Is7VwFJuxtiZvsTveCxX0NpRxk3TLfHbA4V9jz4meJ6smp4UQ > > Z1uRnPbj2z5iucFN/8SelQvNTmqfvbuRSKpZ08XLxBB4XIAjFaNBbmD+REe7iSGD > > xiYNp96oHvKnzGZq/eViqz0rogewsTLHoEBwDkfgyDIqwO0/3qVElNhW7Z6g/v/7 > > 2D4yZiB82wIBf+00taEQNnpI/3naVvqdfl34iYGuq51Fd2S36lfmMZ1DUffd/Eq+ > > jRq8PiNisFK+0A/96hwi2npVN0LS4tA5at6PHhqOfVxMOt/XAmeKu3cCaxHhjbfb > > Oi2+X9/EBDdgVmylssQFwjNaLuXB00109IVDcQGgzTsN8xoTNiwla8gt3fVhDWt+ > > X0jQuSnqtANt75/0mucirBoUppCB59aZ9ygolWe4UwBpVV0ZGH/0MFwcOhlpglGB > > PbrKaTxP3qQeil8wGXQsJyPGOCLBGh1Qj0C6NG1wsJSX/Zq8awEoz+JlYCXezaq6 > > 4R0jSHu50BGp7gt5iePRGeUhjPFVGHucJZ2b6fuDZ3ARN8MtQYmrYDyRqnFJZsCE > > UZFd4SZ8UzfIETd17IowOmOs62HwXyIi1WzoWjiHsNjH2dxwiB6Lh1JBvAFQgzJ1 > > 0wRa0DMnyLzmIoOdyvQm > > =NFtk > > -----END PGP SIGNATURE----- > > > > -- > Kees Cook
On 07/03/2019 18.12, Kees Cook wrote: > On Thu, Mar 7, 2019 at 9:02 AM Leon Romanovsky <leonro@mellanox.com> 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@mellanox.com> 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. Rasmus
On Thu, 2019-03-07 at 08:18 +0100, Rasmus Villemoes wrote: > On 07/03/2019 03.14, Bart Van Assche wrote: > > On 3/6/19 5:24 PM, Jason Gunthorpe wrote: > > > > > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > > > index 40b48e2133cb..8afe0c0ada6f 100644 > > > > +++ b/include/linux/overflow.h > > > > @@ -202,6 +202,24 @@ > > > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > > > > +/* > > > > + * Evaluate a >= 0 without triggering a compiler warning if the type > > > > of a > > > > + * is an unsigned type. > > > > + */ > > > > +#define is_positive(a) ({ \ > > is_non_negative, please! positive means > 0. And perhaps it's better to > move these utility macros closer to the top of the file, together with > the other type/range helpers. Hi Rasmus, Thank you for the feedback. But according to what I found online opinions about whether or not zero is a positive number seem to vary. From https://en.wikipedia.org/wiki/Sign_(mathematics): Terminology for signs When 0 is said to be neither positive nor negative, the following phrases may be used to refer to the sign of a number: * A number is positive if it is greater than zero. * A number is negative if it is less than zero. * A number is non-negative if it is greater than or equal to zero. * A number is non-positive if it is less than or equal to zero. When 0 is said to be both positive and negative, modified phrases are used to refer to the sign of a number: * A number is strictly positive if it is greater than zero. * A number is strictly negative if it is less than zero. * A number is positive if it is greater than or equal to zero. * A number is negative if it is less than or equal to zero. Bart.
On Thu, Mar 07, 2019 at 04:08:23PM -0800, Bart Van Assche wrote: > On Thu, 2019-03-07 at 08:18 +0100, Rasmus Villemoes wrote: > > On 07/03/2019 03.14, Bart Van Assche wrote: > > > On 3/6/19 5:24 PM, Jason Gunthorpe wrote: > > > > > > > > > > diff --git a/include/linux/overflow.h b/include/linux/overflow.h > > > > > index 40b48e2133cb..8afe0c0ada6f 100644 > > > > > +++ b/include/linux/overflow.h > > > > > @@ -202,6 +202,24 @@ > > > > > #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > > > > > +/* > > > > > + * Evaluate a >= 0 without triggering a compiler warning if the type > > > > > of a > > > > > + * is an unsigned type. > > > > > + */ > > > > > +#define is_positive(a) ({ \ > > > > is_non_negative, please! positive means > 0. And perhaps it's better to > > move these utility macros closer to the top of the file, together with > > the other type/range helpers. > > Hi Rasmus, > > Thank you for the feedback. But according to what I found online opinions > about whether or not zero is a positive number seem to vary. From > https://en.wikipedia.org/wiki/Sign_(mathematics): Mathematical therm for discrete numbers greater or equal to zero is "normal numbers". Thanks
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@mellanox.com> 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@mellanox.com> 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
On 08/03/2019 01.08, Bart Van Assche wrote: > On Thu, 2019-03-07 at 08:18 +0100, Rasmus Villemoes wrote: >> On 07/03/2019 03.14, Bart Van Assche wrote: >>> On 3/6/19 5:24 PM, Jason Gunthorpe wrote: >>>>> >>>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h >>>>> index 40b48e2133cb..8afe0c0ada6f 100644 >>>>> +++ b/include/linux/overflow.h >>>>> @@ -202,6 +202,24 @@ >>>>> #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ >>>>> +/* >>>>> + * Evaluate a >= 0 without triggering a compiler warning if the type >>>>> of a >>>>> + * is an unsigned type. >>>>> + */ >>>>> +#define is_positive(a) ({ \ >> >> is_non_negative, please! positive means > 0. And perhaps it's better to >> move these utility macros closer to the top of the file, together with >> the other type/range helpers. > > Hi Rasmus, > > Thank you for the feedback. But according to what I found online opinions > about whether or not zero is a positive number seem to vary. From > https://en.wikipedia.org/wiki/Sign_(mathematics): Yes, I'm a mathematician, I'm aware of that. You can also find people who use "less than" in the "<=" sense, and then say "strictly less than" when they mean "<". > Terminology for signs > > When 0 is said to be neither positive nor negative, the following phrases > may be used to refer to the sign of a number: > * A number is positive if it is greater than zero. > * A number is negative if it is less than zero. > * A number is non-negative if it is greater than or equal to zero. > * A number is non-positive if it is less than or equal to zero. > > When 0 is said to be both positive and negative, modified phrases are used > to refer to the sign of a number: > * A number is strictly positive if it is greater than zero. > * A number is strictly negative if it is less than zero. > * A number is positive if it is greater than or equal to zero. > * A number is negative if it is less than or equal to zero. Right, but in no way does it ever make sense to mix these conventions. So the options for describing ">= 0, < 0" are "non_negative, negative" or "positive, strictly_negative". In the context of the C language, the first convention is used. While not explicitly stated, it can be inferred from usage of the terms. First, the word nonnegative is used (e.g. in defining argc). Second, "If the value of the right operand [in a shift expression] is negative [...] the behaviour is undefined.", so certainly negative cannot include 0. Third, E* constants are required to be positive, and "[errno] is never set to zero by any library function". Etc. etc. The same goes for linux source itself. I'm sure you can find documentation in the linux source along the lines of "0 for success, negative for error", not "strictly negative for error". Rasmus
On 08/03/2019 08.01, Leon Romanovsky wrote: > > Mathematical therm for discrete numbers greater or equal to zero is > "normal numbers". Sorry, WHAT? "Normal" is used and abused for a lot of things in mathematics, but I have never heard it used that way. When attached to the word "number", it means a real number with certain properties related to its digit expansion(s). And then of course there's the isnormal() thing for floating point values in C/computing. Strong NAK to using is_normal/is_negative. Rasmus
On Fri, Mar 08, 2019 at 08:58:21AM +0100, Rasmus Villemoes wrote: > On 08/03/2019 01.08, Bart Van Assche wrote: > > On Thu, 2019-03-07 at 08:18 +0100, Rasmus Villemoes wrote: > >> On 07/03/2019 03.14, Bart Van Assche wrote: > >>> On 3/6/19 5:24 PM, Jason Gunthorpe wrote: > >>>>> > >>>>> diff --git a/include/linux/overflow.h b/include/linux/overflow.h > >>>>> index 40b48e2133cb..8afe0c0ada6f 100644 > >>>>> +++ b/include/linux/overflow.h > >>>>> @@ -202,6 +202,24 @@ > >>>>> #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ > >>>>> +/* > >>>>> + * Evaluate a >= 0 without triggering a compiler warning if the type > >>>>> of a > >>>>> + * is an unsigned type. > >>>>> + */ > >>>>> +#define is_positive(a) ({ \ > >> > >> is_non_negative, please! positive means > 0. And perhaps it's better to > >> move these utility macros closer to the top of the file, together with > >> the other type/range helpers. > > > > Hi Rasmus, > > > > Thank you for the feedback. But according to what I found online opinions > > about whether or not zero is a positive number seem to vary. From > > https://en.wikipedia.org/wiki/Sign_(mathematics): > > Yes, I'm a mathematician, I'm aware of that. You can also find people > who use "less than" in the "<=" sense, and then say "strictly less than" > when they mean "<". > > > Terminology for signs > > > > When 0 is said to be neither positive nor negative, the following phrases > > may be used to refer to the sign of a number: > > * A number is positive if it is greater than zero. > > * A number is negative if it is less than zero. > > * A number is non-negative if it is greater than or equal to zero. > > * A number is non-positive if it is less than or equal to zero. > > > > When 0 is said to be both positive and negative, modified phrases are used > > to refer to the sign of a number: > > * A number is strictly positive if it is greater than zero. > > * A number is strictly negative if it is less than zero. > > * A number is positive if it is greater than or equal to zero. > > * A number is negative if it is less than or equal to zero. > > Right, but in no way does it ever make sense to mix these conventions. > So the options for describing ">= 0, < 0" are "non_negative, negative" > or "positive, strictly_negative". > > In the context of the C language, the first convention is used. While > not explicitly stated, it can be inferred from usage of the terms. > First, the word nonnegative is used (e.g. in defining argc). Second, "If > the value of the right operand [in a shift expression] is negative [...] > the behaviour is undefined.", so certainly negative cannot include 0. > Third, E* constants are required to be positive, and "[errno] is never > set to zero by any library function". Etc. etc. Lets use is_unsigned() or is_unsigned_value() then for the name of the test, that is pretty unambiguous and alot nicer to read than is_not_negative() FWIW, in computer science I generally see the terms used as: negatve: < 0 positive: >= 0 natural: > 0 This language naturally follows the twos complement construction where it is very logical to say a number with the sign bit set is 'negative' and a number with it clear is 'positive', which means 0 is positive. Which is probably enraging to mathematicians.. But has a certain logic. .. and most CS places don't actually care about the difference between > 0 and >= 0 , while < 0 is usually highly interesting. Jason
On Fri, Mar 08, 2019 at 09:09:46AM +0100, Rasmus Villemoes wrote: > On 08/03/2019 08.01, Leon Romanovsky wrote: > > > > Mathematical therm for discrete numbers greater or equal to zero is > > "normal numbers". > > Sorry, WHAT? "Normal" is used and abused for a lot of things in > mathematics, but I have never heard it used that way. When attached to > the word "number", it means a real number with certain properties > related to its digit expansion(s). And then of course there's the > isnormal() thing for floating point values in C/computing. It is hard to argue with this type of arguments: "never heard -> doesn't exist". Luckily enough for me who can't find my fifth grade textbook from school, we have Wikipedia which has pointer to ISO standard with clear declaration of "normal numbers" as 0,1,2, .... https://en.wikipedia.org/wiki/Natural_number#cite_note-ISO80000-1 "Standard number sets and intervals". ISO 80000-2:2009. International Organization for Standardization. p. 6. > > Strong NAK to using is_normal/is_negative. > > Rasmus > >
On 08/03/2019 16.53, Leon Romanovsky wrote: > On Fri, Mar 08, 2019 at 09:09:46AM +0100, Rasmus Villemoes wrote: >> On 08/03/2019 08.01, Leon Romanovsky wrote: >>> >>> Mathematical therm for discrete numbers greater or equal to zero is >>> "normal numbers". >> >> Sorry, WHAT? "Normal" is used and abused for a lot of things in >> mathematics, but I have never heard it used that way. When attached to >> the word "number", it means a real number with certain properties >> related to its digit expansion(s). And then of course there's the >> isnormal() thing for floating point values in C/computing. > > It is hard to argue with this type of arguments: "never heard -> doesn't > exist". Luckily enough for me who can't find my fifth grade textbook > from school, we have Wikipedia which has pointer to ISO standard with > clear declaration of "normal numbers" as 0,1,2, .... I'm not really sure how to respond. The word "natural" is not the same as the word "normal". The wiki page you link to is titled "Natural number". I'm not going to pay for a copy of that iso standard, but it's easy enough to google a pdf, which shows a very clear declararation of "the set of natural numbers" on page 6. Nowhere do any of those sources talk about "normal numbers". [As the second paragraph of the wiki page points out, there is not universal agreement on whether 0 is considered a natural number - though I'm happy to learn that what I believe to be the right convention is sanctioned by an ISO standard.] Rasmus
diff --git a/include/linux/overflow.h b/include/linux/overflow.h index 40b48e2133cb..8afe0c0ada6f 100644 --- a/include/linux/overflow.h +++ b/include/linux/overflow.h @@ -202,6 +202,24 @@ #endif /* COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW */ +/* + * Evaluate a >= 0 without triggering a compiler warning if the type of a + * is an unsigned type. + */ +#define is_positive(a) ({ \ + typeof(a) _minus_one = -1LL; \ + typeof((a) + 0U) _sign_mask = _minus_one > 0 ? 0 : \ + 1ULL << (8 * sizeof(a) - 1); \ + \ + ((a) & _sign_mask) == 0; \ +}) + +/* + * Evaluate a < 0 without triggering a compiler warning if the type of a + * is an unsigned type. + */ +#define is_negative(a) !is_positive(a) + /** check_shl_overflow() - Calculate a left-shifted value and check overflow * * @a: Value to be shifted @@ -227,9 +245,9 @@ typeof(d) _d = d; \ u64 _a_full = _a; \ unsigned int _to_shift = \ - _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \ + is_positive(_s) && _s < 8 * sizeof(*d) ? _s : 0; \ *_d = (_a_full << _to_shift); \ - (_to_shift != _s || *_d < 0 || _a < 0 || \ + (_to_shift != _s || is_negative(*_d) || is_negative(_a) || \ (*_d >> _to_shift) != _a); \ })
This patch avoids that the following warning is reported when building the mlx5 driver with W=1: drivers/infiniband/hw/mlx5/qp.c: In function set_user_rq_size: ./include/linux/overflow.h:230:6: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] _s >= 0 && _s < 8 * sizeof(*d) ? _s : 0; \ ^ drivers/infiniband/hw/mlx5/qp.c:5820:6: note: in expansion of macro check_shl_overflow if (check_shl_overflow(rwq->wqe_count, rwq->wqe_shift, &rwq->buf_size)) ^~~~~~~~~~~~~~~~~~ Cc: Jason Gunthorpe <jgg@mellanox.com> Cc: Leon Romanovsky <leonro@mellanox.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Fixes: 0c66847793d1 ("overflow.h: Add arithmetic shift helper") # v4.19 Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- include/linux/overflow.h | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)