From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8061 invoked by alias); 5 Dec 2016 18:53:06 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 8031 invoked by uid 89); 5 Dec 2016 18:53:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy= X-HELO: mail-qk0-f194.google.com Received: from mail-qk0-f194.google.com (HELO mail-qk0-f194.google.com) (209.85.220.194) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 05 Dec 2016 18:52:55 +0000 Received: by mail-qk0-f194.google.com with SMTP id y205so40713612qkb.1 for ; Mon, 05 Dec 2016 10:52:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=v3yisTjQh4HwFaYaxnvOgHdV0EKwb4R4eSoXYuDU7cM=; b=UYwjveyRGi6xbXZ9Y7437VLNt8rRE4Ywhylienosg2TkMKzlyD2tNYJCPD7KKj7joV wHxaW6im8culd4nm/jimLij02ZbVkNBnauz5XXEfgDiJJ38qCz23BUHCYvML5MQs5a1c 2JGoChZsvwuLTgUMiKx3GVDj5jLnDgsCbHI+Fi5BhT5wNK7R5KmtGKKmSTqkGhKYvS6T yRigMBJmlLqHzgY8/ZczwJZrXYc7vR7dh0Bdkt1vbdO9xNeiLYAN8XY/sEaIuYpxf0VP eP4YM+KOi/19dynsc9f6vQChlJAxJhtqePj6pomyTnNUeA1Sw96e++dhy+XIOB/t7Zo1 X2xA== X-Gm-Message-State: AKaTC02/1zX35RNKO8ejEblTjHDtcgT7ADcbA9QHrYRWx6UWHhqofIrThqluSW7v4/Uj4Q== X-Received: by 10.55.190.66 with SMTP id o63mr51545535qkf.254.1480963974036; Mon, 05 Dec 2016 10:52:54 -0800 (PST) Received: from [192.168.0.26] (97-122-174-22.hlrn.qwest.net. [97.122.174.22]) by smtp.gmail.com with ESMTPSA id a81sm10044197qkg.28.2016.12.05.10.52.53 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Dec 2016 10:52:53 -0800 (PST) Subject: Re: [PATCH] correct handling of non-constant width and precision (pr 78521) To: Jeff Law , Rainer Orth References: <9b412cbb-1cb0-6341-5b85-78f235f7ae6f@gmail.com> <8ee392e4-6c09-1a7d-b6f1-0aa20ba81cae@gmail.com> <8b27f594-f852-4b78-98f1-b3e48230bcc6@gmail.com> Cc: Gcc Patch List From: Martin Sebor Message-ID: <5febaa53-4f66-6a86-29fd-749403c1532d@gmail.com> Date: Mon, 05 Dec 2016 18:53:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00382.txt.bz2 On 12/05/2016 11:25 AM, Jeff Law wrote: > On 12/05/2016 08:50 AM, Martin Sebor wrote: >> On 12/02/2016 08:52 AM, Martin Sebor wrote: >>> On 12/02/2016 01:31 AM, Rainer Orth wrote: >>>> Hi Martin, >>>> >>>>> PR 78521 notes that the gimple-ssa-sprintf pass doesn't do the right >>>>> thing (i.e., the -Wformat-length and -fprintf-return-value options >>>>> behave incorrectly) when a conversion specification includes a width >>>>> or precision with a non-constant value. The code treats such cases >>>>> as if they were not provided which is incorrect and results in >>>>> the wrong bytes counts in warning messages and in the wrong ranges >>>>> being generated for such calls (or in the case sprintf(0, 0, ...) >>>>> for some such calls being eliminated). >>>>> >>>>> The attached patch corrects the handling of these cases, plus a couple >>>>> of other edge cases in the same area: it adjusts the parser to accept >>>>> precision in the form of just a period with no asterisk or decimal >>>>> digits after it (this sets the precision to zero), and corrects the >>>>> handling of zero precision and zero argument in integer directives >>>>> to produce no bytes on output. >>>>> >>>>> Finally, the patch also tightens up the constraint on the upper bound >>>>> of bounded functions like snprintf to be INT_MAX. The functions >>>>> cannot >>>>> produce output in excess of INT_MAX + 1 bytes and some implementations >>>>> (e.g., Solaris) fail with EINVAL when the bound is INT_MAX or more. >>>>> This is the subject of PR 78520. >>>> >>>> this patch broke Solaris bootstrap: >>>> >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c: In function >>>> 'void {anonymous}::get_width_and_precision(const >>>> {anonymous}::conversion_spec&, long long int*, long long int*)': >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: error: >>>> call of overloaded 'abs(long long int)' is ambiguous >>>> width = abs (tree_to_shwi (spec.star_width)); >>>> ^ >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:777:45: note: >>>> candidates are: >>>> In file included from /usr/include/stdlib.h:12:0, >>>> from /vol/gcc/src/hg/trunk/local/gcc/system.h:258, >>>> from >>>> /vol/gcc/src/hg/trunk/local/gcc/gimple-ssa-sprintf.c:49: >>>> /usr/include/iso/stdlib_iso.h:205:16: note: long int std::abs(long int) >>>> inline long abs(long _l) { return labs(_l); } >>>> ^ >>>> /usr/include/iso/stdlib_iso.h:160:12: note: int std::abs(int) >>>> extern int abs(int); >>>> ^ >>>> >>>> The following patch fixed this for me, but I've no idea if it's right. >>>> It bootstrapped successfully on sparc-sun-solaris2.12, >>>> i386-pc-solaris2.12, and x86_64-pc-linux-gnu. >>> >>> Thanks for the heads up! I just looked at that code yesterday while >>> analyzing bug 78608, wondering if it was safe. Now I know it isn't. >>> I think it might be best to simply hand code the expression instead >>> of taking a chance on abs. Let me take care of it today along with >>> 78608. >> >> I posted a bigger patch to fix this and other related problems on >> Friday (https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00262.html). >> In hindsight, I should have probably committed the fix for this >> on its own. Please let me know if this is blocking you and I'll >> commit this fix by itself today so you don't have to wait for >> the bigger patch to get reviewed and approved. > What's the concern with using std::abs? My concern, when I wrote the reply n Friday, was that not all C++98 implementations may get std::abs right, declare it in the right header, avoid defining the abs macro, or put it in namespace std. (IIRC, the standard itself wasn't quite right.) I also need to avoid calling abs with a TYPE_MIN argument because that's undefined and flagged by ubsan (as per the bug in the subject, though it was not a result of calling abs but rather that of negating it). Besides avoiding the undefined behavior in the compiler I also need diagnose it (in the program). The test case for it goes like this: int n = sprintf (0, 0, "%*i", INT_MIN, 0); where the INT_MIN is interpreted as the left justification flag followed by a positive width of -(unsigned long)INT_MIN. The problem is that the function (declared to return int0 is being asked to return INT_MAX + 1 which is undefined (in the program). Martin