From patchwork Mon Apr 27 15:46:25 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 465103 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 2B73B140318 for ; Tue, 28 Apr 2015 01:46:42 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=CBNbmXLO; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=xx7wZv6LnpXtZjUmu llJbgWUu0pWNfkCsW6DDF+1EiOZvoOqpDdiiErNX1xAX9FhM2+iuNLhD4lnrUUpP 0mCk4yQeD1woLBWinU7djoqpHKVZTA5HQYJjNNrAQd7jPHMBXwvSf9NtDo+ON2yl bAp8RbOZ5K5VQ5cil8QGdlBJEs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=Pv8Y9PbM36m1tY0YKDYBB+N y8JQ=; b=CBNbmXLO12Pl9ohhVDoMVf39WQTiA2oz8fZSaTSpXkhVvSHUt6XQWVI AL0VtohjI4y3HnQ+W8YO8CTcbUeBDzBC55Qddj1HtQSzMU9CqhjkrvrCjAswgLog CQ2/cD/6jHi/LkfguFgkSeXXQEDlelgYEhn2bpw5tFxxcziEACt4= Received: (qmail 121334 invoked by alias); 27 Apr 2015 15:46:34 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 121321 invoked by uid 89); 27 Apr 2015 15:46:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 27 Apr 2015 15:46:32 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3RFkTeU023838 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 27 Apr 2015 11:46:29 -0400 Received: from redhat.com (ovpn-204-54.brq.redhat.com [10.40.204.54]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3RFkQPS006786 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NO); Mon, 27 Apr 2015 11:46:28 -0400 Date: Mon, 27 Apr 2015 17:46:25 +0200 From: Marek Polacek To: Joseph Myers Cc: Martin Sebor , GCC Patches , Jason Merrill Subject: Re: [C/C++ PATCH] Implement -Wshift-negative-value (PR c/65179) Message-ID: <20150427154625.GC8489@redhat.com> References: <20150422183633.GI28950@redhat.com> <5539B46B.8030704@redhat.com> <20150424162703.GK2813@redhat.com> <553AB64B.1070508@redhat.com> <20150425173052.GL2813@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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 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. Marek 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 } */