Message ID | b60ec149-2d7d-be35-272f-5e93577b852c@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v2] fix PR 103143 | expand |
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
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,