Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179)

[Marek Polacek <polacek@redhat.com>]

On Fri, Apr 24, 2015 at 03:31:55PM -0600, Martin Sebor wrote:
> There's a significant difference between the reasons why
> the behavior of the left shift is undefined when the left
> operand is negative vs when the right operand is, and
> between the results of such expressions computed by GCC
> and common hardware.
> 
> When the right operand is negative the operation simply
> has no meaning (some languages define the operation as
> shifting right by the inverse number of bits but that's
> by no means universal). In practice, the right operand
> is sometimes limited by the hardware to a small non-
> negative value (e.g., 32 for the i386 shll instruction)
> so there's no way to shift a value by a negative number
> of bits or by more than there are bits in the first
> operand. As a result, GCC can compute a different result
> than common hardware. For example, it substitutes 0 for
> the result of 1 << -1 while x86 computes INT_MIN)
> 
> In contrast, shifting a negative value by a positive number
> of bits does have a natural meaning (i.e., shifting the bit
> pattern the same way as unsigned). The reason why it's
> undefined in C and C++ is because some processors don't
> shift the sign bit out and may raise an overflow when
> a one bit is shifted into the sign position (typically
> those that provide an arithmetic left shift). But most
> processors implement a logical left shift and behave
> the same way for signed operands as for unsigned.
> 
> The result of a left shift of a negative number computed
> by GCC matches that of hardware that doesn't differentiate
> between arithmetic and logical left shifts (which is all
> the common CPUs, including ARM, MIPS, PowerPC, x86), so
> the only value in diagnosing it is portability to rare
> CPUs or to compilers that behave differently than GCC
> (if there are any).

Point taken.  The following variant warns only in pedantic mode.  In case even
that is overly strict, we can move this warning out of pedantic and require it
be specifically enabled to trigger.  I'm happy as long as there's an option to
warn about something that ISO C/C++ say is undefined.

I'm not applying this yet; instead, I'll wait to give other folks a chance to
chime in.

Thanks for your comments Martin!

Bootstrapped/regtested on x86_64-linux.

2015-04-25  Marek Polacek  <polacek@redhat.com>

	PR c/65179
	* c-common.c (c_fully_fold_internal): Warn when left shifting a
	negative value.
	* c.opt (Wshift-negative-value): New option.

	* c-typeck.c (build_binary_op): Warn when left shifting a negative
	value.

	* typeck.c (cp_build_binary_op): Warn when left shifting a negative
	value.

	* doc/invoke.texi: Document -Wshift-negative-value.

	* c-c++-common/Wshift-negative-value-1.c: New test.
	* c-c++-common/Wshift-negative-value-2.c: New test.
	* c-c++-common/Wshift-negative-value-3.c: New test.
	* c-c++-common/Wshift-negative-value-4.c: New test.
	* gcc.dg/c99-const-expr-7.c: Add dg-error.
	* g++.dg/init/array11.C: Likewise.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 9797e17..b98c48b 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1361,6 +1361,15 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands,
 	  && !TREE_OVERFLOW_P (op0)
 	  && !TREE_OVERFLOW_P (op1))
 	overflow_warning (EXPR_LOCATION (expr), ret);
+      if (code == LSHIFT_EXPR
+	  && TREE_CODE (orig_op0) != INTEGER_CST
+	  && TREE_CODE (TREE_TYPE (orig_op0)) == INTEGER_TYPE
+	  && TREE_CODE (op0) == INTEGER_CST
+	  && c_inhibit_evaluation_warnings == 0
+	  && tree_int_cst_sgn (op0) < 0
+	  && flag_isoc99)
+	pedwarn (loc, OPT_Wshift_negative_value,
+		 "left shift of negative value");
       if ((code == LSHIFT_EXPR || code == RSHIFT_EXPR)
 	  && TREE_CODE (orig_op1) != INTEGER_CST
 	  && TREE_CODE (op1) == INTEGER_CST
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 983f4a8..1871cb3 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -781,6 +781,10 @@ Wshift-count-overflow
 C ObjC C++ ObjC++ Var(warn_shift_count_overflow) Init(1) Warning
 Warn if shift count >= width of type
 
+Wshift-negative-value
+C ObjC C++ ObjC++ Var(warn_shift_negative_value) Warning LangEnabledBy(C ObjC C++ ObjC++,Wpedantic)
+Warn if left shifting a negative value
+
 Wsign-compare
 C ObjC C++ ObjC++ Var(warn_sign_compare) Warning LangEnabledBy(C++ ObjC++,Wall)
 Warn about signed-unsigned comparisons
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 91735b5..2982817 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10707,6 +10707,16 @@ build_binary_op (location_t location, enum tree_code code,
 	  && code1 == INTEGER_TYPE)
 	{
 	  doing_shift = true;
+	  if (TREE_CODE (op0) == INTEGER_CST
+	      && tree_int_cst_sgn (op0) < 0
+	      && flag_isoc99
+	      && (pedantic || warn_shift_negative_value))
+	    {
+	      int_const = false;
+	      if (c_inhibit_evaluation_warnings == 0)
+		pedwarn (location, OPT_Wshift_negative_value,
+			 "left shift of negative value");
+	    }
 	  if (TREE_CODE (op1) == INTEGER_CST)
 	    {
 	      if (tree_int_cst_sgn (op1) < 0)
diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 91db32a..ccf3fd6 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4326,11 +4326,21 @@ cp_build_binary_op (location_t location,
 	}
       else if (code0 == INTEGER_TYPE && code1 == INTEGER_TYPE)
 	{
+	  tree const_op0 = fold_non_dependent_expr (op0);
+	  if (TREE_CODE (const_op0) != INTEGER_CST)
+	    const_op0 = op0;
 	  tree const_op1 = fold_non_dependent_expr (op1);
 	  if (TREE_CODE (const_op1) != INTEGER_CST)
 	    const_op1 = op1;
 	  result_type = type0;
 	  doing_shift = true;
+	  if (TREE_CODE (const_op0) == INTEGER_CST
+	      && tree_int_cst_sgn (const_op0) < 0
+	      && (complain & tf_warning)
+	      && c_inhibit_evaluation_warnings == 0
+	      && cxx_dialect >= cxx11)
+	    pedwarn (input_location, OPT_Wshift_negative_value,
+		     "left shift of negative value");
 	  if (TREE_CODE (const_op1) == INTEGER_CST)
 	    {
 	      if (tree_int_cst_lt (const_op1, integer_zero_node))
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 7d2f6e5..a52fe2c 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -271,7 +271,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wpointer-arith  -Wno-pointer-to-int-cast @gol
 -Wredundant-decls  -Wno-return-local-addr @gol
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
--Wshift-count-negative -Wshift-count-overflow @gol
+-Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol
 -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
 -Wsizeof-pointer-memaccess  -Wsizeof-array-argument @gol
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
@@ -3914,6 +3914,12 @@ Warn if shift count is negative. This warning is enabled by default.
 @opindex Wno-shift-count-overflow
 Warn if shift count >= width of type. This warning is enabled by default.
 
+@item -Wshift-negative-value
+@opindex Wshift-negative-value
+@opindex Wno-shift-negative-value
+Warn if left shifting a negative value.  This warning only occurs in C99 and
+C++11 modes (and newer).  This warning is enabled by @option{-Wpedantic}.
+
 @item -Wswitch
 @opindex Wswitch
 @opindex Wno-switch
diff --git gcc/testsuite/c-c++-common/Wshift-negative-value-1.c gcc/testsuite/c-c++-common/Wshift-negative-value-1.c
index e69de29..b58b2e4 100644
--- gcc/testsuite/c-c++-common/Wshift-negative-value-1.c
+++ gcc/testsuite/c-c++-common/Wshift-negative-value-1.c
@@ -0,0 +1,49 @@
+/* PR c/65179 */
+/* { dg-do compile } */
+/* { dg-options "-O -pedantic" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+enum E {
+  A = 0 << 1,
+  B = 1 << 1,
+  C = -1 << 1, /* { dg-warning "left shift of negative value|not an integer constant" } */
+  D = 0 >> 1,
+  E = 1 >> 1,
+  F = -1 >> 1
+};
+
+int
+left (int x)
+{
+  /* Warn for LSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z << x;
+  r += o << x;
+  r += m << x; /* { dg-warning "left shift of negative value" } */
+  r += 0 << x;
+  r += 1 << x;
+  r += -1 << x; /* { dg-warning "left shift of negative value" } */
+  r += -1U << x;
+  return r;
+}
+
+int
+right (int x)
+{
+  /* Shouldn't warn for RSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z >> x;
+  r += o >> x;
+  r += m >> x;
+  r += 0 >> x;
+  r += 1 >> x;
+  r += -1 >> x;
+  r += -1U >> x;
+  return r;
+}
diff --git gcc/testsuite/c-c++-common/Wshift-negative-value-2.c gcc/testsuite/c-c++-common/Wshift-negative-value-2.c
index e69de29..fc89af1 100644
--- gcc/testsuite/c-c++-common/Wshift-negative-value-2.c
+++ gcc/testsuite/c-c++-common/Wshift-negative-value-2.c
@@ -0,0 +1,49 @@
+/* PR c/65179 */
+/* { dg-do compile } */
+/* { dg-options "-O -Wshift-negative-value" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+enum E {
+  A = 0 << 1,
+  B = 1 << 1,
+  C = -1 << 1, /* { dg-warning "left shift of negative value" } */
+  D = 0 >> 1,
+  E = 1 >> 1,
+  F = -1 >> 1
+};
+
+int
+left (int x)
+{
+  /* Warn for LSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z << x;
+  r += o << x;
+  r += m << x; /* { dg-warning "left shift of negative value" } */
+  r += 0 << x;
+  r += 1 << x;
+  r += -1 << x; /* { dg-warning "left shift of negative value" } */
+  r += -1U << x;
+  return r;
+}
+
+int
+right (int x)
+{
+  /* Shouldn't warn for RSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z >> x;
+  r += o >> x;
+  r += m >> x;
+  r += 0 >> x;
+  r += 1 >> x;
+  r += -1 >> x;
+  r += -1U >> x;
+  return r;
+}
diff --git gcc/testsuite/c-c++-common/Wshift-negative-value-3.c gcc/testsuite/c-c++-common/Wshift-negative-value-3.c
index e69de29..244dbec 100644
--- gcc/testsuite/c-c++-common/Wshift-negative-value-3.c
+++ gcc/testsuite/c-c++-common/Wshift-negative-value-3.c
@@ -0,0 +1,49 @@
+/* PR c/65179 */
+/* { dg-do compile } */
+/* { dg-options "-O -pedantic -Wno-shift-negative-value" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+enum E {
+  A = 0 << 1,
+  B = 1 << 1,
+  C = -1 << 1, /* { dg-warning "not an integer constant" "" { target c } 9 } */
+  D = 0 >> 1,
+  E = 1 >> 1,
+  F = -1 >> 1
+};
+
+int
+left (int x)
+{
+  /* Warn for LSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z << x;
+  r += o << x;
+  r += m << x; /* { dg-bogus "left shift of negative value" } */
+  r += 0 << x;
+  r += 1 << x;
+  r += -1 << x; /* { dg-bogus "left shift of negative value" } */
+  r += -1U << x;
+  return r;
+}
+
+int
+right (int x)
+{
+  /* Shouldn't warn for RSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z >> x;
+  r += o >> x;
+  r += m >> x;
+  r += 0 >> x;
+  r += 1 >> x;
+  r += -1 >> x;
+  r += -1U >> x;
+  return r;
+}
diff --git gcc/testsuite/c-c++-common/Wshift-negative-value-4.c gcc/testsuite/c-c++-common/Wshift-negative-value-4.c
index e69de29..85fbd0e 100644
--- gcc/testsuite/c-c++-common/Wshift-negative-value-4.c
+++ gcc/testsuite/c-c++-common/Wshift-negative-value-4.c
@@ -0,0 +1,49 @@
+/* PR c/65179 */
+/* { dg-do compile } */
+/* { dg-options "-O" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+enum E {
+  A = 0 << 1,
+  B = 1 << 1,
+  C = -1 << 1,
+  D = 0 >> 1,
+  E = 1 >> 1,
+  F = -1 >> 1
+};
+
+int
+left (int x)
+{
+  /* Warn for LSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z << x;
+  r += o << x;
+  r += m << x; /* { dg-bogus "left shift of negative value" } */
+  r += 0 << x;
+  r += 1 << x;
+  r += -1 << x; /* { dg-bogus "left shift of negative value" } */
+  r += -1U << x;
+  return r;
+}
+
+int
+right (int x)
+{
+  /* Shouldn't warn for RSHIFT_EXPR.  */
+  const int z = 0;
+  const int o = 1;
+  const int m = -1;
+  int r = 0;
+  r += z >> x;
+  r += o >> x;
+  r += m >> x;
+  r += 0 >> x;
+  r += 1 >> x;
+  r += -1 >> x;
+  r += -1U >> x;
+  return r;
+}
diff --git gcc/testsuite/g++.dg/init/array11.C gcc/testsuite/g++.dg/init/array11.C
index e52effe..769a2a6 100644
--- gcc/testsuite/g++.dg/init/array11.C
+++ gcc/testsuite/g++.dg/init/array11.C
@@ -21,7 +21,7 @@ struct gdt gdt_table[2]=
 {
     {
 		0,
-		( (((size_t)(&x))<<(24))&(-1<<(8)) ),
+		( (((size_t)(&x))<<(24))&(-1<<(8)) ), // { dg-error "left shift of negative value" "" { target c++11 } }
     },
 };
 }
diff --git gcc/testsuite/gcc.dg/c99-const-expr-7.c gcc/testsuite/gcc.dg/c99-const-expr-7.c
index b872077..667686c 100644
--- gcc/testsuite/gcc.dg/c99-const-expr-7.c
+++ gcc/testsuite/gcc.dg/c99-const-expr-7.c
@@ -30,8 +30,8 @@ int f1 = (0 ? 0 << -1 : 0);
 int g1 = (0 ? 0 >> 1000 : 0);
 int h1 = (0 ? 0 >> -1: 0);
 
-/* Allowed for now, but actually undefined behavior in C99.  */
-int i = -1 << 0;
+int i = -1 << 0; /* { dg-error "left shift of negative value" } */
+/* { dg-error "constant" "constant" { target *-*-* } 33 } */
 
 int j[1] = { DBL_MAX }; /* { dg-warning "overflow in implicit constant conversion" } */
 /* { dg-error "overflow in constant expression" "constant" { target *-*-* } 36 } */

	Marek