Re: [PATCH] Convert strlen pass from evrp to ranger.

[Martin Sebor <msebor@gmail.com>]

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 think there still are quite a few calls to get_addr_stridx() and
>> get_addr() that don't pass in rvals.  I started doing this back in
>> the GCC 11 cycle but didn't finish the job.  Eventually, rvals
>> should be passed to all their callers (or they made class members
>> with a ranger member).  I mention this in case it has an effect on
>> the range propagation (since the pass also sets ranges).
> 
> Definitely.  You'll get even better ranges, though perhaps more false
> positives as discussed above :-/.

We want better ranges and better analysis.  Ultimately, it will
lead to better quality warnings, as has been one of the goals
behind Ranger.  If along the way it means a few false positives
in some edge cases, we'll deal with them.  I don't see this one
as a significant problem.

Martin