Re: [PATCH] fix up compute_objsize (including PR 103143)

[Jeff Law <jeffreyalaw@gmail.com>]

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