[PATCH] correct handling of non-constant width and precision (pr 78521)

[Jeff Law]

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?

We're already using std::min std::max, std::swap and others.

jeff