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
