From patchwork Tue Feb 14 20:32:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 727980 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 3vNDcy3Ty2z9s0Z for ; Wed, 15 Feb 2017 07:32:49 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="Fzq7Dsft"; 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:reply-to:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=mPC5dA3of1MYlQyNp TX7JZ12mZhGMu3aR7W+ss/iMUwHqnCBlVOGI1vbTHtQeGkAb0taU3MSFgD/NDlre mJ9FIyQl6/1x3FJZFv5aCXXzbwzoXFnqpwOs7hJjoraKfYJi3CCkw0IFPSWTym1e M/t9pH21APu1dtxabwQBf7QBUI= 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:reply-to:references:mime-version :content-type:in-reply-to; s=default; bh=7uz4Gd5lRE4nGJHzYmi0bmz VJz0=; b=Fzq7Dsfti66h3dS51dzMjPK2eeAgTw4TD3qKmCO70BxoUla08eO58O/ ccZbhN4oNryY+Wqp50/PJY48d+Bx1ekR+JBiFUcrjPae9L38HiVW1nfVqqhvFQq2 GIvNmY2YTFAlQpFo2Q3Nyu+cYFXVLBm3hr/V2vUZHUwcanWUKVVQ= Received: (qmail 48272 invoked by alias); 14 Feb 2017 20:32:41 -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 48260 invoked by uid 89); 14 Feb 2017 20:32:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 spammy=1234, hhx, H*i:sk:176b951, H*f:sk:176b951 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; Tue, 14 Feb 2017 20:32:39 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8393B85540; Tue, 14 Feb 2017 20:32:39 +0000 (UTC) Received: from tucnak.zalov.cz (ovpn-116-76.ams2.redhat.com [10.36.116.76]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1EKWb8c022234 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 14 Feb 2017 15:32:38 -0500 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id v1EKWZ3N025073; Tue, 14 Feb 2017 21:32:35 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id v1EKWXT6025072; Tue, 14 Feb 2017 21:32:33 +0100 Date: Tue, 14 Feb 2017 21:32:33 +0100 From: Jakub Jelinek To: Martin Sebor Cc: Jeff Law , Gcc Patch List Subject: Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327) Message-ID: <20170214203233.GA1849@tucnak> Reply-To: Jakub Jelinek References: <5a514649-d71d-e10a-08b1-93d0a42aa443@redhat.com> <20170203163921.GK14051@tucnak> <20170204080723.GT14051@tucnak> <191157de-10d3-a33c-7261-fc680628cd83@redhat.com> <20170214071813.GR1849@tucnak> <20170214163950.GW1849@tucnak> <176b9516-8839-27df-9e96-9990e5ce0348@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <176b9516-8839-27df-9e96-9990e5ce0348@gmail.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-IsSubscribed: yes On Tue, Feb 14, 2017 at 12:15:59PM -0700, Martin Sebor wrote: > That comment explains how the likely_adjust variable ("the adjustment") > is being used, or more precisely, how it was being used in the first > version of the patch. The comment became somewhat out of date with > the committed version of the patch (this was my bad). > > The variable is documented where it's defined and again where it's > assigned to. With the removal of those comments it seems especially > important that the only remaining description of what's going on be > accurate. > > The comment is outdated because it refers to "the adjustment" which > doesn't exist anymore. (It was replaced by a flag in my commit). > To bring it up to date it should say something like: > > /* Set the LIKELY counter to MIN. In base 8 and 16, when > the argument is in range that includes zero, adjust it > upward to include the length of the base prefix since > in that case the MIN counter does include it. */ So for a comment, what about following then? With or without the IMNSHO useless && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0) > On a separate note, while testing the patch I noticed that it's > not exactly equivalent to what's on trunk. Trunk silently accepts > the call below but with the patch it complains. That's great (it > should complain) but the change should be tested. More to my point, > while in this case your change happened to fix a subtle bug (which > I'm certainly happy about), it could have just as easily introduced > one. Yeah, indeed. That should be a clear argument for why writing it in so many places is bad, it is simply much more error-prone, there are too many cases to get right. > char d[2]; > > void f (unsigned i) > { > if (i < 1234 || 12345 < i) > i = 1234; > > __builtin_sprintf (d, "%#hhx", i); > } What happens is that because the original range doesn't contain zero you set likely_adjust to false and then never update it again because the implicit cast changed the range. If some version of the patch is approved, I'll leave addition of this testcase to you (incrementally). 2017-02-14 Jakub Jelinek PR tree-optimization/79327 * gimple-ssa-sprintf.c (format_integer): Remove likely_adjust variable, its initialization and use. Jakub --- gcc/gimple-ssa-sprintf.c.jj 2017-02-14 21:21:56.048745037 +0100 +++ gcc/gimple-ssa-sprintf.c 2017-02-14 21:25:20.939033174 +0100 @@ -1232,10 +1232,6 @@ format_integer (const directive &dir, tr of the format string by returning [-1, -1]. */ return fmtresult (); - /* True if the LIKELY counter should be adjusted upward from the MIN - counter to account for arguments with unknown values. */ - bool likely_adjust = false; - fmtresult res; /* Using either the range the non-constant argument is in, or its @@ -1265,14 +1261,6 @@ format_integer (const directive &dir, tr res.argmin = argmin; res.argmax = argmax; - - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - wide_int wzero = wi::zero (wi::get_precision (min)); - if (wi::le_p (min, wzero, SIGNED) - && !wi::neg_p (max)) - likely_adjust = true; } else if (range_type == VR_ANTI_RANGE) { @@ -1307,11 +1295,6 @@ format_integer (const directive &dir, tr if (!argmin) { - /* Set the adjustment for an argument whose range includes - zero since that doesn't include the octal or hexadecimal - base prefix. */ - likely_adjust = true; - if (TREE_CODE (argtype) == POINTER_TYPE) { argmin = build_int_cst (pointer_sized_int_node, 0); @@ -1364,14 +1347,19 @@ format_integer (const directive &dir, tr res.range.max = MAX (max1, max2); } - /* Add the adjustment for an argument whose range includes zero - since it doesn't include the octal or hexadecimal base prefix. */ + /* If the range is known, use the maximum as the likely length. */ if (res.knownrange) res.range.likely = res.range.max; else { + /* Otherwise, use the minimum. Except for the case where for %#x or + %#o the minimum is just for a single value in the range (0) and + for all other values it is something longer, like 0x1 or 01. + Use the length for value 1 in that case instead as the likely + length. */ res.range.likely = res.range.min; - if (likely_adjust && maybebase && base != 10) + if (maybebase && base != 10 + && (tree_int_cst_sgn (argmin) < 0 || tree_int_cst_sgn (argmax) > 0)) { if (res.range.min == 1) res.range.likely += base == 8 ? 1 : 2;