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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

[-- 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;
+}