[PATCH] Add a warning for suspicious use of conditional expressions in boolean context

[PATCH] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

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

Hi!

As reported in PR77434 and PR77421 there should be a warning for
suspicious uses of conditional expressions with non-boolean arguments.

This warning triggers on conditional expressions in boolean context,
when both possible results are non-zero integer constants, so that
the resulting truth value does in fact not depend on the condition
itself.  Thus something like "if (a == b ? 1 : 2)" is always bogus,
and was most likely meant to be "if (a == (b ? 1 : 2))".


Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions.
Is it OK for trunk.


Thanks
Bernd.

[-- Attachment #2: changelog-pr77434.txt --]
[-- Type: text/plain, Size: 562 bytes --]

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

	PR c++/77434
	* doc/invoke.texi: Document -Wcond-in-bool-context.

	PR middle-end/77421
	* dwarf2out.c (output_loc_operands): Fix assertion.

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

	PR c++/77434
	* c.opt (Wcond-in-bool-context): New warning.
	* c-common.c (c_common_truthvalue_conversion): Warn on integer
	constants in boolean context.

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

	PR c++/77434
	* c-c++-common/Wcond-in-bool-context.c: New test.

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

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 239953)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4618,6 +4618,14 @@ c_common_truthvalue_conversion (location_t locatio
 					       TREE_OPERAND (expr, 0));
 
     case COND_EXPR:
+      if (TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST
+	  && TREE_CODE (TREE_OPERAND (expr, 2)) == INTEGER_CST
+	  && !integer_zerop (TREE_OPERAND (expr, 1))
+	  && !integer_zerop (TREE_OPERAND (expr, 2))
+	  && (!integer_onep (TREE_OPERAND (expr, 1))
+	      || !integer_onep (TREE_OPERAND (expr, 2))))
+	warning_at (EXPR_LOCATION (expr), OPT_Wcond_in_bool_context,
+		    "?: using integer constants in boolean context");
       /* Distribute the conversion into the arms of a COND_EXPR.  */
       if (c_dialect_cxx ())
 	{
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 239953)
+++ gcc/c-family/c.opt	(working copy)
@@ -350,6 +350,10 @@ Wcomments
 C ObjC C++ ObjC++ Warning Alias(Wcomment)
 Synonym for -Wcomment.
 
+Wcond-in-bool-context
+C ObjC C++ ObjC++ Var(warn_cond_in_bool_context) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn for conditional expressions (?:) using integer constants in boolean context.
+
 Wconditionally-supported
 C++ ObjC++ Var(warn_conditionally_supported) Warning
 Warn for conditionally-supported constructs.
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 239953)
+++ gcc/doc/invoke.texi	(working copy)
@@ -259,7 +259,8 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-attributes -Wbool-compare -Wno-builtin-macro-redefined @gol
 -Wc90-c99-compat -Wc99-c11-compat @gol
 -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align  -Wcast-qual  @gol
--Wchar-subscripts -Wclobbered  -Wcomment -Wconditionally-supported  @gol
+-Wchar-subscripts -Wclobbered -Wcomment @gol
+-Wcond-in-bool-context -Wconditionally-supported  @gol
 -Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol
 -Wdelete-incomplete @gol
 -Wno-deprecated -Wno-deprecated-declarations -Wno-designated-init @gol
@@ -5179,6 +5180,13 @@ programs.
 Warn for variables that might be changed by @code{longjmp} or
 @code{vfork}.  This warning is also enabled by @option{-Wextra}.
 
+@item -Wcond-in-bool-context
+@opindex Wcond-in-bool-context
+@opindex Wno-cond-in-bool-context
+Warn for conditional expressions (?:) using non-boolean integer constants in
+boolean context, like @code{if (a <= b ? 2 : 3)}.  This warning is enabled
+by @option{-Wall}.
+
 @item -Wconditionally-supported @r{(C++ and Objective-C++ only)}
 @opindex Wconditionally-supported
 @opindex Wno-conditionally-supported
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 239953)
+++ gcc/dwarf2out.c	(working copy)
@@ -2051,9 +2051,9 @@ output_loc_operands (dw_loc_descr_ref loc, int for
 	/* Make sure the offset has been computed and that we can encode it as
 	   an operand.  */
 	gcc_assert (die_offset > 0
-		    && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
+		    && die_offset <= (loc->dw_loc_opc == DW_OP_call2
 				     ? 0xffff
-				     : 0xffffffff);
+				     : 0xffffffff));
 	dw2_asm_output_data ((loc->dw_loc_opc == DW_OP_call2) ? 2 : 4,
 			     die_offset, NULL);
       }
Index: gcc/testsuite/c-c++-common/Wcond-in-bool-context.c
===================================================================
--- gcc/testsuite/c-c++-common/Wcond-in-bool-context.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wcond-in-bool-context.c	(working copy)
@@ -0,0 +1,17 @@
+/* PR c++/77434 */
+/* { dg-options "-Wcond-in-bool-context" } */
+/* { dg-do compile } */
+
+int foo (int a, int b)
+{
+  if (a > 0 && a <= (b == 1) ? 1 : 2) /* { dg-warning "using integer constants in boolean context" } */
+    return 1;
+
+  if (a > 0 && a <= (b == 2) ? 1 : 1) /* { dg-bogus "using integer constants in boolean context" } */
+    return 2;
+
+  if (a > 0 && a <= (b == 3) ? 0 : 2) /* { dg-bogus "using integer constants in boolean context" } */
+    return 3;
+
+  return 0;
+}

[PATCHv2] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

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

On 09/02/16 20:52, Bernd Edlinger wrote:
> Hi!
>
> As reported in PR77434 and PR77421 there should be a warning for
> suspicious uses of conditional expressions with non-boolean arguments.
>
> This warning triggers on conditional expressions in boolean context,
> when both possible results are non-zero integer constants, so that
> the resulting truth value does in fact not depend on the condition
> itself.  Thus something like "if (a == b ? 1 : 2)" is always bogus,
> and was most likely meant to be "if (a == (b ? 1 : 2))".
>
>
> Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions.
> Is it OK for trunk.
>
>
> Thanks
> Bernd.


Due to the discussion on the PR77434 I thought that it might be
helpful to diagnose signed integer shift left in boolean context
too, because the result can never depend on the shift count.

It is more likely that the precedence of << and ?: may be the
reason like in (x << y ? 1 : 2) == 4 which is apparently warned
by clang and a -Wparantheses warning for this construct may be
good to have as well.  But this code would still be suspicious
even if (x << y) is put in parentheses, because the shift count does
not change the result of the condition, as the integer overflow is
undefined behavior, and if it does not have side effects or does
not throw something, it can even be optimized away.

Therefore I thought that it might be good to generalize the
-Wcond-in-bool-context warning to cover all kinds of suspicious
integer ops in boolean context, making it a -Wint-in-bool-context
warning.

I tried to implement that and the warning immediately found something
of the form "bool flags = 1<<2;" in cp/parser.c at cp_parser_condition.

This should obviously be fixed by making flags an int.  I tried a bit
with declaring variables with "if (type v = x)", but was unable to find
any test case where this actually made a difference.


Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions.
Is it OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog-pr77434v2.txt --]
[-- Type: text/plain, Size: 704 bytes --]

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

	PR c++/77434
	* doc/invoke.texi: Document -Wint-in-bool-context.

	PR middle-end/77421
	* dwarf2out.c (output_loc_operands): Fix assertion.

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

	PR c++/77434
	* c.opt (Wint-in-bool-context): New warning.
	* c-common.c (c_common_truthvalue_conversion): Warn for suspicious
	integer expressions in boolean context.

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

	PR c++/77434
	* parser.c (cp_parser_condition): Make flags an int.


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

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

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

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 239971)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4617,7 +4617,22 @@ c_common_truthvalue_conversion (location_t locatio
 	return c_common_truthvalue_conversion (location,
 					       TREE_OPERAND (expr, 0));
 
+    case LSHIFT_EXPR:
+      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 (TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST
+	  && TREE_CODE (TREE_OPERAND (expr, 2)) == INTEGER_CST
+	  && !integer_zerop (TREE_OPERAND (expr, 1))
+	  && !integer_zerop (TREE_OPERAND (expr, 2))
+	  && (!integer_onep (TREE_OPERAND (expr, 1))
+	      || !integer_onep (TREE_OPERAND (expr, 2))))
+	warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		    "?: using integer constants in boolean context");
       /* Distribute the conversion into the arms of a COND_EXPR.  */
       if (c_dialect_cxx ())
 	{
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 239971)
+++ gcc/c-family/c.opt	(working copy)
@@ -525,6 +525,10 @@ Wint-conversion
 C ObjC Var(warn_int_conversion) Init(1) Warning
 Warn about incompatible integer to pointer and pointer to integer conversions.
 
+Wint-in-bool-context
+C ObjC C++ ObjC++ Var(warn_int_in_bool_context) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn for suspicious integer expressions in boolean context.
+
 Wint-to-pointer-cast
 C ObjC C++ ObjC++ Var(warn_int_to_pointer_cast) Init(1) Warning
 Warn when there is a cast to a pointer from an integer of a different size.
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 239971)
+++ 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 239971)
+++ gcc/doc/invoke.texi	(working copy)
@@ -273,7 +273,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wframe-larger-than=@var{len} -Wno-free-nonheap-object -Wjump-misses-init @gol
 -Wignored-qualifiers  -Wignored-attributes  -Wincompatible-pointer-types @gol
 -Wimplicit  -Wimplicit-function-declaration  -Wimplicit-int @gol
--Winit-self  -Winline  -Wno-int-conversion @gol
+-Winit-self  -Winline  -Wno-int-conversion  -Wint-in-bool-context @gol
 -Wno-int-to-pointer-cast -Winvalid-memory-model -Wno-invalid-offsetof @gol
 -Winvalid-pch -Wlarger-than=@var{len} @gol
 -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
@@ -5816,6 +5816,15 @@ warning about it.
 The restrictions on @code{offsetof} may be relaxed in a future version
 of the C++ standard.
 
+@item -Wint-in-bool-context
+@opindex Wint-in-bool-context
+@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)}.  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
 @opindex Wno-int-to-pointer-cast
 @opindex Wint-to-pointer-cast
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 239971)
+++ gcc/dwarf2out.c	(working copy)
@@ -2051,9 +2051,9 @@ output_loc_operands (dw_loc_descr_ref loc, int for
 	/* Make sure the offset has been computed and that we can encode it as
 	   an operand.  */
 	gcc_assert (die_offset > 0
-		    && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
+		    && die_offset <= (loc->dw_loc_opc == DW_OP_call2
 				     ? 0xffff
-				     : 0xffffffff);
+				     : 0xffffffff));
 	dw2_asm_output_data ((loc->dw_loc_opc == DW_OP_call2) ? 2 : 4,
 			     die_offset, NULL);
       }
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(working copy)
@@ -0,0 +1,19 @@
+/* PR c++/77434 */
+/* { dg-options "-Wint-in-bool-context" } */
+/* { dg-do compile } */
+
+int foo (int a, int b)
+{
+  if (a > 0 && a <= (b == 1) ? 1 : 2) /* { dg-warning "boolean context" } */
+    return 1;
+
+  if (a > 0 && a <= (b == 2) ? 1 : 1) /* { dg-bogus "boolean context" } */
+    return 2;
+
+  if (a > 0 && a <= (b == 3) ? 0 : 2) /* { dg-bogus "boolean context" } */
+    return 3;
+
+  for (a = 0; 1 << a; a++); /* { dg-warning "boolean context" } */
+
+  return 0;
+}

Re: [PATCHv2] Add a warning for suspicious use of conditional expressions in boolean context

[Joseph Myers]

On Sun, 4 Sep 2016, Bernd Edlinger wrote:

> good to have as well.  But this code would still be suspicious
> even if (x << y) is put in parentheses, because the shift count does
> not change the result of the condition, as the integer overflow is
> undefined behavior, and if it does not have side effects or does
> not throw something, it can even be optimized away.

It's defined in GNU C (when the shift count is nonnegative and less than 
the width of the type) - we document that we do not optimize on the basis 
of it being undefined (although ubsan will still detect it).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCHv2] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/05/16 13:23, Joseph Myers wrote:
> On Sun, 4 Sep 2016, Bernd Edlinger wrote:
>
>> good to have as well.  But this code would still be suspicious
>> even if (x << y) is put in parentheses, because the shift count does
>> not change the result of the condition, as the integer overflow is
>> undefined behavior, and if it does not have side effects or does
>> not throw something, it can even be optimized away.
>
> It's defined in GNU C (when the shift count is nonnegative and less than
> the width of the type) - we document that we do not optimize on the basis
> of it being undefined (although ubsan will still detect it).
>

Oh, good to know, thanks for this info.

Fortunately this will only be a warning, no optimization.

But I think the reasoning is still correct, left shifting
in a boolean context is suspicious, even if -fwrapv may make
it defined.  Do you agree?


Bernd.

Re: [PATCHv2] Add a warning for suspicious use of conditional expressions in boolean context

[Joseph Myers]

On Mon, 5 Sep 2016, Bernd Edlinger wrote:

> But I think the reasoning is still correct, left shifting
> in a boolean context is suspicious, even if -fwrapv may make
> it defined.  Do you agree?

Well, you can argue that if you want to test whether low bits are all 
zero, you should just use binary & with a suitable mask, or else cast to 
unsigned for the left shift.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCHv2] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/05/16 21:23, Denis Campredon wrote:
> Hi,
> Your patch does not emit warning for the following case:
> void f(int j){bool i = j ?: 3;}
>
> But for emit one for
> void f(){bool i = 4 ?: 2;}
> Regards
>

Yes, good point.

It is probably not completely unrealistic
that the middle expression may be accidentally
left out?

The first example is a bit too complicated, because
the warning does only work with integer constants
in the moment, but "j ?: 3" is expanded as "j ? j : 3",
while "4 ?: 2" is expanded as "4 ? 4 : 2" which
matches the check.

I'm not sure if detecting the special case j ?: 3
is worth the effort, but it may be doable.

Should I try that as a follow-up patch?


Thanks
Bernd.

Re: [PATCH] Add a warning for suspicious use of conditional expressions in boolean context

[Jeff Law]

On 09/02/2016 12:53 PM, Bernd Edlinger wrote:
> Hi!
>
> As reported in PR77434 and PR77421 there should be a warning for
> suspicious uses of conditional expressions with non-boolean arguments.
>
> This warning triggers on conditional expressions in boolean context,
> when both possible results are non-zero integer constants, so that
> the resulting truth value does in fact not depend on the condition
> itself.  Thus something like "if (a == b ? 1 : 2)" is always bogus,
> and was most likely meant to be "if (a == (b ? 1 : 2))".
>
>
> Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions.
> Is it OK for trunk.
>
>
> Thanks
> Bernd.
>
>
> changelog-pr77434.txt
>
>
> gcc:
> 2016-09-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77434
> 	* doc/invoke.texi: Document -Wcond-in-bool-context.
>
> 	PR middle-end/77421
> 	* dwarf2out.c (output_loc_operands): Fix assertion.
>
> c-family:
> 2016-09-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77434
> 	* c.opt (Wcond-in-bool-context): New warning.
> 	* c-common.c (c_common_truthvalue_conversion): Warn on integer
> 	constants in boolean context.
>
> testsuite:
> 2016-09-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR c++/77434
> 	* c-c++-common/Wcond-in-bool-context.c: New test.
For some reason the non-symmerty of the changes to 
c_common_truthvalue_conversion caused me to have to think far more about 
this than I probably should have.

Couldn't we have a new function
integer_zerop_or_onep

Then use

&& (!integer_zerop_or_onep (TREE_OPERAND (expr, 1))
     || !integer_zerop_or_onep (TREE_OPERAND (expr, 2)))

Ie, if they're both constants and either is not [0,1], then we warn.

With that cleanup, this is OK.

jeff

Re: [PATCH] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/12/16 21:30, Jeff Law wrote:
> On 09/02/2016 12:53 PM, Bernd Edlinger wrote:
>> Hi!
>>
>> As reported in PR77434 and PR77421 there should be a warning for
>> suspicious uses of conditional expressions with non-boolean arguments.
>>
>> This warning triggers on conditional expressions in boolean context,
>> when both possible results are non-zero integer constants, so that
>> the resulting truth value does in fact not depend on the condition
>> itself.  Thus something like "if (a == b ? 1 : 2)" is always bogus,
>> and was most likely meant to be "if (a == (b ? 1 : 2))".
>>
>>
>> Boot-strap and reg-testing on x86_64-pc-linux-gnu without regressions.
>> Is it OK for trunk.
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> changelog-pr77434.txt
>>
>>
>> gcc:
>> 2016-09-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>>     PR c++/77434
>>     * doc/invoke.texi: Document -Wcond-in-bool-context.
>>
>>     PR middle-end/77421
>>     * dwarf2out.c (output_loc_operands): Fix assertion.
>>
>> c-family:
>> 2016-09-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>>     PR c++/77434
>>     * c.opt (Wcond-in-bool-context): New warning.
>>     * c-common.c (c_common_truthvalue_conversion): Warn on integer
>>     constants in boolean context.
>>
>> testsuite:
>> 2016-09-02  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>>     PR c++/77434
>>     * c-c++-common/Wcond-in-bool-context.c: New test.
> For some reason the non-symmerty of the changes to
> c_common_truthvalue_conversion caused me to have to think far more about
> this than I probably should have.
>
> Couldn't we have a new function
> integer_zerop_or_onep
>
> Then use
>
> && (!integer_zerop_or_onep (TREE_OPERAND (expr, 1))
>      || !integer_zerop_or_onep (TREE_OPERAND (expr, 2)))
>
> Ie, if they're both constants and either is not [0,1], then we warn.
>
> With that cleanup, this is OK.
>
> jeff
>


Yes, I will update the patch accordingly.

I agree, a statement like "if (x ? 0 : 2)" can be called suspicious as
well, even if the result of x is not ignored as in "if (x ? 1 : 2)".

After I posted this patch, I extended the patch to cover also
suspicious int shift-left operations, at
https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00155.html
[PATCHv2] Add a warning for suspicious use of conditional expressions in 
boolean context

Therefore, I would like to rename the warning to -Wint-in-bool-context
so that the other patch can extend that instead of creating even more
warning names with much longer names.


Is renaming the warning OK as well, or could you have a look at the
follow-up patch please?


Thanks
Bernd.

Re: [PATCH] Add a warning for suspicious use of conditional expressions in boolean context

[Jeff Law]

On 09/12/2016 02:00 PM, Bernd Edlinger wrote:

> Yes, I will update the patch accordingly.
>
> I agree, a statement like "if (x ? 0 : 2)" can be called suspicious as
> well, even if the result of x is not ignored as in "if (x ? 1 : 2)".
>
> After I posted this patch, I extended the patch to cover also
> suspicious int shift-left operations, at
> https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00155.html
> [PATCHv2] Add a warning for suspicious use of conditional expressions in
> boolean context
>
> Therefore, I would like to rename the warning to -Wint-in-bool-context
> so that the other patch can extend that instead of creating even more
> warning names with much longer names.
>
>
> Is renaming the warning OK as well, or could you have a look at the
> follow-up patch please?
Renaming is OK as well.

jeff

Re: [PATCH] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/12/16 22:08, Jeff Law wrote:
> On 09/12/2016 02:00 PM, Bernd Edlinger wrote:
>
>> Yes, I will update the patch accordingly.
>>
>> I agree, a statement like "if (x ? 0 : 2)" can be called suspicious as
>> well, even if the result of x is not ignored as in "if (x ? 1 : 2)".
>>
>> After I posted this patch, I extended the patch to cover also
>> suspicious int shift-left operations, at
>> https://gcc.gnu.org/ml/gcc-patches/2016-09/msg00155.html
>> [PATCHv2] Add a warning for suspicious use of conditional expressions in
>> boolean context
>>
>> Therefore, I would like to rename the warning to -Wint-in-bool-context
>> so that the other patch can extend that instead of creating even more
>> warning names with much longer names.
>>
>>
>> Is renaming the warning OK as well, or could you have a look at the
>> follow-up patch please?
> Renaming is OK as well.
>

Hmm, well.  There is a problem.

If I change the warning to warn if either of the integer constants
is not [0|1] the boot-strap fails here:
./../gcc-trunk/gcc/gensupport.c: In function 'void 
compute_test_codes(rtx, file_location, char*)':
../../gcc-trunk/gcc/gensupport.c:202:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
    ((a) == I ? ((b) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:5: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
      ^
../../gcc-trunk/gcc/gensupport.c:256:26: note: in expansion of macro 
'TRISTATE_AND'
   codes[i] = TRISTATE_OR (TRISTATE_AND (op0_codes[i], op1_codes[i]),
                           ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:203:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
     (b) == I ? ((a) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:5: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
      ^
../../gcc-trunk/gcc/gensupport.c:256:26: note: in expansion of macro 
'TRISTATE_AND'
   codes[i] = TRISTATE_OR (TRISTATE_AND (op0_codes[i], op1_codes[i]),
                           ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:202:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
    ((a) == I ? ((b) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:5: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
      ^
../../gcc-trunk/gcc/gensupport.c:256:26: note: in expansion of macro 
'TRISTATE_AND'
   codes[i] = TRISTATE_OR (TRISTATE_AND (op0_codes[i], op1_codes[i]),
                           ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:203:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
     (b) == I ? ((a) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:5: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
      ^
../../gcc-trunk/gcc/gensupport.c:256:26: note: in expansion of macro 
'TRISTATE_AND'
   codes[i] = TRISTATE_OR (TRISTATE_AND (op0_codes[i], op1_codes[i]),
                           ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:202:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
    ((a) == I ? ((b) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:12: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
             ^
../../gcc-trunk/gcc/gensupport.c:257:5: note: in expansion of macro 
'TRISTATE_AND'
      TRISTATE_AND (TRISTATE_NOT (op0_codes[i]),
      ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:203:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
     (b) == I ? ((a) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:12: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
             ^
../../gcc-trunk/gcc/gensupport.c:257:5: note: in expansion of macro 
'TRISTATE_AND'
      TRISTATE_AND (TRISTATE_NOT (op0_codes[i]),
      ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:202:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
    ((a) == I ? ((b) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:12: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
             ^
../../gcc-trunk/gcc/gensupport.c:257:5: note: in expansion of macro 
'TRISTATE_AND'
      TRISTATE_AND (TRISTATE_NOT (op0_codes[i]),
      ^~~~~~~~~~~~
../../gcc-trunk/gcc/gensupport.c:203:25: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
     (b) == I ? ((a) == N ? N : I) :  \
                ~~~~~~~~~~^~~~~~~~
../../gcc-trunk/gcc/gensupport.c:209:12: note: in definition of macro 
'TRISTATE_OR'
     (a) || (b))
             ^
../../gcc-trunk/gcc/gensupport.c:257:5: note: in expansion of macro 
'TRISTATE_AND'
      TRISTATE_AND (TRISTATE_NOT (op0_codes[i]),
      ^~~~~~~~~~~~
cc1plus: all warnings being treated as errors


I had not seen that with the un-symmetric version of the warning.
But that is of course possible because it warns on a few more
cases than before.

At first sight that code looks bogus.

But I think it could fix the warning:

Index: gensupport.c
===================================================================
--- gensupport.c	(revision 240097)
+++ gensupport.c	(working copy)
@@ -201,15 +201,15 @@
  #define TRISTATE_AND(a,b)			\
    ((a) == I ? ((b) == N ? N : I) :		\
     (b) == I ? ((a) == N ? N : I) :		\
-   (a) && (b))
+   (a) == Y && (b) == Y ? Y : N)

  #define TRISTATE_OR(a,b)			\
    ((a) == I ? ((b) == Y ? Y : I) :		\
     (b) == I ? ((a) == Y ? Y : I) :		\
-   (a) || (b))
+   (a) == Y || (b) == Y ? Y : N)

  #define TRISTATE_NOT(a)				\
-  ((a) == I ? I : !(a))
+  ((a) == I ? I : (a) == Y ? N : Y)

  /* 0 means no warning about that code yet, 1 means warned.  */
  static char did_you_mean_codes[NUM_RTX_CODE];

I think the warning is justified on code like that,
what do you think?


Bernd.

[PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

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

Hi Jeff,

it turned out, that the changed symmetric condition in the warning hit
at several places, but also caused two false positives which I had to
address first.  I tried also building a recent glibc, which hit a false
positive when using __builtin_isinf_sign in boolean context, which
is used extensively by glibc.  Furtunately there were zero new warnings
in linux, well done Linus ;)

glibc's math.h has this definition:

# if __GNUC_PREREQ (4,4) && !defined __SUPPORT_SNAN__
#  define isinf(x) __builtin_isinf_sign (x)

and __builtin_isinf_sign(x) is internally folded as

isinf(x) ? (signbit(x) ? -1 : 1) : 0

which can trigger the warning if used in a boolean context.

We do not want to warn for if (isinf(x)) of course.

The other false positive was in dwarf2out, where we have this:

../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer 
constants in boolean context [-Werror=int-in-bool-context]
   if (s->refcount
       == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2))
           ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~

and:
#define DEBUG_STR_SECTION_FLAGS                                 \
   (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings               \
    ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1        \
    : SECTION_DEBUG)

which got folded in C++ to
   if (s->refcount ==
       ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2))

Because SECTION_MERGE = 0x08000, the condition for the warning is
satisfied here, but that is only an artifact that is created by the
folding of the outer COND_EXPR.

Additional to what you suggested, I relaxed the condition even more,
because I think it is also suspicious, if one of the branches is known
to be outside [0:1] and the other is unknown.

That caused another warning in the fortran FE, which was caused by
wrong placement of braces in gfc_simplify_repeat.

We have:
#define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0)

Therefore the condition must be  .. && mpz_sgn(X) != 0)

Previously that did not match, because -1 is outside [0:1]
but (Z)->size > 0 is unknown.

To suppress the bogus C++ warnings I had to suppress the warning
when the condition is fully folded in cp_convert_and_check and
c_common_truthvalue_conversion is called for the second time.

To suppress the bogus C warning on __builtin_isinf_sign I found
no other way than changing the expansion of that to
isinf(x) ? (signbit(x) ? -1 : 1) + 0 : 0
which is just enough to hide the inner COND_EXPR from
c_common_truthvalue_conversion, but gets immediately folded
away by fold_binary so it will cause no different code.

Furthermore the C++ test case df-warn-signedunsigned1.C also
triggered the warning, because here we have:

#define MAK (fl < 0 ? 1 : (fl ? -1 : 0))

And thus "if (MAK)" should be written as if (MAK != 0)

Finally, what I already wrote in my previous mail
the macros in gensupport.c mix binary and ternary
logic and should be cleaned up.


Bootstrap and reg-test with all languages on x86_64-pc-linux-gnu.
Is it OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog-pr77434.txt --]
[-- Type: text/plain, Size: 1076 bytes --]

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

	PR c++/77434
	* doc/invoke.texi: Document -Wcond-in-bool-context.
	* gensupport.c (TRISTATE_AND, TRISTATE_OR,
	TRISTATE_NOT): Fix a warning.
	* tree.h (integer_zerop_or_onep): New helper function.

	PR middle-end/77421
	* dwarf2out.c (output_loc_operands): Fix an assertion.

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

	PR c++/77434
	* c.opt (Wcond-in-bool-context): New warning.
	* c-common.c (c_common_truthvalue_conversion): Warn on integer
	constants in boolean context.

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

	PR c++/77434
	* cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.

fortran:
2016-09-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR c++/77434
	* simplify.c (gfc_simplify_repeat): Fix a warning.

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

	PR c++/77434
	* c-c++-common/Wcond-in-bool-context.c: New test.
	* g++.dg/delayedfold/df-warn-signedunsigned1.C: Fix a warning.

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

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	(revision 240135)
+++ gcc/builtins.c	(working copy)
@@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree fndecl
 	    tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);
 
 	    signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-					signbit_call, integer_zero_node);
+					    signbit_call, integer_zero_node);
 	    isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-				      isinf_call, integer_zero_node);
+					  isinf_call, integer_zero_node);
 
-	    tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, signbit_call,
-			       integer_minus_one_node, integer_one_node);
 	    tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
-			       isinf_call, tmp,
-			       integer_zero_node);
+				   signbit_call, integer_minus_one_node,
+				   integer_one_node);
+	    /* Avoid a possible -Wint-in-bool-context warning in C.  */
+	    tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
+				   integer_zero_node);
+	    tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
+				   isinf_call, tmp, integer_zero_node);
 	  }
 
 	return tmp;
Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 240135)
+++ gcc/c-family/c-common.c	(working copy)
@@ -4652,6 +4652,17 @@ c_common_truthvalue_conversion (location_t locatio
 					       TREE_OPERAND (expr, 0));
 
     case COND_EXPR:
+      if (warn_int_in_bool_context)
+	{
+	  tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
+	  tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
+	  if ((TREE_CODE (val1) == INTEGER_CST
+	       && !integer_zerop_or_onep (val1))
+	      || (TREE_CODE (val2) == INTEGER_CST
+		  && !integer_zerop_or_onep (val2)))
+	    warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+		        "?: using integer constants in boolean context");
+	}
       /* Distribute the conversion into the arms of a COND_EXPR.  */
       if (c_dialect_cxx ())
 	{
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 240135)
+++ gcc/c-family/c.opt	(working copy)
@@ -545,6 +545,10 @@ Wint-conversion
 C ObjC Var(warn_int_conversion) Init(1) Warning
 Warn about incompatible integer to pointer and pointer to integer conversions.
 
+Wint-in-bool-context
+C ObjC C++ ObjC++ Var(warn_int_in_bool_context) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn for suspicious integer expressions in boolean context.
+
 Wint-to-pointer-cast
 C ObjC C++ ObjC++ Var(warn_int_to_pointer_cast) Init(1) Warning
 Warn when there is a cast to a pointer from an integer of a different size.
Index: gcc/cp/cvt.c
===================================================================
--- gcc/cp/cvt.c	(revision 240135)
+++ gcc/cp/cvt.c	(working copy)
@@ -645,6 +645,7 @@ cp_convert_and_check (tree type, tree expr, tsubst
 	{
 	  /* Avoid bogus -Wparentheses warnings.  */
 	  warning_sentinel w (warn_parentheses);
+	  warning_sentinel c (warn_int_in_bool_context);
 	  folded_result = cp_convert (type, folded, tf_none);
 	}
       folded_result = fold_simple (folded_result);
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 240135)
+++ gcc/doc/invoke.texi	(working copy)
@@ -273,7 +273,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wframe-larger-than=@var{len} -Wno-free-nonheap-object -Wjump-misses-init @gol
 -Wignored-qualifiers  -Wignored-attributes  -Wincompatible-pointer-types @gol
 -Wimplicit  -Wimplicit-function-declaration  -Wimplicit-int @gol
--Winit-self  -Winline  -Wno-int-conversion @gol
+-Winit-self  -Winline  -Wno-int-conversion  -Wint-in-bool-context @gol
 -Wno-int-to-pointer-cast -Winvalid-memory-model -Wno-invalid-offsetof @gol
 -Winvalid-pch -Wlarger-than=@var{len} @gol
 -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
@@ -5837,6 +5837,14 @@ warning about it.
 The restrictions on @code{offsetof} may be relaxed in a future version
 of the C++ standard.
 
+@item -Wint-in-bool-context
+@opindex Wint-in-bool-context
+@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)}.
+This warning is enabled by @option{-Wall}.
+
 @item -Wno-int-to-pointer-cast
 @opindex Wno-int-to-pointer-cast
 @opindex Wint-to-pointer-cast
Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 240135)
+++ gcc/dwarf2out.c	(working copy)
@@ -2051,9 +2051,9 @@ output_loc_operands (dw_loc_descr_ref loc, int for
 	/* Make sure the offset has been computed and that we can encode it as
 	   an operand.  */
 	gcc_assert (die_offset > 0
-		    && die_offset <= (loc->dw_loc_opc == DW_OP_call2)
+		    && die_offset <= (loc->dw_loc_opc == DW_OP_call2
 				     ? 0xffff
-				     : 0xffffffff);
+				     : 0xffffffff));
 	dw2_asm_output_data ((loc->dw_loc_opc == DW_OP_call2) ? 2 : 4,
 			     die_offset, NULL);
       }
Index: gcc/fortran/simplify.c
===================================================================
--- gcc/fortran/simplify.c	(revision 240135)
+++ gcc/fortran/simplify.c	(working copy)
@@ -5127,7 +5127,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
 
   if (len ||
       (e->ts.u.cl->length &&
-       mpz_sgn (e->ts.u.cl->length->value.integer)) != 0)
+       mpz_sgn (e->ts.u.cl->length->value.integer) != 0))
     {
       const char *res = gfc_extract_int (n, &ncop);
       gcc_assert (res == NULL);
Index: gcc/gensupport.c
===================================================================
--- gcc/gensupport.c	(revision 240135)
+++ gcc/gensupport.c	(working copy)
@@ -201,15 +201,15 @@ add_implicit_parallel (rtvec vec)
 #define TRISTATE_AND(a,b)			\
   ((a) == I ? ((b) == N ? N : I) :		\
    (b) == I ? ((a) == N ? N : I) :		\
-   (a) && (b))
+   (a) == Y && (b) == Y ? Y : N)
 
 #define TRISTATE_OR(a,b)			\
   ((a) == I ? ((b) == Y ? Y : I) :		\
    (b) == I ? ((a) == Y ? Y : I) :		\
-   (a) || (b))
+   (a) == Y || (b) == Y ? Y : N)
 
 #define TRISTATE_NOT(a)				\
-  ((a) == I ? I : !(a))
+  ((a) == I ? I : (a) == Y ? N : Y)
 
 /* 0 means no warning about that code yet, 1 means warned.  */
 static char did_you_mean_codes[NUM_RTX_CODE];
Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c
===================================================================
--- gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c	(working copy)
@@ -0,0 +1,32 @@
+/* PR c++/77434 */
+/* { dg-options "-Wint-in-bool-context" } */
+/* { dg-do compile } */
+
+int foo (int a, int b)
+{
+  if (a > 0 && a <= (b == 1) ? 1 : 2) /* { dg-warning "boolean context" } */
+    return 1;
+
+  if (a > 0 && a <= (b == 2) ? 1 : 1) /* { dg-bogus "boolean context" } */
+    return 2;
+
+  if (a > 0 && a <= (b == 3) ? 0 : 2) /* { dg-warning "boolean context" } */
+    return 3;
+
+  if (a == (((b == 3) ? 1|2|4 : 1) & 2 ? 0 : 2)) /* { dg-bogus "boolean context" } */
+    return 4;
+
+  if (a <= b ? 1000-1 : 0 && (!a || !b)) /* { dg-warning "boolean context" } */
+    return 5;
+
+  if (a ? a : 1+1) /* { dg-warning "boolean context" } */
+    return 6;
+
+  if (a ? b : 0) /* { dg-bogus "boolean context" } */
+    return 7;
+
+  if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean context" } */
+    return 8;
+
+  return 0;
+}
Index: gcc/testsuite/g++.dg/delayedfold/df-warn-signedunsigned1.C
===================================================================
--- gcc/testsuite/g++.dg/delayedfold/df-warn-signedunsigned1.C	(revision 240135)
+++ gcc/testsuite/g++.dg/delayedfold/df-warn-signedunsigned1.C	(working copy)
@@ -7,7 +7,7 @@ extern int fl;
 
 int foo (int sz)
 {
-  if (MAK) return 1;
+  if (MAK != 0) return 1;
   return 0;
 }
 
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 240135)
+++ gcc/tree.h	(working copy)
@@ -4361,6 +4361,15 @@ extern int integer_nonzerop (const_tree);
 
 extern int integer_truep (const_tree);
 
+/* integer_zerop_or_onep (tree x) is nonzero if X is an integer constant
+   of value 0 or 1.  */
+
+static inline int
+integer_zerop_or_onep (const_tree x)
+{
+  return integer_zerop (x) || integer_onep (x);
+}
+
 extern bool cst_and_fits_in_hwi (const_tree);
 extern tree num_ending_zeros (const_tree);
 

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Jason Merrill]

On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> The other false positive was in dwarf2out, where we have this:
>
> ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer
> constants in boolean context [-Werror=int-in-bool-context]
>    if (s->refcount
>        == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2))
>            ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
>
> and:
> #define DEBUG_STR_SECTION_FLAGS                                 \
>    (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings               \
>     ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1        \
>     : SECTION_DEBUG)
>
> which got folded in C++ to
>    if (s->refcount ==
>        ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2))

Hmm, I'd think this warning should be looking at unfolded trees.

> Additional to what you suggested, I relaxed the condition even more,
> because I think it is also suspicious, if one of the branches is known
> to be outside [0:1] and the other is unknown.
>
> That caused another warning in the fortran FE, which was caused by
> wrong placement of braces in gfc_simplify_repeat.
>
> We have:
> #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0)
>
> Therefore the condition must be  .. && mpz_sgn(X) != 0)
>
> Previously that did not match, because -1 is outside [0:1]
> but (Z)->size > 0 is unknown.

But (Z)->size > 0 is known to be boolean; that seems like it should
suppress this warning.

> Furthermore the C++ test case df-warn-signedunsigned1.C also
> triggered the warning, because here we have:
>
> #define MAK (fl < 0 ? 1 : (fl ? -1 : 0))
>
> And thus "if (MAK)" should be written as if (MAK != 0)

This also seems like a false positive to me.

It's very common for code to rely on the implicit !=0 in boolean
conversion; it seems to me that the generalization to warn about
expressions with 0 arms significantly increases the frequency of false
positives, making the flag less useful.  The original form of the
warning seems like a -Wall candidate to me, but the current form
doesn't.

Jason

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/14/16 18:37, Jason Merrill wrote:
> On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>> The other false positive was in dwarf2out, where we have this:
>>
>> ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer
>> constants in boolean context [-Werror=int-in-bool-context]
>>     if (s->refcount
>>         == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2))
>>             ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
>>
>> and:
>> #define DEBUG_STR_SECTION_FLAGS                                 \
>>     (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings               \
>>      ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1        \
>>      : SECTION_DEBUG)
>>
>> which got folded in C++ to
>>     if (s->refcount ==
>>         ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2))
>
> Hmm, I'd think this warning should be looking at unfolded trees.
>

Yes.  The truthvalue_conversion is happening in cp_convert_and_check
at cp_convert, afterwards the cp_fully_fold happens and does this
transformation that I did not notice before, but just because I did
not have the right test case.  Then if the folding did change
something the truthvalue_conversion happens again, and this time
the tree is folded potentially in a surprising way.

The new version of the patch disables the warning in the second
truthvalue_conversion and as a consequence has to use fold_for_warn
on the now unfolded parameters.  This looks like an improvement
that I would keep, because I would certainly like to warn on
x ? 1 : 1+1, which the previous version did, but just by accident.

>> Additional to what you suggested, I relaxed the condition even more,
>> because I think it is also suspicious, if one of the branches is known
>> to be outside [0:1] and the other is unknown.
>>
>> That caused another warning in the fortran FE, which was caused by
>> wrong placement of braces in gfc_simplify_repeat.
>>
>> We have:
>> #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0)
>>
>> Therefore the condition must be  .. && mpz_sgn(X) != 0)
>>
>> Previously that did not match, because -1 is outside [0:1]
>> but (Z)->size > 0 is unknown.
>
> But (Z)->size > 0 is known to be boolean; that seems like it should
> suppress this warning.
>

Hmm, maybe it got a bit too pedantic, but in some cases this
warning is pointing out something that is at least questionable
and in some cases real bugs:

For instance PR 77574, where in gcc/predict.c we had this:

    /* If edge is already improbably or cold, just return.  */
    if (e->probability <= impossible ? PROB_VERY_UNLIKELY : 0
        && (!impossible || !e->count))
       return;

thus if (x <= y ? 4999 : 0 && (...))

>> Furthermore the C++ test case df-warn-signedunsigned1.C also
>> triggered the warning, because here we have:
>>
>> #define MAK (fl < 0 ? 1 : (fl ? -1 : 0))
>>
>> And thus "if (MAK)" should be written as if (MAK != 0)
>
> This also seems like a false positive to me.
>
> It's very common for code to rely on the implicit !=0 in boolean
> conversion; it seems to me that the generalization to warn about
> expressions with 0 arms significantly increases the frequency of false
> positives, making the flag less useful.  The original form of the
> warning seems like a -Wall candidate to me, but the current form
> doesn't.
>

Yes.  The reasoning I initially had was that it is completely
pointless to have something of the form "if (x ? 1 : 2)" or
"if (x ? 0 : 0)" because the result does not even depend on x
in this case.  But something like "if (x ? 4999 : 0)" looks
bogus but does at least not ignore x.

If the false-positives are becoming too much of a problem here,
then I should of course revert to the previous heuristic again.



Thanks
Bernd.

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Steve Kargl]

On Wed, Sep 14, 2016 at 04:10:46PM +0000, Bernd Edlinger wrote:
> 
> fortran:
> 2016-09-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	PR c++/77434
> 	* simplify.c (gfc_simplify_repeat): Fix a warning.
> 
> Index: gcc/fortran/simplify.c
> ===================================================================
> --- gcc/fortran/simplify.c	(revision 240135)
> +++ gcc/fortran/simplify.c	(working copy)
> @@ -5127,7 +5127,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
>  
>    if (len ||
>        (e->ts.u.cl->length &&
> -       mpz_sgn (e->ts.u.cl->length->value.integer)) != 0)
> +       mpz_sgn (e->ts.u.cl->length->value.integer) != 0))
>      {
>        const char *res = gfc_extract_int (n, &ncop);
>        gcc_assert (res == NULL);

This part should be committed regardless of the
outcome of a review of the complete patch.  The
closing ')' is clearly missed placed.

-- 
Steve

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/14/16 19:40, Steve Kargl wrote:
> On Wed, Sep 14, 2016 at 04:10:46PM +0000, Bernd Edlinger wrote:
>>
>> fortran:
>> 2016-09-14  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>
>> 	PR c++/77434
>> 	* simplify.c (gfc_simplify_repeat): Fix a warning.
>>
>> Index: gcc/fortran/simplify.c
>> ===================================================================
>> --- gcc/fortran/simplify.c	(revision 240135)
>> +++ gcc/fortran/simplify.c	(working copy)
>> @@ -5127,7 +5127,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n)
>>
>>     if (len ||
>>         (e->ts.u.cl->length &&
>> -       mpz_sgn (e->ts.u.cl->length->value.integer)) != 0)
>> +       mpz_sgn (e->ts.u.cl->length->value.integer) != 0))
>>       {
>>         const char *res = gfc_extract_int (n, &ncop);
>>         gcc_assert (res == NULL);
>
> This part should be committed regardless of the
> outcome of a review of the complete patch.  The
> closing ')' is clearly missed placed.
>

OK, thanks, then I will go ahead and commit that one as obvious.


Bernd.

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Jason Merrill]

On Wed, Sep 14, 2016 at 1:37 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> On 09/14/16 18:37, Jason Merrill wrote:
>> On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger
>> <bernd.edlinger@hotmail.de> wrote:
>>> The other false positive was in dwarf2out, where we have this:
>>>
>>> ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer
>>> constants in boolean context [-Werror=int-in-bool-context]
>>>     if (s->refcount
>>>         == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2))
>>>             ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
>>>
>>> and:
>>> #define DEBUG_STR_SECTION_FLAGS                                 \
>>>     (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings               \
>>>      ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1        \
>>>      : SECTION_DEBUG)
>>>
>>> which got folded in C++ to
>>>     if (s->refcount ==
>>>         ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2))
>>
>> Hmm, I'd think this warning should be looking at unfolded trees.
>>
>
> Yes.  The truthvalue_conversion is happening in cp_convert_and_check
> at cp_convert, afterwards the cp_fully_fold happens and does this
> transformation that I did not notice before, but just because I did
> not have the right test case.  Then if the folding did change
> something the truthvalue_conversion happens again, and this time
> the tree is folded potentially in a surprising way.
>
> The new version of the patch disables the warning in the second
> truthvalue_conversion and as a consequence has to use fold_for_warn
> on the now unfolded parameters.  This looks like an improvement
> that I would keep, because I would certainly like to warn on
> x ? 1 : 1+1, which the previous version did, but just by accident.
>
>>> Additional to what you suggested, I relaxed the condition even more,
>>> because I think it is also suspicious, if one of the branches is known
>>> to be outside [0:1] and the other is unknown.
>>>
>>> That caused another warning in the fortran FE, which was caused by
>>> wrong placement of braces in gfc_simplify_repeat.
>>>
>>> We have:
>>> #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0)
>>>
>>> Therefore the condition must be  .. && mpz_sgn(X) != 0)
>>>
>>> Previously that did not match, because -1 is outside [0:1]
>>> but (Z)->size > 0 is unknown.
>>
>> But (Z)->size > 0 is known to be boolean; that seems like it should
>> suppress this warning.
>
> Hmm, maybe it got a bit too pedantic, but in some cases this
> warning is pointing out something that is at least questionable
> and in some cases real bugs:
>
> For instance PR 77574, where in gcc/predict.c we had this:
>
>     /* If edge is already improbably or cold, just return.  */
>     if (e->probability <= impossible ? PROB_VERY_UNLIKELY : 0
>         && (!impossible || !e->count))
>        return;
>
> thus if (x <= y ? 4999 : 0 && (...))
>
>>> Furthermore the C++ test case df-warn-signedunsigned1.C also
>>> triggered the warning, because here we have:
>>>
>>> #define MAK (fl < 0 ? 1 : (fl ? -1 : 0))
>>>
>>> And thus "if (MAK)" should be written as if (MAK != 0)
>>
>> This also seems like a false positive to me.
>>
>> It's very common for code to rely on the implicit !=0 in boolean
>> conversion; it seems to me that the generalization to warn about
>> expressions with 0 arms significantly increases the frequency of false
>> positives, making the flag less useful.  The original form of the
>> warning seems like a -Wall candidate to me, but the current form
>> doesn't.
>
> Yes.  The reasoning I initially had was that it is completely
> pointless to have something of the form "if (x ? 1 : 2)" or
> "if (x ? 0 : 0)" because the result does not even depend on x
> in this case.  But something like "if (x ? 4999 : 0)" looks
> bogus but does at least not ignore x.
>
> If the false-positives are becoming too much of a problem here,
> then I should of course revert to the previous heuristic again.

I think we could have both, where the weaker form is part of -Wall and
people can explicitly select the stronger form.

Jason

Re: [PATCHv3] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

... resent, because message apparently bounced.

On 09/14/16 21:22, Bernd Edlinger wrote:
> On 09/14/16 20:11, Jason Merrill wrote:
>>>
>>> Yes.  The reasoning I initially had was that it is completely
>>> pointless to have something of the form "if (x ? 1 : 2)" or
>>> "if (x ? 0 : 0)" because the result does not even depend on x
>>> in this case.  But something like "if (x ? 4999 : 0)" looks
>>> bogus but does at least not ignore x.
>>>
>>> If the false-positives are becoming too much of a problem here,
>>> then I should of course revert to the previous heuristic again.
>>
>> I think we could have both, where the weaker form is part of -Wall and
>> people can explicitly select the stronger form.
>>
>
>
> Yes, agreed.  So here is what I would think will be the first version.
>
> It can later be extended to cover the more pedantic cases which
> will not be enabled by -Wall.
>
> I would like to send a follow-up patch for the warning on
> signed-integer shift left in boolean context, which I think
> should also be good for Wall.
> (I already had that feature in patch version 2 but that's meanwhile
> outdated).
>
>
> Bootstrap on x86_64-pc-linux-gnu and reg-testing not yet completed.
> Is it OK for trunk when reg-testing finished?
>
>
> Thanks
> Bernd.

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Jeff Law]

On 09/14/2016 12:11 PM, Jason Merrill wrote:
>
> I think we could have both, where the weaker form is part of -Wall and
> people can explicitly select the stronger form.
That's been a fairly standard way to handle this kind of thing.  It 
works for me.

Jeff

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/15/16 17:44, Jeff Law wrote:
> On 09/14/2016 12:11 PM, Jason Merrill wrote:
>>
>> I think we could have both, where the weaker form is part of -Wall and
>> people can explicitly select the stronger form.
> That's been a fairly standard way to handle this kind of thing.  It
> works for me.
>
> Jeff

The warning could for instance be more aggressive when -pedantic is in
effect?


Bernd.

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Jeff Law]

On 09/15/2016 10:00 AM, Bernd Edlinger wrote:
> On 09/15/16 17:44, Jeff Law wrote:
>> On 09/14/2016 12:11 PM, Jason Merrill wrote:
>>>
>>> I think we could have both, where the weaker form is part of -Wall and
>>> people can explicitly select the stronger form.
>> That's been a fairly standard way to handle this kind of thing.  It
>> works for me.
>>
>> Jeff
>
> The warning could for instance be more aggressive when -pedantic is in
> effect?
I wouldn't do it on -pedantic.  We've usually used levels or -Wfoo 
-Wfoo-bar kinds of schemes.

jeff

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/15/16 18:23, Jeff Law wrote:
> On 09/15/2016 10:00 AM, Bernd Edlinger wrote:
>> On 09/15/16 17:44, Jeff Law wrote:
>>> On 09/14/2016 12:11 PM, Jason Merrill wrote:
>>>>
>>>> I think we could have both, where the weaker form is part of -Wall and
>>>> people can explicitly select the stronger form.
>>> That's been a fairly standard way to handle this kind of thing.  It
>>> works for me.
>>>
>>> Jeff
>>
>> The warning could for instance be more aggressive when -pedantic is in
>> effect?
> I wouldn't do it on -pedantic.  We've usually used levels or -Wfoo
> -Wfoo-bar kinds of schemes.

It would be kind of good to enable the extended warning level on
the gcc bootstrap, where we have -Wall and -pedantic and more.

So level 1 could be enabled with -Wall and level 2 could be enabled
with -pedantic and/or -Wpedantic.


Bernd.

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Joseph Myers]

On Thu, 15 Sep 2016, Bernd Edlinger wrote:

> So level 1 could be enabled with -Wall and level 2 could be enabled
> with -pedantic and/or -Wpedantic.

But this warning has absolutely nothing to do with things that are 
prohibited by, or undefined in, ISO standards, and so it would be 
completely inappropriate for -pedantic to affect it in any way.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Jason Merrill]

On Thu, Sep 15, 2016 at 1:42 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 15 Sep 2016, Bernd Edlinger wrote:
>
>> So level 1 could be enabled with -Wall and level 2 could be enabled
>> with -pedantic and/or -Wpedantic.
>
> But this warning has absolutely nothing to do with things that are
> prohibited by, or undefined in, ISO standards, and so it would be
> completely inappropriate for -pedantic to affect it in any way.

Agreed.  We already pass several warning flags to GCC bootstrap, we
can add another.

Jason

Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/15/16 21:54, Jason Merrill wrote:
> On Thu, Sep 15, 2016 at 1:42 PM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Thu, 15 Sep 2016, Bernd Edlinger wrote:
>>
>>> So level 1 could be enabled with -Wall and level 2 could be enabled
>>> with -pedantic and/or -Wpedantic.
>>
>> But this warning has absolutely nothing to do with things that are
>> prohibited by, or undefined in, ISO standards, and so it would be
>> completely inappropriate for -pedantic to affect it in any way.
>
> Agreed.  We already pass several warning flags to GCC bootstrap, we
> can add another.
>

Yes.

Maybe -Wextra could also enable the more strict warning.

That option is already enabled in the bootstrap and elsewhere.


Bernd.