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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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: changelog-pr77434.txt
Description: changelog-pr77434.txt

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