[PATCH v2 4/5] fix up compute_objsize: refactor it into helpers

[Jeff Law]

On 12/6/2021 10:32 AM, Martin Sebor wrote:
> Attached is the subset of the patch in part (2) below: refactor
> compute_objsize_r into helpers.  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-4.diff
>
> commit 7d1d8cc18678ccaec5f274919e0c9910ccfea86e
> Author: Martin Sebor <msebor@redhat.com>
> Date:   Mon Dec 6 09:33:32 2021 -0700
>
>      Refactor compute_objsize_r into helpers.
>      
>      gcc/ChangeLog:
>      
>              * pointer-query.cc (compute_objsize_r): Add an argument.
>              (gimple_call_return_array): Pass a new argument to compute_objsize_r.
>              (access_ref::merge_ref): Same.
>              (access_ref::inform_access): Add an argument and use it.
>              (access_data::access_data): Initialize new member.
>              (handle_min_max_size): Pass a new argument to compute_objsize_r.
>              (handle_decl): New function.
>              (handle_array_ref): Pass a new argument to compute_objsize_r.
>              Avoid incrementing deref.
>              (set_component_ref_size): New function.
>              (handle_component_ref): New function.
>              (handle_mem_ref): Pass a new argument to compute_objsize_r.
>              Only increment deref after successfully computing object size.
>              (handle_ssa_name): New function.
>              (compute_objsize_r): Move code into helpers and call them.
>              (compute_objsize): Pass a new argument to compute_objsize_r.
>              * pointer-query.h (access_ref::inform_access): Add an argument.
>              (access_data::ostype): New member.
Thanks.  This was a bit uglier than I expected to review -- but I think 
that largely stems from the somewhat chaotic nature of the original code 
where we have fragments like

   block A

   block B

   block C

Where blocks A & C are closely related and you've refactored them into a 
common function.  Parts of B may stay while other parts end up in a 
different refactored function.  In and of itself that usually isn't too 
bad, but in this case git diff made a bit of a mess out of things making 
the rearrangements harder to follow.  I chased most stuff around and it 
all looked sensible.

OK.

Thanks,
Jeff