PR c/102245: Don't warn that ((_Bool)x<<0) isn't a truthvalue.

Message ID 020801d7a88c$0133aa90$039affb0$@nextmovesoftware.com
State New
Headers show
Series
  • PR c/102245: Don't warn that ((_Bool)x<<0) isn't a truthvalue.
Related show

Commit Message

Roger Sayle Sept. 13, 2021, 10:42 a.m.
If the tree expression X is a truthvalue, then X << 0 is a truthvalue.
In fact, because _Bool (truthvalue_type) has 1 bit precision, and shifts
are only well defined for bit counts less than the precision, the only
reasonable(?) left shift of a _Bool is by zero [where this reasonable
overlooks that shifts by zero should be optimized away as no-ops].

Now consider a language front-end that doesn't fold binary expressions,
hence retains (x<<0), but does fold type conversions, and can therefore
see that ((_Bool)x<<0) can be shortened to _Bool, but then warns that
any LSHIFT_EXPR in a boolean context is suspicious.

The answer is that shifts by zero are special, and that all other
shifts are indeed suspicious.  The most suspicious thing about a
(BImode) shift by zero, is why it hasn't already been optimized away.
Indeed, in Bernd Edlinger's original 2016 patch submission to warn
of LSHIFT_EXPR with -Wint-in-bool-context he included exceptions
for shifts (of truthvalues) by zero,
https://gcc.gnu.org/pipermail/gcc-patches/2016-September/457716.html
but was talked out of this during the review process, and unconditionally
warned of all LSHIFT_EXPRs by
https://gcc.gnu.org/pipermail/gcc-patches/2016-September/458263.html

This patch teaches c_common_truthvalue_conversion that a left shift
by zero is special/a no-op, and to apply the conversion to the first
operand, which both fixes the bogus warning and generates more sensible
expression trees.  [Some part of me thinks increasing the amount of
folding in the front-ends is bad, but another part thinks that calling
fold on trees that haven't had their operands folded/canonicalized
(then complaining about suspicious looking but perfectly valid results)
is sometimes worse].

This patch has been tested on x86_64-pc-linux-gnu with "make bootstrap"
and "make -k check" with no new failures.  Ok for mainline?


2021-09-13  Roger Sayle  <roger@nextmovesoftware.com>

gcc/c-family/ChangeLog
	PR c/102245
	* c-common.c (c_common_truthvalue_conversion) [LSHIFT_EXPR]:
	Special case (optimize) shifts by zero.

gcc/testsuite/ChangeLog
	PR c/102245
	* gcc.dg/Wint-in-bool-context-4.c: New test case.

Roger
--

/* PR c/102245 */
/* { dg-options "-Wint-in-bool-context" } */
/* { dg-do compile } */

_Bool test1(_Bool x)
{
  return !(x << 0);  /* { dg-bogus "boolean context" } */
}

_Bool test2(_Bool x)
{
  return !(x << 1);  /* { dg-warning "boolean context" } */
}

_Bool test3(_Bool x, int y)
{
  return !(x << y);  /* { dg-warning "boolean context" } */
}

_Bool test4(int x, int y)
{
  return !(x << y);  /* { dg-warning "boolean context" } */
}

_Bool test5(int x, int y)
{
  return !((x << y) << 0);  /* { dg-warning "boolean context" } */
}

int test6(_Bool x)
{
  int v = 0;
  return (v & ~1L) | (1L & (x << 0));  /* { dg-bogus "boolean context" } */
}

Comments

apinski--- via Gcc-patches Sept. 13, 2021, 10:58 a.m. | #1
On Mon, Sep 13, 2021 at 11:42:08AM +0100, Roger Sayle wrote:
> gcc/c-family/ChangeLog

> 	PR c/102245

> 	* c-common.c (c_common_truthvalue_conversion) [LSHIFT_EXPR]:

> 	Special case (optimize) shifts by zero.

> 

> gcc/testsuite/ChangeLog

> 	PR c/102245

> 	* gcc.dg/Wint-in-bool-context-4.c: New test case.

> 

> Roger

> --

> 


> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c

> index 017e415..44b5fcc 100644

> --- a/gcc/c-family/c-common.c

> +++ b/gcc/c-family/c-common.c

> @@ -3541,6 +3541,10 @@ c_common_truthvalue_conversion (location_t location, tree expr)

>        break;

>  

>      case LSHIFT_EXPR:

> +      /* Treat shifts by zero as a special case.  */

> +      if (integer_zerop (TREE_OPERAND (expr, 1)))

> +	return c_common_truthvalue_conversion (location,

> +					       TREE_OPERAND (expr, 0));

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


> /* PR c/102245 */

> /* { dg-options "-Wint-in-bool-context" } */

> /* { dg-do compile } */

> 

> _Bool test1(_Bool x)

> {

>   return !(x << 0);  /* { dg-bogus "boolean context" } */

> }


While this exact case is unlikely a misspelling of !(x < 0) as
no _Bool is less than zero and hopefully we get a warning for
!(x < 0), what about
_Bool test1a(int x)
{
  return !(x << 0);
}
?  I think there is a non-zero chance this was meant to be !(x < 0)
and the current
pr102245.c: In function ‘test1a’:
pr102245.c:3:14: warning: ‘<<’ in boolean context, did you mean ‘<’? [-Wint-in-bool-context]
    3 |   return !(x << 0);
      |           ~~~^~~~~
warning seems to be useful.

	Jakub

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 017e415..44b5fcc 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -3541,6 +3541,10 @@  c_common_truthvalue_conversion (location_t location, tree expr)
       break;
 
     case LSHIFT_EXPR:
+      /* Treat shifts by zero as a special case.  */
+      if (integer_zerop (TREE_OPERAND (expr, 1)))
+	return c_common_truthvalue_conversion (location,
+					       TREE_OPERAND (expr, 0));
       /* 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.