Re: [PATCH] use zero as the lower bound for a signed-unsigned range (PR 79327)

[Jakub Jelinek <jakub@redhat.com>]

Hi!

Note, the second patch I've posted passed bootstrap/regtest on both
x86_64-linux and i686-linux.

On Thu, Feb 02, 2017 at 09:58:06AM -0700, Martin Sebor wrote:
> > int
> > main (void)
> > {
> >   int i;
> >   char buf[64];
> >   if (__builtin_sprintf (buf, "%#hho", a) > 1)
> >     __builtin_abort ();
> >   if (__builtin_sprintf (buf, "%#hhx", a) > 1)
> >     __builtin_abort ();
> >   return 0;
> > }
> > Current trunk as well as trunk + your patch computes ranges [2, 4] and
> > [3, 4], but the correct ranges are [1, 4] and [1, 4], because for a 0
> > (or say -131072 etc.) it sets buf to "0" in both cases.
> 
> That's right.  This is a good find.  This case is being tested in
> builtin-sprintf-warn-1.c but the assertions (on lines 1154 and
> 1155) seem to expect the wrong numbers.  I would expect your fix
> to cause failures here. (And now that I've read the rest of the
> patch I see it does.)

That testcase is derived from the builtin-sprintf-warn-1.c:115{4,5}
FAILs I got with the patch + analysis (and included in the second patch
in the testsuite as executable testcase too).

> The range in the note is the artificial one the pass uses to compute
> the range of output.  I went back and forth on this as I think it's
> debatable if [-128, 127] is clearer than [1, -128] (or [-128, 1]).
> The informative notes aren't completely consistent in what ranges
> they print.  It would be nice to nail down what we think is the most
> useful output and make them all consistent.

You say in the wording range and print it as a range, then it really should
be a range, and not an artificial one, but one reflecting actual possible
values.  If the user knows that the variable can have values -123 and 126
at that point and you still print [1, -128] or [-128, 1] as range, the
users will just think the compiler is buggy and disable the warning.

> > Plus there are certain notes removed:
> > -builtin-sprintf-warn-1.c:1311:3: note: using the range [1, -2147483648] for directive argument
> >   T (0, "%d",           a);     /* { dg-warning "into a region" } */
> > without replacement, here a has [-2147483648, 2147483647] range, but as that
> > is equal to the type's range, I bet it omits it.
> 
> Could be.  As I said, there are opportunities for improvements in
> what the notes print.  Someone pointed out, for example, that very
> large numbers are hard to read.  It might be clearer to print some
> of them using constants like LONG_MAX - 2, or in hex, etc.

The *_MAX or hex is a separate issue, that can be done or doesn't have to be
done, the values are simply the same.  But picking some random numbers in
the ranges is just wrong.

> > -  tree dirmin = TYPE_MIN_VALUE (dirtype);
> > -  tree dirmax = TYPE_MAX_VALUE (dirtype);
> > -
> > -  if (TYPE_UNSIGNED (dirtype))
> > -    {
> > -      *argmin = dirmin;
> > -      *argmax = dirmax;
> > -    }
> > -  else
> > -    {
> > -      *argmin = integer_zero_node;
> > -      *argmax = dirmin;
> > -    }
> > -
> > +  *argmin = TYPE_MIN_VALUE (dirtype);
> > +  *argmax = TYPE_MAX_VALUE (dirtype);
> 
> This is likely behind the changes in the notes you pointed out above.
> The code here is intentional to reflect the range synthesized by
> the pass to compute the output.  I don't have a big issue with

The change I've done was completely intentional, it is again mixing of
value ranges with already premature guessing on what values are shorter or
longer.

> As for the rest of the patch, while I appreciate your help and
> acknowledge it's not my call to make I'm not entirely comfortable
> with this much churn at this stage.  My preference would to just
> fix the bugs and do the restructuring in stage 1.

The thing is, right now we have 3 independent but intermixed code paths,
one for arguments with VR_RANGE that doesn't need conversion for dirtype
(then argmin/argmax until very lately is actual range and the
shortest/longest computation is done at the latest point), then
VR_RANGE that does need conversion for dirtype (that one is prematurely
partly converted to the shortest/longest above), and then
another one that handles the non-VR_RANGE, which commits to the
shortest/longest immediately.  What my patch does is that it unifies all
the 3 paths on essentially what the first path does.  If we don't apply that
patch, we are going to have 3 times as many possibilities for bugs; you will
need to add some hack to get the above mentioned wrong-code fixed, and
IMNSHO another hack to get the bogus printed ranges fixed.
It is always better to remove code than to add it.

	Jakub