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

[Markus Trippelsdorf <markus@trippelsdorf.de>]

On 2016.10.17 at 17:30 +0000, Bernd Edlinger wrote:
> On 10/17/16 19:11, Markus Trippelsdorf wrote:
> >>> 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.
> >
> 
> I would be fine with disabling the warning in cases where the shift
> is done in unsigned int.  Note however, that the linux code is
> dependent on sizeof(int)<=sizeof(u32), but the warning would certainly
> be more helpful if it comes back at the day when int starts to be 64
> bits wide.
> 
> 
> How about this untested patch, does it reduce the false positive rate
> for you?

Yes, now everything is fine. Thank you.

-- 
Markus