From patchwork Fri Dec 22 19:10:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 852521 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-469791-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="FRcL/G26"; dkim-atps=neutral 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 3z3J1C5dMZz9s03 for ; Sat, 23 Dec 2017 06:07:42 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:in-reply-to:references; q=dns; s= default; b=PBIvDSrPYYOod2vLOTghK2qOk35SkbtOlhKBiodq/NZi9Ry5USKND mRXojtDtleVOsGMLFG01Ct0HO8LupD4d4QS6u1GFW+U6L1lzO/sn/Qc+OifutiYl MEnKyvQQpSRoJx0KIRJ4kDeitLVwzaoqldcN4ubV4JsJkzlPJRBni8= 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:from :to:cc:subject:date:message-id:in-reply-to:references; s= default; bh=B4HBAKvZOpMwklrBDc6hHWQtLxs=; b=FRcL/G26pw1U74Rvg0UT +4v2xlBQe+v6LOSN9i03ibP26wVpfzTPKrcbl/qkh07hG4tLr+lMVS9LLovm7pbk XjJqULA1e5hMf60g/O4+wGKZFYYGSW4YzOpuPY77xq2cEgaLzcPAu0BSEl+hpm1e q6m3SVXp73O8sIIdhs/iTmY= Received: (qmail 83872 invoked by alias); 22 Dec 2017 19:07:35 -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 83863 invoked by uid 89); 22 Dec 2017 19:07:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=H*i:2kKG893f, H*i:sk:_DtNRVH, H*f:2kKG893f, H*f:sk:_DtNRVH 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 ESMTP; Fri, 22 Dec 2017 19:07:33 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CAA1481DEE; Fri, 22 Dec 2017 19:07:31 +0000 (UTC) Received: from c64.redhat.com (ovpn-112-35.phx2.redhat.com [10.3.112.35]) by smtp.corp.redhat.com (Postfix) with ESMTP id 280DB176D6; Fri, 22 Dec 2017 19:07:29 +0000 (UTC) From: David Malcolm To: Jason Merrill Cc: Nathan Sidwell , Jakub Jelinek , Richard Biener , gcc-patches List , David Malcolm Subject: [v3 of PATCH 13/14] c-format.c: handle location wrappers Date: Fri, 22 Dec 2017 14:10:58 -0500 Message-Id: <1513969858-13170-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: References: X-IsSubscribed: yes On Thu, 2017-12-21 at 00:00 -0500, Jason Merrill wrote: > On Wed, Dec 20, 2017 at 12:33 PM, David Malcolm > wrote: > > On Tue, 2017-12-19 at 14:55 -0500, Jason Merrill wrote: > > > On 12/17/2017 11:29 AM, David Malcolm wrote: > > > > On Mon, 2017-12-11 at 18:45 -0500, Jason Merrill wrote: > > > > > On 11/10/2017 04:45 PM, David Malcolm wrote: > > > > > > + STRIP_ANY_LOCATION_WRAPPER (format_tree); > > > > > > + > > > > > > if (VAR_P (format_tree)) > > > > > > { > > > > > > /* Pull out a constant value if the front end > > > > > > didn't. */ > > > > > > > > > > It seems like we want fold_for_warn here instead of the > > > > > special > > > > > variable > > > > > handling. That probably makes sense for the other places you > > > > > change in > > > > > this patch, too. > > > > > > > > Here's an updated version of the patch which uses > > > > fold_for_warn, > > > > rather than STRIP_ANY_LOCATION_WRAPPER. > > > > > > > > In one place it was necessary to add a STRIP_NOPS, since the > > > > fold_for_warn can add a cast around a: > > > > ADDR_EXPR ( > > > > STRING_CST) > > > > turning it into a: > > > > NOP_EXPR ( > > > > ADDR_EXPR ( > > > > STRING_CST)) > > > > > > Hmm, that seems like a bug. fold_for_warn shouldn't change the > > > type of > > > the result. > > > > In a similar vein, is the result of decl_constant_value (decl) > > required > > to be the same type as that of the decl? > > > > What's happening for this testcase (g++.dg/warn/Wformat-1.C) is > > that we > > have a VAR_DECL with a DECL_INITIAL, but the types of the two don't > > quite match up. > > > > decl_constant_value returns an unshare_expr clone of the > > DECL_INITIAL, > > and this is what's returned from fold_for_warn. > > > > Am I right in thinking > > > > (a) that the bug here is that a DECL_INITIAL ought to have the same > > type as its decl, and so there's a missing cast where that gets > > set up, or > > This seems right. > > > (b) should decl_constant_value handle such cases, and introduce a > > cast > > if it uncovers them? > > decl_constant_value should probably assert that the types match > closely enough. Thanks. You describe the types needing to match "closely enough" (as opposed to *exactly*), and that may be the key here: what I didn't say in my message above is that the decl is "const" whereas the value isn't. To identify where a DECL_INITIAL can have a different type to its decl (and thus fold_for_warn "changing type"), I tried adding this assertion: --- a/gcc/cp/typeck2.c +++ b/gcc/cp/typeck2.c @@ -857,6 +857,7 @@ store_init_value (tree decl, tree init, vec** cleanups, int flags) /* If the value is a constant, just put it in DECL_INITIAL. If DECL is an automatic variable, the middle end will turn this into a dynamic initialization later. */ + gcc_assert (TREE_TYPE (value) == TREE_TYPE (decl)); DECL_INITIAL (decl) = value; return NULL_TREE; } The assertion fires when a "const" decl is initialized with a value of a non-const type; e.g. in g++.dg/warn/Wformat-1.C: const char *const msg = "abc"; but I can also reproduce it with: const int i = 42; What happens is that the type of the decl has the "readonly_flag" set, whereas the type of the value has that flag clear. For the string case, convert_for_initialization finishes here: 8894 return convert_for_assignment (type, rhs, errtype, fndecl, parmnum, 8895 complain, flags); and convert_for_assignment finishes by calling: 8801 return perform_implicit_conversion_flags (strip_top_quals (type), rhs, 8802 complain, flags); The "strip_top_quals" in that latter call strips away the "readonly_flag" from type (the decl's type), giving the type of the rhs, and hence this does nothing. So we end up with a decl of type const char * const being initialized with a value of type const char * (or a decl of type "const int" being initialized with a value of type "int"). So the types are slightly inconsistent (const vs non-const), but given your wording of types needing to match "closely enough" it's not clear to me that it's a problem - if I'm reading things right, it's coming from that strip_top_quals in the implicit conversion in convert_for_assignment. This also happens with a pristine build of trunk. Rather than attempting to change decl_constant_value or the folding code, the following patch simply updates the: if (VAR_P (format_tree)) { /* Pull out a constant value if the front end didn't. */ format_tree = decl_constant_value (format_tree); STRIP_NOPS (format_tree); } that you added in r219964 to fix PR c++/64629 to instead be: format_tree = fold_for_warn (format_tree); STRIP_NOPS (format_tree); and adds a little test coverage for the pointer addition case. Successfully bootstrapped®rtested on x86_64-pc-linux-gnu, as part of the kit. OK for trunk once the rest of the kit is approved? gcc/c-family/ChangeLog: * c-format.c (check_format_arg): Convert VAR_P check to a fold_for_warn. gcc/testsuite/ChangeLog: * g++.dg/warn/Wformat-1.C: Add tests of pointer arithmetic on format strings. --- gcc/c-family/c-format.c | 10 ++++------ gcc/testsuite/g++.dg/warn/Wformat-1.C | 2 ++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index 164d035..1fcac2f 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -1536,12 +1536,10 @@ check_format_arg (void *ctx, tree format_tree, location_t fmt_param_loc = EXPR_LOC_OR_LOC (format_tree, input_location); - if (VAR_P (format_tree)) - { - /* Pull out a constant value if the front end didn't. */ - format_tree = decl_constant_value (format_tree); - STRIP_NOPS (format_tree); - } + /* Pull out a constant value if the front end didn't, and handle location + wrappers. */ + format_tree = fold_for_warn (format_tree); + STRIP_NOPS (format_tree); if (integer_zerop (format_tree)) { diff --git a/gcc/testsuite/g++.dg/warn/Wformat-1.C b/gcc/testsuite/g++.dg/warn/Wformat-1.C index 6094a9c..f2e772a 100644 --- a/gcc/testsuite/g++.dg/warn/Wformat-1.C +++ b/gcc/testsuite/g++.dg/warn/Wformat-1.C @@ -7,4 +7,6 @@ foo (void) { const char *const msg = "abc"; bar (1, msg); + bar (1, msg + 1); + bar (1, 1 + msg); }