[PATCH] integrate sprintf pass into strlen (PR 83431)

[Martin Sebor]

...
>>> We've already touched on the need to limit the backwards walk out of
>>> this code.  Limiting based on the product of # phi args * number of phis
>>> visited, recursion depth, or number of SSA_NAME_DEF_STMTs visited all
>>> seem reasonable to me.
>>>
>>> I do think Richi's suggestion for figuring out a suitable inflection
>>> point is reasonable.
>>
>> It's easy enough to add here.  But I know I've introduced other
>> algorithms that recurse on SSA_NAME_DEF_STMT, and I'm quite sure
>> others predate those.  To make a difference I think we need to
>> review at least the one most likely to be exposed to this problem
>> and introduce the same limit there.  I could probably fix the ones
>> I wrote reasonably quickly, but making the change to the others
>> would be much more of a project.  I looked to see how pervasive
>> this might be and here is just a small sampling of things that
>> jumped out at me in a quick grep search:
>>
>>    *  compute_builtin_object_size (for _FORTIFY_SOURCE)
>>    *  compute_objsize (for -Wstringop-overflow)
>>    *  get_range_strlen
>>    *  maybe_fold_{and,or}_comparisons in gimple-fold.c
>>    *  -Warray-bounds (computing an offset into an object)
>>    *  -Wrestrict (computing an offset into an object)
>>    *  -Wreturn-local-addr (is_addr_local)
>>    *  -Wuninitialized (collect_phi_def_edges)
> 
> I don't see the recursion in maybe_fold_{and,or}_comparisons.

I didn't study any of these very carefully, I just looked for uses
of SSA_NAME_DEF_STMT around recursive calls.  same_bool_comparison_p
looks like it calls itself with that result.  I could be wrong.

> 
> The others all smell like they might be yours ;)  (besides object-size
> maybe but that one is limited by having a cache - hopefully also
> used when not used in the compute-everything mode).

It doesn't matter who introduced them.  What I'm saying is that
if the recursion is a problem it should be limited everywhere,
not just in one place.  A change with global scope like that would
best be made independently of other unrelated changes, to minimize
the risk of introducing bugs and then the difficulty of debugging
and fixing them.

>> Given how wide-spread this technique seems to be, if the recursion
>> is in fact a problem it's just as likely (if not more) to come up
>> in the folder or in BOS or some other place as it is here.  So if
>> it needs fixing it seems it should be done as its own project and
>> everywhere (or as close as we can get), and not as part of this
>> integration.
> 
> It's never an excuse to add new cases though and adding a limit
> is _really_ simple.

An "excuse?"  I'm just explaining that I would prefer to introduce
the limit independently of the integration.

Sure, it is simple in that one place.  I said that above.  It's
also probably relatively simple in each of the other instances
(at least those I'm familiar with).  It's just cleaner and safer
to add it independently of other changes, that's all.

Anyway, I posted a simple patch to introduce the limit.  I'm also
changing the integrated pass to make use of it once it's committed.
If it's considered necessary I'm also willing to adjust the rest
of the algorithms I introduced to respect the limit.

Thanks
Martin