On 02/02/2017 07:12 AM, Jakub Jelinek wrote:
On Thu, Feb 02, 2017 at 10:52:18AM +0100, Jakub Jelinek wrote:
That said, as I've said in the PR and earlier in December on gcc-patches,
the way format_integer is structured is not really maintainable, has
very different code paths for arguments with known ranges and without them
intermixed together and I still ran into wrong-code issues or wrong warnings
with your patch. See below. Thus I'd like to propose to just very much
simplify the code:
gimple-ssa-sprintf.c | 108 ++++++++++++---------------------------------------
1 file changed, 27 insertions(+), 81 deletions(-)
So far I haven't done full bootstrap/regtest on this, just
make check-gcc -j16 -k RUNTESTFLAGS=tree-ssa.exp
which revealed the need for gcc.dg/tree-ssa/builtin-sprintf-warn-1.c hunk
below. Here it is turned into a short wrong-code without the patch below:
Testing revealed a bug in my patch (POINTER_TYPE args really need special
handling, restored), and one further testcase glitch, plus I've added
another testcase for the other wrong-code.
Going to bootstrap/regtest this again:
2017-02-02 Jakub Jelinek <ja...@redhat.com>
Martin Sebor <mse...@redhat.com>
PR tree-optimization/79327
* gimple-ssa-sprintf.c (adjust_range_for_overflow): If returning
true, always set *argmin and *argmax to TYPE_{MIN,MAX}_VALUE of
dirtype.
(format_integer): Use wide_int_to_tree instead of build_int_cst
+ to_?hwi. If argmin is NULL, just set argmin and argmax to
TYPE_{MIN,MAX}_VALUE of argtype. Simplify and fix computation
of shortest and longest sequence.
* gcc.dg/tree-ssa/pr79327.c: New test.
* gcc.dg/tree-ssa/builtin-sprintf-warn-1.c
(test_sprintf_chk_hh_nonconst): Don't expect 2 bogus warnings.
* gcc.dg/tree-ssa/builtin-sprintf-warn-3.c
(test_sprintf_chk_range_schar): Adjust dg-message.
* gcc.dg/tree-ssa/builtin-sprintf-warn-12.c: New test.
* gcc.c-torture/execute/pr79327.c: New test.
After a lot of thinking about the situation, I think we're better off
doing this bit of cleanup now. Jakub and I both expect we're going to
be in this code again during the gcc-7 cycle, so the cleanups have
immediate benefit. I don't see them as inherently destabilizing and
they're a smaller than I had anticipated.
They may (or may not) need tweaks to adjust for the vla/flexible array
support that went in last night. I don't see an obvious conflict, but
if there is use your best judgment about whether or not you need to go
through another review/approval cycle.
--- gcc/gimple-ssa-sprintf.c.jj 2017-02-02 11:03:46.536526801 +0100
+++ gcc/gimple-ssa-sprintf.c 2017-02-02 14:53:54.657592450 +0100
@@ -1260,10 +1248,8 @@ format_integer (const directive &dir, tr
@@ -1307,47 +1293,16 @@ format_integer (const directive &dir, tr
if (!argmin)
{
- /* For an unknown argument (e.g., one passed to a vararg function)
- or one whose value range cannot be determined, create a T_MIN
- constant if the argument's type is signed and T_MAX otherwise,
- and use those to compute the range of bytes that the directive
- can output. When precision may be zero, use zero as the minimum
- since it results in no bytes on output (unless width is specified
- to be greater than 0). */
- bool zero = dir.prec[0] <= 0 && dir.prec[1] >= 0;
- argmin = build_int_cst (argtype, !zero);
-
- int typeprec = TYPE_PRECISION (dirtype);
- int argprec = TYPE_PRECISION (argtype);
-
- if (argprec < typeprec)
+ if (TREE_CODE (argtype) == POINTER_TYPE)
{
- if (POINTER_TYPE_P (argtype))
- argmax = build_all_ones_cst (argtype);
- else if (TYPE_UNSIGNED (argtype))
- argmax = TYPE_MAX_VALUE (argtype);
- else
- argmax = TYPE_MIN_VALUE (argtype);
+ argmin = build_int_cst (pointer_sized_int_node, 0);
+ argmax = build_all_ones_cst (pointer_sized_int_node);
Curious why you didn't use the wide_int_to_tree API here. I'm not
objecting to using build_XXX, it's more to help guide me when I need to
make a choice between the APIs for building INTEGER_CST nodes.
OK pending resolution of any conflicts with vla/flexible array changes
from last night.
jeff
