Re: [PATCH] run early sprintf warning after SSA (PR 100325)

[Aldy Hernandez <aldyh@redhat.com>]

On 5/7/21 11:34 AM, Richard Biener wrote:
> On Fri, May 7, 2021 at 2:12 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 5/6/21 8:32 AM, Aldy Hernandez wrote:
>>>
>>>
>>> On 5/5/21 9:26 AM, Richard Biener wrote:
>>>> On Wed, May 5, 2021 at 1:32 AM Martin Sebor via Gcc-patches
>>>> <gcc-patches@gcc.gnu.org> wrote:
>>>>>
>>>>> With no optimization, -Wformat-overflow and -Wformat-truncation
>>>>> runs early to detect a subset of simple bugs.  But as it turns out,
>>>>> the pass runs just a tad too early, before SSA.  That causes it to
>>>>> miss a class of problems that can easily be detected once code is
>>>>> in SSA form, and I would expect might also cause false positives.
>>>>>
>>>>> The attached change moves the sprintf pass just after pass_build_ssa,
>>>>> similar to other early flow-sensitive warnings (-Wnonnull-compare and
>>>>> -Wuninitialized).
>>>>
>>>> Makes sense.  I suppose walloca might also benefit from SSA - it seems
>>>> to do range queries which won't work quite well w/o SSA?
>>>
>>> The early Walloca pass that runs without optimization doesn't do much,
>>> as we've never had ranges so early.  All it does is diagnose _every_
>>> call to alloca(), if -Walloca is passed:
>>>
>>>     // The first time this pass is called, it is called before
>>>     // optimizations have been run and range information is unavailable,
>>>     // so we can only perform strict alloca checking.
>>>     if (first_time_p)
>>>       return warn_alloca != 0;
>>>
>>> Though, I suppose we could move the first alloca pass after SSA is
>>> available and make it the one and only pass, since ranger only needs
>>> SSA.  However, I don't know how well this would work without value
>>> numbering or CSE.  For example, for gcc.dg/Walloca-4.c the gimple is:
>>>
>>>     <bb 2> :
>>>     _1 = rear_ptr_9(D) - w_10(D);
>>>     _2 = (long unsigned int) _1;
>>>     if (_2 <= 4095)
>>>       goto <bb 3>; [INV]
>>>     else
>>>       goto <bb 4>; [INV]
>>>
>>>     <bb 3> :
>>>     _3 = rear_ptr_9(D) - w_10(D);
>>>     _4 = (long unsigned int) _3;
>>>     src_16 = __builtin_alloca (_4);
>>>     goto <bb 5>; [INV]
>>>
>>> No ranges can be determined for _4.  However, if either FRE or DOM run,
>>> as they do value numbering and CSE respectively, we could easily
>>> determine a range as the above would become:
>>>
>>>    <bb 2> :
>>>     _1 = rear_ptr_9(D) - w_10(D);
>>>     _2 = (long unsigned int) _1;
>>>     if (_2 <= 4095)
>>>       goto <bb 3>; [INV]
>>>     else
>>>       goto <bb 4>; [INV]
>>>
>>>     <bb 3> :
>>>     src_16 = __builtin_alloca (_2);
>>>     goto <bb 5>; [INV]
>>>
>>> I'm inclined to leave the first alloca pass before SSA runs, as it
>>> doesn't do anything with ranges.  If anyone's open to a simple -O0 CSE
>>> type pass, it would be a different story.  Thoughts?
>>
>> Improving the analysis at -O0 and getting better warnings that are
>> more consistent with what is issued with optimization would be very
>> helpful (as as long as it doesn't compromise debugging experience
>> of course).
> 
> I agree.  It shouldn't be too difficult to for example run the VN
> propagation part without doing actual elimiation and keep
> value-numbers for consumption.  do_rpo_vn (not exported)
> might even already support iterate = false, eliminate = false,
> it would just need factoring out the init/deinit somewhat.

Interesting.  This could give good ranges at -O0 and make it possible to 
move all these pesky range needy passes early in the pipeline.

> Of course it will be a lot more expensive to do since it cannot
> do "on-demand" value-numbering of interesting SSA names.
> I'm not sure that would be possible anyhow.  Though for
> the alloca case quickly scanning the function whether there's
> any would of course be faster than throwing VN at it.

That's exact what we do for strict -Walloca warnings.  For 
-Walloca-larger-than=, you need ranges though, so your VN idea would fit 
the bill.

Aldy