Re: [PATCH] fix integer overflow bugs in gimple-ssa-sprintf.c (PR 78608)

[Jeff Law <law@redhat.com>]

On 12/14/2016 09:41 AM, Martin Sebor wrote:
>
>>> -      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.
>
> I learned the first style (const OP variable, and also using a less
> than in favor of other relational operators) many years ago and I'm
> trying to unlearn it for GCC.  It's a hard habit to break.
>
> FWIW, the const OP variable style used to be recommended to avoid
> subtle bugs due to typos like 'if (var = CST)'  But I realize that
> with -Wparentheses warning on this there is no need for the style
> when using GCC.
Yea.   And I can see some argument to use the CST == var style.   But 
I'm not up to pushing on that in our coding standards.  I realize it'll 
take time to unlearn :-)


>
> I think the patch for bug 78696 resolves the FIXME (but see below).
>
>> 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.
>
> The patch for bug 78696 resolves the FIXME but there will need to
> be another change here to improve the handling of unknown precisions
> and widths:
>
> 1) Use get_range_info to constrain non-constant width and precision,
>    and if that fails...
>
> 2) ...use some reasonable default (e.g., based on the precision of
>    the type of the directive).
>
> Without these changes sprintf (d, "%.*f", p, 0.0) causes
>
>   warning: writing a terminating nul past the end of the destination
>
> even at -Wformat-length=1 with no good way to suppress it.  At
> -Wformat-length=2, sprintf(d, "%.*i", 0) also causes a similar:
>
>   warning: ‘%.*i’ directive writing 0 or more bytes into a region...
>
> also with no way to suppress it. (The two warnings should also be
> worded the same.)
>
> I've started to work on a general fix for both of these in my patch
> for bug 78703 - -fprintf-return-value floating point handling
> incorrect in locales with a mulltibyte decimal point.  These fit
> well together because they both separate the heuristic-based warning
> byte counters from the actual counters good for optimization (which
> are based on what GCC knows for certain).
Understood.  Thanks for the update.

jeff