This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]