* [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) @ 2015-04-22 18:36 Marek Polacek 2015-04-23 22:04 ` Jeff Law 2015-04-24 3:11 ` Martin Sebor 0 siblings, 2 replies; 19+ messages in thread From: Marek Polacek @ 2015-04-22 18:36 UTC (permalink / raw) To: GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-22 18:36 [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) Marek Polacek @ 2015-04-23 22:04 ` Jeff Law 2015-04-24 3:11 ` Martin Sebor 1 sibling, 0 replies; 19+ messages in thread From: Jeff Law @ 2015-04-23 22:04 UTC (permalink / raw) To: Marek Polacek, GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-22 18:36 [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) Marek Polacek 2015-04-23 22:04 ` Jeff Law @ 2015-04-24 3:11 ` Martin Sebor 2015-04-24 16:27 ` Marek Polacek 1 sibling, 1 reply; 19+ messages in thread From: Martin Sebor @ 2015-04-24 3:11 UTC (permalink / raw) To: Marek Polacek, GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-24 3:11 ` Martin Sebor @ 2015-04-24 16:27 ` Marek Polacek 2015-04-24 21:32 ` Martin Sebor 0 siblings, 1 reply; 19+ messages in thread From: Marek Polacek @ 2015-04-24 16:27 UTC (permalink / raw) To: Martin Sebor; +Cc: GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-24 16:27 ` Marek Polacek @ 2015-04-24 21:32 ` Martin Sebor 2015-04-24 21:44 ` Martin Sebor 2015-04-25 17:31 ` Marek Polacek 0 siblings, 2 replies; 19+ messages in thread From: Martin Sebor @ 2015-04-24 21:32 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-24 21:32 ` Martin Sebor @ 2015-04-24 21:44 ` Martin Sebor 2015-04-25 17:31 ` Marek Polacek 1 sibling, 0 replies; 19+ messages in thread From: Martin Sebor @ 2015-04-24 21:44 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-24 21:32 ` Martin Sebor 2015-04-24 21:44 ` Martin Sebor @ 2015-04-25 17:31 ` Marek Polacek 2015-04-25 20:13 ` Joseph Myers 1 sibling, 1 reply; 19+ messages in thread From: Marek Polacek @ 2015-04-25 17:31 UTC (permalink / raw) To: Martin Sebor; +Cc: GCC Patches, Jason Merrill, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-25 17:31 ` Marek Polacek @ 2015-04-25 20:13 ` Joseph Myers 2015-04-27 15:46 ` Marek Polacek 0 siblings, 1 reply; 19+ messages in thread From: Joseph Myers @ 2015-04-25 20:13 UTC (permalink / raw) To: Marek Polacek; +Cc: Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-25 20:13 ` Joseph Myers @ 2015-04-27 15:46 ` Marek Polacek 2015-04-29 23:57 ` Joseph Myers 0 siblings, 1 reply; 19+ messages in thread From: Marek Polacek @ 2015-04-27 15:46 UTC (permalink / raw) To: Joseph Myers; +Cc: Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-27 15:46 ` Marek Polacek @ 2015-04-29 23:57 ` Joseph Myers 2015-05-06 11:38 ` Marek Polacek 0 siblings, 1 reply; 19+ messages in thread From: Joseph Myers @ 2015-04-29 23:57 UTC (permalink / raw) To: Marek Polacek; +Cc: Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-04-29 23:57 ` Joseph Myers @ 2015-05-06 11:38 ` Marek Polacek 2015-05-07 18:00 ` Jeff Law 0 siblings, 1 reply; 19+ messages in thread From: Marek Polacek @ 2015-05-06 11:38 UTC (permalink / raw) To: Joseph Myers; +Cc: Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-06 11:38 ` Marek Polacek @ 2015-05-07 18:00 ` Jeff Law 2015-05-07 19:15 ` Marek Polacek 0 siblings, 1 reply; 19+ messages in thread From: Jeff Law @ 2015-05-07 18:00 UTC (permalink / raw) To: Marek Polacek, Joseph Myers; +Cc: Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-07 18:00 ` Jeff Law @ 2015-05-07 19:15 ` Marek Polacek 2015-05-08 16:38 ` Steve Ellcey 0 siblings, 1 reply; 19+ messages in thread From: Marek Polacek @ 2015-05-07 19:15 UTC (permalink / raw) To: Jeff Law; +Cc: Joseph Myers, Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-07 19:15 ` Marek Polacek @ 2015-05-08 16:38 ` Steve Ellcey 2015-05-08 16:40 ` Markus Trippelsdorf 2015-05-08 16:59 ` Joseph Myers 0 siblings, 2 replies; 19+ messages in thread From: Steve Ellcey @ 2015-05-08 16:38 UTC (permalink / raw) To: Marek Polacek Cc: Jeff Law, Joseph Myers, Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-08 16:38 ` Steve Ellcey @ 2015-05-08 16:40 ` Markus Trippelsdorf 2015-05-08 16:59 ` Joseph Myers 1 sibling, 0 replies; 19+ messages in thread From: Markus Trippelsdorf @ 2015-05-08 16:40 UTC (permalink / raw) To: Steve Ellcey Cc: Marek Polacek, Jeff Law, Joseph Myers, Martin Sebor, GCC Patches, Jason Merrill 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-08 16:38 ` Steve Ellcey 2015-05-08 16:40 ` Markus Trippelsdorf @ 2015-05-08 16:59 ` Joseph Myers 2015-05-08 21:54 ` Paul Eggert 1 sibling, 1 reply; 19+ messages in thread From: Joseph Myers @ 2015-05-08 16:59 UTC (permalink / raw) To: Steve Ellcey Cc: Marek Polacek, Jeff Law, Martin Sebor, GCC Patches, Jason Merrill, Paul Eggert 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-08 16:59 ` Joseph Myers @ 2015-05-08 21:54 ` Paul Eggert 2015-05-12 2:58 ` Steve Ellcey 0 siblings, 1 reply; 19+ messages in thread From: Paul Eggert @ 2015-05-08 21:54 UTC (permalink / raw) To: Joseph Myers, Steve Ellcey Cc: Marek Polacek, Jeff Law, Martin Sebor, GCC Patches, Jason Merrill, Time Zone Mailing List [-- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-08 21:54 ` Paul Eggert @ 2015-05-12 2:58 ` Steve Ellcey 2015-05-12 12:40 ` [tz] " Christos Zoulas 0 siblings, 1 reply; 19+ messages in thread From: Steve Ellcey @ 2015-05-12 2:58 UTC (permalink / raw) To: Paul Eggert Cc: Joseph Myers, Marek Polacek, Jeff Law, Martin Sebor, GCC Patches, Jason Merrill, Time Zone Mailing List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [tz] [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) 2015-05-12 2:58 ` Steve Ellcey @ 2015-05-12 12:40 ` Christos Zoulas 0 siblings, 0 replies; 19+ messages in thread From: Christos Zoulas @ 2015-05-12 12:40 UTC (permalink / raw) To: sellcey, Paul Eggert Cc: Marek Polacek, Jason Merrill, Time Zone Mailing List, Martin Sebor, GCC Patches, Jeff Law, Joseph Myers 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-05-12 11:53 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-22 18:36 [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) Marek Polacek 2015-04-23 22:04 ` Jeff Law 2015-04-24 3:11 ` Martin Sebor 2015-04-24 16:27 ` Marek Polacek 2015-04-24 21:32 ` Martin Sebor 2015-04-24 21:44 ` Martin Sebor 2015-04-25 17:31 ` Marek Polacek 2015-04-25 20:13 ` Joseph Myers 2015-04-27 15:46 ` Marek Polacek 2015-04-29 23:57 ` Joseph Myers 2015-05-06 11:38 ` Marek Polacek 2015-05-07 18:00 ` Jeff Law 2015-05-07 19:15 ` Marek Polacek 2015-05-08 16:38 ` Steve Ellcey 2015-05-08 16:40 ` Markus Trippelsdorf 2015-05-08 16:59 ` Joseph Myers 2015-05-08 21:54 ` Paul Eggert 2015-05-12 2:58 ` Steve Ellcey 2015-05-12 12:40 ` [tz] " Christos Zoulas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).