diff mbox

Make -Wint-in-bool-context warn on suspicious shift ops

Message ID AM4PR0701MB2162458A7F27D68C8AF290D8E4D00@AM4PR0701MB2162.eurprd07.prod.outlook.com
State New
Headers show

Commit Message

Bernd Edlinger Oct. 17, 2016, 5:30 p.m. UTC 
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.

Comments

Markus Trippelsdorf Oct. 17, 2016, 5:44 p.m. UTC | #1 
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.
diff mbox

Patch

2016-10-17  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-common.c (c_common_truthvalue_conversion): Warn only for signed
	integer shift ops in boolean context.

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 241270)
+++ gcc/c-family/c-common.c	(working copy)
@@ -3328,8 +3328,10 @@ 
 					       TREE_OPERAND (expr, 0));
 
     case LSHIFT_EXPR:
-      warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
-		  "<< in boolean context, did you mean '<' ?");
+      if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+	  && !TYPE_UNSIGNED (TREE_TYPE (expr)))
+	warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		    "<< in boolean context, did you mean '<' ?");
       break;
 
     case COND_EXPR: