diff mbox series

[v2,2/5] fix up compute_objsize: pass GIMPLE statement to it

Message ID b60ec149-2d7d-be35-272f-5e93577b852c@gmail.com
State New
Headers
Series [v2] fix PR 103143 |

Commit Message

Martin Sebor Dec. 6, 2021, 5:31 p.m. UTC 
  Attached is the subset of the patch in part (3) below: Pass
GIMPLE statement to compute_objsize.  It applies on top of
patch 1/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
>

Comments

Jeff Law Dec. 8, 2021, 6:48 p.m. UTC | #1 
On 12/6/2021 10:31 AM, Martin Sebor wrote:
> Attached is the subset of the patch in part (3) below: Pass
> GIMPLE statement to compute_objsize.  It applies on top of
> patch 1/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-2.diff
>
> commit 64d34f249fb98d53ae8fcb3b36b01c2b9b05ea4f
> Author: Martin Sebor <msebor@redhat.com>
> Date:   Sat Dec 4 16:57:48 2021 -0700
>
>      Pass GIMPLE statement to compute_objsize.
>      
>      gcc/ChangeLog:
>      
>              * gimple-ssa-warn-restrict.c (builtin_access::builtin_access): Pass
>              GIMPLE statement to compute_objsize.
>              * pointer-query.cc (compute_objsize): Add a statement argument.
>              * pointer-query.h (compute_objsize): Define a new overload.
OK
jeff
diff mbox series

Patch

commit 64d34f249fb98d53ae8fcb3b36b01c2b9b05ea4f
Author: Martin Sebor <msebor@redhat.com>
Date:   Sat Dec 4 16:57:48 2021 -0700

    Pass GIMPLE statement to compute_objsize.
    
    gcc/ChangeLog:
    
            * gimple-ssa-warn-restrict.c (builtin_access::builtin_access): Pass
            GIMPLE statement to compute_objsize.
            * pointer-query.cc (compute_objsize): Add a statement argument.
            * pointer-query.h (compute_objsize): Define a new overload.

diff --git a/gcc/gimple-ssa-warn-restrict.c b/gcc/gimple-ssa-warn-restrict.c
index d1df9ca8d4f..ca2d4c2c32e 100644
--- a/gcc/gimple-ssa-warn-restrict.c
+++ b/gcc/gimple-ssa-warn-restrict.c
@@ -777,7 +777,7 @@  builtin_access::builtin_access (range_query *query, gimple *call,
       if (!POINTER_TYPE_P (TREE_TYPE (addr)))
 	addr = build1 (ADDR_EXPR, (TREE_TYPE (addr)), addr);
 
-      if (tree dstsize = compute_objsize (addr, ostype))
+      if (tree dstsize = compute_objsize (addr, call, ostype))
 	dst.basesize = wi::to_offset (dstsize);
       else if (POINTER_TYPE_P (TREE_TYPE (addr)))
 	dst.basesize = HOST_WIDE_INT_MIN;
@@ -791,7 +791,7 @@  builtin_access::builtin_access (range_query *query, gimple *call,
       if (!POINTER_TYPE_P (TREE_TYPE (addr)))
 	addr = build1 (ADDR_EXPR, (TREE_TYPE (addr)), addr);
 
-      if (tree srcsize = compute_objsize (addr, ostype))
+      if (tree srcsize = compute_objsize (addr, call, ostype))
 	src.basesize = wi::to_offset (srcsize);
       else if (POINTER_TYPE_P (TREE_TYPE (addr)))
 	src.basesize = HOST_WIDE_INT_MIN;
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index a8c9671e3ba..c75c4da6b60 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -1645,7 +1645,7 @@  handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
   offset_int orng[2];
   tree off = pref->eval (TREE_OPERAND (aref, 1));
   range_query *const rvals = qry ? qry->rvals : NULL;
-  if (!get_offset_range (off, NULL, orng, rvals))
+  if (!get_offset_range (off, stmt, orng, rvals))
     {
       /* Set ORNG to the maximum offset representable in ptrdiff_t.  */
       orng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
@@ -1732,7 +1732,7 @@  handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
   offset_int orng[2];
   tree off = pref->eval (TREE_OPERAND (mref, 1));
   range_query *const rvals = qry ? qry->rvals : NULL;
-  if (!get_offset_range (off, NULL, orng, rvals))
+  if (!get_offset_range (off, stmt, orng, rvals))
     {
       /* Set ORNG to the maximum offset representable in ptrdiff_t.  */
       orng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
@@ -1948,7 +1948,7 @@  compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref,
 
       offset_int orng[2];
       tree off = pref->eval (TREE_OPERAND (ptr, 1));
-      if (get_offset_range (off, NULL, orng, rvals))
+      if (get_offset_range (off, stmt, orng, rvals))
 	pref->add_offset (orng[0], orng[1]);
       else
 	pref->add_max_offset ();
@@ -2197,13 +2197,13 @@  compute_objsize (tree ptr, gimple *stmt, int ostype, access_ref *pref,
    once callers transition to one of the two above.  */
 
 tree
-compute_objsize (tree ptr, int ostype, tree *pdecl /* = NULL */,
+compute_objsize (tree ptr, gimple *stmt, int ostype, tree *pdecl /* = NULL */,
 		 tree *poff /* = NULL */, range_query *rvals /* = NULL */)
 {
   /* Set the initial offsets to zero and size to negative to indicate
      none has been computed yet.  */
   access_ref ref;
-  tree size = compute_objsize (ptr, nullptr, ostype, &ref, rvals);
+  tree size = compute_objsize (ptr, stmt, ostype, &ref, rvals);
   if (!size || !ref.base0)
     return NULL_TREE;
 
diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h
index 82cb81b3987..cbc87c86ed3 100644
--- a/gcc/pointer-query.h
+++ b/gcc/pointer-query.h
@@ -260,8 +260,13 @@  inline tree compute_objsize (tree ptr, int ostype, access_ref *pref)
 }
 
 /* Legacy/transitional API.  Should not be used in new code.  */
-extern tree compute_objsize (tree, int, tree * = nullptr, tree * = nullptr,
-			     range_query * = nullptr);
+extern tree compute_objsize (tree, gimple *, int, tree * = nullptr,
+			     tree * = nullptr, range_query * = nullptr);
+inline tree compute_objsize (tree ptr, int ostype, tree *pdecl = nullptr,
+			     tree *poff = nullptr, range_query *rvals = nullptr)
+{
+  return compute_objsize (ptr, nullptr, ostype, pdecl, poff, rvals);
+}
 
 /* Return the field at the constant offset.  */
 extern tree field_at_offset (tree, tree, HOST_WIDE_INT,