Re: [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling

[Martin Sebor <msebor@gmail.com>]

On 12/8/21 1:08 PM, Jeff Law wrote:
> 
> 
> On 12/6/2021 10:32 AM, Martin Sebor wrote:
>> Attached is subset of the patch in part (4) below: factor out
>> PHI handling.  It applies on top of patch 3/5.
>>
>> On 12/3/21 5:00 PM, Jeff Law wrote:
>>>
>>>
>>> On 11/8/2021 7:34 PM, Martin Sebor via Gcc-patches wrote:
>>>> The pointer-query code that implements compute_objsize() that's
>>>> in turn used by most middle end access warnings now has a few
>>>> warts in it and (at least) one bug.  With the exception of
>>>> the bug the warts aren't behind any user-visible bugs that
>>>> I know of but they do cause problems in new code I've been
>>>> implementing on top of it.  Besides fixing the one bug (just
>>>> a typo) the attached patch cleans up these latent issues:
>>>>
>>>> 1) It moves the bndrng member from the access_ref class to
>>>>    access_data.  As a FIXME in the code notes, the member never
>>>>    did belong in the former and only takes up space in the cache.
>>>>
>>>> 2) The compute_objsize_r() function is big, unwieldy, and tedious
>>>>    to step through because of all the if statements that are better
>>>>    coded as one switch statement.  This change factors out more
>>>>    of its code into smaller handler functions as has been suggested
>>>>    and done a few times before.
>>>>
>>>> 3) (2) exposed a few places where I fail to pass the current
>>>>    GIMPLE statement down to ranger.  This leads to worse quality
>>>>    range info, including possible false positives and negatives.
>>>>    I just spotted these problems in code review but I haven't
>>>>    taken the time to come up with test cases.  This change fixes
>>>>    these oversights as well.
>>>>
>>>> 4) The handling of PHI statements is also in one big, hard-to-
>>>>    follow function.  This change moves the handling of each PHI
>>>>    argument into its own handler which merges it into the previous
>>>>    argument.  This makes the code easier to work with and opens it
>>>>    to reuse also for MIN_EXPR and MAX_EXPR.  (This is primarily
>>>>    used to print informational notes after warnings.)
>>>>
>>>> 5) Finally, the patch factors code to dump each access_ref
>>>>    cached by the pointer_query cache out of pointer_query::dump
>>>>    and into access_ref::dump.  This helps with debugging.
>>>>
>>>> These changes should have no user-visible effect and other than
>>>> a regression test for the typo (PR 103143) come with no tests.
>>>> They've been tested on x86_64-linux.
>>> Sigh.  You've identified 6 distinct changes above.  The 5 you've 
>>> enumerated plus a typo fix somewhere.  There's no reason why they 
>>> need to be a single patch and many reasons why they should be a 
>>> series of independent patches.    Combining them into a single patch 
>>> isn't how we do things and it hides the actual bugfix in here.
>>>
>>> Please send a fix for the typo first since that should be able to 
>>> trivially go forward.  Then  a patch for item #1.  That should be 
>>> trivial to review when it's pulled out from teh rest of the patch. 
>>> Beyond that, your choice on ordering, but you need to break this down.
>>>
>>>
>>>
>>>
>>> Jeff
>>>
>>
>>
>> gcc-pointer_query-refactor-3.diff
>>
>> commit 6ac1d37947ad5cf07fe133faaf8414f00e0eed13
>> Author: Martin Sebor <msebor@redhat.com>
>> Date:   Mon Dec 6 09:23:22 2021 -0700
>>
>>      Introduce access_ref::merge_ref.
>>      gcc/ChangeLog:
>>              * pointer-query.cc (access_ref::merge_ref): Define new 
>> function.
>>              (access_ref::get_ref): Move code into merge_ref and call it.
>>              * pointer-query.h (access_ref::merge_ref): Declare new 
>> function.
> OK.  But it's probably worth noting that this patch does more than just 
> factoring out the PHI handling.  It also adds the MIN/MAX bits noted in 
> the original cover letter.   That's not inherently as bad now that this 
> patch isn't intermixed with the other work.

Thank you for the review.

The MIN_MAX change was in the original ChangeLog but I wrote this
one from scratch and neglected to mention it here.  Let me add it.

>>
>> diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
>> index c75c4da6b60..24fbac84ec4 100644
>> --- a/gcc/pointer-query.cc
>> +++ b/gcc/pointer-query.cc
>>
>>
>>
>>
>> @ -766,7 +818,14 @@ access_ref::get_ref (vec<access_ref> *all_refs,
>>     /* Avoid changing *THIS.  */
>>     if (pref && pref != this)
>> -    *pref = phi_ref;
>> +    {
>> +      /* Keep the SSA_NAME of the PHI unchanged so that all PHI 
>> arguments
>> +     can be referred to later if necessary.  This is useful even if
>> +     they all refer to the same object.  */
>> +      tree ref = pref->ref;
>> +      *pref = phi_ref;
>> +      pref->ref = ref;
>> +    }
> I don't see any mention of this in the ChangeLog.

This only matters for informational notes, and it's necessary
because the new merge_ref() function might replace the ref
member with that of the "merged" object.  It's needed to keep
the existing behavior where we want the informational notes
printed after a warning to point to all the objects that might
be subject to the out of bounds access.  This is verified by
the Wstringop-overflow-58.c and -59.c tests.

> So I'm fine with the patch itself.  I would just ask for a better 
> ChangeLog.  If one was to read the current ChangeLog they could easily 
> be led to believe this patch was just refactoring, but it brings in 
> other changes as well.

It wasn't my intention to change any observable behavior with
this change, and I don't think it does.

I was going to update the ChangeLog to mention the MIN/MAX part
above but I just ended up pushing it to the main repository by
mistake.

Martin