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

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

[Marek Polacek]

Currently, we warn if the right operand of a shift expression is negative,
or greater than or equal to the length in bits of the promoted left operand.

But we don't warn when we see a left shift of a negative value.  That is
undefined behavior since C99 and I believe C++11, so this patch implements
a new warning, -Wshift-negative-value, only active in C99/C++11.

A bunch of tests needed tweaking; I find it scary that some vect tests are
invoking UB.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-04-22  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/torture/vector-shift2.c: Use -Wno-shift-negative-value.
	* gcc.dg/torture/vector-shift2.c: Likewise.
	* gcc.c-torture/execute/pr40386.c: Likewise.
	* gcc.dg/tree-ssa/vrp65.c: Likewise.
	* gcc.dg/tree-ssa/vrp66.c: Likewise.
	* gcc.dg/vect/vect-sdivmod-1.c: Likewise.
	* gcc.dg/vect/vect-shift-2-big-array.c: Likewise.
	* gcc.dg/vect/vect-shift-2.c: Likewise.
	* gcc.target/i386/pr31167.c: Likewise.
	* g++.dg/init/array11.C: Add dg-warning.
	* gcc.dg/c99-const-expr-7.c: Add dg-warning and dg-error.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 7fe7fa6..e944f11 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)
+	warning_at (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..47e0913 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) Init(1) Warning
+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 ebe4c73..17d2cac 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10701,6 +10701,15 @@ 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)
+	    {
+	      int_const = false;
+	      if (c_inhibit_evaluation_warnings == 0)
+		warning_at (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 250b5d6..d5d36bf 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4327,11 +4327,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)
+	    warning (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 a939ff7..2e14921 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 default.
+
 @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..49104e8 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" } */
+/* { 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-2.c gcc/testsuite/c-c++-common/Wshift-negative-value-2.c
index e69de29..0aea931 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 -Wno-shift-negative-value" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+
+enum E {
+  A = 0 << 1,
+  B = 1 << 1,
+  C = -1 << 1, /* { dg-bogus "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-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/torture/vector-shift2.c gcc/testsuite/c-c++-common/torture/vector-shift2.c
index d3a2ef8..6d280d8 100644
--- gcc/testsuite/c-c++-common/torture/vector-shift2.c
+++ gcc/testsuite/c-c++-common/torture/vector-shift2.c
@@ -1,4 +1,6 @@
 /* { dg-do run } */
+/* { dg-additional-options "-Wno-shift-negative-value" } */
+
 #define vector(elcount, type)  \
 __attribute__((vector_size((elcount)*sizeof(type)))) type
 
diff --git gcc/testsuite/g++.dg/init/array11.C gcc/testsuite/g++.dg/init/array11.C
index e52effe..096b145 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-warning "left shift of negative value" "" { target c++11 } }
     },
 };
 }
diff --git gcc/testsuite/gcc.c-torture/execute/pr40386.c gcc/testsuite/gcc.c-torture/execute/pr40386.c
index f39f1de..15966f6 100644
--- gcc/testsuite/gcc.c-torture/execute/pr40386.c
+++ gcc/testsuite/gcc.c-torture/execute/pr40386.c
@@ -1,4 +1,4 @@
-/* { dg-options "-fno-ira-share-spill-slots" } */
+/* { dg-options "-fno-ira-share-spill-slots -Wno-shift-negative-value" } */
 
 extern void abort (void);
 extern void exit (int);
diff --git gcc/testsuite/gcc.dg/c99-const-expr-7.c gcc/testsuite/gcc.dg/c99-const-expr-7.c
index b872077..ef87722 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-warning "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 } */
diff --git gcc/testsuite/gcc.dg/torture/vector-shift2.c gcc/testsuite/gcc.dg/torture/vector-shift2.c
index 0e8a0eb..2cb3bdc 100644
--- gcc/testsuite/gcc.dg/torture/vector-shift2.c
+++ gcc/testsuite/gcc.dg/torture/vector-shift2.c
@@ -1,4 +1,5 @@
 /* { dg-do run } */
+/* { dg-additional-options "-Wno-shift-negative-value" } */
 
 #define vector(elcount, type)  \
 __attribute__((vector_size((elcount)*sizeof(type)))) type
diff --git gcc/testsuite/gcc.dg/tree-ssa/vrp65.c gcc/testsuite/gcc.dg/tree-ssa/vrp65.c
index d109068..a118c71 100644
--- gcc/testsuite/gcc.dg/tree-ssa/vrp65.c
+++ gcc/testsuite/gcc.dg/tree-ssa/vrp65.c
@@ -1,6 +1,6 @@
 /* PR tree-optimization/52267 */
 /* { dg-do link } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -Wno-shift-negative-value" } */
 
 extern void link_error (void);
 
diff --git gcc/testsuite/gcc.dg/tree-ssa/vrp66.c gcc/testsuite/gcc.dg/tree-ssa/vrp66.c
index 6a6ab34..76cbeed 100644
--- gcc/testsuite/gcc.dg/tree-ssa/vrp66.c
+++ gcc/testsuite/gcc.dg/tree-ssa/vrp66.c
@@ -1,6 +1,6 @@
 /* PR tree-optimization/52267 */
 /* { dg-do run { target { ! int16 } } } */
-/* { dg-options "-O2" } */
+/* { dg-options "-O2 -Wno-shift-negative-value" } */
 
 extern void abort (void);
 
diff --git gcc/testsuite/gcc.dg/vect/vect-sdivmod-1.c gcc/testsuite/gcc.dg/vect/vect-sdivmod-1.c
index c18204e..2f5e86d 100644
--- gcc/testsuite/gcc.dg/vect/vect-sdivmod-1.c
+++ gcc/testsuite/gcc.dg/vect/vect-sdivmod-1.c
@@ -1,3 +1,5 @@
+/* { dg-additional-options "-Wno-shift-negative-value" } */
+
 #include "tree-vect.h"
 
 extern void abort (void);
diff --git gcc/testsuite/gcc.dg/vect/vect-shift-2-big-array.c gcc/testsuite/gcc.dg/vect/vect-shift-2-big-array.c
index 0e1acfb..a36dc5e 100644
--- gcc/testsuite/gcc.dg/vect/vect-shift-2-big-array.c
+++ gcc/testsuite/gcc.dg/vect/vect-shift-2-big-array.c
@@ -1,3 +1,4 @@
+/* { dg-additional-options "-Wno-shift-negative-value" } */
 /* { dg-require-effective-target vect_shift } */
 /* { dg-require-effective-target vect_int } */
 /* Check the standard integer types for left and right shifts to see if the
diff --git gcc/testsuite/gcc.dg/vect/vect-shift-2.c gcc/testsuite/gcc.dg/vect/vect-shift-2.c
index 83211eb..4388b78 100644
--- gcc/testsuite/gcc.dg/vect/vect-shift-2.c
+++ gcc/testsuite/gcc.dg/vect/vect-shift-2.c
@@ -1,3 +1,4 @@
+/* { dg-additional-options "-Wno-shift-negative-value" } */
 /* { dg-require-effective-target vect_shift } */
 /* { dg-require-effective-target vect_int } */
 /* Check the standard integer types for left and right shifts to see if the
diff --git gcc/testsuite/gcc.target/i386/pr31167.c gcc/testsuite/gcc.target/i386/pr31167.c
index 43d9f84..5470d36 100644
--- gcc/testsuite/gcc.target/i386/pr31167.c
+++ gcc/testsuite/gcc.target/i386/pr31167.c
@@ -1,6 +1,6 @@
 /* { dg-do compile } */
 /* { dg-require-effective-target int128 } */
-/* { dg-options "-O" } */
+/* { dg-options "-O -Wno-shift-negative-value" } */
 
 typedef int int32_t;
 

	Marek

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

[Jeff Law]

On 04/22/2015 12:36 PM, Marek Polacek wrote:
> Currently, we warn if the right operand of a shift expression is negative,
> or greater than or equal to the length in bits of the promoted left operand.
>
> But we don't warn when we see a left shift of a negative value.  That is
> undefined behavior since C99 and I believe C++11, so this patch implements
> a new warning, -Wshift-negative-value, only active in C99/C++11.
>
> A bunch of tests needed tweaking; I find it scary that some vect tests are
> invoking UB.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-04-22  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/torture/vector-shift2.c: Use -Wno-shift-negative-value.
> 	* gcc.dg/torture/vector-shift2.c: Likewise.
> 	* gcc.c-torture/execute/pr40386.c: Likewise.
> 	* gcc.dg/tree-ssa/vrp65.c: Likewise.
> 	* gcc.dg/tree-ssa/vrp66.c: Likewise.
> 	* gcc.dg/vect/vect-sdivmod-1.c: Likewise.
> 	* gcc.dg/vect/vect-shift-2-big-array.c: Likewise.
> 	* gcc.dg/vect/vect-shift-2.c: Likewise.
> 	* gcc.target/i386/pr31167.c: Likewise.
> 	* g++.dg/init/array11.C: Add dg-warning.
> 	* gcc.dg/c99-const-expr-7.c: Add dg-warning and dg-error.
OK.
jeff

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

[Martin Sebor]

On 04/22/2015 12:36 PM, Marek Polacek wrote:
> Currently, we warn if the right operand of a shift expression is negative,
> or greater than or equal to the length in bits of the promoted left operand.
>
> But we don't warn when we see a left shift of a negative value.  That is
> undefined behavior since C99 and I believe C++11, so this patch implements
> a new warning, -Wshift-negative-value, only active in C99/C++11.
>
> A bunch of tests needed tweaking; I find it scary that some vect tests are
> invoking UB.

I wonder if the tests where the left shift operands are both
constants really do invoke undefined behavior in GCC. For
example, AFAICS, in (-1 << 0) and other constant expressions
gcc computes the shift in unsigned HOST_WIDE_INT which is well
defined.

It seems the warning would be more valuable (and less likely
dismissed as overly pedantic) if it was issued when the second
operand was not constant and the computation had to be done in
hardware (or even better, in hardware not known to  use the
same instructions for positive and negative operands).

The warning would also be valuable in some sort of a portability
mode (with -pedantic?) where the code is intended to be portable
to implementations that don't provide well-defined semantics for
left shifts of negative values.

Martin

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

[Marek Polacek]

On Thu, Apr 23, 2015 at 09:11:39PM -0600, Martin Sebor wrote:
> I wonder if the tests where the left shift operands are both
> constants really do invoke undefined behavior in GCC. For
> example, AFAICS, in (-1 << 0) and other constant expressions
> gcc computes the shift in unsigned HOST_WIDE_INT which is well
> defined.
 
Yeah, two INTEGER_CSTs are computed in wide-int using uhwi.  But -1 << 0
certainly is UB according to ISO C/C++ and I think we should follow the
standards.

> It seems the warning would be more valuable (and less likely
> dismissed as overly pedantic) if it was issued when the second
> operand was not constant and the computation had to be done in
> hardware (or even better, in hardware not known to  use the
> same instructions for positive and negative operands).
 
What I've tried to do in the patch was to mimic the other two Wshift-*
warnings.  While the new warning triggered a few times in the testsuite, GCC
bootstrap itself was clean.  You raise a very good point though, we don't
want to be overly pedantic.

I suppose we could go with the patch as-is, and if users complain too much,
warn only if the second operand is not constant or so.  But I hope that
left-shifting a negative value is not a common thing.

> The warning would also be valuable in some sort of a portability
> mode (with -pedantic?) where the code is intended to be portable
> to implementations that don't provide well-defined semantics for
> left shifts of negative values.

I really think -Wshift-negative-value should be kin to -Wshift-count-negative
and -Wshift-count-overflow, those are enabled by default.

Thanks,

	Marek

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

[Martin Sebor]

On 04/24/2015 10:27 AM, Marek Polacek wrote:
> On Thu, Apr 23, 2015 at 09:11:39PM -0600, Martin Sebor wrote:
>> I wonder if the tests where the left shift operands are both
>> constants really do invoke undefined behavior in GCC. For
>> example, AFAICS, in (-1 << 0) and other constant expressions
>> gcc computes the shift in unsigned HOST_WIDE_INT which is well
>> defined.
>
> Yeah, two INTEGER_CSTs are computed in wide-int using uhwi.  But -1 << 0
> certainly is UB according to ISO C/C++ and I think we should follow the
> standards.
>
>> It seems the warning would be more valuable (and less likely
>> dismissed as overly pedantic) if it was issued when the second
>> operand was not constant and the computation had to be done in
>> hardware (or even better, in hardware not known to  use the
>> same instructions for positive and negative operands).
>
> What I've tried to do in the patch was to mimic the other two Wshift-*
> warnings.  While the new warning triggered a few times in the testsuite, GCC
> bootstrap itself was clean.  You raise a very good point though, we don't
> want to be overly pedantic.
>
> I suppose we could go with the patch as-is, and if users complain too much,
> warn only if the second operand is not constant or so.  But I hope that
> left-shifting a negative value is not a common thing.
>
>> The warning would also be valuable in some sort of a portability
>> mode (with -pedantic?) where the code is intended to be portable
>> to implementations that don't provide well-defined semantics for
>> left shifts of negative values.
>
> I really think -Wshift-negative-value should be kin to -Wshift-count-negative
> and -Wshift-count-overflow, those are enabled by default.

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

I checked Clang to see what it does. It generates the
same code as GCC but only issues a warning for negative
left shifts. With -Wpedantic, it does give a similar
warning for left shifts of negative values. I recommend
GCC to do the same.

Martin

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

[Martin Sebor]

On 04/24/2015 03:31 PM, Martin Sebor wrote:
> On 04/24/2015 10:27 AM, Marek Polacek wrote:
>> On Thu, Apr 23, 2015 at 09:11:39PM -0600, Martin Sebor wrote:
>>> I wonder if the tests where the left shift operands are both
>>> constants really do invoke undefined behavior in GCC. For
>>> example, AFAICS, in (-1 << 0) and other constant expressions
>>> gcc computes the shift in unsigned HOST_WIDE_INT which is well
>>> defined.
>>
>> Yeah, two INTEGER_CSTs are computed in wide-int using uhwi.  But -1 << 0
>> certainly is UB according to ISO C/C++ and I think we should follow the
>> standards.
>>
>>> It seems the warning would be more valuable (and less likely
>>> dismissed as overly pedantic) if it was issued when the second
>>> operand was not constant and the computation had to be done in
>>> hardware (or even better, in hardware not known to  use the
>>> same instructions for positive and negative operands).
>>
>> What I've tried to do in the patch was to mimic the other two Wshift-*
>> warnings.  While the new warning triggered a few times in the
>> testsuite, GCC
>> bootstrap itself was clean.  You raise a very good point though, we don't
>> want to be overly pedantic.
>>
>> I suppose we could go with the patch as-is, and if users complain too
>> much,
>> warn only if the second operand is not constant or so.  But I hope that
>> left-shifting a negative value is not a common thing.
>>
>>> The warning would also be valuable in some sort of a portability
>>> mode (with -pedantic?) where the code is intended to be portable
>>> to implementations that don't provide well-defined semantics for
>>> left shifts of negative values.
>>
>> I really think -Wshift-negative-value should be kin to
>> -Wshift-count-negative
>> and -Wshift-count-overflow, those are enabled by default.
>
> 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).
>
> I checked Clang to see what it does. It generates the
> same code as GCC but only issues a warning for negative
> left shifts. With -Wpedantic, it does give a similar
> warning for left shifts of negative values.

Actually, I had misread the diagnostic. Clang only warns
for invalid shift counts and doesn't warn for negative
operands even with -Wpedantic.

> I recommend
> GCC to do the same.
>
> Martin

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

[Marek Polacek]

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

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

[Joseph Myers]

On Sat, 25 Apr 2015, Marek Polacek wrote:

> +		pedwarn (location, OPT_Wshift_negative_value,
> +			 "left shift of negative value");

Use of pedwarn is always suspect for something only undefined at runtime; 
it must not produce an error with -pedantic-errors in any context where a 
constant expression is not required.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Marek Polacek]

On Sat, Apr 25, 2015 at 08:13:08PM +0000, Joseph Myers wrote:
> On Sat, 25 Apr 2015, Marek Polacek wrote:
> 
> > +		pedwarn (location, OPT_Wshift_negative_value,
> > +			 "left shift of negative value");
> 
> Use of pedwarn is always suspect for something only undefined at runtime; 
> it must not produce an error with -pedantic-errors in any context where a 
> constant expression is not required.

Makes sense.  So how about moving the warning into -Wextra?  This way it won't
trigger by default.  One change is that we reject programs that use shift with
undefined behavior in a context where a constant expression is required, thus
e.g. enum E { A = -1 << 0 };
But I hope that's reasonable.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-04-27  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.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 9797e17..f2fe65e 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)
+	warning_at (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..c61dfb1 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 EnabledBy(Wextra)
+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..36cebc6 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10707,6 +10707,15 @@ 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)
+	    {
+	      int_const = false;
+	      if (c_inhibit_evaluation_warnings == 0)
+		warning_at (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..3cb5a8a 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)
+	    warning (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..dcfe4cf 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
@@ -3481,6 +3481,7 @@ name is still supported, but the newer name is more descriptive.)
 -Wsign-compare  @gol
 -Wtype-limits  @gol
 -Wuninitialized  @gol
+-Wshift-negative-value  @gol
 -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)} @gol
 -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)}  @gol
 }
@@ -3914,6 +3915,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 also enabled by @option{-Wextra}.
+
 @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..5d803ad 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 -Wextra" } */
+/* { 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..bf9b1a0 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 -Wextra -Wno-shift-negative-value" } */
+/* { 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/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/gcc.dg/c99-const-expr-7.c gcc/testsuite/gcc.dg/c99-const-expr-7.c
index b872077..75b0567 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;
+/* { 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

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

[Joseph Myers]

On Mon, 27 Apr 2015, Marek Polacek wrote:

> trigger by default.  One change is that we reject programs that use shift with
> undefined behavior in a context where a constant expression is required, thus
> e.g. enum E { A = -1 << 0 };
> But I hope that's reasonable.

That seems appropriate (for C99 and above).

But if someone explicitly uses -Wshift-negative-value, I'd expect that to 
produce the warnings (as opposed to the rejections where a constant 
expression is required) even in C90 mode.  That is, for the warnings, I 
think flag_isoc99 should maybe affect the default (whether -Wextra enables 
the warning, or whatever such approach gets taken), but not whether 
-Wshift-negative-value, given that the option has been enabled, produces 
warnings.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Marek Polacek]

On Wed, Apr 29, 2015 at 10:54:58PM +0000, Joseph Myers wrote:
> On Mon, 27 Apr 2015, Marek Polacek wrote:
> 
> > trigger by default.  One change is that we reject programs that use shift with
> > undefined behavior in a context where a constant expression is required, thus
> > e.g. enum E { A = -1 << 0 };
> > But I hope that's reasonable.
> 
> That seems appropriate (for C99 and above).
> 
> But if someone explicitly uses -Wshift-negative-value, I'd expect that to 
> produce the warnings (as opposed to the rejections where a constant 
> expression is required) even in C90 mode.  That is, for the warnings, I 
> think flag_isoc99 should maybe affect the default (whether -Wextra enables 
> the warning, or whatever such approach gets taken), but not whether 
> -Wshift-negative-value, given that the option has been enabled, produces 
> warnings.

Ah, indeed.  The following patch hopefully addresses those defects.  The
tests show when the warning triggers and when not as well as when we reject
invalid shifts and when not.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-05-06  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-opts.c (c_common_post_options): Set warn_shift_negative_value
	when -Wextra and C99/C++11 mode.

	* 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.
	* testsuite/c-c++-common/Wshift-negative-value-2.c: New test.
	* testsuite/c-c++-common/Wshift-negative-value-3.c: New test.
	* testsuite/c-c++-common/Wshift-negative-value-4.c: New test.
	* testsuite/c-c++-common/Wshift-negative-value-5.c: New test.
	* testsuite/c-c++-common/Wshift-negative-value-6.c: New test.
	* testsuite/gcc.dg/c90-left-shift-1.c: New test.
	* testsuite/gcc.dg/c99-const-expr-7.c: Add dg-error.
	* testsuite/gcc.dg/c99-left-shift-1.c: New test.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index ada8e8a..378f237 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1361,6 +1361,14 @@ 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)
+	warning_at (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-opts.c gcc/c-family/c-opts.c
index 1a67b5a..a61d6a8 100644
--- gcc/c-family/c-opts.c
+++ gcc/c-family/c-opts.c
@@ -866,6 +866,11 @@ c_common_post_options (const char **pfilename)
   if (warn_implicit_int == -1)
     warn_implicit_int = flag_isoc99;
 
+  /* -Wshift-negative-value is enabled by -Wextra in C99 and C++11 modes.  */
+  if (warn_shift_negative_value == -1)
+    warn_shift_negative_value = (extra_warnings
+				 && (cxx_dialect >= cxx11 || flag_isoc99));
+
   /* Declone C++ 'structors if -Os.  */
   if (flag_declone_ctor_dtor == -1)
     flag_declone_ctor_dtor = optimize_size;
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 8ef0cea..48947b4 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) Init(-1) Warning
+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 328f294..73275aa 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10697,6 +10697,17 @@ 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)
+	    {
+	      /* Don't reject a left shift of a negative value in a context
+		 where a constant expression is needed in C90.  */
+	      if (flag_isoc99)
+		int_const = false;
+	      if (c_inhibit_evaluation_warnings == 0)
+		warning_at (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..549e4b1 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -4326,11 +4326,20 @@ 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)
+	    warning (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 9c8aa99..30f8ac7 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
@@ -3489,6 +3489,7 @@ name is still supported, but the newer name is more descriptive.)
 -Wsign-compare  @gol
 -Wtype-limits  @gol
 -Wuninitialized  @gol
+-Wshift-negative-value  @gol
 -Wunused-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)} @gol
 -Wunused-but-set-parameter @r{(only with} @option{-Wunused} @r{or} @option{-Wall}@r{)}  @gol
 }
@@ -3922,6 +3923,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 is enabled by
+@option{-Wextra} in C99 and C++11 modes (and newer).
+
 @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..5d803ad 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 -Wextra" } */
+/* { 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..bf9b1a0 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 -Wextra -Wno-shift-negative-value" } */
+/* { 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/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/c-c++-common/Wshift-negative-value-5.c gcc/testsuite/c-c++-common/Wshift-negative-value-5.c
index e69de29..74ecd1e 100644
--- gcc/testsuite/c-c++-common/Wshift-negative-value-5.c
+++ gcc/testsuite/c-c++-common/Wshift-negative-value-5.c
@@ -0,0 +1,50 @@
+/* PR c/65179 */
+/* { dg-do compile } */
+/* { dg-options "-O -Wshift-negative-value" } */
+/* { dg-additional-options "-std=c++03" { target c++ } } */
+/* { dg-additional-options "-std=c90" { 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-6.c gcc/testsuite/c-c++-common/Wshift-negative-value-6.c
index e69de29..de9db52 100644
--- gcc/testsuite/c-c++-common/Wshift-negative-value-6.c
+++ gcc/testsuite/c-c++-common/Wshift-negative-value-6.c
@@ -0,0 +1,50 @@
+/* PR c/65179 */
+/* { dg-do compile } */
+/* { dg-options "-O -Wextra" } */
+/* { dg-additional-options "-std=c++03" { target c++ } } */
+/* { dg-additional-options "-std=c90" { target c } } */
+
+enum E {
+  A = 0 << 1,
+  B = 1 << 1,
+  C = -1 << 1, /* { dg-bogus "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-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/gcc.dg/c90-left-shift-1.c gcc/testsuite/gcc.dg/c90-left-shift-1.c
index e69de29..755595f 100644
--- gcc/testsuite/gcc.dg/c90-left-shift-1.c
+++ gcc/testsuite/gcc.dg/c90-left-shift-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-std=iso9899:1990 -pedantic-errors" } */
+
+enum E { A = -2 << 1 };
+int i = -1 << 0;
+
+int
+f (int i)
+{
+  switch (i)
+  case -1 << 0: break;
+}
diff --git gcc/testsuite/gcc.dg/c99-const-expr-7.c gcc/testsuite/gcc.dg/c99-const-expr-7.c
index b872077..75b0567 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;
+/* { 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 } */
diff --git gcc/testsuite/gcc.dg/c99-left-shift-1.c gcc/testsuite/gcc.dg/c99-left-shift-1.c
index e69de29..9a73049 100644
--- gcc/testsuite/gcc.dg/c99-left-shift-1.c
+++ gcc/testsuite/gcc.dg/c99-left-shift-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-std=iso9899:1999 -pedantic-errors" } */
+
+enum E { A = -2 << 1 }; /* { dg-error "constant expression" } */
+int i = -1 << 0; /* { dg-error "constant expression" } */
+
+int
+f (int i)
+{
+  switch (i)
+  case -1 << 0: break; /* { dg-error "constant expression" } */
+}

	Marek

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

[Jeff Law]

On 05/06/2015 05:37 AM, Marek Polacek wrote:
> On Wed, Apr 29, 2015 at 10:54:58PM +0000, Joseph Myers wrote:
>> On Mon, 27 Apr 2015, Marek Polacek wrote:
>>
>>> trigger by default.  One change is that we reject programs that use shift with
>>> undefined behavior in a context where a constant expression is required, thus
>>> e.g. enum E { A = -1 << 0 };
>>> But I hope that's reasonable.
>>
>> That seems appropriate (for C99 and above).
>>
>> But if someone explicitly uses -Wshift-negative-value, I'd expect that to
>> produce the warnings (as opposed to the rejections where a constant
>> expression is required) even in C90 mode.  That is, for the warnings, I
>> think flag_isoc99 should maybe affect the default (whether -Wextra enables
>> the warning, or whatever such approach gets taken), but not whether
>> -Wshift-negative-value, given that the option has been enabled, produces
>> warnings.
>
> Ah, indeed.  The following patch hopefully addresses those defects.  The
> tests show when the warning triggers and when not as well as when we reject
> invalid shifts and when not.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-05-06  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-opts.c (c_common_post_options): Set warn_shift_negative_value
> 	when -Wextra and C99/C++11 mode.
>
> 	* 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.
> 	* testsuite/c-c++-common/Wshift-negative-value-2.c: New test.
> 	* testsuite/c-c++-common/Wshift-negative-value-3.c: New test.
> 	* testsuite/c-c++-common/Wshift-negative-value-4.c: New test.
> 	* testsuite/c-c++-common/Wshift-negative-value-5.c: New test.
> 	* testsuite/c-c++-common/Wshift-negative-value-6.c: New test.
> 	* testsuite/gcc.dg/c90-left-shift-1.c: New test.
> 	* testsuite/gcc.dg/c99-const-expr-7.c: Add dg-error.
> 	* testsuite/gcc.dg/c99-left-shift-1.c: New test.
OK.  Please install if you haven't already.

jeff

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

[Marek Polacek]

On Thu, May 07, 2015 at 12:00:20PM -0600, Jeff Law wrote:
> OK.  Please install if you haven't already.

I have not, so will do momentarily.  Thanks,

	Marek

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

[Steve Ellcey]

On Thu, 2015-05-07 at 21:15 +0200, Marek Polacek wrote:
> On Thu, May 07, 2015 at 12:00:20PM -0600, Jeff Law wrote:
> > OK.  Please install if you haven't already.
> 
> I have not, so will do momentarily.  Thanks,
> 
> 	Marek

Marek,

This patch has broken the glibc build.  I am not sure if the problem is
a bug in your patch or a bug in the code used by glibc.  Here is a
cutdown test case from glibc (timezone/scheck.c).  This code compiled
before your patch but now it fails with:

x.c:4:3: error: initializer element is not constant
   ((((time_t) -1) < 0)



__extension__ typedef long int __time_t;
typedef __time_t time_t;
static time_t const time_t_min =
  ((((time_t) -1) < 0)
   ? (time_t) -1 << (8 * sizeof (time_t) - 1)
   : 0)



Steve Ellcey
sellcey@imgtec.com

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

[Markus Trippelsdorf]

On 2015.05.08 at 09:38 -0700, Steve Ellcey wrote:
> 
> This patch has broken the glibc build.  I am not sure if the problem is
> a bug in your patch or a bug in the code used by glibc.  Here is a
> cutdown test case from glibc (timezone/scheck.c).  This code compiled
> before your patch but now it fails with:
> 
> x.c:4:3: error: initializer element is not constant
>    ((((time_t) -1) < 0)
> 
> 
> 
> __extension__ typedef long int __time_t;
> typedef __time_t time_t;
> static time_t const time_t_min =
>   ((((time_t) -1) < 0)
>    ? (time_t) -1 << (8 * sizeof (time_t) - 1)
>    : 0)
> 

See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66066

-- 
Markus

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

[Joseph Myers]

On Fri, 8 May 2015, Steve Ellcey wrote:

> On Thu, 2015-05-07 at 21:15 +0200, Marek Polacek wrote:
> > On Thu, May 07, 2015 at 12:00:20PM -0600, Jeff Law wrote:
> > > OK.  Please install if you haven't already.
> > 
> > I have not, so will do momentarily.  Thanks,
> > 
> > 	Marek
> 
> Marek,
> 
> This patch has broken the glibc build.  I am not sure if the problem is
> a bug in your patch or a bug in the code used by glibc.  Here is a
> cutdown test case from glibc (timezone/scheck.c).  This code compiled
> before your patch but now it fails with:
> 
> x.c:4:3: error: initializer element is not constant
>    ((((time_t) -1) < 0)
> 
> 
> 
> __extension__ typedef long int __time_t;
> typedef __time_t time_t;
> static time_t const time_t_min =
>   ((((time_t) -1) < 0)
>    ? (time_t) -1 << (8 * sizeof (time_t) - 1)
>    : 0)

Paul, although glibc's copy of parts of tzcode is a bit out of date, it 
looks like the current https://github.com/eggert/tz.git still has the 
problematic code in private.h, relying on left-shifting -1 which has 
undefined behavior in C99/C11 (implementation-defined in C90, as per 
DR#081).

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On 05/08/2015 09:59 AM, Joseph Myers wrote:
> Paul, although glibc's copy of parts of tzcode is a bit out of date, it
> looks like the currenthttps://github.com/eggert/tz.git  still has the
> problematic code in private.h, relying on left-shifting -1 which has
> undefined behavior in C99/C11 (implementation-defined in C90, as per
> DR#081).
Thanks for reporting that.  I installed the attached patch into the 
experimental tz version on github <https://github.com/eggert/tz>, with 
the intent that this fix propagate into the next tz release and thus 
into glibc etc.


[-- Attachment #2: 0001-Avoid-left-shift-into-sign-bit-undefined-behavior.patch --]
[-- Type: text/x-patch, Size: 5177 bytes --]

From b8f4f998104e74fc2c4a3759317b5153e95db16e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 8 May 2015 14:47:40 -0700
Subject: [PATCH] Avoid left-shift-into-sign-bit undefined behavior

Problem reported by Joseph Myers in:
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg00704.html
* localtime.c (detzcode, detzcode64): Don't rely on
undefined behavior with left shift into sign bit.
Port better to non-2's-complement machines.
* private.h (TWOS_COMPLEMENT, MAXVAL, MINVAL): New macros.
* private.h (time_t_min, time_t_max):
* zic.c (min_time, max_time): Use them to avoid undefined behavior.
* zdump.c (atime_shift): New constant.
(absolute_min_time, absolute_max_time):
Use it to avoid undefined behavior.
---
 localtime.c | 32 +++++++++++++++++++++++++++-----
 private.h   | 23 ++++++++++++++---------
 zdump.c     |  6 ++++--
 zic.c       |  4 ++--
 4 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/localtime.c b/localtime.c
index a2d9aa8..423e13e 100644
--- a/localtime.c
+++ b/localtime.c
@@ -216,22 +216,44 @@ detzcode(const char *const codep)
 {
 	register int_fast32_t	result;
 	register int		i;
+	int_fast32_t one = 1;
+	int_fast32_t halfmaxval = one << (32 - 2);
+	int_fast32_t maxval = halfmaxval - 1 + halfmaxval;
+	int_fast32_t minval = -1 - maxval;
 
-	result = (codep[0] & 0x80) ? -1 : 0;
-	for (i = 0; i < 4; ++i)
+	result = codep[0] & 0x7f;
+	for (i = 1; i < 4; ++i)
 		result = (result << 8) | (codep[i] & 0xff);
+
+	if (codep[0] & 0x80) {
+	  /* Do two's-complement negation even on non-two's-complement machines.
+	     If the result would be minval - 1, return minval.  */
+	  result -= !TWOS_COMPLEMENT(int_fast32_t) && result != 0;
+	  result += minval;
+	}
 	return result;
 }
 
 static int_fast64_t
 detzcode64(const char *const codep)
 {
-	register int_fast64_t result;
+	register uint_fast64_t result;
 	register int	i;
+	int_fast64_t one = 1;
+	int_fast64_t halfmaxval = one << (64 - 2);
+	int_fast64_t maxval = halfmaxval - 1 + halfmaxval;
+	int_fast64_t minval = -TWOS_COMPLEMENT(int_fast64_t) - maxval;
 
-	result = (codep[0] & 0x80) ? -1 : 0;
-	for (i = 0; i < 8; ++i)
+	result = codep[0] & 0x7f;
+	for (i = 1; i < 8; ++i)
 		result = (result << 8) | (codep[i] & 0xff);
+
+	if (codep[0] & 0x80) {
+	  /* Do two's-complement negation even on non-two's-complement machines.
+	     If the result would be minval - 1, return minval.  */
+	  result -= !TWOS_COMPLEMENT(int_fast64_t) && result != 0;
+	  result += minval;
+	}
 	return result;
 }
 
diff --git a/private.h b/private.h
index 61cf922..f277e7a 100644
--- a/private.h
+++ b/private.h
@@ -472,15 +472,20 @@ time_t time2posix_z(timezone_t, time_t) ATTRIBUTE_PURE;
 #define TYPE_SIGNED(type) (((type) -1) < 0)
 #endif /* !defined TYPE_SIGNED */
 
-/* The minimum and maximum finite time values.  */
-static time_t const time_t_min =
-  (TYPE_SIGNED(time_t)
-   ? (time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1)
-   : 0);
-static time_t const time_t_max =
-  (TYPE_SIGNED(time_t)
-   ? - (~ 0 < 0) - ((time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1))
-   : -1);
+#define TWOS_COMPLEMENT(t) ((t) ~ (t) 0 < 0)
+
+/* Max and min values of the integer type T, of which only the bottom
+   B bits are used, and where the highest-order used bit is considered
+   to be a sign bit if T is signed.  */
+#define MAXVAL(t, b)						\
+  ((t) (((t) 1 << ((b) - 1 - TYPE_SIGNED(t)))			\
+	- 1 + ((t) 1 << ((b) - 1 - TYPE_SIGNED(t)))))
+#define MINVAL(t, b)						\
+  ((t) (TYPE_SIGNED(t) ? - TWOS_COMPLEMENT(t) - MAXVAL(t, b) : 0))
+
+/* The minimum and maximum finite time values.  This assumes no padding.  */
+static time_t const time_t_min = MINVAL(time_t, TYPE_BIT(time_t));
+static time_t const time_t_max = MAXVAL(time_t, TYPE_BIT(time_t));
 
 #ifndef INT_STRLEN_MAXIMUM
 /*
diff --git a/zdump.c b/zdump.c
index 13bbb0e..adb806c 100644
--- a/zdump.c
+++ b/zdump.c
@@ -246,13 +246,15 @@ extern int	optind;
 extern char *	tzname[2];
 
 /* The minimum and maximum finite time values.  */
+enum { atime_shift = CHAR_BIT * sizeof (time_t) - 2 };
 static time_t const absolute_min_time =
   ((time_t) -1 < 0
-   ? (time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1)
+   ? (- ((time_t) ~ (time_t) 0 < 0)
+      - (((time_t) 1 << atime_shift) - 1 + ((time_t) 1 << atime_shift)))
    : 0);
 static time_t const absolute_max_time =
   ((time_t) -1 < 0
-   ? - (~ 0 < 0) - ((time_t) -1 << (CHAR_BIT * sizeof (time_t) - 1))
+   ? (((time_t) 1 << atime_shift) - 1 + ((time_t) 1 << atime_shift))
    : -1);
 static int	longest;
 static char *	progname;
diff --git a/zic.c b/zic.c
index 636649b..52b45ad 100644
--- a/zic.c
+++ b/zic.c
@@ -813,8 +813,8 @@ warning(_("hard link failed, symbolic link used"));
 
 #define TIME_T_BITS_IN_FILE	64
 
-static const zic_t min_time = (zic_t) -1 << (TIME_T_BITS_IN_FILE - 1);
-static const zic_t max_time = -1 - ((zic_t) -1 << (TIME_T_BITS_IN_FILE - 1));
+static zic_t const min_time = MINVAL (zic_t, TIME_T_BITS_IN_FILE);
+static zic_t const max_time = MAXVAL (zic_t, TIME_T_BITS_IN_FILE);
 
 /* Estimated time of the Big Bang, in seconds since the POSIX epoch.
    rounded downward to the negation of a power of two that is
-- 
2.1.0

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

[Steve Ellcey]

On Fri, 2015-05-08 at 14:54 -0700, Paul Eggert wrote:
> On 05/08/2015 09:59 AM, Joseph Myers wrote:
> > Paul, although glibc's copy of parts of tzcode is a bit out of date, it
> > looks like the currenthttps://github.com/eggert/tz.git  still has the
> > problematic code in private.h, relying on left-shifting -1 which has
> > undefined behavior in C99/C11 (implementation-defined in C90, as per
> > DR#081).
> Thanks for reporting that.  I installed the attached patch into the 
> experimental tz version on github <https://github.com/eggert/tz>, with 
> the intent that this fix propagate into the next tz release and thus 
> into glibc etc.

FYI: I put this into my glibc sources (the private.h, zdump.c, and zic.c
parts, glibc doesn't have localtime.c in the timezone directory and the
one in the time directory doesn't look like it matches what was
patched).  And my GCC/glibc toolchain now builds.  I didn't run the
glibc testsuite but I ran the GCC testsuite using this patch and
everything looked fine.

Steve Ellcey
sellcey@imgtec.com

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

[Christos Zoulas]

On May 11,  3:18pm, sellcey@imgtec.com (Steve Ellcey) wrote:
-- Subject: Re: [tz] [C/C++ PATCH] Implement -Wshift-negative-value (PR c/651

| FYI: I put this into my glibc sources (the private.h, zdump.c, and zic.c
| parts, glibc doesn't have localtime.c in the timezone directory and the
| one in the time directory doesn't look like it matches what was
| patched).  And my GCC/glibc toolchain now builds.  I didn't run the
| glibc testsuite but I ran the GCC testsuite using this patch and
| everything looked fine.

If you want it to work exactly like glibc as a drop in replacement,
you need to copy the NO_ERROR_IN_DST_GAP code from:

    http://nxr.netbsd.org/xref/src/lib/libc/time/localtime.c

christos