diff mbox

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

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

Commit Message

Bernd Edlinger Sept. 4, 2016, 8:03 a.m. UTC 
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.
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.

Comments

Joseph Myers Sept. 5, 2016, 11:23 a.m. UTC | #1 
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).
Bernd Edlinger Sept. 5, 2016, 2:39 p.m. UTC | #2 
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.
Joseph Myers Sept. 5, 2016, 4:46 p.m. UTC | #3 
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.
Bernd Edlinger Sept. 5, 2016, 8:02 p.m. UTC | #4 
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.
diff mbox

Patch

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