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


commit 64d34f249fb98d53ae8fcb3b36b01c2b9b05ea4f
Author: Martin Sebor <mse...@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,

Reply via email to