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