On 12/02/2016 05:36 PM, Martin Sebor wrote:
Bug 78608 - gimple-ssa-sprintf.c:570:17: runtime error: negation
of -9223372036854775808 cannot be represented in type 'long int'
points out an integer overflow bug in the pass caught by ubsan.
The bug was due to negating a number without checking for equality
to INT_MIN.
In addition, my recent change to fix 78521 introduced a call to
abs() that broke the Solaris bootstrap:
https://gcc.gnu.org/ml/gcc-patches/2016-12/msg00161.html
While fixing these two problems I noticed that the rest of the pass
wasn't handling the corner case of a width with the value of INT_MIN
specified via an argument to the asterisk, such as in:
int n = snprintf(0, 0, "%*i", INT_MIN, 0);
This is undefined behavior because negative width is supposed to be
treated as the left justification flag followed by a positive width
(thus resulting in INT_MAX + 1 bytes). This problem affected all
integer and floating point directives.
Finally, while there, I decided to include in information messages
a bit of detail about ranges of floating point values that was
missing. I did this to help answer questions like those raised
earlier this week by Gerald here ("where does the 317 come from?):
https://gcc.gnu.org/ml/gcc/2016-11/msg00102.html
The attached patch adjusts the pass to handle these problems.
Martin
gcc-78608.diff
PR tree-optimization/78608 - gimple-ssa-sprintf.c:570:17: runtime error:
negation of -9223372036854775808 cannot be represented in type 'long int'
gcc/ChangeLog:
PR tree-optimization/78608
* gimple-ssa-sprintf.c (tree_digits): Avoid negating a minimum signed
value.
(get_width_and_precision): Same.
(format_integer): Use HOST_WIDE_INT for expected sprintf return value
to allow for width being the absolte value of INT_MIN.
(format_floating): Use HOST_WIDE_INT for width and precision. Store
argument type for use in diagnostics. Use target INT_MAX rather than
the host constant.
(format_floating): Reuse get_width_and_precision instead of hadcoding
s/hadcoding/hardcoding
the same.
(maybe_get_value_range_from_type): New function.
(format_directive): Treat INT_MAX + 1 as a valid (if excessive) byte
count. Call maybe_get_value_range_from_type.
gcc/testsuite/ChangeLog:
PR tree-optimization/78608
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Add test cases.
So I've been going back and forth on whether or not to suggest a slight
change in how we're working.
Specifically the practice of lumping multiple fixes into a single patch
-- I realize that it's usually the case that you're finding related
issues so there's a desire to address them as a group. Doing things
that way also tends to avoid interdependent patches.
The problem is that makes reviewing more difficult and thus I'm much
less likely to knock it out as soon as it flys by my inbox. And I
think I've seen several trivial to review, but important fixes inside
larger, harder to review changes.
Let's try to keep patches to addressing one issue for the future. If
you have multiple related issues, you should consider a series of patches.
Hopefully that will cut down on the number of things are getting stalled.
Index: gcc/gimple-ssa-sprintf.c
===================================================================
--- gcc/gimple-ssa-sprintf.c (revision 243196)
+++ gcc/gimple-ssa-sprintf.c (working copy)
@@ -565,8 +565,14 @@ tree_digits (tree x, int base, bool plus, bool pre
if (tree_fits_shwi_p (x))
{
HOST_WIDE_INT i = tree_to_shwi (x);
- if (i < 0)
+ if (HOST_WIDE_INT_MIN == i)
nit. I think most folks would probably prefer this as
if (i == HOST_WIDE_INT_MIN).
HOST_WIDE_INT_MIN is a constant and when we can write an expression in
either order variable OP const is the preferred order.
You seem to be going back and forth between both styles. Let' try to
stick with variable OP const. I don't think you need to go back and fix
all of the existing const OP variable instances right now, but we may in
the future.
@@ -1221,13 +1251,15 @@ static fmtresult
{
/* The minimum output is "0x.p+0". */
res.range.min = 6 + (prec > 0 ? prec : 0);
- res.range.max = (width == INT_MIN
- ? HOST_WIDE_INT_MAX
+ /* The maximum is the greater of widh and the number of bytes
s/widh/width/
@@ -1316,42 +1350,23 @@ format_floating (const conversion_spec &spec, tree
/* Set WIDTH to -1 when it's not specified, to INT_MIN when it is
specified by the asterisk to an unknown value, and otherwise to
a non-negative value corresponding to the specified width. */
- int width = -1;
- int prec = -1;
+ HOST_WIDE_INT width;
+ HOST_WIDE_INT prec;
+ get_width_and_precision (spec, &width, &prec);
+ /* FIXME: Handle specified precision with an unknown value. */
+ if (prec == HOST_WIDE_INT_MIN)
+ return fmtresult ();
Is this something that needs to be fixed now or was this a marker for
future?
+ /* Adjust the count if it's less than the specified width. */
+ if (0 < width && *minmax[i] < (unsigned HOST_WIDE_INT)width)
+ *minmax[i] = width;
width > 0 && ...
@@ -1684,6 +1701,44 @@ format_string (const conversion_spec &spec, tree a
return res;
}
+/* Helper to construct tree nodes corresponding to the minimum and
+ maxium values for a type. When *ARGMIN is a real floating type
s/maxium/maximum/
+
+ /* Get the real type format desription for the target. */
s/desription/description/
If the FIXME was a future thing, then this is OK with the nits fixed.
If the FIXME was a marker for something you intended to address now and
just forgot, then we either need another iteration or a follow-up patch
depending on the severity of the FIXME in your mind.
jeff
