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

Rasmus