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
