[PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Bart Van Assche]

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
--- 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);				\
 })
 
-- 
2.21.0

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Jason Gunthorpe]

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

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Bart Van Assche]

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.

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Rasmus Villemoes]

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

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Bart Van Assche]

On 3/6/19 11:24 PM, Leon Romanovsky wrote:
> My simple patch passes too :).

Can you repost your patch?

Thanks,

Bart.

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

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.

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Kees Cook]

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

-- 
Kees Cook

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 761 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[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-----



-- 
Kees Cook

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Rasmus Villemoes]

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

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Bart Van Assche]

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.

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Rasmus Villemoes]

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

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Rasmus Villemoes]

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

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Jason Gunthorpe]

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

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

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
>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] Avoid that check_shl_overflow() triggers a compiler warning when building with W=1

[Rasmus Villemoes]

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