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

[Markus Trippelsdorf <markus at trippelsdorf dot de>]

On 2016.10.17 at 16:51 +0000, Bernd Edlinger wrote:
> On 10/17/16 17:23, Markus Trippelsdorf wrote:
> > On 2016.09.29 at 18:52 +0000, Bernd Edlinger wrote:
> >> 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 ?
> >
> > I'm seeing this warning a lot in valid low level C code for unsigned
> > integers. And I must say it look bogus in this context. Some examples:

(All these examples are from qemu trunk.)

> >  return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
> >

typedef struct {
    uint64_t low;
    uint16_t high;
} floatx80;

static inline int floatx80_is_any_nan(floatx80 a)
{
    return ((a.high & 0x7fff) == 0x7fff) && (a.low<<1);
}

> With the shift op, the result depends on integer promotion rules,
> and if the value is signed, it can invoke undefined behavior.
>
> But if a.low is a unsigned short for instance, a warning would be
> more than justified here.

> >  if ( (uint32_t) ( aSig<<( shiftCount & 31 ) ) ) {
> >
>
> Yes interesting, aSig is signed int, right?

No, it is uint32_t.

> So if the << will overflow, the code is invoking undefined behavior.
>
>
> >  && (uint64_t) (extractFloatx80Frac(a) << 1))
> >
>
> What is the result type of extractFloatx80Frac() ?

static inline uint64_t extractFloatx80Frac( floatx80 a )

>
> >  if ((plen < KEYLENGTH) && (key << plen))
> >
>
> This is from linux, yes, I have not seen that with the first
> version where the warning is only for signed shift ops.
>
> At first sight it looks really, like could it be that "key < plen"
> was meant? But yes, actually it works correctly as long
> as int is 32 bit, if int is 64 bits, that code would break
> immediately.

u8 plen;
u32 key;

> I think in the majority of code, where the author was aware of
> possible overflow issues and integer promotion rules, he will
> have used unsigned integer types, of sufficient precision.

As I wrote above, all these warning were for unsigned integer types.
And all examples are perfectly valid code as far as I can see.

--
Markus