On Sat, Oct 9, 2021 at 7:59 PM Martin Sebor <mse...@gmail.com> wrote: > > On 10/9/21 10:19 AM, Martin Sebor wrote: > > On 10/9/21 9:04 AM, Aldy Hernandez wrote: > >> On Fri, Oct 8, 2021 at 6:52 PM Martin Sebor <mse...@gmail.com> wrote: > >>> > >>> On 10/8/21 9:12 AM, Aldy Hernandez via Gcc-patches wrote: > >>>> The following patch converts the strlen pass from evrp to ranger, > >>>> leaving DOM as the last remaining user. > >>> > >>> Thanks for doing this. I know I said I'd work on it but I'm still > >>> bogged down in my stage 1 work that's not going so great :( I just > >>> have a few minor comments/questions on the strlen change (inline) > >>> but I am a bit concerned about the test failure. > >>> > >>>> > >>>> No additional cleanups have been done. For example, the strlen pass > >>>> still has uses of VR_ANTI_RANGE, and the sprintf still passes around > >>>> pairs of integers instead of using a proper range. Fixing this > >>>> could further improve these passes. > >>>> > >>>> As a further enhancement, if the relevant maintainers deem useful, > >>>> the domwalk could be removed from strlen. That is, unless the pass > >>>> needs it for something else. > >>>> > >>>> With ranger we are now able to remove the range calculation from > >>>> before_dom_children entirely. Just working with the ranger on-demand > >>>> catches all the strlen and sprintf testcases with the exception of > >>>> builtin-sprintf-warn-22.c which is due to a limitation of the sprintf > >>>> code. I have XFAILed the test and documented what the problem is. > >>> > >>> builtin-sprintf-warn-22.c is a regression test for a false positive > >>> in Glibc. If it fails we'll have to deal with the Glibc failure > >>> again, which I would rather avoid. Have you checked to see if > >>> Glibc is affected by the change? > > > > Have you tested Glibc with the patch to see if the warning comes > > back? If it does there are other ways to suppress it, and I can > > take care of it. I just want us to avoid reintroducing it without > > doing our due diligence first. > > I've built Glibc with the latest GCC and your patch applied and > the warning is back so we'll need to suppress it there. I opened > Glibc bug 28439 and will submit a patch for it: > > https://sourceware.org/bugzilla/show_bug.cgi?id=28439
The above patch rewrites the sprintf call into a pair of strcpy. This seems like a burdensome thing to ask of our users to silence a false positive, but I leave it to the glibc experts to opine. > > There are other Glibc warnings to deal with so I don't think your > patch needs to wait until this one is resolved. > > I also missed the following in your patch: > > > gcc/ChangeLog: > > > > * Makefile.in: Disable -Wformat-overflow for > > gimple-ssa-warn-access.o. > > Rather than disabling the warning for the whole file (or any > others), unless suppressing it in the code is overly intrusive, > doing it that way is preferable. For %s sprintf directives, > specifying precision can be used to constrain the output. In > this case where each %s output is the result of formatting > an (at most) 64-bit integer, we know that the length of each > %s argument is at most 21 bytes, so specifying a precision as > big as 37 should both make sure the output fits and avoid > the warning. I.e., this should (and in my tests does) work: > > char *s1 = print_generic_expr_to_str (sizrng[1]); > sprintf (sizstr, "[%.37s, %.37s]", s0, s1); > free (s1); Thanks. I've tweaked the patch accordingly. Aldy
