[PATCH] Convert strlen pass from evrp to ranger.

[Aldy Hernandez]

On Sat, Oct 9, 2021 at 7:59 PM Martin Sebor <msebor@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 <msebor@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