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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

Hi!

This patch makes -Wint-in-bool-context warn on suspicious integer left
shifts, when the integer is signed, which is most likely some kind of 
programming error, for instance using "<<" instead of "<".

The warning is motivated by the fact, that an overflow on integer shift
left is undefined behavior, even if gcc won't optimize the shift based
on the undefined behavior.

So in absence of undefined behavior the boolean result does not depend
on the shift value, thus the whole shifting is pointless.


Of course the warning happened to find one bug already.  That is in
cp/parser.c at cp_parser_condition, where we have this:


bool flags = LOOKUP_ONLYCONVERTING;

BUT (cp-tree.h):

#define LOOKUP_ONLYCONVERTING (1 << 2)


So "flags" is actually set to true, which is LOOKUP_PROTECT instead.

Although I tried hard to find a test case where this changes something,
I was not able to construct one.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog-bool-context.txt --]
[-- Type: text/plain, Size: 529 bytes --]

gcc:
2016-09-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Update -Wint-in-bool-context.

c-family:
2016-09-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

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

cp:
2016-09-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* parser.c (cp_parser_condition): Fix a warning.

testsuite:
2016-09-25  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wint-in-bool-context.c: Update test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-bool-context.diff --]
[-- Type: text/x-patch; name="patch-bool-context.diff", Size: 2729 bytes --]

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 240437)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4651,6 +4651,19 @@ c_common_truthvalue_conversion (location_t locatio
 	return c_common_truthvalue_conversion (location,
 					       TREE_OPERAND (expr, 0));
 
+    case LSHIFT_EXPR:
+      /* Warn on signed integer left shift, except 0 << 0, 1 << 0.  */
+      if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+	  && !TYPE_UNSIGNED (TREE_TYPE (expr))
+	  && !(TREE_CODE (TREE_OPERAND (expr, 0)) == INTEGER_CST
+	       && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST
+	       && (integer_zerop (TREE_OPERAND (expr, 0))
+		   || integer_onep (TREE_OPERAND (expr, 0)))
+	       && integer_zerop (TREE_OPERAND (expr, 1))))
+	warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		    "<< on signed integer in boolean context");
+      break;
+
     case COND_EXPR:
       if (warn_int_in_bool_context
 	  && !from_macro_definition_at (EXPR_LOCATION (expr)))
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 240437)
+++ gcc/cp/parser.c	(working copy)
@@ -11172,7 +11172,7 @@ cp_parser_condition (cp_parser* parser)
 	{
 	  tree pushed_scope;
 	  bool non_constant_p;
-	  bool flags = LOOKUP_ONLYCONVERTING;
+	  int flags = LOOKUP_ONLYCONVERTING;
 
 	  /* Create the declaration.  */
 	  decl = start_decl (declarator, &type_specifiers,
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 240437)
+++ gcc/doc/invoke.texi	(working copy)
@@ -5927,7 +5927,8 @@ of the C++ standard.
 @opindex Wno-int-in-bool-context
 Warn for suspicious use of integer values where boolean values are expected,
 such as conditional expressions (?:) using non-boolean integer constants in
-boolean context, like @code{if (a <= b ? 2 : 3)}.
+boolean context, like @code{if (a <= b ? 2 : 3)}.  Or left shifting of a
+signed integer in boolean context, like @code{for (a = 0; 1 << a; a++);}.
 This warning is enabled by @option{-Wall}.
 
 @item -Wno-int-to-pointer-cast
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(revision 240437)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(working copy)
@@ -25,5 +25,7 @@ int foo (int a, int b)
   if (b ? 1+1 : 1) /* { dg-warning "boolean context" } */
     return 7;
 
+  for (a = 0; 1 << a; a++); /* { dg-warning "boolean context" } */
+
   return 0;
 }

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

[Jason Merrill]

On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> This patch makes -Wint-in-bool-context warn on suspicious integer left
> shifts, when the integer is signed, which is most likely some kind of
> programming error, for instance using "<<" instead of "<".
>
> The warning is motivated by the fact, that an overflow on integer shift
> left is undefined behavior, even if gcc won't optimize the shift based
> on the undefined behavior.
>
> So in absence of undefined behavior the boolean result does not depend
> on the shift value, thus the whole shifting is pointless.

It's pointless for unsigned integers, too; why not warn for them as
well?  And why not warn for 0 << 0 and 1 << 0, which are just as
pointless?

Jason

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

[Florian Weimer]

* Jason Merrill:

> On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> This patch makes -Wint-in-bool-context warn on suspicious integer left
>> shifts, when the integer is signed, which is most likely some kind of
>> programming error, for instance using "<<" instead of "<".
>>
>> The warning is motivated by the fact, that an overflow on integer shift
>> left is undefined behavior, even if gcc won't optimize the shift based
>> on the undefined behavior.
>>
>> So in absence of undefined behavior the boolean result does not depend
>> on the shift value, thus the whole shifting is pointless.
>
> It's pointless for unsigned integers, too; why not warn for them as
> well?  And why not warn for 0 << 0 and 1 << 0, which are just as
> pointless?

“1 << 0“ is often used in a sequence of flag mask definitions.  This
example is from <bits/termios.h>:

| /* Terminal control structure.  */
| struct termios
| {
|   /* Input modes.  */
|   tcflag_t c_iflag;
| #define IGNBRK  (1 << 0)        /* Ignore break condition.  */
| #define BRKINT  (1 << 1)        /* Signal interrupt on break.  */
| #define IGNPAR  (1 << 2)        /* Ignore characters with parity errors.  */
| #define PARMRK  (1 << 3)        /* Mark parity and framing errors.  */

“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);

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

[Michael Matz]

Hi,

On Tue, 27 Sep 2016, Jason Merrill wrote:

> On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
> > This patch makes -Wint-in-bool-context warn on suspicious integer left
> > shifts, when the integer is signed, which is most likely some kind of
> > programming error, for instance using "<<" instead of "<".
> >
> > The warning is motivated by the fact, that an overflow on integer shift
> > left is undefined behavior, even if gcc won't optimize the shift based
> > on the undefined behavior.
> >
> > So in absence of undefined behavior the boolean result does not depend
> > on the shift value, thus the whole shifting is pointless.
> 
> It's pointless for unsigned integers, too; why not warn for them as
> well?

Um, because left shift on unsigned integers is never undefined, so
  !!(1u << a)
is meaningful and effectively tests if a < CHAR_BITS*sizeof(unsigned) ?


Ciao,
Michael.

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

[Bernd Edlinger]

On 09/27/16 14:49, Florian Weimer wrote:
> * Jason Merrill:
>
>> On Sun, Sep 25, 2016 at 3:46 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> This patch makes -Wint-in-bool-context warn on suspicious integer left
>>> shifts, when the integer is signed, which is most likely some kind of
>>> programming error, for instance using "<<" instead of "<".
>>>
>>> The warning is motivated by the fact, that an overflow on integer shift
>>> left is undefined behavior, even if gcc won't optimize the shift based
>>> on the undefined behavior.
>>>
>>> So in absence of undefined behavior the boolean result does not depend
>>> on the shift value, thus the whole shifting is pointless.
>>
>> It's pointless for unsigned integers, too; why not warn for them as
>> well?  And why not warn for 0 << 0 and 1 << 0, which are just as
>> pointless?
>
> “1 << 0“ is often used in a sequence of flag mask definitions.  This
> example is from <bits/termios.h>:
>
> | /* Terminal control structure.  */
> | struct termios
> | {
> |   /* Input modes.  */
> |   tcflag_t c_iflag;
> | #define IGNBRK  (1 << 0)        /* Ignore break condition.  */
> | #define BRKINT  (1 << 1)        /* Signal interrupt on break.  */
> | #define IGNPAR  (1 << 2)        /* Ignore characters with parity errors.  */
> | #define PARMRK  (1 << 3)        /* Mark parity and framing errors.  */
>
> “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....



Bernd.

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

[Florian Weimer]

* 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?

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

[Bernd Edlinger]

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.


Bernd.

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

[Jason Merrill]

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.

Jason

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

[Bernd Edlinger]

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.


Bernd.

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

[Jason Merrill]

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.

Jason

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

[Bernd Edlinger]

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"?


Bernd.

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

[Jason Merrill]

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.

Jason

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

[Bernd Edlinger]

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.

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

[Bernd Edlinger]

On 09/29/16 20:52, Bernd Edlinger wrote:
> On 09/29/16 20:03, Jason Merrill wrote:
>>
>> 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 ?
>
>

Unfortunately, without that exception there is a false positive:

In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int 
adjust_packed(tree, tree, int)':
../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in 
boolean context [-Werror=int-in-bool-context]
       ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
         ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../gcc-trunk/gcc/ada/gcc-interface/decl.c:6928:7: note: in expansion 
of macro 'TYPE_ALIGN'
    if (TYPE_ALIGN (record_type)
        ^~~~~~~~~~


But that did not happen with this version:

Index: c-common.c
===================================================================
--- c-common.c	(revision 240571)
+++ c-common.c	(working copy)
@@ -4655,6 +4655,14 @@ c_common_truthvalue_conversion (location_t locatio
  	return c_common_truthvalue_conversion (location,
  					       TREE_OPERAND (expr, 0));

+    case LSHIFT_EXPR:
+      /* Warn on signed integer left shift.  */
+      if (TREE_CODE (TREE_TYPE (expr)) == INTEGER_TYPE
+	  && !TYPE_UNSIGNED (TREE_TYPE (expr)))
+	warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		    "<< on signed integer in boolean context");
+      break;
+
      case COND_EXPR:
        if (warn_int_in_bool_context
  	  && !from_macro_definition_at (EXPR_LOCATION (expr)))


Is that version OK for you?


Bernd.

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

[Jason Merrill]

On Thu, Sep 29, 2016 at 3:58 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Unfortunately, without that exception there is a false positive:
>
> In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
> ../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int
> adjust_packed(tree, tree, int)':
> ../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in
> boolean context [-Werror=int-in-bool-context]
>        ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
>          ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Ah, this issue again: the shift isn't in boolean context, it's in
integer context.  I think we want to be a lot more conservative about
these warnings in the arms of a COND_EXPR.  In fact, I think the
entire

      /* Distribute the conversion into the arms of a COND_EXPR.  */

section is wrong now that we're doing delayed folding.

Jason

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

On 09/29/16 22:38, Jason Merrill wrote:
> On Thu, Sep 29, 2016 at 3:58 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> Unfortunately, without that exception there is a false positive:
>>
>> In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
>> ../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int
>> adjust_packed(tree, tree, int)':
>> ../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in
>> boolean context [-Werror=int-in-bool-context]
>>         ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
>>           ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Ah, this issue again: the shift isn't in boolean context, it's in
> integer context.  I think we want to be a lot more conservative about
> these warnings in the arms of a COND_EXPR.  In fact, I think the
> entire
>
>        /* Distribute the conversion into the arms of a COND_EXPR.  */
>
> section is wrong now that we're doing delayed folding.
>

Could you take care of this ?


For the warning, I think I can suppress it just while
the recursing into the condition arms.

As in this updated patch.

Is it OK?


Bernd.

[-- Attachment #2: changelog-bool-context.txt --]
[-- Type: text/plain, Size: 514 bytes --]

gcc:
2016-09-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* doc/invoke.texi: Update -Wint-in-bool-context.

c-family:
2016-09-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-common.c (c_common_truthvalue_conversion): Warn for suspicious
	left shift in boolean context.

cp:
2016-09-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* parser.c (cp_parser_condition): Fix a warning.

testsuite:
2016-09-29  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	* c-c++-common/Wint-in-bool-context.c: Update test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-bool-context.diff --]
[-- Type: text/x-patch; name="patch-bool-context.diff", Size: 3448 bytes --]

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 240571)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4655,6 +4655,11 @@ c_common_truthvalue_conversion (location_t locatio
 	return c_common_truthvalue_conversion (location,
 					       TREE_OPERAND (expr, 0));
 
+    case LSHIFT_EXPR:
+      warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		  "<< in boolean context, did you mean '<' ?");
+      break;
+
     case COND_EXPR:
       if (warn_int_in_bool_context
 	  && !from_macro_definition_at (EXPR_LOCATION (expr)))
@@ -4676,6 +4681,8 @@ c_common_truthvalue_conversion (location_t locatio
 	{
 	  tree op1 = TREE_OPERAND (expr, 1);
 	  tree op2 = TREE_OPERAND (expr, 2);
+	  int w = warn_int_in_bool_context;
+	  warn_int_in_bool_context = 0;
 	  /* In C++ one of the arms might have void type if it is throw.  */
 	  if (!VOID_TYPE_P (TREE_TYPE (op1)))
 	    op1 = c_common_truthvalue_conversion (location, op1);
@@ -4683,10 +4690,13 @@ c_common_truthvalue_conversion (location_t locatio
 	    op2 = c_common_truthvalue_conversion (location, op2);
 	  expr = fold_build3_loc (location, COND_EXPR, truthvalue_type_node,
 				  TREE_OPERAND (expr, 0), op1, op2);
+	  warn_int_in_bool_context = w;
 	  goto ret;
 	}
       else
 	{
+	  int w = warn_int_in_bool_context;
+	  warn_int_in_bool_context = 0;
 	  /* Folding will happen later for C.  */
 	  expr = build3 (COND_EXPR, truthvalue_type_node,
 			 TREE_OPERAND (expr, 0),
@@ -4694,6 +4704,7 @@ c_common_truthvalue_conversion (location_t locatio
 							 TREE_OPERAND (expr, 1)),
 			 c_common_truthvalue_conversion (location,
 							 TREE_OPERAND (expr, 2)));
+	  warn_int_in_bool_context = w;
 	  goto ret;
 	}
 
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 240571)
+++ gcc/cp/parser.c	(working copy)
@@ -11244,7 +11244,7 @@ cp_parser_condition (cp_parser* parser)
 	{
 	  tree pushed_scope;
 	  bool non_constant_p;
-	  bool flags = LOOKUP_ONLYCONVERTING;
+	  int flags = LOOKUP_ONLYCONVERTING;
 
 	  /* Create the declaration.  */
 	  decl = start_decl (declarator, &type_specifiers,
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 240571)
+++ gcc/doc/invoke.texi	(working copy)
@@ -6028,7 +6028,8 @@ of the C++ standard.
 @opindex Wno-int-in-bool-context
 Warn for suspicious use of integer values where boolean values are expected,
 such as conditional expressions (?:) using non-boolean integer constants in
-boolean context, like @code{if (a <= b ? 2 : 3)}.
+boolean context, like @code{if (a <= b ? 2 : 3)}.  Or left shifting in
+boolean context, like @code{for (a = 0; 1 << a; a++);}.
 This warning is enabled by @option{-Wall}.
 
 @item -Wno-int-to-pointer-cast
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(revision 240571)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(working copy)
@@ -25,5 +25,7 @@ int foo (int a, int b)
   if (b ? 1+1 : 1) /* { dg-warning "boolean context" } */
     return 7;
 
+  for (a = 0; 1 << a; a++); /* { dg-warning "boolean context" } */
+
   return 0;
 }

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

[Jason Merrill]

OK.

On Fri, Sep 30, 2016 at 1:07 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 09/29/16 22:38, Jason Merrill wrote:
>> On Thu, Sep 29, 2016 at 3:58 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Unfortunately, without that exception there is a false positive:
>>>
>>> In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
>>> ../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int
>>> adjust_packed(tree, tree, int)':
>>> ../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in
>>> boolean context [-Werror=int-in-bool-context]
>>>         ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
>>>           ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Ah, this issue again: the shift isn't in boolean context, it's in
>> integer context.  I think we want to be a lot more conservative about
>> these warnings in the arms of a COND_EXPR.  In fact, I think the
>> entire
>>
>>        /* Distribute the conversion into the arms of a COND_EXPR.  */
>>
>> section is wrong now that we're doing delayed folding.
>>
>
> Could you take care of this ?
>
>
> For the warning, I think I can suppress it just while
> the recursing into the condition arms.
>
> As in this updated patch.
>
> Is it OK?
>
>
> Bernd.

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

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Fri, Sep 30, 2016 at 1:07 AM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 09/29/16 22:38, Jason Merrill wrote:
>> On Thu, Sep 29, 2016 at 3:58 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> Unfortunately, without that exception there is a false positive:
>>>
>>> In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
>>> ../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int
>>> adjust_packed(tree, tree, int)':
>>> ../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in
>>> boolean context [-Werror=int-in-bool-context]
>>>         ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
>>>           ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Ah, this issue again: the shift isn't in boolean context, it's in
>> integer context.  I think we want to be a lot more conservative about
>> these warnings in the arms of a COND_EXPR.  In fact, I think the
>> entire
>>
>>        /* Distribute the conversion into the arms of a COND_EXPR.  */
>>
>> section is wrong now that we're doing delayed folding.
>
> Could you take care of this ?

Done thus:

[-- Attachment #2: cond-bool.diff --]
[-- Type: text/plain, Size: 2129 bytes --]

commit 0a124bbb6f0598345c98e3a91f8c69548518d4c3
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Sep 30 17:52:21 2016 -0400

            Delay folding of bool conversion into COND_EXPR.
    
    gcc/c-family/
            * c-common.c (c_common_truthvalue_conversion): Don't distribute
            into COND_EXPR in C++.
    gcc/cp/
            * cp-gimplify.c (cp_fold): Distribute cp_truthvalue_conversion
            into COND_EXPR.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index f7a5d62..dbdb276 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -4694,21 +4694,8 @@ c_common_truthvalue_conversion (location_t location, tree expr)
 	}
       /* Distribute the conversion into the arms of a COND_EXPR.  */
       if (c_dialect_cxx ())
-	{
-	  tree op1 = TREE_OPERAND (expr, 1);
-	  tree op2 = TREE_OPERAND (expr, 2);
-	  int w = warn_int_in_bool_context;
-	  warn_int_in_bool_context = 0;
-	  /* In C++ one of the arms might have void type if it is throw.  */
-	  if (!VOID_TYPE_P (TREE_TYPE (op1)))
-	    op1 = c_common_truthvalue_conversion (location, op1);
-	  if (!VOID_TYPE_P (TREE_TYPE (op2)))
-	    op2 = c_common_truthvalue_conversion (location, op2);
-	  expr = fold_build3_loc (location, COND_EXPR, truthvalue_type_node,
-				  TREE_OPERAND (expr, 0), op1, op2);
-	  warn_int_in_bool_context = w;
-	  goto ret;
-	}
+	/* Avoid premature folding.  */
+	break;
       else
 	{
 	  int w = warn_int_in_bool_context;
diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 5aca8f2..4879632 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2253,6 +2253,15 @@ cp_fold (tree x)
       op1 = cp_fold (TREE_OPERAND (x, 1));
       op2 = cp_fold (TREE_OPERAND (x, 2));
 
+      if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
+	{
+	  warning_sentinel (warn_int_in_bool_context);
+	  if (!VOID_TYPE_P (TREE_TYPE (op1)))
+	    op1 = cp_truthvalue_conversion (op1);
+	  if (!VOID_TYPE_P (TREE_TYPE (op2)))
+	    op2 = cp_truthvalue_conversion (op2);
+	}
+
       if (op0 != TREE_OPERAND (x, 0)
 	  || op1 != TREE_OPERAND (x, 1)
 	  || op2 != TREE_OPERAND (x, 2))

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

[Bernd Edlinger]

On 10/08/16 19:40, Jason Merrill wrote:
> On Fri, Sep 30, 2016 at 1:07 AM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> On 09/29/16 22:38, Jason Merrill wrote:
>>> On Thu, Sep 29, 2016 at 3:58 PM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Unfortunately, without that exception there is a false positive:
>>>>
>>>> In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
>>>> ../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int
>>>> adjust_packed(tree, tree, int)':
>>>> ../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in
>>>> boolean context [-Werror=int-in-bool-context]
>>>>          ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
>>>>            ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Ah, this issue again: the shift isn't in boolean context, it's in
>>> integer context.  I think we want to be a lot more conservative about
>>> these warnings in the arms of a COND_EXPR.  In fact, I think the
>>> entire
>>>
>>>         /* Distribute the conversion into the arms of a COND_EXPR.  */
>>>
>>> section is wrong now that we're doing delayed folding.
>>
>> Could you take care of this ?
>
> Done thus:
>

Thanks.

But I have one question:

--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2253,6 +2253,15 @@ cp_fold (tree x)
        op1 = cp_fold (TREE_OPERAND (x, 1));
        op2 = cp_fold (TREE_OPERAND (x, 2));

+      if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
+	{
+	  warning_sentinel (warn_int_in_bool_context);


Yes, it compiles, but ...
how can this compile at all?

Doesn't it miss a name of a local?
like warning_sentinel c (warn_int_in_bool_context);

If I disassemble this function I see a constructor of
warning_sentinel directly followed by a destuctor.

And apparently warn_int_in_bool_context is not zero
in the block, thus:

       if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
	{
	  warning_sentinel (warn_int_in_bool_context);
  	  gcc_assert (!warn_int_in_bool_context);
	  if (!VOID_TYPE_P (TREE_TYPE (op1)))
	    op1 = cp_truthvalue_conversion (op1);


fails the assertion.


Bernd.

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

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Sat, Oct 8, 2016 at 4:05 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 10/08/16 19:40, Jason Merrill wrote:
>> On Fri, Sep 30, 2016 at 1:07 AM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> On 09/29/16 22:38, Jason Merrill wrote:
>>>> On Thu, Sep 29, 2016 at 3:58 PM, Bernd Edlinger
>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>> Unfortunately, without that exception there is a false positive:
>>>>>
>>>>> In file included from ../../gcc-trunk/gcc/ada/gcc-interface/decl.c:30:0:
>>>>> ../../gcc-trunk/gcc/ada/gcc-interface/decl.c: In function 'int
>>>>> adjust_packed(tree, tree, int)':
>>>>> ../../gcc-trunk/gcc/tree.h:1874:22: error: << on signed integer in
>>>>> boolean context [-Werror=int-in-bool-context]
>>>>>          ? ((unsigned)1) << ((NODE)->type_common.align - 1) : 0)
>>>>>            ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Ah, this issue again: the shift isn't in boolean context, it's in
>>>> integer context.  I think we want to be a lot more conservative about
>>>> these warnings in the arms of a COND_EXPR.  In fact, I think the
>>>> entire
>>>>
>>>>         /* Distribute the conversion into the arms of a COND_EXPR.  */
>>>>
>>>> section is wrong now that we're doing delayed folding.
>>>
>>> Could you take care of this ?
>>
>> Done thus:
>>
>
> Thanks.
>
> But I have one question:
>
> --- a/gcc/cp/cp-gimplify.c
> +++ b/gcc/cp/cp-gimplify.c
> @@ -2253,6 +2253,15 @@ cp_fold (tree x)
>         op1 = cp_fold (TREE_OPERAND (x, 1));
>         op2 = cp_fold (TREE_OPERAND (x, 2));
>
> +      if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
> +       {
> +         warning_sentinel (warn_int_in_bool_context);
>
>
> Yes, it compiles, but ...
> how can this compile at all?
>
> Doesn't it miss a name of a local?
> like warning_sentinel c (warn_int_in_bool_context);

Oops, yes, thanks.  What I wrote is an expression that creates a
temporary warning_sentinel that is then immediately destroyed.

Jason

[-- Attachment #2: cond-bool2.diff --]
[-- Type: text/plain, Size: 651 bytes --]

commit 5df6967f5f787f8da43b0122a7d01542ea70adad
Author: Jason Merrill <jason@redhat.com>
Date:   Sat Oct 8 17:51:51 2016 -0400

            * cp-gimplify.c (cp_fold): Add variable name.

diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c
index 4879632..b085f3a 100644
--- a/gcc/cp/cp-gimplify.c
+++ b/gcc/cp/cp-gimplify.c
@@ -2255,7 +2255,7 @@ cp_fold (tree x)
 
       if (TREE_CODE (TREE_TYPE (x)) == BOOLEAN_TYPE)
 	{
-	  warning_sentinel (warn_int_in_bool_context);
+	  warning_sentinel s (warn_int_in_bool_context);
 	  if (!VOID_TYPE_P (TREE_TYPE (op1)))
 	    op1 = cp_truthvalue_conversion (op1);
 	  if (!VOID_TYPE_P (TREE_TYPE (op2)))

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

[Markus Trippelsdorf]

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:

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

 if ( (uint32_t) ( aSig<<( shiftCount & 31 ) ) ) {

 && (uint64_t) (extractFloatx80Frac(a) << 1))

 if ((plen < KEYLENGTH) && (key << plen))

-- 
Markus

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

[Bernd Edlinger]

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:
>
>  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?

So if the << will overflow, the code is invoking undefined behavior.


>  && (uint64_t) (extractFloatx80Frac(a) << 1))
>

What is the result type of extractFloatx80Frac() ?


>  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.

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.

I think all of the places where I have seen this warning was issued for
wrong code it was with signed integer shifts.



Bernd.

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

[Markus Trippelsdorf]

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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-bool-context4.diff --]
[-- Type: text/x-patch; name="patch-bool-context4.diff", Size: 830 bytes --]

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:

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

[Markus Trippelsdorf]

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

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

Hi,

this restricts the -Wint-in-bool-context warning to signed shifts,
to reduce the number of false positives Markus reported yesterday.

Bootstrap and reg-testing on x86_64-pc-linux-gnu was fine.
Is it OK for trunk?


Thanks
Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-bool-context4.diff --]
[-- Type: text/x-patch; name="patch-bool-context4.diff", Size: 830 bytes --]

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:

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

[Joseph Myers]

On Tue, 18 Oct 2016, Bernd Edlinger wrote:

> Hi,
> 
> this restricts the -Wint-in-bool-context warning to signed shifts,
> to reduce the number of false positives Markus reported yesterday.

This patch seems to be missing testcases (that warned before the patch 
and don't warn after it).

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On 10/18/16 19:05, Joseph Myers wrote:
> On Tue, 18 Oct 2016, Bernd Edlinger wrote:
>
>> Hi,
>>
>> this restricts the -Wint-in-bool-context warning to signed shifts,
>> to reduce the number of false positives Markus reported yesterday.
>
> This patch seems to be missing testcases (that warned before the patch
> and don't warn after it).
>

Yes of course.

New patch, this time with a test case, compiled from the linux sample.

Bootstrapped and reg-tested as usual.
Is it OK for trunk?


Bernd.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-bool-context4.diff --]
[-- Type: text/x-patch; name="patch-bool-context4.diff", Size: 1666 bytes --]

c-family:
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.

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

	* c-c++-common/Wint-in-bool-context-2.c: New test.

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:
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context-2.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context-2.c	(working copy)
@@ -0,0 +1,17 @@
+/* { dg-options "-Wint-in-bool-context" } */
+/* { dg-do compile } */
+
+typedef unsigned u32;
+typedef unsigned char u8;
+#define KEYLENGTH 8
+
+int foo (u8 plen, u32 key)
+{
+  if ((plen < KEYLENGTH) && (key << plen)) /* { dg-bogus "boolean context" } */
+    return -1;
+
+  if ((plen << KEYLENGTH) && (key < plen)) /* { dg-warning "boolean context" } */
+    return -2;
+
+  return 0;
+}

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

[Jeff Law]

On 10/18/2016 12:14 PM, Bernd Edlinger wrote:
> On 10/18/16 19:05, Joseph Myers wrote:
>> > On Tue, 18 Oct 2016, Bernd Edlinger wrote:
>> >
>>> >> Hi,
>>> >>
>>> >> this restricts the -Wint-in-bool-context warning to signed shifts,
>>> >> to reduce the number of false positives Markus reported yesterday.
>> >
>> > This patch seems to be missing testcases (that warned before the patch
>> > and don't warn after it).
>> >
> Yes of course.
>
> New patch, this time with a test case, compiled from the linux sample.
>
> Bootstrapped and reg-tested as usual.
> Is it OK for trunk?
>
>
> Bernd.
>
>
> patch-bool-context4.diff
>
>
> c-family:
> 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.
>
> testsuite:
> 2016-10-17  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	* c-c++-common/Wint-in-bool-context-2.c: New test.
Comment please in the code indicating why we're restricting this to 
signed shifts.     I'm not entirely sure I agree with avoiding the 
warning for this case, but I'm not up for fighting about it.  So OK 
after adding the comment.

jeff

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

[Markus Trippelsdorf]

On 2016.10.19 at 14:13 -0600, Jeff Law wrote:
> On 10/18/2016 12:14 PM, Bernd Edlinger wrote:
> > On 10/18/16 19:05, Joseph Myers wrote:
> > > > On Tue, 18 Oct 2016, Bernd Edlinger wrote:
> > > >
> > > > >> Hi,
> > > > >>
> > > > >> this restricts the -Wint-in-bool-context warning to signed shifts,
> > > > >> to reduce the number of false positives Markus reported yesterday.
> > > >
> > > > This patch seems to be missing testcases (that warned before the patch
> > > > and don't warn after it).
> > > >
> > Yes of course.
> > 
> > New patch, this time with a test case, compiled from the linux sample.
> > 
> > Bootstrapped and reg-tested as usual.
> > Is it OK for trunk?
> > 
> > 
> > Bernd.
> > 
> > 
> > patch-bool-context4.diff
> > 
> > 
> > c-family:
> > 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.
> > 
> > testsuite:
> > 2016-10-17  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> > 
> > 	* c-c++-common/Wint-in-bool-context-2.c: New test.
> Comment please in the code indicating why we're restricting this to signed
> shifts.     I'm not entirely sure I agree with avoiding the warning for this
> case, but I'm not up for fighting about it.  So OK after adding the comment.

Thanks for the commit. But I think the comment is wrong:

+      /* We will only warn on unsigned shifts here, because the majority of
                               ^^ 
This should be »signed«.

-- 
Markus

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

[Bernd Edlinger]

On 10/20/16 10:05, Markus Trippelsdorf wrote:
>
> Thanks for the commit. But I think the comment is wrong:
>
> +      /* We will only warn on unsigned shifts here, because the majority of
>                                ^^
> This should be »signed«.
>

Oops.  Thanks for noticing.

This is what I am going to check in as obvious:

--- ChangeLog	(revision 241376)
+++ ChangeLog	(working copy)
@@ -1,3 +1,7 @@
+2016-10-20  Bernd Edlinger  <bernd.edlinger@hotmail.de>
+
+	* c-common.c (c_common_truthvalue_conversion): Fix the comment.
+
  2016-10-20  Jason Merrill  <jason@redhat.com>

  	* c-cppbuiltin.c (c_cpp_builtins): Update __cpp_concepts value.
Index: c-common.c
===================================================================
--- c-common.c	(revision 241376)
+++ c-common.c	(working copy)
@@ -3328,7 +3328,7 @@
  					       TREE_OPERAND (expr, 0));

      case LSHIFT_EXPR:
-      /* We will only warn on unsigned shifts here, because the majority of
+      /* We will only warn on signed shifts here, because the majority of
  	 false positive warnings happen in code where unsigned arithmetic
  	 was used in anticipation of a possible overflow.
  	 Furthermore, if we see an unsigned type here we know that the
  	 result of the shift is not subject to integer promotion rules.  */


Bernd