Re: [PATCH] Make -Wint-in-bool-context warn on suspicious shift ops

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 09/29/16 20:03, Jason Merrill wrote:
> On Wed, Sep 28, 2016 at 12:09 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On 09/28/16 16:41, Jason Merrill wrote:
>>> On Tue, Sep 27, 2016 at 11:10 AM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> On 09/27/16 16:42, Jason Merrill wrote:
>>>>> On Tue, Sep 27, 2016 at 10:28 AM, Bernd Edlinger
>>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>>> On 09/27/16 16:10, Florian Weimer wrote:
>>>>>>> * Bernd Edlinger:
>>>>>>>
>>>>>>>>> “0 << 0” is used in a similar context, to create a zero constant for a
>>>>>>>>> multi-bit subfield of an integer.
>>>>>>>>>
>>>>>>>>> This example comes from GDB, in bfd/elf64-alpha.c:
>>>>>>>>>
>>>>>>>>> |   insn = INSN_ADDQ | (16 << 21) | (0 << 16) | (0 << 0);
>>>>>>>>>
>>>>>>>>
>>>>>>>> Of course that is not a boolean context, and will not get a warning.
>>>>>>>>
>>>>>>>> Question is if "if (1 << 0)" is possibly a miss-spelled "if (1 < 0)".
>>>>>>>>
>>>>>>>> Maybe 1 and 0 come from macro expansion....
>>>>>>>
>>>>>>> But what's the intent of treating 1 << 0 and 0 << 0 differently in the
>>>>>>> patch, then?
>>>>>>
>>>>>> I am not sure if it was a good idea.
>>>>>>
>>>>>> I saw, we had code of the form
>>>>>> bool flag = 1 << 2;
>>>>>>
>>>>>> another value LOOKUP_PROTECT is  1 << 0, and
>>>>>> bool flag = 1 << 0;
>>>>>>
>>>>>> would at least not overflow the allowed value range of a boolean.
>>>>>
>>>>> Assigning a bit mask to a bool variable is still probably not what was
>>>>> intended, even if it doesn't change the value.
>>>>
>>>> That works for me too.
>>>> I can simply remove that exception.
>>>
>>> Sounds good.
>>
>> Great.  Is that an "OK with that change"?
>
> What do you think about dropping the TYPE_UNSIGNED exception as well?
> I don't see what difference that makes.
>


If I drop that exception, then I could also drop the check for
INTEGER_TYPE and the whole if, because I think other types can not
happen, but if they are allowed they are as well bogus here.

I can try a bootstrap and see if there are false positives.

But I can do that as well in a follow-up patch, this should probably
be done step by step, especially when it may trigger some false
positives.

I think I could also add more stuff, like unary + or - ?
or maybe also binary +, -, * and / ?

We already discussed making this a multi-level option,
and maybe enabling the higher level explicitly in the
boot-strap.

As long as the warning continues to find more bugs than false
positives, it is probably worth extending it to more cases.

However unsigned integer shift are not undefined if they overflow.

It is possible that this warning will then trigger also on valid
code that does loop termination with unsigned int left shifting.
I dont have a real example, but maybe  like this hypothetical C-code:

  unsigned int x=1, bits=0;
  while (x << bits) bits++;
  printf("bits=%d\n", bits);


Is it OK for everybody to warn for this on -Wall, or maybe only
when -Wextra or for instance -Wint-in-bool-context=2 is used ?



Bernd.