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?


Thanks
Bernd.