Re: [PATCH] correct %g handling with unknown arguments in -fprintf-return-value (PR 78696)

[Jeff Law <law at redhat dot com>]

On 12/09/2016 11:07 AM, Martin Sebor wrote:
> Bug 78696 points out a bug in the handling of the %g directive in
> the gimple-ssa-sprintf pass where precision wasn't being correctly
> handled.  In the process of developing and testing a fix for it
> I noticed a few other subtle problems in the floating point handling
> of unknown values.  The attached patch corrects them and adds test
> cases to better exercise this area.
>
> In the same bug, Jakub also pointed out that the -fprintf-retrun-value
> optimization isn't correct in locales where the floating point radix
> character (normally the decimal point, '.') is a multibyte character
> longer than 1 byte.  Since that's unrelated to this report I raised
> bug 78703 and will fix it in a subsequent patch.
Definitely address in a subsequent patch.

> Martin
>
>
> gcc-78696.diff
>
>
> PR tree-optimization/78696 - [7 Regression] -fprintf-return-value misoptimizes %.Ng where N is greater than 10
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/78696
> 	* gimple-ssa-sprintf.c (format_floating): Correct handling of
> 	precision.  Use MPFR for %f for greater fidelity.  Correct handling
> 	of %g.
> 	(pass_sprintf_length::compute_format_length): Set width and precision
> 	specified by asrerisk to void_node for vararg functions.
> 	(try_substitute_return_value): Adjust dump output.
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/78696
> 	* gcc.dg/tree-ssa/builtin-sprintf-5.c: Add test cases.
> 	* gcc.dg/tree-ssa/builtin-sprintf-warn-7.c: New test.
> 	* gcc.dg/tree-ssa/builtin-sprintf.c (checkv): Add test cases.
[ snip ]


> +	/* The lower bound when precision isn't specified is 8 bytes
> +	   ("1.23456" since precision is taken to be 6).  When precision
> +	   is zero, the lower bound is 1 byte (e.g., "1").  Otherwise,
> +	   when precision is greater than zero, then the lower bound
> +	   is 2 plus precision (plus flags).  */
> +	res.range.min = (flagmin
> +			 + (prec != INT_MIN)   /* for decimal point */
> +			 + (prec == INT_MIN
> +			    ? 0 : prec < 0 ? 6 : prec ? prec : -1));
Note for the future, nest/chained ternary operators can sometimes just 
be hard to visually parse when they're squashed on a single line. 
Formatting like this has often been used in the past to help clarify the 
intent:

(flagmin
 + (prec != INT_MIN)
 + (prec == INT_MIN ? 0
    : prec < 0 ? 6
    : prec ? prec
    : -1)

If we ignore the flagmin component, I get the following evaluations for 
PREC.

PREC                       RESULT
INTMIN	                   0
0                          0
negative (but not INTMIN)  7
positive                   prec + 1

That doesn't seem in-line with the comment.


Jeff