* [PATCH] fix up compute_objsize (including PR 103143)
@ 2021-11-09 2:34 Martin Sebor
2021-11-15 16:49 ` PING " Martin Sebor
2021-12-04 0:00 ` Jeff Law
0 siblings, 2 replies; 19+ messages in thread
From: Martin Sebor @ 2021-11-09 2:34 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]
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.
Martin
[-- Attachment #2: gcc-pointer-query-refactor.diff --]
[-- Type: text/x-patch, Size: 62431 bytes --]
Improve compute_objsize() and fix PR 103143.
gcc/ChangeLog:
PR middle-end/103143
* gimple-ssa-warn-access.cc (check_access): Use access_data members.
(pass_waccess::check_stxncpy): Set access_data::ostype.
(pass_waccess::check_strncmp): Use access_data members.
* gimple-ssa-warn-restrict.c (builtin_access::builtin_access): Pass
compute_objsize the call statement as an argument.
* pointer-query.cc (compute_objsize_r): Add an argument.
(gimple_call_return_array): Pass compute_objsize_r a new argument.
(access_ref::access_ref): Move code to access_data::set_bound.
(access_ref::merge_ref): New function.
(access_ref::get_ref): Call it.
(access_ref::inform_access): Add an argument.
(access_ref::dump): New function.
(access_data::access_data): Call set_bound.
(access_data::set_bound): New function.
(pointer_query::get_ref): Factor out code into merge_ref. Handle
MIN_EXPR and MAX_EXPR.
(pointer_query::dump): Factor code out into access_ref::dump.
(handle_min_max_size): Pass a new argument to compute_objsize_r.
(handle_decl): New function.
(handle_array_ref): Avoid incrementing deref. Pass a new argument
to compute_objsize_r.
(set_component_ref_size): New function.
(handle_component_ref): New function.
(handle_mem_ref): Pass a new argument to compute_objsize_r.
Increment deref after calling it, not before.
(handle_ssa_name): New function.
(compute_objsize): Call compute_objsize_r with a new argument.
* pointer-query.h (struct access_ref): Move members to access_data.
(struct access_data): Add members.
(compute_objsize): Declare a new overload.
gcc/testsuite/ChangeLog:
* c-c++-common/Wstringop-overflow-2.c: Avoid vectorization.
PR middle-end/103143
* gcc.dg/Wstringop-overflow-83.c: New test.
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 63fc27a1487..7f225a30a8c 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -1337,10 +1337,10 @@ check_access (GimpleOrTree exp, tree dstwrite,
if (!dstsize)
dstsize = maxobjsize;
- /* Set RANGE to that of DSTWRITE if non-null, bounded by PAD->DST.BNDRNG
+ /* Set RANGE to that of DSTWRITE if non-null, bounded by PAD->DST_BNDRNG
if valid. */
gimple *stmt = pad ? pad->stmt : nullptr;
- get_size_range (rvals, dstwrite, stmt, range, pad ? pad->dst.bndrng : NULL);
+ get_size_range (rvals, dstwrite, stmt, range, pad ? pad->dst_bndrng : NULL);
tree func = get_callee_fndecl (exp);
/* Read vs write access by built-ins can be determined from the const
@@ -1419,7 +1419,7 @@ check_access (GimpleOrTree exp, tree dstwrite,
{
suppress_warning (exp, OPT_Wstringop_overflow_);
if (pad)
- pad->dst.inform_access (pad->mode);
+ pad->dst.inform_access (pad->mode, pad->ostype);
}
/* Return error when an overflow has been detected. */
@@ -1432,9 +1432,9 @@ check_access (GimpleOrTree exp, tree dstwrite,
of an object. */
if (maxread)
{
- /* Set RANGE to that of MAXREAD, bounded by PAD->SRC.BNDRNG if
+ /* Set RANGE to that of MAXREAD, bounded by PAD->SRC_BNDRNG if
PAD is nonnull and BNDRNG is valid. */
- get_size_range (rvals, maxread, stmt, range, pad ? pad->src.bndrng : NULL);
+ get_size_range (rvals, maxread, stmt, range, pad ? pad->src_bndrng : NULL);
location_t loc = get_location (exp);
tree size = dstsize;
@@ -1479,12 +1479,12 @@ check_access (GimpleOrTree exp, tree dstwrite,
&& (pad->src.offrng[1] < 0
|| pad->src.offrng[0] <= pad->src.offrng[1]))
{
- /* Set RANGE to that of MAXREAD, bounded by PAD->SRC.BNDRNG if
+ /* Set RANGE to that of MAXREAD, bounded by PAD->SRC_BNDRNG if
PAD is nonnull and BNDRNG is valid. */
- get_size_range (rvals, maxread, stmt, range, pad ? pad->src.bndrng : NULL);
+ get_size_range (rvals, maxread, stmt, range, pad ? pad->src_bndrng : NULL);
/* Set OVERREAD for reads starting just past the end of an object. */
- overread = pad->src.sizrng[1] - pad->src.offrng[0] < pad->src.bndrng[0];
- range[0] = wide_int_to_tree (sizetype, pad->src.bndrng[0]);
+ overread = pad->src.sizrng[1] - pad->src.offrng[0] < pad->src_bndrng[0];
+ range[0] = wide_int_to_tree (sizetype, pad->src_bndrng[0]);
slen = size_zero_node;
}
@@ -2525,9 +2525,10 @@ pass_waccess::check_stxncpy (gcall *stmt)
/* The number of bytes to write (not the maximum). */
tree len = call_arg (stmt, 2);
- access_data data (m_ptr_qry.rvals, stmt, access_read_write, len, true, len,
- true);
const int ost = warn_stringop_overflow ? warn_stringop_overflow - 1 : 1;
+ access_data data (m_ptr_qry.rvals, stmt, access_read_write,
+ len, true, len, true);
+ data.ostype = ost;
compute_objsize (src, stmt, ost, &data.src, &m_ptr_qry);
tree dstsize = compute_objsize (dst, stmt, ost, &data.dst, &m_ptr_qry);
@@ -2590,7 +2591,7 @@ pass_waccess::check_strncmp (gcall *stmt)
/* Determine the range of the bound first and bail if it fails; it's
cheaper than computing the size of the objects. */
tree bndrng[2] = { NULL_TREE, NULL_TREE };
- get_size_range (m_ptr_qry.rvals, bound, stmt, bndrng, adata1.src.bndrng);
+ get_size_range (m_ptr_qry.rvals, bound, stmt, bndrng, adata1.src_bndrng);
if (!bndrng[0] || integer_zerop (bndrng[0]))
return;
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 a0e4543d8a3..873cb66b17e 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -43,7 +43,7 @@
#include "tree-ssanames.h"
#include "target.h"
-static bool compute_objsize_r (tree, gimple *, int, access_ref *,
+static bool compute_objsize_r (tree, gimple *, bool, int, access_ref *,
ssa_name_limit_t &, pointer_query *);
/* Wrapper around the wide_int overload of get_range that accepts
@@ -199,7 +199,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
of the source object. */
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize (src, stmt, 1, &aref, qry)
+ if (compute_objsize_r (src, stmt, false, 1, &aref, snlim, qry)
&& aref.sizrng[1] < offrng[1])
offrng[1] = aref.sizrng[1];
}
@@ -233,7 +233,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
{
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry))
+ if (compute_objsize_r (src, stmt, false, 1, &aref, snlim, qry))
offrng[1] = aref.sizrng[1] - 1;
else
offrng[1] = HOST_WIDE_INT_M1U;
@@ -258,7 +258,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
of the source object. */
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry)
+ if (compute_objsize_r (src, stmt, false, 1, &aref, snlim, qry)
&& aref.sizrng[1] < offrng[1])
offrng[1] = aref.sizrng[1];
}
@@ -596,15 +596,7 @@ gimple_parm_array_size (tree ptr, wide_int rng[2],
return var;
}
-/* Given a statement STMT, set the bounds of the reference to at most
- as many bytes as BOUND or unknown when null, and at least one when
- the MINACCESS is true unless BOUND is a constant zero. STMT is
- used for context to get accurate range info. */
-
-access_ref::access_ref (range_query *qry /* = nullptr */,
- tree bound /* = NULL_TREE */,
- gimple *stmt /* = nullptr */,
- bool minaccess /* = false */)
+access_ref::access_ref ()
: ref (), eval ([](tree x){ return x; }), deref (), trail1special (true),
base0 (true), parmarray ()
{
@@ -613,21 +605,6 @@ access_ref::access_ref (range_query *qry /* = nullptr */,
offmax[0] = offmax[1] = 0;
/* Invalidate. */
sizrng[0] = sizrng[1] = -1;
-
- /* Set the default bounds of the access and adjust below. */
- bndrng[0] = minaccess ? 1 : 0;
- bndrng[1] = HOST_WIDE_INT_M1U;
-
- /* When BOUND is nonnull and a range can be extracted from it,
- set the bounds of the access to reflect both it and MINACCESS.
- BNDRNG[0] is the size of the minimum access. */
- tree rng[2];
- if (bound && get_size_range (qry, bound, stmt, rng, SR_ALLOW_ZERO))
- {
- bndrng[0] = wi::to_offset (rng[0]);
- bndrng[1] = wi::to_offset (rng[1]);
- bndrng[0] = bndrng[0] > 0 && minaccess ? 1 : 0;
- }
}
/* Return the PHI node REF refers to or null if it doesn't. */
@@ -645,6 +622,97 @@ access_ref::phi () const
return as_a <gphi *> (def_stmt);
}
+/* Determine the size and offset for ARG, append it to ALL_REFS, and
+ merge the result with *THIS. Ignore ARG if SKIP_NULL is set and
+ ARG refers to the null pointer. Return true on success and false
+ on failure. */
+
+bool
+access_ref::merge_ref (vec<access_ref> *all_refs, tree arg, gimple *stmt,
+ int ostype, bool skip_null,
+ ssa_name_limit_t &snlim, pointer_query &qry)
+{
+ access_ref aref;
+ if (!compute_objsize_r (arg, stmt, false, ostype, &aref, snlim, &qry)
+ || aref.sizrng[0] < 0)
+ /* This may be a PHI with all null pointer arguments. */
+ return false;
+
+ if (all_refs)
+ {
+ access_ref dummy_ref;
+ aref.get_ref (all_refs, &dummy_ref, ostype, &snlim, &qry);
+ }
+
+ if (TREE_CODE (arg) == SSA_NAME)
+ qry.put_ref (arg, aref, ostype);
+
+ if (all_refs)
+ all_refs->safe_push (aref);
+
+ aref.deref += deref;
+
+ bool merged_parmarray = aref.parmarray;
+
+ const bool nullp = skip_null && integer_zerop (arg);
+ const offset_int maxobjsize = wi::to_offset (max_object_size ());
+ offset_int minsize = sizrng[0];
+
+ if (sizrng[0] < 0)
+ {
+ /* If *THIS doesn't contain a meaningful result yet set it to AREF
+ unless the argument is null and it's okay to ignore it. */
+ if (!nullp)
+ *this = aref;
+
+ /* Set if the current argument refers to one or more objects of
+ known size (or range of sizes), as opposed to referring to
+ one or more unknown object(s). */
+ const bool arg_known_size = (aref.sizrng[0] != 0
+ || aref.sizrng[1] != maxobjsize);
+ if (arg_known_size)
+ sizrng[0] = aref.sizrng[0];
+
+ return true;
+ }
+
+ /* Disregard null pointers in PHIs with two or more arguments.
+ TODO: Handle this better! */
+ if (nullp)
+ return true;
+
+ const bool known_size = (sizrng[0] != 0 || sizrng[1] != maxobjsize);
+
+ if (known_size && aref.sizrng[0] < minsize)
+ minsize = aref.sizrng[0];
+
+ /* Determine the amount of remaining space in the argument. */
+ offset_int argrem[2];
+ argrem[1] = aref.size_remaining (argrem);
+
+ /* Determine the amount of remaining space computed so far and
+ if the remaining space in the argument is more use it instead. */
+ offset_int merged_rem[2];
+ merged_rem[1] = size_remaining (merged_rem);
+
+ /* Reset the PHI's BASE0 flag if any of the nonnull arguments
+ refers to an object at an unknown offset. */
+ if (!aref.base0)
+ base0 = false;
+
+ if (merged_rem[1] < argrem[1]
+ || (merged_rem[1] == argrem[1]
+ && sizrng[1] < aref.sizrng[1]))
+ /* Use the argument with the most space remaining as the result,
+ or the larger one if the space is equal. */
+ *this = aref;
+
+ sizrng[0] = minsize;
+ parmarray = merged_parmarray;
+
+ return true;
+}
+
/* Determine and return the largest object to which *THIS refers. If
*THIS refers to a PHI and PREF is nonnull, fill *PREF with the details
of the object determined by compute_objsize(ARG, OSTYPE) for each PHI
@@ -657,9 +725,8 @@ access_ref::get_ref (vec<access_ref> *all_refs,
ssa_name_limit_t *psnlim /* = NULL */,
pointer_query *qry /* = NULL */) const
{
- gphi *phi_stmt = this->phi ();
- if (!phi_stmt)
- return ref;
+ if (!ref || TREE_CODE (ref) != SSA_NAME)
+ return NULL;
/* FIXME: Calling get_ref() with a null PSNLIM is dangerous and might
cause unbounded recursion. */
@@ -667,13 +734,49 @@ access_ref::get_ref (vec<access_ref> *all_refs,
if (!psnlim)
psnlim = &snlim_buf;
- if (!psnlim->visit_phi (ref))
- return NULL_TREE;
-
pointer_query empty_qry;
if (!qry)
qry = &empty_qry;
+ if (gimple *def_stmt = SSA_NAME_DEF_STMT (ref))
+ {
+ if (is_gimple_assign (def_stmt))
+ {
+ tree_code code = gimple_assign_rhs_code (def_stmt);
+ if (code != MIN_EXPR && code != MAX_EXPR)
+ return NULL_TREE;
+
+ access_ref aref;
+ tree arg1 = gimple_assign_rhs1 (def_stmt);
+ if (!aref.merge_ref (all_refs, arg1, def_stmt, ostype, false,
+ *psnlim, *qry))
+ return NULL_TREE;
+
+ tree arg2 = gimple_assign_rhs2 (def_stmt);
+ if (!aref.merge_ref (all_refs, arg2, def_stmt, ostype, false,
+ *psnlim, *qry))
+ return NULL_TREE;
+
+ if (pref && pref != this)
+ {
+ tree ref = pref->ref;
+ *pref = aref;
+ pref->ref = ref;
+ }
+
+ return aref.ref;
+ }
+ }
+ else
+ return NULL_TREE;
+
+ gphi *phi_stmt = this->phi ();
+ if (!phi_stmt)
+ return ref;
+
+ if (!psnlim->visit_phi (ref))
+ return NULL_TREE;
+
/* The conservative result of the PHI reflecting the offset and size
of the largest PHI argument, regardless of whether or not they all
refer to the same object. */
@@ -691,91 +794,17 @@ access_ref::get_ref (vec<access_ref> *all_refs,
phi_ref = *pref;
}
- /* Set if any argument is a function array (or VLA) parameter not
- declared [static]. */
- bool parmarray = false;
- /* The size of the smallest object referenced by the PHI arguments. */
- offset_int minsize = 0;
- const offset_int maxobjsize = wi::to_offset (max_object_size ());
-
const unsigned nargs = gimple_phi_num_args (phi_stmt);
for (unsigned i = 0; i < nargs; ++i)
{
access_ref phi_arg_ref;
+ bool skip_null = i || i + 1 < nargs;
tree arg = gimple_phi_arg_def (phi_stmt, i);
- if (!compute_objsize_r (arg, phi_stmt, ostype, &phi_arg_ref, *psnlim,
- qry)
- || phi_arg_ref.sizrng[0] < 0)
- /* A PHI with all null pointer arguments. */
+ if (!phi_ref.merge_ref (all_refs, arg, phi_stmt, ostype, skip_null,
+ *psnlim, *qry))
return NULL_TREE;
-
- if (TREE_CODE (arg) == SSA_NAME)
- qry->put_ref (arg, phi_arg_ref);
-
- if (all_refs)
- all_refs->safe_push (phi_arg_ref);
-
- parmarray |= phi_arg_ref.parmarray;
-
- const bool nullp = integer_zerop (arg) && (i || i + 1 < nargs);
-
- if (phi_ref.sizrng[0] < 0)
- {
- /* If PHI_REF doesn't contain a meaningful result yet set it
- to the result for the first argument. */
- if (!nullp)
- phi_ref = phi_arg_ref;
-
- /* Set if the current argument refers to one or more objects of
- known size (or range of sizes), as opposed to referring to
- one or more unknown object(s). */
- const bool arg_known_size = (phi_arg_ref.sizrng[0] != 0
- || phi_arg_ref.sizrng[1] != maxobjsize);
- if (arg_known_size)
- minsize = phi_arg_ref.sizrng[0];
-
- continue;
- }
-
- const bool phi_known_size = (phi_ref.sizrng[0] != 0
- || phi_ref.sizrng[1] != maxobjsize);
-
- if (phi_known_size && phi_arg_ref.sizrng[0] < minsize)
- minsize = phi_arg_ref.sizrng[0];
-
- /* Disregard null pointers in PHIs with two or more arguments.
- TODO: Handle this better! */
- if (nullp)
- continue;
-
- /* Determine the amount of remaining space in the argument. */
- offset_int argrem[2];
- argrem[1] = phi_arg_ref.size_remaining (argrem);
-
- /* Determine the amount of remaining space computed so far and
- if the remaining space in the argument is more use it instead. */
- offset_int phirem[2];
- phirem[1] = phi_ref.size_remaining (phirem);
-
- /* Reset the PHI's BASE0 flag if any of the nonnull arguments
- refers to an object at an unknown offset. */
- if (!phi_arg_ref.base0)
- phi_ref.base0 = false;
-
- if (phirem[1] < argrem[1]
- || (phirem[1] == argrem[1]
- && phi_ref.sizrng[1] < phi_arg_ref.sizrng[1]))
- /* Use the argument with the most space remaining as the result,
- or the larger one if the space is equal. */
- phi_ref = phi_arg_ref;
}
- /* Replace the lower bound of the largest argument with the size
- of the smallest argument, and set PARMARRAY if any argument
- was one. */
- phi_ref.sizrng[0] = minsize;
- phi_ref.parmarray = parmarray;
-
if (phi_ref.sizrng[0] < 0)
{
/* Fail if none of the PHI's arguments resulted in updating PHI_REF
@@ -787,7 +816,14 @@ access_ref::get_ref (vec<access_ref> *all_refs,
/* Avoid changing *THIS. */
if (pref && pref != this)
- *pref = phi_ref;
+ {
+ /* Keep the SSA_NAME of the PHI unchanged so that all PHI arguments
+ can be referred to later if necessary. This is useful even if
+ they all refer to the same object. */
+ tree ref = pref->ref;
+ *pref = phi_ref;
+ pref->ref = ref;
+ }
psnlim->leave_phi (ref);
@@ -959,42 +995,45 @@ void access_ref::add_offset (const offset_int &min, const offset_int &max)
WRITE is set for a write access and clear for a read access. */
void
-access_ref::inform_access (access_mode mode) const
+access_ref::inform_access (access_mode mode, int ostype /* = 1 */) const
{
const access_ref &aref = *this;
if (!aref.ref)
return;
- if (aref.phi ())
+ if (phi ())
{
/* Set MAXREF to refer to the largest object and fill ALL_REFS
with data for all objects referenced by the PHI arguments. */
access_ref maxref;
auto_vec<access_ref> all_refs;
- if (!get_ref (&all_refs, &maxref))
+ if (!get_ref (&all_refs, &maxref, ostype))
return;
- /* Except for MAXREF, the rest of the arguments' offsets need not
- reflect one added to the PHI itself. Determine the latter from
- MAXREF on which the result is based. */
- const offset_int orng[] =
+ if (all_refs.length ())
{
- offrng[0] - maxref.offrng[0],
- wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
- };
+ /* Except for MAXREF, the rest of the arguments' offsets need not
+ reflect one added to the PHI itself. Determine the latter from
+ MAXREF on which the result is based. */
+ const offset_int orng[] =
+ {
+ offrng[0] - maxref.offrng[0],
+ wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
+ };
- /* Add the final PHI's offset to that of each of the arguments
- and recurse to issue an inform message for it. */
- for (unsigned i = 0; i != all_refs.length (); ++i)
- {
- /* Skip any PHIs; those could lead to infinite recursion. */
- if (all_refs[i].phi ())
- continue;
+ /* Add the final PHI's offset to that of each of the arguments
+ and recurse to issue an inform message for it. */
+ for (unsigned i = 0; i != all_refs.length (); ++i)
+ {
+ /* Skip any PHIs; those could lead to infinite recursion. */
+ if (all_refs[i].phi ())
+ continue;
- all_refs[i].add_offset (orng[0], orng[1]);
- all_refs[i].inform_access (mode);
+ all_refs[i].add_offset (orng[0], orng[1]);
+ all_refs[i].inform_access (mode, ostype);
+ }
+ return;
}
- return;
}
/* Convert offset range and avoid including a zero range since it
@@ -1199,6 +1238,111 @@ access_ref::inform_access (access_mode mode) const
sizestr, allocfn);
}
+/* Dump *THIS to FILE. */
+
+void
+access_ref::dump (FILE *file) const
+{
+ for (int i = deref; i < 0; ++i)
+ fputc ('&', file);
+
+ for (int i = 0; i < deref; ++i)
+ fputc ('*', file);
+
+ if (gphi *phi_stmt = phi ())
+ {
+ fputs ("PHI <", file);
+ unsigned nargs = gimple_phi_num_args (phi_stmt);
+ for (unsigned i = 0; i != nargs; ++i)
+ {
+ tree arg = gimple_phi_arg_def (phi_stmt, i);
+ print_generic_expr (file, arg);
+ if (i + 1 < nargs)
+ fputs (", ", file);
+ }
+ fputc ('>', file);
+ }
+ else
+ print_generic_expr (file, ref);
+
+ if (offrng[0] != offrng[1])
+ fprintf (file, " + [%lli, %lli]",
+ (long long) offrng[0].to_shwi (),
+ (long long) offrng[1].to_shwi ());
+ else if (offrng[0] != 0)
+ fprintf (file, " %c %lli",
+ offrng[0] < 0 ? '-' : '+',
+ (long long) offrng[0].to_shwi ());
+
+ if (base0)
+ fputs (" (base0)", file);
+
+ fputs ("; size: ", file);
+ if (sizrng[0] != sizrng[1])
+ {
+ offset_int maxsize = wi::to_offset (max_object_size ());
+ if (sizrng[0] == 0 && sizrng[1] >= maxsize)
+ fputs ("unknown", file);
+ else
+ fprintf (file, "[%llu, %llu]",
+ (unsigned long long) sizrng[0].to_uhwi (),
+ (unsigned long long) sizrng[1].to_uhwi ());
+ }
+ else if (sizrng[0] != 0)
+ fprintf (file, "%llu",
+ (unsigned long long) sizrng[0].to_uhwi ());
+
+ fputc ('\n', file);
+}
+
+/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1
+ when MINWRITE or MINREAD, respectively, is set. */
+access_data::access_data (range_query *query, gimple *stmt, access_mode mode,
+ tree maxwrite /* = NULL_TREE */,
+ bool minwrite /* = false */,
+ tree maxread /* = NULL_TREE */,
+ bool minread /* = false */)
+ : stmt (stmt), call (), dst (), src (), mode (mode), ostype ()
+{
+ set_bound (dst_bndrng, maxwrite, minwrite, query, stmt);
+ set_bound (src_bndrng, maxread, minread, query, stmt);
+}
+
+/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1
+ when MINWRITE or MINREAD, respectively, is set. */
+access_data::access_data (range_query *query, tree expr, access_mode mode,
+ tree maxwrite /* = NULL_TREE */,
+ bool minwrite /* = false */,
+ tree maxread /* = NULL_TREE */,
+ bool minread /* = false */)
+ : stmt (), call (expr), dst (), src (), mode (mode), ostype ()
+{
+ set_bound (dst_bndrng, maxwrite, minwrite, query, stmt);
+ set_bound (src_bndrng, maxread, minread, query, stmt);
+}
+
+/* Set BNDRNG to the range of BOUND for the statement STMT. */
+
+void
+access_data::set_bound (offset_int bndrng[2], tree bound, bool minaccess,
+ range_query *query, gimple *stmt)
+{
+ /* Set the default bounds of the access and adjust below. */
+ bndrng[0] = minaccess ? 1 : 0;
+ bndrng[1] = HOST_WIDE_INT_M1U;
+
+ /* When BOUND is nonnull and a range can be extracted from it,
+ set the bounds of the access to reflect both it and MINACCESS.
+ BNDRNG[0] is the size of the minimum access. */
+ tree rng[2];
+ if (bound && get_size_range (query, bound, stmt, rng, SR_ALLOW_ZERO))
+ {
+ bndrng[0] = wi::to_offset (rng[0]);
+ bndrng[1] = wi::to_offset (rng[1]);
+ bndrng[0] = bndrng[0] > 0 && minaccess ? 1 : 0;
+ }
+}
+
/* Set a bit for the PHI in VISITED and return true if it wasn't
already set. */
@@ -1320,7 +1464,8 @@ pointer_query::get_ref (tree ptr, int ostype /* = 1 */) const
there or compute it and insert it into the cache if it's nonnonull. */
bool
-pointer_query::get_ref (tree ptr, gimple *stmt, access_ref *pref, int ostype /* = 1 */)
+pointer_query::get_ref (tree ptr, gimple *stmt, access_ref *pref,
+ int ostype /* = 1 */)
{
const unsigned version
= TREE_CODE (ptr) == SSA_NAME ? SSA_NAME_VERSION (ptr) : 0;
@@ -1408,6 +1553,9 @@ pointer_query::flush_cache ()
void
pointer_query::dump (FILE *dump_file, bool contents /* = false */)
{
+ if (!var_cache)
+ return;
+
unsigned nused = 0, nrefs = 0;
unsigned nidxs = var_cache->indices.length ();
for (unsigned i = 0; i != nidxs; ++i)
@@ -1468,35 +1616,40 @@ pointer_query::dump (FILE *dump_file, bool contents /* = false */)
else
fprintf (dump_file, " _%u = ", ver);
- if (gphi *phi = aref.phi ())
- {
- fputs ("PHI <", dump_file);
- unsigned nargs = gimple_phi_num_args (phi);
- for (unsigned i = 0; i != nargs; ++i)
- {
- tree arg = gimple_phi_arg_def (phi, i);
- print_generic_expr (dump_file, arg);
- if (i + 1 < nargs)
- fputs (", ", dump_file);
- }
- fputc ('>', dump_file);
- }
- else
- print_generic_expr (dump_file, aref.ref);
-
- if (aref.offrng[0] != aref.offrng[1])
- fprintf (dump_file, " + [%lli, %lli]",
- (long long) aref.offrng[0].to_shwi (),
- (long long) aref.offrng[1].to_shwi ());
- else if (aref.offrng[0] != 0)
- fprintf (dump_file, " %c %lli",
- aref.offrng[0] < 0 ? '-' : '+',
- (long long) aref.offrng[0].to_shwi ());
-
- fputc ('\n', dump_file);
+ aref.dump (dump_file);
}
fputc ('\n', dump_file);
+
+ {
+ fputs ("\npointer_query cache contents (again):\n", dump_file);
+
+ tree var;
+ unsigned i;
+ FOR_EACH_SSA_NAME (i, var, cfun)
+ {
+ if (TREE_CODE (TREE_TYPE (var)) != POINTER_TYPE)
+ continue;
+
+ for (unsigned ost = 0; ost != 2; ++ost)
+ {
+ if (const access_ref *cache_ref = get_ref (var, ost))
+ {
+ unsigned ver = SSA_NAME_VERSION (var);
+ fprintf (dump_file, " %u.%u: ", ver, ost);
+ if (tree name = ssa_name (ver))
+ {
+ print_generic_expr (dump_file, name);
+ fputs (" = ", dump_file);
+ }
+ else
+ fprintf (dump_file, " _%u = ", ver);
+
+ cache_ref->dump (dump_file);
+ }
+ }
+ }
+ }
}
/* A helper of compute_objsize_r() to determine the size from an assignment
@@ -1520,7 +1673,7 @@ handle_min_max_size (tree ptr, int ostype, access_ref *pref,
for the expression. */
access_ref aref[2] = { *pref, *pref };
tree arg1 = gimple_assign_rhs1 (stmt);
- if (!compute_objsize_r (arg1, stmt, ostype, &aref[0], snlim, qry))
+ if (!compute_objsize_r (arg1, stmt, false, ostype, &aref[0], snlim, qry))
{
aref[0].base0 = false;
aref[0].offrng[0] = aref[0].offrng[1] = 0;
@@ -1529,7 +1682,7 @@ handle_min_max_size (tree ptr, int ostype, access_ref *pref,
}
tree arg2 = gimple_assign_rhs2 (stmt);
- if (!compute_objsize_r (arg2, stmt, ostype, &aref[1], snlim, qry))
+ if (!compute_objsize_r (arg2, stmt, false, ostype, &aref[1], snlim, qry))
{
aref[1].base0 = false;
aref[1].offrng[0] = aref[1].offrng[1] = 0;
@@ -1592,6 +1745,46 @@ handle_min_max_size (tree ptr, int ostype, access_ref *pref,
return true;
}
+/* A helper of compute_objsize_r() to determine the size of a DECL.
+ Return true on success and (possibly in the future) false on failure. */
+
+static bool
+handle_decl (tree decl, bool addr, access_ref *pref)
+{
+ tree decl_type = TREE_TYPE (decl);
+
+ pref->ref = decl;
+
+ /* Reset the offset in case it was set by a prior call and not
+ cleared by the caller. The offset is only adjusted after
+ the identity of the object has been determined. */
+ pref->offrng[0] = pref->offrng[1] = 0;
+
+ if (!addr && POINTER_TYPE_P (decl_type))
+ {
+ /* Set the maximum size if the reference is to the pointer
+ itself (as opposed to what it points to), and clear
+ BASE0 since the offset isn't necessarily zero-based. */
+ pref->set_max_size_range ();
+ pref->base0 = false;
+ return true;
+ }
+
+ /* Valid offsets into the object are nonnegative. */
+ pref->base0 = true;
+
+ if (tree size = decl_init_size (decl, false))
+ if (TREE_CODE (size) == INTEGER_CST)
+ {
+ pref->sizrng[0] = wi::to_offset (size);
+ pref->sizrng[1] = pref->sizrng[0];
+ return true;
+ }
+
+ pref->set_max_size_range ();
+ return true;
+}
+
/* A helper of compute_objsize_r() to determine the size from ARRAY_REF
AREF. ADDR is true if PTR is the operand of ADDR_EXPR. Return true
on success and false on failure. */
@@ -1603,8 +1796,6 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
{
gcc_assert (TREE_CODE (aref) == ARRAY_REF);
- ++pref->deref;
-
tree arefop = TREE_OPERAND (aref, 0);
tree reftype = TREE_TYPE (arefop);
if (!addr && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
@@ -1612,13 +1803,13 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
of known bound. */
return false;
- if (!compute_objsize_r (arefop, stmt, ostype, pref, snlim, qry))
+ if (!compute_objsize_r (arefop, stmt, addr, ostype, pref, snlim, qry))
return false;
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));
@@ -1673,6 +1864,96 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
return true;
}
+/* Given a COMPONENT_REF CREF, set *PREF size to the size of the referenced
+ member. */
+
+static void
+set_component_ref_size (tree cref, access_ref *pref)
+{
+ const tree base = TREE_OPERAND (cref, 0);
+ const tree base_type = TREE_TYPE (base);
+
+ /* SAM is set for array members that might need special treatment. */
+ special_array_member sam;
+ tree size = component_ref_size (cref, &sam);
+ if (sam == special_array_member::int_0)
+ pref->sizrng[0] = pref->sizrng[1] = 0;
+ else if (!pref->trail1special && sam == special_array_member::trail_1)
+ pref->sizrng[0] = pref->sizrng[1] = 1;
+ else if (size && TREE_CODE (size) == INTEGER_CST)
+ pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
+ else
+ {
+ /* When the size of the member is unknown it's either a flexible
+ array member or a trailing special array member (either zero
+ length or one-element). Set the size to the maximum minus
+ the constant size of the base object's type. */
+ pref->sizrng[0] = 0;
+ pref->sizrng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
+ if (tree base_size = TYPE_SIZE_UNIT (base_type))
+ if (TREE_CODE (base_size) == INTEGER_CST)
+ pref->sizrng[1] -= wi::to_offset (base_size);
+ }
+}
+
+/* A helper of compute_objsize_r() to determine the size from COMPONENT_REF
+ CREF. Return true on success and false on failure. */
+
+static bool
+handle_component_ref (tree cref, gimple *stmt, bool addr, int ostype,
+ access_ref *pref, ssa_name_limit_t &snlim,
+ pointer_query *qry)
+{
+ gcc_assert (TREE_CODE (cref) == COMPONENT_REF);
+
+ const tree base = TREE_OPERAND (cref, 0);
+ const tree base_type = TREE_TYPE (base);
+ if (TREE_CODE (base_type) == UNION_TYPE)
+ /* In accesses through union types consider the entire unions
+ rather than just their members. */
+ ostype = 0;
+
+ tree field = TREE_OPERAND (cref, 1);
+
+ if (ostype == 0)
+ {
+ /* In OSTYPE zero (for raw memory functions like memcpy), use
+ the maximum size instead if the identity of the enclosing
+ object cannot be determined. */
+ if (!compute_objsize_r (base, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ /* Otherwise, use the size of the enclosing object and add
+ the offset of the member to the offset computed so far. */
+ tree offset = byte_position (field);
+ if (TREE_CODE (offset) == INTEGER_CST)
+ pref->add_offset (wi::to_offset (offset));
+ else
+ pref->add_max_offset ();
+
+ if (!pref->ref)
+ /* PREF->REF may have been already set to an SSA_NAME earlier
+ to provide better context for diagnostics. In that case,
+ leave it unchanged. */
+ pref->ref = base;
+
+ return true;
+ }
+
+ pref->ref = field;
+
+ if (!addr && POINTER_TYPE_P (TREE_TYPE (field)))
+ {
+ /* Set maximum size if the reference is to the pointer member
+ itself (as opposed to what it points to). */
+ pref->set_max_size_range ();
+ return true;
+ }
+
+ set_component_ref_size (cref, pref);
+ return true;
+}
+
/* A helper of compute_objsize_r() to determine the size from MEM_REF
MREF. Return true on success and false on failure. */
@@ -1682,10 +1963,9 @@ handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
{
gcc_assert (TREE_CODE (mref) == MEM_REF);
- ++pref->deref;
-
- if (VECTOR_TYPE_P (TREE_TYPE (mref)))
- {
+ tree mreftype = TYPE_MAIN_VARIANT (TREE_TYPE (mref));
+ if (VECTOR_TYPE_P (mreftype))
+ {
/* Hack: Handle MEM_REFs of vector types as those to complete
objects; those may be synthesized from multiple assignments
to consecutive data members (see PR 93200 and 96963).
@@ -1699,13 +1979,15 @@ handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
}
tree mrefop = TREE_OPERAND (mref, 0);
- if (!compute_objsize_r (mrefop, stmt, ostype, pref, snlim, qry))
+ if (!compute_objsize_r (mrefop, stmt, false, ostype, pref, snlim, qry))
return false;
+ ++pref->deref;
+
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));
@@ -1716,168 +1998,282 @@ handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
return true;
}
-/* Helper to compute the size of the object referenced by the PTR
- expression which must have pointer type, using Object Size type
- OSTYPE (only the least significant 2 bits are used).
- On success, sets PREF->REF to the DECL of the referenced object
- if it's unique, otherwise to null, PREF->OFFRNG to the range of
- offsets into it, and PREF->SIZRNG to the range of sizes of
- the object(s).
- SNLIM is used to avoid visiting the same PHI operand multiple
- times, and, when nonnull, RVALS to determine range information.
- Returns true on success, false when a meaningful size (or range)
- cannot be determined.
-
- The function is intended for diagnostics and should not be used
- to influence code generation or optimization. */
+/* A helper of compute_objsize_r() to determine the size from SSA_NAME
+ PTR. Return true on success and false on failure. */
static bool
-compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref,
- ssa_name_limit_t &snlim, pointer_query *qry)
+handle_ssa_name (tree ptr, bool addr, int ostype,
+ access_ref *pref, ssa_name_limit_t &snlim,
+ pointer_query *qry)
{
- STRIP_NOPS (ptr);
+ if (!snlim.next ())
+ return false;
- const bool addr = TREE_CODE (ptr) == ADDR_EXPR;
- if (addr)
+ /* Only process an SSA_NAME if the recursion limit has not yet
+ been reached. */
+ if (qry)
{
- --pref->deref;
- ptr = TREE_OPERAND (ptr, 0);
+ if (++qry->depth > qry->max_depth)
+ qry->max_depth = qry->depth;
+ if (const access_ref *cache_ref = qry->get_ref (ptr, ostype))
+ {
+ /* Add the number of DEREFerences accummulated so far. */
+ const int deref = pref->deref;
+ *pref = *cache_ref;
+ pref->deref += deref;
+ return true;
+ }
}
- if (DECL_P (ptr))
+ gimple *stmt = SSA_NAME_DEF_STMT (ptr);
+ if (is_gimple_call (stmt))
{
- pref->ref = ptr;
-
- /* Reset the offset in case it was set by a prior call and not
- cleared by the caller. The offset is only adjusted after
- the identity of the object has been determined. */
- pref->offrng[0] = pref->offrng[1] = 0;
+ /* If STMT is a call to an allocation function get the size
+ from its argument(s). If successful, also set *PREF->REF
+ to PTR for the caller to include in diagnostics. */
+ wide_int wr[2];
+ range_query *const rvals = qry ? qry->rvals : NULL;
+ if (gimple_call_alloc_size (stmt, wr, rvals))
+ {
+ pref->ref = ptr;
+ pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
+ pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
+ /* Constrain both bounds to a valid size. */
+ offset_int maxsize = wi::to_offset (max_object_size ());
+ if (pref->sizrng[0] > maxsize)
+ pref->sizrng[0] = maxsize;
+ if (pref->sizrng[1] > maxsize)
+ pref->sizrng[1] = maxsize;
+ }
+ else
+ {
+ /* For functions known to return one of their pointer arguments
+ try to determine what the returned pointer points to, and on
+ success add OFFRNG which was set to the offset added by
+ the function (e.g., memchr) to the overall offset. */
+ bool past_end;
+ offset_int offrng[2];
+ if (tree ret = gimple_call_return_array (stmt, offrng, &past_end,
+ snlim, qry))
+ {
+ if (!compute_objsize_r (ret, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ /* Cap OFFRNG[1] to at most the remaining size of
+ the object. */
+ offset_int remrng[2];
+ remrng[1] = pref->size_remaining (remrng);
+ if (remrng[1] != 0 && !past_end)
+ /* Decrement the size for functions that never return
+ a past-the-end pointer. */
+ remrng[1] -= 1;
+
+ if (remrng[1] < offrng[1])
+ offrng[1] = remrng[1];
+ pref->add_offset (offrng[0], offrng[1]);
+ }
+ else
+ {
+ /* For other calls that might return arbitrary pointers
+ including into the middle of objects set the size
+ range to maximum, clear PREF->BASE0, and also set
+ PREF->REF to include in diagnostics. */
+ pref->set_max_size_range ();
+ pref->base0 = false;
+ pref->ref = ptr;
+ }
+ }
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- if (!addr && POINTER_TYPE_P (TREE_TYPE (ptr)))
+ if (gimple_nop_p (stmt))
+ {
+ /* For a function argument try to determine the byte size
+ of the array from the current function declaratation
+ (e.g., attribute access or related). */
+ wide_int wr[2];
+ bool static_array = false;
+ if (tree ref = gimple_parm_array_size (ptr, wr, &static_array))
{
- /* Set the maximum size if the reference is to the pointer
- itself (as opposed to what it points to), and clear
- BASE0 since the offset isn't necessarily zero-based. */
- pref->set_max_size_range ();
- pref->base0 = false;
+ pref->parmarray = !static_array;
+ pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
+ pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
+ pref->ref = ref;
+ qry->put_ref (ptr, *pref, ostype);
return true;
}
- /* Valid offsets into the object are nonnegative. */
- pref->base0 = true;
-
- if (tree size = decl_init_size (ptr, false))
- if (TREE_CODE (size) == INTEGER_CST)
- {
- pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
- return true;
- }
-
pref->set_max_size_range ();
+ pref->base0 = false;
+ pref->ref = ptr;
+ qry->put_ref (ptr, *pref, ostype);
return true;
}
- const tree_code code = TREE_CODE (ptr);
- range_query *const rvals = qry ? qry->rvals : NULL;
-
- if (code == BIT_FIELD_REF)
+ if (gimple_code (stmt) == GIMPLE_PHI)
{
- tree ref = TREE_OPERAND (ptr, 0);
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
+ /* Pass PTR to get_ref() via PREF. If all PHI arguments refer
+ to the same object the function will replace it with it. */
+ pref->ref = ptr;
+ access_ref phi_ref = *pref;
+ if (!pref->get_ref (NULL, &phi_ref, ostype, &snlim, qry))
return false;
+ *pref = phi_ref;
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- offset_int off = wi::to_offset (pref->eval (TREE_OPERAND (ptr, 2)));
- pref->add_offset (off / BITS_PER_UNIT);
+ if (!is_gimple_assign (stmt))
+ {
+ /* Clear BASE0 since the assigned pointer might point into
+ the middle of the object, set the maximum size range and,
+ if the SSA_NAME refers to a function argumnent, set
+ PREF->REF to it. */
+ pref->base0 = false;
+ pref->set_max_size_range ();
+ pref->ref = ptr;
return true;
}
- if (code == COMPONENT_REF)
+ tree_code code = gimple_assign_rhs_code (stmt);
+
+ if (code == MAX_EXPR || code == MIN_EXPR)
{
- tree ref = TREE_OPERAND (ptr, 0);
- if (TREE_CODE (TREE_TYPE (ref)) == UNION_TYPE)
- /* In accesses through union types consider the entire unions
- rather than just their members. */
- ostype = 0;
- tree field = TREE_OPERAND (ptr, 1);
+ if (!handle_min_max_size (ptr, ostype, pref, snlim, qry))
+ return false;
- if (ostype == 0)
- {
- /* In OSTYPE zero (for raw memory functions like memcpy), use
- the maximum size instead if the identity of the enclosing
- object cannot be determined. */
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
- return false;
-
- /* Otherwise, use the size of the enclosing object and add
- the offset of the member to the offset computed so far. */
- tree offset = byte_position (field);
- if (TREE_CODE (offset) == INTEGER_CST)
- pref->add_offset (wi::to_offset (offset));
- else
- pref->add_max_offset ();
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- if (!pref->ref)
- /* REF may have been already set to an SSA_NAME earlier
- to provide better context for diagnostics. In that case,
- leave it unchanged. */
- pref->ref = ref;
- return true;
- }
+ tree rhs = gimple_assign_rhs1 (stmt);
- pref->ref = field;
+ if (code == ASSERT_EXPR)
+ {
+ rhs = TREE_OPERAND (rhs, 0);
+ return compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry);
+ }
- if (!addr && POINTER_TYPE_P (TREE_TYPE (field)))
- {
- /* Set maximum size if the reference is to the pointer member
- itself (as opposed to what it points to). */
- pref->set_max_size_range ();
- return true;
- }
+ if (code == POINTER_PLUS_EXPR
+ && TREE_CODE (TREE_TYPE (rhs)) == POINTER_TYPE)
+ {
+ /* Compute the size of the object first. */
+ if (!compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry))
+ return false;
- /* SAM is set for array members that might need special treatment. */
- special_array_member sam;
- tree size = component_ref_size (ptr, &sam);
- if (sam == special_array_member::int_0)
- pref->sizrng[0] = pref->sizrng[1] = 0;
- else if (!pref->trail1special && sam == special_array_member::trail_1)
- pref->sizrng[0] = pref->sizrng[1] = 1;
- else if (size && TREE_CODE (size) == INTEGER_CST)
- pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
+ offset_int orng[2];
+ tree off = gimple_assign_rhs2 (stmt);
+ range_query *const rvals = qry ? qry->rvals : NULL;
+ if (get_offset_range (off, stmt, orng, rvals))
+ pref->add_offset (orng[0], orng[1]);
else
- {
- /* When the size of the member is unknown it's either a flexible
- array member or a trailing special array member (either zero
- length or one-element). Set the size to the maximum minus
- the constant size of the type. */
- pref->sizrng[0] = 0;
- pref->sizrng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
- if (tree recsize = TYPE_SIZE_UNIT (TREE_TYPE (ref)))
- if (TREE_CODE (recsize) == INTEGER_CST)
- pref->sizrng[1] -= wi::to_offset (recsize);
- }
+ pref->add_max_offset ();
+
+ qry->put_ref (ptr, *pref, ostype);
return true;
}
- if (code == ARRAY_REF)
- return handle_array_ref (ptr, stmt, addr, ostype, pref, snlim, qry);
-
- if (code == MEM_REF)
- return handle_mem_ref (ptr, stmt, ostype, pref, snlim, qry);
+ if (code == ADDR_EXPR || code == SSA_NAME)
+ {
+ if (!compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- if (code == TARGET_MEM_REF)
+ if (ostype > 1 && POINTER_TYPE_P (TREE_TYPE (rhs)))
{
- tree ref = TREE_OPERAND (ptr, 0);
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
+ /* When determining the qualifiers follow the pointer but
+ avoid caching the result. As the pointer is added to
+ and/or dereferenced the computed size and offset need
+ not be meaningful for other queries involving the same
+ pointer. */
+ if (!compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry))
return false;
- /* TODO: Handle remaining operands. Until then, add maximum offset. */
- pref->ref = ptr;
- pref->add_max_offset ();
- return true;
+ rhs = pref->ref;
}
- if (code == INTEGER_CST)
+ /* (This could also be an assignment from a nonlocal pointer.) Save
+ PTR to mention in diagnostics but otherwise treat it as a pointer
+ to an unknown object. */
+ pref->ref = rhs;
+ pref->base0 = false;
+ pref->set_max_size_range ();
+ return true;
+}
+
+/* Helper to compute the size of the object referenced by the PTR
+ expression which must have pointer type, using Object Size type
+ OSTYPE (only the least significant 2 bits are used).
+ On success, sets PREF->REF to the DECL of the referenced object
+ if it's unique, otherwise to null, PREF->OFFRNG to the range of
+ offsets into it, and PREF->SIZRNG to the range of sizes of
+ the object(s).
+ SNLIM is used to avoid visiting the same PHI operand multiple
+ times, and, when nonnull, RVALS to determine range information.
+ Returns true on success, false when a meaningful size (or range)
+ cannot be determined.
+
+ The function is intended for diagnostics and should not be used
+ to influence code generation or optimization. */
+
+static bool
+compute_objsize_r (tree ptr, gimple *stmt, bool addr, int ostype,
+ access_ref *pref, ssa_name_limit_t &snlim,
+ pointer_query *qry)
+{
+ STRIP_NOPS (ptr);
+
+ if (DECL_P (ptr))
+ return handle_decl (ptr, addr, pref);
+
+ switch (TREE_CODE (ptr))
{
+ case ADDR_EXPR:
+ {
+ tree ref = TREE_OPERAND (ptr, 0);
+ if (!compute_objsize_r (ref, stmt, true, ostype, pref, snlim, qry))
+ return false;
+
+ --pref->deref;
+ return true;
+ }
+
+ case BIT_FIELD_REF:
+ {
+ tree ref = TREE_OPERAND (ptr, 0);
+ if (!compute_objsize_r (ref, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ offset_int off = wi::to_offset (pref->eval (TREE_OPERAND (ptr, 2)));
+ pref->add_offset (off / BITS_PER_UNIT);
+ return true;
+ }
+
+ case ARRAY_REF:
+ return handle_array_ref (ptr, stmt, addr, ostype, pref, snlim, qry);
+
+ case COMPONENT_REF:
+ return handle_component_ref (ptr, stmt, addr, ostype, pref, snlim, qry);
+
+ case MEM_REF:
+ return handle_mem_ref (ptr, stmt, ostype, pref, snlim, qry);
+
+ case TARGET_MEM_REF:
+ {
+ tree ref = TREE_OPERAND (ptr, 0);
+ if (!compute_objsize_r (ref, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ /* TODO: Handle remaining operands. Until then, add maximum offset. */
+ pref->ref = ptr;
+ pref->add_max_offset ();
+ return true;
+ }
+
+ case INTEGER_CST:
/* Pointer constants other than null are most likely the result
of erroneous null pointer addition/subtraction. Unless zero
is a valid address set size to zero. For null pointers, set
@@ -1898,21 +2294,17 @@ compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref,
pref->sizrng[0] = pref->sizrng[1] = 0;
pref->ref = ptr;
-
return true;
- }
- if (code == STRING_CST)
- {
+ case STRING_CST:
pref->sizrng[0] = pref->sizrng[1] = TREE_STRING_LENGTH (ptr);
pref->ref = ptr;
return true;
- }
- if (code == POINTER_PLUS_EXPR)
+ case POINTER_PLUS_EXPR:
{
tree ref = TREE_OPERAND (ptr, 0);
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
+ if (!compute_objsize_r (ref, stmt, addr, ostype, pref, snlim, qry))
return false;
/* Clear DEREF since the offset is being applied to the target
@@ -1921,206 +2313,22 @@ 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, qry->rvals))
pref->add_offset (orng[0], orng[1]);
else
pref->add_max_offset ();
return true;
}
- if (code == VIEW_CONVERT_EXPR)
- {
+ case VIEW_CONVERT_EXPR:
ptr = TREE_OPERAND (ptr, 0);
- return compute_objsize_r (ptr, stmt, ostype, pref, snlim, qry);
- }
+ return compute_objsize_r (ptr, stmt, addr, ostype, pref, snlim, qry);
- if (code == SSA_NAME)
- {
- if (!snlim.next ())
- return false;
+ case SSA_NAME:
+ return handle_ssa_name (ptr, addr, ostype, pref, snlim, qry);
- /* Only process an SSA_NAME if the recursion limit has not yet
- been reached. */
- if (qry)
- {
- if (++qry->depth)
- qry->max_depth = qry->depth;
- if (const access_ref *cache_ref = qry->get_ref (ptr))
- {
- /* If the pointer is in the cache set *PREF to what it refers
- to and return success.
- FIXME: BNDRNG is determined by each access and so it doesn't
- belong in access_ref. Until the design is changed, keep it
- unchanged here. */
- const offset_int bndrng[2] = { pref->bndrng[0], pref->bndrng[1] };
- *pref = *cache_ref;
- pref->bndrng[0] = bndrng[0];
- pref->bndrng[1] = bndrng[1];
- return true;
- }
- }
-
- stmt = SSA_NAME_DEF_STMT (ptr);
- if (is_gimple_call (stmt))
- {
- /* If STMT is a call to an allocation function get the size
- from its argument(s). If successful, also set *PREF->REF
- to PTR for the caller to include in diagnostics. */
- wide_int wr[2];
- if (gimple_call_alloc_size (stmt, wr, rvals))
- {
- pref->ref = ptr;
- pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
- pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
- /* Constrain both bounds to a valid size. */
- offset_int maxsize = wi::to_offset (max_object_size ());
- if (pref->sizrng[0] > maxsize)
- pref->sizrng[0] = maxsize;
- if (pref->sizrng[1] > maxsize)
- pref->sizrng[1] = maxsize;
- }
- else
- {
- /* For functions known to return one of their pointer arguments
- try to determine what the returned pointer points to, and on
- success add OFFRNG which was set to the offset added by
- the function (e.g., memchr) to the overall offset. */
- bool past_end;
- offset_int offrng[2];
- if (tree ret = gimple_call_return_array (stmt, offrng,
- &past_end, snlim, qry))
- {
- if (!compute_objsize_r (ret, stmt, ostype, pref, snlim, qry))
- return false;
-
- /* Cap OFFRNG[1] to at most the remaining size of
- the object. */
- offset_int remrng[2];
- remrng[1] = pref->size_remaining (remrng);
- if (remrng[1] != 0 && !past_end)
- /* Decrement the size for functions that never return
- a past-the-end pointer. */
- remrng[1] -= 1;
-
- if (remrng[1] < offrng[1])
- offrng[1] = remrng[1];
- pref->add_offset (offrng[0], offrng[1]);
- }
- else
- {
- /* For other calls that might return arbitrary pointers
- including into the middle of objects set the size
- range to maximum, clear PREF->BASE0, and also set
- PREF->REF to include in diagnostics. */
- pref->set_max_size_range ();
- pref->base0 = false;
- pref->ref = ptr;
- }
- }
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (gimple_nop_p (stmt))
- {
- /* For a function argument try to determine the byte size
- of the array from the current function declaratation
- (e.g., attribute access or related). */
- wide_int wr[2];
- bool static_array = false;
- if (tree ref = gimple_parm_array_size (ptr, wr, &static_array))
- {
- pref->parmarray = !static_array;
- pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
- pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
- pref->ref = ref;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- pref->set_max_size_range ();
- pref->base0 = false;
- pref->ref = ptr;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (gimple_code (stmt) == GIMPLE_PHI)
- {
- pref->ref = ptr;
- access_ref phi_ref = *pref;
- if (!pref->get_ref (NULL, &phi_ref, ostype, &snlim, qry))
- return false;
- *pref = phi_ref;
- pref->ref = ptr;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (!is_gimple_assign (stmt))
- {
- /* Clear BASE0 since the assigned pointer might point into
- the middle of the object, set the maximum size range and,
- if the SSA_NAME refers to a function argumnent, set
- PREF->REF to it. */
- pref->base0 = false;
- pref->set_max_size_range ();
- pref->ref = ptr;
- return true;
- }
-
- tree_code code = gimple_assign_rhs_code (stmt);
-
- if (code == MAX_EXPR || code == MIN_EXPR)
- {
- if (!handle_min_max_size (ptr, ostype, pref, snlim, qry))
- return false;
-
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- tree rhs = gimple_assign_rhs1 (stmt);
-
- if (code == ASSERT_EXPR)
- {
- rhs = TREE_OPERAND (rhs, 0);
- return compute_objsize_r (rhs, stmt, ostype, pref, snlim, qry);
- }
-
- if (code == POINTER_PLUS_EXPR
- && TREE_CODE (TREE_TYPE (rhs)) == POINTER_TYPE)
- {
- /* Compute the size of the object first. */
- if (!compute_objsize_r (rhs, stmt, ostype, pref, snlim, qry))
- return false;
-
- offset_int orng[2];
- tree off = gimple_assign_rhs2 (stmt);
- if (get_offset_range (off, stmt, orng, rvals))
- pref->add_offset (orng[0], orng[1]);
- else
- pref->add_max_offset ();
-
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (code == ADDR_EXPR || code == SSA_NAME)
- {
- if (!compute_objsize_r (rhs, stmt, ostype, pref, snlim, qry))
- return false;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- /* (This could also be an assignment from a nonlocal pointer.) Save
- PTR to mention in diagnostics but otherwise treat it as a pointer
- to an unknown object. */
- pref->ref = rhs;
- pref->base0 = false;
- pref->set_max_size_range ();
- return true;
+ default:
+ break;
}
/* Assume all other expressions point into an unknown object
@@ -2151,7 +2359,7 @@ compute_objsize (tree ptr, gimple *stmt, int ostype, access_ref *pref,
pref->sizrng[0] = pref->sizrng[1] = -1;
ssa_name_limit_t snlim;
- if (!compute_objsize_r (ptr, stmt, ostype, pref, snlim, ptr_qry))
+ if (!compute_objsize_r (ptr, stmt, false, ostype, pref, snlim, ptr_qry))
return NULL_TREE;
offset_int maxsize = pref->size_remaining ();
@@ -2176,13 +2384,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 c8215b681ef..df19154a3d5 100644
--- a/gcc/pointer-query.h
+++ b/gcc/pointer-query.h
@@ -61,15 +61,19 @@ class pointer_query;
struct access_ref
{
/* Set the bounds of the reference. */
- access_ref (range_query *query = nullptr, tree = NULL_TREE,
- gimple * = nullptr, bool = false);
+ access_ref ();
/* Return the PHI node REF refers to or null if it doesn't. */
gphi *phi () const;
+ /* Merge the result for a pointer with *THIS. */
+ bool merge_ref (vec<access_ref> *all_refs, tree, gimple *, int, bool,
+ ssa_name_limit_t &, pointer_query &);
+
/* Return the object to which REF refers. */
- tree get_ref (vec<access_ref> *, access_ref * = nullptr, int = 1,
- ssa_name_limit_t * = nullptr, pointer_query * = nullptr) const;
+ tree get_ref (vec<access_ref> *, access_ref * = NULL, int = 1,
+ ssa_name_limit_t * = nullptr,
+ pointer_query * = nullptr) const;
/* Return true if OFFRNG is the constant zero. */
bool offset_zero () const
@@ -119,7 +123,10 @@ struct access_ref
/* Issue an informational message describing the target of an access
with the given mode. */
- void inform_access (access_mode) const;
+ void inform_access (access_mode, int = 1) const;
+
+ /* Dump *THIS to file. */
+ void dump (FILE *) const;
/* Reference to the accessed object(s). */
tree ref;
@@ -129,11 +136,6 @@ struct access_ref
offset_int sizrng[2];
/* The minimum and maximum offset computed. */
offset_int offmax[2];
- /* Range of the bound of the access: denotes that the access
- is at least BNDRNG[0] bytes but no more than BNDRNG[1].
- For string functions the size of the actual access is
- further constrained by the length of the string. */
- offset_int bndrng[2];
/* Used to fold integer expressions when called from front ends. */
tree (*eval)(tree);
@@ -206,23 +208,18 @@ struct access_data
{
/* Set the access to at most MAXWRITE and MAXREAD bytes, and
at least 1 when MINWRITE or MINREAD, respectively, is set. */
- access_data (range_query *query, gimple *stmt, access_mode mode,
- tree maxwrite = NULL_TREE, bool minwrite = false,
- tree maxread = NULL_TREE, bool minread = false)
- : stmt (stmt), call (),
- dst (query, maxwrite, stmt, minwrite),
- src (query, maxread, stmt, minread),
- mode (mode) { }
+ access_data (range_query *, gimple *, access_mode,
+ tree = NULL_TREE, bool = false,
+ tree = NULL_TREE, bool = false);
/* Set the access to at most MAXWRITE and MAXREAD bytes, and
at least 1 when MINWRITE or MINREAD, respectively, is set. */
- access_data (range_query *query, tree expr, access_mode mode,
- tree maxwrite = NULL_TREE, bool minwrite = false,
- tree maxread = NULL_TREE, bool minread = false)
- : stmt (), call (expr),
- dst (query, maxwrite, nullptr, minwrite),
- src (query, maxread, nullptr, minread),
- mode (mode) { }
+ access_data (range_query *, tree, access_mode,
+ tree = NULL_TREE, bool = false,
+ tree = NULL_TREE, bool = false);
+
+ /* Constructor helper. */
+ static void set_bound (offset_int[2], tree, bool, range_query *, gimple *);
/* Access statement. */
gimple *stmt;
@@ -230,9 +227,19 @@ struct access_data
tree call;
/* Destination and source of the access. */
access_ref dst, src;
+
+ /* Range of the bound of the access: denotes that the access is at
+ least XXX_BNDRNG[0] bytes but no more than XXX_BNDRNG[1]. For
+ string functions the size of the actual access is further
+ constrained by the length of the string. */
+ offset_int dst_bndrng[2];
+ offset_int src_bndrng[2];
+
/* Read-only for functions like memcmp or strlen, write-only
for memset, read-write for memcpy or strcat. */
access_mode mode;
+ /* The object size type. */
+ int ostype;
};
enum size_range_flags
@@ -263,8 +270,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,
diff --git a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
index e5802613a9c..a25cc1fb2ca 100644
--- a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
+++ b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
@@ -287,9 +287,11 @@ void ga1i__ (void)
a1i__.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" }
struct A1i a = { 0 };
+ sink (&a);
+
a.a[0] = 0;
a.a[1] = 1; // { dg-warning "\\\[-Wstringop-overflow" }
- a.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" "pr102462" { xfail { vect_slp_v2qi_store_align } } }
+ a.a[2] = 2; // { dg-warning "\\\[-Wstringop-overflow" }
sink (&a);
}
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c
new file mode 100644
index 00000000000..6928ee4d559
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c
@@ -0,0 +1,19 @@
+/* PR middle-end/103143 - ICE due to infinite recursion in pointer-query.cc
+ { dg-do compile }
+ { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+void foo (size_t x)
+{
+ struct T { char buf[64]; char buf2[64]; } t;
+ char *p = &t.buf[8];
+ char *r = t.buf2;
+ size_t i;
+
+ for (i = 0; i < x; i++)
+ {
+ r = __builtin_mempcpy (r, p, i);
+ p = r + 1;
+ }
+}
^ permalink raw reply [flat|nested] 19+ messages in thread
* PING [PATCH] fix up compute_objsize (including PR 103143)
2021-11-09 2:34 [PATCH] fix up compute_objsize (including PR 103143) Martin Sebor
@ 2021-11-15 16:49 ` Martin Sebor
2021-11-22 16:41 ` PING 2 " Martin Sebor
2021-12-04 0:00 ` Jeff Law
1 sibling, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-11-15 16:49 UTC (permalink / raw)
To: gcc-patches
Ping for the following cleanup patch:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583735.html
On 11/8/21 7:34 PM, Martin Sebor 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.
>
> Martin
^ permalink raw reply [flat|nested] 19+ messages in thread
* PING 2 [PATCH] fix up compute_objsize (including PR 103143)
2021-11-15 16:49 ` PING " Martin Sebor
@ 2021-11-22 16:41 ` Martin Sebor
2021-11-29 15:49 ` PING 3 " Martin Sebor
0 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-11-22 16:41 UTC (permalink / raw)
To: gcc-patches
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583735.html
On 11/15/21 9:49 AM, Martin Sebor wrote:
> Ping for the following cleanup patch:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583735.html
>
> On 11/8/21 7:34 PM, Martin Sebor 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.
>>
>> Martin
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* PING 3 [PATCH] fix up compute_objsize (including PR 103143)
2021-11-22 16:41 ` PING 2 " Martin Sebor
@ 2021-11-29 15:49 ` Martin Sebor
0 siblings, 0 replies; 19+ messages in thread
From: Martin Sebor @ 2021-11-29 15:49 UTC (permalink / raw)
To: gcc-patches
Ping:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583735.html
On 11/22/21 9:41 AM, Martin Sebor wrote:
> Ping:
> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583735.html
>
> On 11/15/21 9:49 AM, Martin Sebor wrote:
>> Ping for the following cleanup patch:
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-November/583735.html
>>
>> On 11/8/21 7:34 PM, Martin Sebor 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.
>>>
>>> Martin
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fix up compute_objsize (including PR 103143)
2021-11-09 2:34 [PATCH] fix up compute_objsize (including PR 103143) Martin Sebor
2021-11-15 16:49 ` PING " Martin Sebor
@ 2021-12-04 0:00 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2] fix PR 103143 Martin Sebor
` (5 more replies)
1 sibling, 6 replies; 19+ messages in thread
From: Jeff Law @ 2021-12-04 0:00 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] fix PR 103143
2021-12-04 0:00 ` Jeff Law
@ 2021-12-06 17:31 ` Martin Sebor
2021-12-06 20:14 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data Martin Sebor
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 17:31 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]
I have broken up the patch into a series of six. Attached is
part (1), the fix for the typo that causes PR 103143.
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
>
[-- Attachment #2: gcc-103413.diff --]
[-- Type: text/x-patch, Size: 1638 bytes --]
commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9
Author: Martin Sebor <msebor@redhat.com>
Date: Sat Dec 4 16:22:07 2021 -0700
Use the recursive form of compute_objsize [PR 103143].
gcc/ChangeLog:
PR middle-end/103143
* pointer-query.cc (gimple_call_return_array): Call compute_objsize_r.
gcc/testsuite/ChangeLog:
PR middle-end/103143
* gcc.dg/Wstringop-overflow-83.c: New test.
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 2ead0271617..25ce4303849 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -199,7 +199,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
of the source object. */
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize (src, stmt, 1, &aref, qry)
+ if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry)
&& aref.sizrng[1] < offrng[1])
offrng[1] = aref.sizrng[1];
}
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c
new file mode 100644
index 00000000000..6928ee4d559
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow-83.c
@@ -0,0 +1,19 @@
+/* PR middle-end/103143 - ICE due to infinite recursion in pointer-query.cc
+ { dg-do compile }
+ { dg-options "-O2 -Wall" } */
+
+typedef __SIZE_TYPE__ size_t;
+
+void foo (size_t x)
+{
+ struct T { char buf[64]; char buf2[64]; } t;
+ char *p = &t.buf[8];
+ char *r = t.buf2;
+ size_t i;
+
+ for (i = 0; i < x; i++)
+ {
+ r = __builtin_mempcpy (r, p, i);
+ p = r + 1;
+ }
+}
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data
2021-12-04 0:00 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2] fix PR 103143 Martin Sebor
@ 2021-12-06 17:31 ` Martin Sebor
2021-12-08 18:47 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2 2/5] fix up compute_objsize: pass GIMPLE statement to it Martin Sebor
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 17:31 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]
Attached is the subset of the patch in part (1) below: Move
bndrng from access_ref to access_data.
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
>
[-- Attachment #2: gcc-pointer_query-refactor-1.diff --]
[-- Type: text/x-patch, Size: 10845 bytes --]
commit 1062c1154beeeae26e86f053946ea733ffb5d136
Author: Martin Sebor <msebor@redhat.com>
Date: Sat Dec 4 16:46:17 2021 -0700
Move bndrng from access_ref to access_data.
gcc/ChangeLog:
* gimple-ssa-warn-access.cc (check_access): Adjust to member name
change.
(pass_waccess::check_strncmp): Same.
* pointer-query.cc (access_ref::access_ref): Remove arguments.
Simpilfy.
(access_data::access_data): Define new ctors.
(access_data::set_bound): Define new member function.
(compute_objsize_r): Remove unnecessary code.
* pointer-query.h (struct access_ref): Remove ctor arguments.
(struct access_data): Declare ctor overloads.
(access_data::dst_bndrng): New member.
(access_data::src_bndrng): New member.
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 48bf8aaff50..05b8d91b71d 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -1337,10 +1337,10 @@ check_access (GimpleOrTree exp, tree dstwrite,
if (!dstsize)
dstsize = maxobjsize;
- /* Set RANGE to that of DSTWRITE if non-null, bounded by PAD->DST.BNDRNG
+ /* Set RANGE to that of DSTWRITE if non-null, bounded by PAD->DST_BNDRNG
if valid. */
gimple *stmt = pad ? pad->stmt : nullptr;
- get_size_range (rvals, dstwrite, stmt, range, pad ? pad->dst.bndrng : NULL);
+ get_size_range (rvals, dstwrite, stmt, range, pad ? pad->dst_bndrng : NULL);
tree func = get_callee_fndecl (exp);
/* Read vs write access by built-ins can be determined from the const
@@ -1432,9 +1432,9 @@ check_access (GimpleOrTree exp, tree dstwrite,
of an object. */
if (maxread)
{
- /* Set RANGE to that of MAXREAD, bounded by PAD->SRC.BNDRNG if
+ /* Set RANGE to that of MAXREAD, bounded by PAD->SRC_BNDRNG if
PAD is nonnull and BNDRNG is valid. */
- get_size_range (rvals, maxread, stmt, range, pad ? pad->src.bndrng : NULL);
+ get_size_range (rvals, maxread, stmt, range, pad ? pad->src_bndrng : NULL);
location_t loc = get_location (exp);
tree size = dstsize;
@@ -1479,12 +1479,12 @@ check_access (GimpleOrTree exp, tree dstwrite,
&& (pad->src.offrng[1] < 0
|| pad->src.offrng[0] <= pad->src.offrng[1]))
{
- /* Set RANGE to that of MAXREAD, bounded by PAD->SRC.BNDRNG if
+ /* Set RANGE to that of MAXREAD, bounded by PAD->SRC_BNDRNG if
PAD is nonnull and BNDRNG is valid. */
- get_size_range (rvals, maxread, stmt, range, pad ? pad->src.bndrng : NULL);
+ get_size_range (rvals, maxread, stmt, range, pad ? pad->src_bndrng : NULL);
/* Set OVERREAD for reads starting just past the end of an object. */
- overread = pad->src.sizrng[1] - pad->src.offrng[0] < pad->src.bndrng[0];
- range[0] = wide_int_to_tree (sizetype, pad->src.bndrng[0]);
+ overread = pad->src.sizrng[1] - pad->src.offrng[0] < pad->src_bndrng[0];
+ range[0] = wide_int_to_tree (sizetype, pad->src_bndrng[0]);
slen = size_zero_node;
}
@@ -2592,7 +2592,7 @@ pass_waccess::check_strncmp (gcall *stmt)
/* Determine the range of the bound first and bail if it fails; it's
cheaper than computing the size of the objects. */
tree bndrng[2] = { NULL_TREE, NULL_TREE };
- get_size_range (m_ptr_qry.rvals, bound, stmt, bndrng, adata1.src.bndrng);
+ get_size_range (m_ptr_qry.rvals, bound, stmt, bndrng, adata1.src_bndrng);
if (!bndrng[0] || integer_zerop (bndrng[0]))
return;
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 25ce4303849..a8c9671e3ba 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -596,15 +596,9 @@ gimple_parm_array_size (tree ptr, wide_int rng[2],
return var;
}
-/* Given a statement STMT, set the bounds of the reference to at most
- as many bytes as BOUND or unknown when null, and at least one when
- the MINACCESS is true unless BOUND is a constant zero. STMT is
- used for context to get accurate range info. */
-
-access_ref::access_ref (range_query *qry /* = nullptr */,
- tree bound /* = NULL_TREE */,
- gimple *stmt /* = nullptr */,
- bool minaccess /* = false */)
+/* Initialize the object. */
+
+access_ref::access_ref ()
: ref (), eval ([](tree x){ return x; }), deref (), trail1special (true),
base0 (true), parmarray ()
{
@@ -613,21 +607,6 @@ access_ref::access_ref (range_query *qry /* = nullptr */,
offmax[0] = offmax[1] = 0;
/* Invalidate. */
sizrng[0] = sizrng[1] = -1;
-
- /* Set the default bounds of the access and adjust below. */
- bndrng[0] = minaccess ? 1 : 0;
- bndrng[1] = HOST_WIDE_INT_M1U;
-
- /* When BOUND is nonnull and a range can be extracted from it,
- set the bounds of the access to reflect both it and MINACCESS.
- BNDRNG[0] is the size of the minimum access. */
- tree rng[2];
- if (bound && get_size_range (qry, bound, stmt, rng, SR_ALLOW_ZERO))
- {
- bndrng[0] = wi::to_offset (rng[0]);
- bndrng[1] = wi::to_offset (rng[1]);
- bndrng[0] = bndrng[0] > 0 && minaccess ? 1 : 0;
- }
}
/* Return the PHI node REF refers to or null if it doesn't. */
@@ -1199,6 +1178,54 @@ access_ref::inform_access (access_mode mode) const
sizestr, allocfn);
}
+/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1
+ when MINWRITE or MINREAD, respectively, is set. */
+access_data::access_data (range_query *query, gimple *stmt, access_mode mode,
+ tree maxwrite /* = NULL_TREE */,
+ bool minwrite /* = false */,
+ tree maxread /* = NULL_TREE */,
+ bool minread /* = false */)
+ : stmt (stmt), call (), dst (), src (), mode (mode)
+{
+ set_bound (dst_bndrng, maxwrite, minwrite, query, stmt);
+ set_bound (src_bndrng, maxread, minread, query, stmt);
+}
+
+/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1
+ when MINWRITE or MINREAD, respectively, is set. */
+access_data::access_data (range_query *query, tree expr, access_mode mode,
+ tree maxwrite /* = NULL_TREE */,
+ bool minwrite /* = false */,
+ tree maxread /* = NULL_TREE */,
+ bool minread /* = false */)
+ : stmt (), call (expr), dst (), src (), mode (mode)
+{
+ set_bound (dst_bndrng, maxwrite, minwrite, query, stmt);
+ set_bound (src_bndrng, maxread, minread, query, stmt);
+}
+
+/* Set BNDRNG to the range of BOUND for the statement STMT. */
+
+void
+access_data::set_bound (offset_int bndrng[2], tree bound, bool minaccess,
+ range_query *query, gimple *stmt)
+{
+ /* Set the default bounds of the access and adjust below. */
+ bndrng[0] = minaccess ? 1 : 0;
+ bndrng[1] = HOST_WIDE_INT_M1U;
+
+ /* When BOUND is nonnull and a range can be extracted from it,
+ set the bounds of the access to reflect both it and MINACCESS.
+ BNDRNG[0] is the size of the minimum access. */
+ tree rng[2];
+ if (bound && get_size_range (query, bound, stmt, rng, SR_ALLOW_ZERO))
+ {
+ bndrng[0] = wi::to_offset (rng[0]);
+ bndrng[1] = wi::to_offset (rng[1]);
+ bndrng[0] = bndrng[0] > 0 && minaccess ? 1 : 0;
+ }
+}
+
/* Set a bit for the PHI in VISITED and return true if it wasn't
already set. */
@@ -1948,14 +1975,8 @@ compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref,
if (const access_ref *cache_ref = qry->get_ref (ptr))
{
/* If the pointer is in the cache set *PREF to what it refers
- to and return success.
- FIXME: BNDRNG is determined by each access and so it doesn't
- belong in access_ref. Until the design is changed, keep it
- unchanged here. */
- const offset_int bndrng[2] = { pref->bndrng[0], pref->bndrng[1] };
+ to and return success. */
*pref = *cache_ref;
- pref->bndrng[0] = bndrng[0];
- pref->bndrng[1] = bndrng[1];
return true;
}
}
diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h
index fbea3316f14..82cb81b3987 100644
--- a/gcc/pointer-query.h
+++ b/gcc/pointer-query.h
@@ -61,8 +61,7 @@ class pointer_query;
struct access_ref
{
/* Set the bounds of the reference. */
- access_ref (range_query *query = nullptr, tree = NULL_TREE,
- gimple * = nullptr, bool = false);
+ access_ref ();
/* Return the PHI node REF refers to or null if it doesn't. */
gphi *phi () const;
@@ -129,11 +128,6 @@ struct access_ref
offset_int sizrng[2];
/* The minimum and maximum offset computed. */
offset_int offmax[2];
- /* Range of the bound of the access: denotes that the access
- is at least BNDRNG[0] bytes but no more than BNDRNG[1].
- For string functions the size of the actual access is
- further constrained by the length of the string. */
- offset_int bndrng[2];
/* Used to fold integer expressions when called from front ends. */
tree (*eval)(tree);
@@ -206,23 +200,18 @@ struct access_data
{
/* Set the access to at most MAXWRITE and MAXREAD bytes, and
at least 1 when MINWRITE or MINREAD, respectively, is set. */
- access_data (range_query *query, gimple *stmt, access_mode mode,
- tree maxwrite = NULL_TREE, bool minwrite = false,
- tree maxread = NULL_TREE, bool minread = false)
- : stmt (stmt), call (),
- dst (query, maxwrite, stmt, minwrite),
- src (query, maxread, stmt, minread),
- mode (mode) { }
+ access_data (range_query *, gimple *, access_mode,
+ tree = NULL_TREE, bool = false,
+ tree = NULL_TREE, bool = false);
/* Set the access to at most MAXWRITE and MAXREAD bytes, and
at least 1 when MINWRITE or MINREAD, respectively, is set. */
- access_data (range_query *query, tree expr, access_mode mode,
- tree maxwrite = NULL_TREE, bool minwrite = false,
- tree maxread = NULL_TREE, bool minread = false)
- : stmt (), call (expr),
- dst (query, maxwrite, nullptr, minwrite),
- src (query, maxread, nullptr, minread),
- mode (mode) { }
+ access_data (range_query *, tree, access_mode,
+ tree = NULL_TREE, bool = false,
+ tree = NULL_TREE, bool = false);
+
+ /* Constructor helper. */
+ static void set_bound (offset_int[2], tree, bool, range_query *, gimple *);
/* Access statement. */
gimple *stmt;
@@ -230,6 +219,14 @@ struct access_data
tree call;
/* Destination and source of the access. */
access_ref dst, src;
+
+ /* Range of the bound of the access: denotes that the access is at
+ least XXX_BNDRNG[0] bytes but no more than XXX_BNDRNG[1]. For
+ string functions the size of the actual access is further
+ constrained by the length of the string. */
+ offset_int dst_bndrng[2];
+ offset_int src_bndrng[2];
+
/* Read-only for functions like memcmp or strlen, write-only
for memset, read-write for memcpy or strcat. */
access_mode mode;
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/5] fix up compute_objsize: pass GIMPLE statement to it
2021-12-04 0:00 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2] fix PR 103143 Martin Sebor
2021-12-06 17:31 ` [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data Martin Sebor
@ 2021-12-06 17:31 ` Martin Sebor
2021-12-08 18:48 ` Jeff Law
2021-12-06 17:32 ` [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling Martin Sebor
` (2 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 17:31 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3033 bytes --]
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
>
[-- Attachment #2: gcc-pointer_query-refactor-2.diff --]
[-- Type: text/x-patch, Size: 4551 bytes --]
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,
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling
2021-12-04 0:00 ` Jeff Law
` (2 preceding siblings ...)
2021-12-06 17:31 ` [PATCH v2 2/5] fix up compute_objsize: pass GIMPLE statement to it Martin Sebor
@ 2021-12-06 17:32 ` Martin Sebor
2021-12-08 20:08 ` Jeff Law
2021-12-06 17:32 ` [PATCH v2 4/5] fix up compute_objsize: refactor it into helpers Martin Sebor
2021-12-06 17:32 ` [PATCH v2 5/5] fix up compute_objsize: add a dump function Martin Sebor
5 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 17:32 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3012 bytes --]
Attached is subset of the patch in part (4) below: factor out
PHI handling. 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
>
[-- Attachment #2: gcc-pointer_query-refactor-3.diff --]
[-- Type: text/x-patch, Size: 9749 bytes --]
commit 6ac1d37947ad5cf07fe133faaf8414f00e0eed13
Author: Martin Sebor <msebor@redhat.com>
Date: Mon Dec 6 09:23:22 2021 -0700
Introduce access_ref::merge_ref.
gcc/ChangeLog:
* pointer-query.cc (access_ref::merge_ref): Define new function.
(access_ref::get_ref): Move code into merge_ref and call it.
* pointer-query.h (access_ref::merge_ref): Declare new function.
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index c75c4da6b60..24fbac84ec4 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -624,6 +624,97 @@ access_ref::phi () const
return as_a <gphi *> (def_stmt);
}
+/* Determine the size and offset for ARG, append it to ALL_REFS, and
+ merge the result with *THIS. Ignore ARG if SKIP_NULL is set and
+ ARG refers to the null pointer. Return true on success and false
+ on failure. */
+
+bool
+access_ref::merge_ref (vec<access_ref> *all_refs, tree arg, gimple *stmt,
+ int ostype, bool skip_null,
+ ssa_name_limit_t &snlim, pointer_query &qry)
+{
+ access_ref aref;
+ if (!compute_objsize_r (arg, stmt, ostype, &aref, snlim, &qry)
+ || aref.sizrng[0] < 0)
+ /* This may be a PHI with all null pointer arguments. */
+ return false;
+
+ if (all_refs)
+ {
+ access_ref dummy_ref;
+ aref.get_ref (all_refs, &dummy_ref, ostype, &snlim, &qry);
+ }
+
+ if (TREE_CODE (arg) == SSA_NAME)
+ qry.put_ref (arg, aref, ostype);
+
+ if (all_refs)
+ all_refs->safe_push (aref);
+
+ aref.deref += deref;
+
+ bool merged_parmarray = aref.parmarray;
+
+ const bool nullp = skip_null && integer_zerop (arg);
+ const offset_int maxobjsize = wi::to_offset (max_object_size ());
+ offset_int minsize = sizrng[0];
+
+ if (sizrng[0] < 0)
+ {
+ /* If *THIS doesn't contain a meaningful result yet set it to AREF
+ unless the argument is null and it's okay to ignore it. */
+ if (!nullp)
+ *this = aref;
+
+ /* Set if the current argument refers to one or more objects of
+ known size (or range of sizes), as opposed to referring to
+ one or more unknown object(s). */
+ const bool arg_known_size = (aref.sizrng[0] != 0
+ || aref.sizrng[1] != maxobjsize);
+ if (arg_known_size)
+ sizrng[0] = aref.sizrng[0];
+
+ return true;
+ }
+
+ /* Disregard null pointers in PHIs with two or more arguments.
+ TODO: Handle this better! */
+ if (nullp)
+ return true;
+
+ const bool known_size = (sizrng[0] != 0 || sizrng[1] != maxobjsize);
+
+ if (known_size && aref.sizrng[0] < minsize)
+ minsize = aref.sizrng[0];
+
+ /* Determine the amount of remaining space in the argument. */
+ offset_int argrem[2];
+ argrem[1] = aref.size_remaining (argrem);
+
+ /* Determine the amount of remaining space computed so far and
+ if the remaining space in the argument is more use it instead. */
+ offset_int merged_rem[2];
+ merged_rem[1] = size_remaining (merged_rem);
+
+ /* Reset the PHI's BASE0 flag if any of the nonnull arguments
+ refers to an object at an unknown offset. */
+ if (!aref.base0)
+ base0 = false;
+
+ if (merged_rem[1] < argrem[1]
+ || (merged_rem[1] == argrem[1]
+ && sizrng[1] < aref.sizrng[1]))
+ /* Use the argument with the most space remaining as the result,
+ or the larger one if the space is equal. */
+ *this = aref;
+
+ sizrng[0] = minsize;
+ parmarray = merged_parmarray;
+
+ return true;
+}
+
/* Determine and return the largest object to which *THIS refers. If
*THIS refers to a PHI and PREF is nonnull, fill *PREF with the details
of the object determined by compute_objsize(ARG, OSTYPE) for each PHI
@@ -636,9 +727,8 @@ access_ref::get_ref (vec<access_ref> *all_refs,
ssa_name_limit_t *psnlim /* = NULL */,
pointer_query *qry /* = NULL */) const
{
- gphi *phi_stmt = this->phi ();
- if (!phi_stmt)
- return ref;
+ if (!ref || TREE_CODE (ref) != SSA_NAME)
+ return NULL;
/* FIXME: Calling get_ref() with a null PSNLIM is dangerous and might
cause unbounded recursion. */
@@ -646,13 +736,49 @@ access_ref::get_ref (vec<access_ref> *all_refs,
if (!psnlim)
psnlim = &snlim_buf;
- if (!psnlim->visit_phi (ref))
- return NULL_TREE;
-
pointer_query empty_qry;
if (!qry)
qry = &empty_qry;
+ if (gimple *def_stmt = SSA_NAME_DEF_STMT (ref))
+ {
+ if (is_gimple_assign (def_stmt))
+ {
+ tree_code code = gimple_assign_rhs_code (def_stmt);
+ if (code != MIN_EXPR && code != MAX_EXPR)
+ return NULL_TREE;
+
+ access_ref aref;
+ tree arg1 = gimple_assign_rhs1 (def_stmt);
+ if (!aref.merge_ref (all_refs, arg1, def_stmt, ostype, false,
+ *psnlim, *qry))
+ return NULL_TREE;
+
+ tree arg2 = gimple_assign_rhs2 (def_stmt);
+ if (!aref.merge_ref (all_refs, arg2, def_stmt, ostype, false,
+ *psnlim, *qry))
+ return NULL_TREE;
+
+ if (pref && pref != this)
+ {
+ tree ref = pref->ref;
+ *pref = aref;
+ pref->ref = ref;
+ }
+
+ return aref.ref;
+ }
+ }
+ else
+ return NULL_TREE;
+
+ gphi *phi_stmt = this->phi ();
+ if (!phi_stmt)
+ return ref;
+
+ if (!psnlim->visit_phi (ref))
+ return NULL_TREE;
+
/* The conservative result of the PHI reflecting the offset and size
of the largest PHI argument, regardless of whether or not they all
refer to the same object. */
@@ -670,91 +796,17 @@ access_ref::get_ref (vec<access_ref> *all_refs,
phi_ref = *pref;
}
- /* Set if any argument is a function array (or VLA) parameter not
- declared [static]. */
- bool parmarray = false;
- /* The size of the smallest object referenced by the PHI arguments. */
- offset_int minsize = 0;
- const offset_int maxobjsize = wi::to_offset (max_object_size ());
-
const unsigned nargs = gimple_phi_num_args (phi_stmt);
for (unsigned i = 0; i < nargs; ++i)
{
access_ref phi_arg_ref;
+ bool skip_null = i || i + 1 < nargs;
tree arg = gimple_phi_arg_def (phi_stmt, i);
- if (!compute_objsize_r (arg, phi_stmt, ostype, &phi_arg_ref, *psnlim,
- qry)
- || phi_arg_ref.sizrng[0] < 0)
- /* A PHI with all null pointer arguments. */
+ if (!phi_ref.merge_ref (all_refs, arg, phi_stmt, ostype, skip_null,
+ *psnlim, *qry))
return NULL_TREE;
-
- if (TREE_CODE (arg) == SSA_NAME)
- qry->put_ref (arg, phi_arg_ref);
-
- if (all_refs)
- all_refs->safe_push (phi_arg_ref);
-
- parmarray |= phi_arg_ref.parmarray;
-
- const bool nullp = integer_zerop (arg) && (i || i + 1 < nargs);
-
- if (phi_ref.sizrng[0] < 0)
- {
- /* If PHI_REF doesn't contain a meaningful result yet set it
- to the result for the first argument. */
- if (!nullp)
- phi_ref = phi_arg_ref;
-
- /* Set if the current argument refers to one or more objects of
- known size (or range of sizes), as opposed to referring to
- one or more unknown object(s). */
- const bool arg_known_size = (phi_arg_ref.sizrng[0] != 0
- || phi_arg_ref.sizrng[1] != maxobjsize);
- if (arg_known_size)
- minsize = phi_arg_ref.sizrng[0];
-
- continue;
- }
-
- const bool phi_known_size = (phi_ref.sizrng[0] != 0
- || phi_ref.sizrng[1] != maxobjsize);
-
- if (phi_known_size && phi_arg_ref.sizrng[0] < minsize)
- minsize = phi_arg_ref.sizrng[0];
-
- /* Disregard null pointers in PHIs with two or more arguments.
- TODO: Handle this better! */
- if (nullp)
- continue;
-
- /* Determine the amount of remaining space in the argument. */
- offset_int argrem[2];
- argrem[1] = phi_arg_ref.size_remaining (argrem);
-
- /* Determine the amount of remaining space computed so far and
- if the remaining space in the argument is more use it instead. */
- offset_int phirem[2];
- phirem[1] = phi_ref.size_remaining (phirem);
-
- /* Reset the PHI's BASE0 flag if any of the nonnull arguments
- refers to an object at an unknown offset. */
- if (!phi_arg_ref.base0)
- phi_ref.base0 = false;
-
- if (phirem[1] < argrem[1]
- || (phirem[1] == argrem[1]
- && phi_ref.sizrng[1] < phi_arg_ref.sizrng[1]))
- /* Use the argument with the most space remaining as the result,
- or the larger one if the space is equal. */
- phi_ref = phi_arg_ref;
}
- /* Replace the lower bound of the largest argument with the size
- of the smallest argument, and set PARMARRAY if any argument
- was one. */
- phi_ref.sizrng[0] = minsize;
- phi_ref.parmarray = parmarray;
-
if (phi_ref.sizrng[0] < 0)
{
/* Fail if none of the PHI's arguments resulted in updating PHI_REF
@@ -766,7 +818,14 @@ access_ref::get_ref (vec<access_ref> *all_refs,
/* Avoid changing *THIS. */
if (pref && pref != this)
- *pref = phi_ref;
+ {
+ /* Keep the SSA_NAME of the PHI unchanged so that all PHI arguments
+ can be referred to later if necessary. This is useful even if
+ they all refer to the same object. */
+ tree ref = pref->ref;
+ *pref = phi_ref;
+ pref->ref = ref;
+ }
psnlim->leave_phi (ref);
diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h
index cbc87c86ed3..fe46f711e96 100644
--- a/gcc/pointer-query.h
+++ b/gcc/pointer-query.h
@@ -66,6 +66,10 @@ struct access_ref
/* Return the PHI node REF refers to or null if it doesn't. */
gphi *phi () const;
+ /* Merge the result for a pointer with *THIS. */
+ bool merge_ref (vec<access_ref> *all_refs, tree, gimple *, int, bool,
+ ssa_name_limit_t &, pointer_query &);
+
/* Return the object to which REF refers. */
tree get_ref (vec<access_ref> *, access_ref * = nullptr, int = 1,
ssa_name_limit_t * = nullptr, pointer_query * = nullptr) const;
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/5] fix up compute_objsize: refactor it into helpers
2021-12-04 0:00 ` Jeff Law
` (3 preceding siblings ...)
2021-12-06 17:32 ` [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling Martin Sebor
@ 2021-12-06 17:32 ` Martin Sebor
2021-12-08 19:12 ` Jeff Law
2021-12-06 17:32 ` [PATCH v2 5/5] fix up compute_objsize: add a dump function Martin Sebor
5 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 17:32 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3032 bytes --]
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
>
[-- Attachment #2: gcc-pointer_query-refactor-4.diff --]
[-- Type: text/x-patch, Size: 36227 bytes --]
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.
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 24fbac84ec4..3f583110c71 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -43,7 +43,7 @@
#include "tree-ssanames.h"
#include "target.h"
-static bool compute_objsize_r (tree, gimple *, int, access_ref *,
+static bool compute_objsize_r (tree, gimple *, bool, int, access_ref *,
ssa_name_limit_t &, pointer_query *);
/* Wrapper around the wide_int overload of get_range that accepts
@@ -199,7 +199,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
of the source object. */
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry)
+ if (compute_objsize_r (src, stmt, false, 1, &aref, snlim, qry)
&& aref.sizrng[1] < offrng[1])
offrng[1] = aref.sizrng[1];
}
@@ -233,7 +233,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
{
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry))
+ if (compute_objsize_r (src, stmt, false, 1, &aref, snlim, qry))
offrng[1] = aref.sizrng[1] - 1;
else
offrng[1] = HOST_WIDE_INT_M1U;
@@ -258,7 +258,7 @@ gimple_call_return_array (gimple *stmt, offset_int offrng[2], bool *past_end,
of the source object. */
access_ref aref;
tree src = gimple_call_arg (stmt, 1);
- if (compute_objsize_r (src, stmt, 1, &aref, snlim, qry)
+ if (compute_objsize_r (src, stmt, false, 1, &aref, snlim, qry)
&& aref.sizrng[1] < offrng[1])
offrng[1] = aref.sizrng[1];
}
@@ -635,7 +635,7 @@ access_ref::merge_ref (vec<access_ref> *all_refs, tree arg, gimple *stmt,
ssa_name_limit_t &snlim, pointer_query &qry)
{
access_ref aref;
- if (!compute_objsize_r (arg, stmt, ostype, &aref, snlim, &qry)
+ if (!compute_objsize_r (arg, stmt, false, ostype, &aref, snlim, &qry)
|| aref.sizrng[0] < 0)
/* This may be a PHI with all null pointer arguments. */
return false;
@@ -997,42 +997,45 @@ void access_ref::add_offset (const offset_int &min, const offset_int &max)
WRITE is set for a write access and clear for a read access. */
void
-access_ref::inform_access (access_mode mode) const
+access_ref::inform_access (access_mode mode, int ostype /* = 1 */) const
{
const access_ref &aref = *this;
if (!aref.ref)
return;
- if (aref.phi ())
+ if (phi ())
{
/* Set MAXREF to refer to the largest object and fill ALL_REFS
with data for all objects referenced by the PHI arguments. */
access_ref maxref;
auto_vec<access_ref> all_refs;
- if (!get_ref (&all_refs, &maxref))
+ if (!get_ref (&all_refs, &maxref, ostype))
return;
- /* Except for MAXREF, the rest of the arguments' offsets need not
- reflect one added to the PHI itself. Determine the latter from
- MAXREF on which the result is based. */
- const offset_int orng[] =
+ if (all_refs.length ())
{
- offrng[0] - maxref.offrng[0],
- wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
- };
+ /* Except for MAXREF, the rest of the arguments' offsets need not
+ reflect one added to the PHI itself. Determine the latter from
+ MAXREF on which the result is based. */
+ const offset_int orng[] =
+ {
+ offrng[0] - maxref.offrng[0],
+ wi::smax (offrng[1] - maxref.offrng[1], offrng[0]),
+ };
- /* Add the final PHI's offset to that of each of the arguments
- and recurse to issue an inform message for it. */
- for (unsigned i = 0; i != all_refs.length (); ++i)
- {
- /* Skip any PHIs; those could lead to infinite recursion. */
- if (all_refs[i].phi ())
- continue;
+ /* Add the final PHI's offset to that of each of the arguments
+ and recurse to issue an inform message for it. */
+ for (unsigned i = 0; i != all_refs.length (); ++i)
+ {
+ /* Skip any PHIs; those could lead to infinite recursion. */
+ if (all_refs[i].phi ())
+ continue;
- all_refs[i].add_offset (orng[0], orng[1]);
- all_refs[i].inform_access (mode);
+ all_refs[i].add_offset (orng[0], orng[1]);
+ all_refs[i].inform_access (mode, ostype);
+ }
+ return;
}
- return;
}
/* Convert offset range and avoid including a zero range since it
@@ -1244,7 +1247,7 @@ access_data::access_data (range_query *query, gimple *stmt, access_mode mode,
bool minwrite /* = false */,
tree maxread /* = NULL_TREE */,
bool minread /* = false */)
- : stmt (stmt), call (), dst (), src (), mode (mode)
+ : stmt (stmt), call (), dst (), src (), mode (mode), ostype ()
{
set_bound (dst_bndrng, maxwrite, minwrite, query, stmt);
set_bound (src_bndrng, maxread, minread, query, stmt);
@@ -1257,7 +1260,7 @@ access_data::access_data (range_query *query, tree expr, access_mode mode,
bool minwrite /* = false */,
tree maxread /* = NULL_TREE */,
bool minread /* = false */)
- : stmt (), call (expr), dst (), src (), mode (mode)
+ : stmt (), call (expr), dst (), src (), mode (mode), ostype ()
{
set_bound (dst_bndrng, maxwrite, minwrite, query, stmt);
set_bound (src_bndrng, maxread, minread, query, stmt);
@@ -1406,7 +1409,8 @@ pointer_query::get_ref (tree ptr, int ostype /* = 1 */) const
there or compute it and insert it into the cache if it's nonnonull. */
bool
-pointer_query::get_ref (tree ptr, gimple *stmt, access_ref *pref, int ostype /* = 1 */)
+pointer_query::get_ref (tree ptr, gimple *stmt, access_ref *pref,
+ int ostype /* = 1 */)
{
const unsigned version
= TREE_CODE (ptr) == SSA_NAME ? SSA_NAME_VERSION (ptr) : 0;
@@ -1606,7 +1610,7 @@ handle_min_max_size (tree ptr, int ostype, access_ref *pref,
for the expression. */
access_ref aref[2] = { *pref, *pref };
tree arg1 = gimple_assign_rhs1 (stmt);
- if (!compute_objsize_r (arg1, stmt, ostype, &aref[0], snlim, qry))
+ if (!compute_objsize_r (arg1, stmt, false, ostype, &aref[0], snlim, qry))
{
aref[0].base0 = false;
aref[0].offrng[0] = aref[0].offrng[1] = 0;
@@ -1615,7 +1619,7 @@ handle_min_max_size (tree ptr, int ostype, access_ref *pref,
}
tree arg2 = gimple_assign_rhs2 (stmt);
- if (!compute_objsize_r (arg2, stmt, ostype, &aref[1], snlim, qry))
+ if (!compute_objsize_r (arg2, stmt, false, ostype, &aref[1], snlim, qry))
{
aref[1].base0 = false;
aref[1].offrng[0] = aref[1].offrng[1] = 0;
@@ -1678,6 +1682,46 @@ handle_min_max_size (tree ptr, int ostype, access_ref *pref,
return true;
}
+/* A helper of compute_objsize_r() to determine the size of a DECL.
+ Return true on success and (possibly in the future) false on failure. */
+
+static bool
+handle_decl (tree decl, bool addr, access_ref *pref)
+{
+ tree decl_type = TREE_TYPE (decl);
+
+ pref->ref = decl;
+
+ /* Reset the offset in case it was set by a prior call and not
+ cleared by the caller. The offset is only adjusted after
+ the identity of the object has been determined. */
+ pref->offrng[0] = pref->offrng[1] = 0;
+
+ if (!addr && POINTER_TYPE_P (decl_type))
+ {
+ /* Set the maximum size if the reference is to the pointer
+ itself (as opposed to what it points to), and clear
+ BASE0 since the offset isn't necessarily zero-based. */
+ pref->set_max_size_range ();
+ pref->base0 = false;
+ return true;
+ }
+
+ /* Valid offsets into the object are nonnegative. */
+ pref->base0 = true;
+
+ if (tree size = decl_init_size (decl, false))
+ if (TREE_CODE (size) == INTEGER_CST)
+ {
+ pref->sizrng[0] = wi::to_offset (size);
+ pref->sizrng[1] = pref->sizrng[0];
+ return true;
+ }
+
+ pref->set_max_size_range ();
+ return true;
+}
+
/* A helper of compute_objsize_r() to determine the size from ARRAY_REF
AREF. ADDR is true if PTR is the operand of ADDR_EXPR. Return true
on success and false on failure. */
@@ -1689,8 +1733,6 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
{
gcc_assert (TREE_CODE (aref) == ARRAY_REF);
- ++pref->deref;
-
tree arefop = TREE_OPERAND (aref, 0);
tree reftype = TREE_TYPE (arefop);
if (!addr && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
@@ -1698,7 +1740,7 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
of known bound. */
return false;
- if (!compute_objsize_r (arefop, stmt, ostype, pref, snlim, qry))
+ if (!compute_objsize_r (arefop, stmt, addr, ostype, pref, snlim, qry))
return false;
offset_int orng[2];
@@ -1759,6 +1801,96 @@ handle_array_ref (tree aref, gimple *stmt, bool addr, int ostype,
return true;
}
+/* Given a COMPONENT_REF CREF, set *PREF size to the size of the referenced
+ member. */
+
+static void
+set_component_ref_size (tree cref, access_ref *pref)
+{
+ const tree base = TREE_OPERAND (cref, 0);
+ const tree base_type = TREE_TYPE (base);
+
+ /* SAM is set for array members that might need special treatment. */
+ special_array_member sam;
+ tree size = component_ref_size (cref, &sam);
+ if (sam == special_array_member::int_0)
+ pref->sizrng[0] = pref->sizrng[1] = 0;
+ else if (!pref->trail1special && sam == special_array_member::trail_1)
+ pref->sizrng[0] = pref->sizrng[1] = 1;
+ else if (size && TREE_CODE (size) == INTEGER_CST)
+ pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
+ else
+ {
+ /* When the size of the member is unknown it's either a flexible
+ array member or a trailing special array member (either zero
+ length or one-element). Set the size to the maximum minus
+ the constant size of the base object's type. */
+ pref->sizrng[0] = 0;
+ pref->sizrng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
+ if (tree base_size = TYPE_SIZE_UNIT (base_type))
+ if (TREE_CODE (base_size) == INTEGER_CST)
+ pref->sizrng[1] -= wi::to_offset (base_size);
+ }
+}
+
+/* A helper of compute_objsize_r() to determine the size from COMPONENT_REF
+ CREF. Return true on success and false on failure. */
+
+static bool
+handle_component_ref (tree cref, gimple *stmt, bool addr, int ostype,
+ access_ref *pref, ssa_name_limit_t &snlim,
+ pointer_query *qry)
+{
+ gcc_assert (TREE_CODE (cref) == COMPONENT_REF);
+
+ const tree base = TREE_OPERAND (cref, 0);
+ const tree base_type = TREE_TYPE (base);
+ if (TREE_CODE (base_type) == UNION_TYPE)
+ /* In accesses through union types consider the entire unions
+ rather than just their members. */
+ ostype = 0;
+
+ tree field = TREE_OPERAND (cref, 1);
+
+ if (ostype == 0)
+ {
+ /* In OSTYPE zero (for raw memory functions like memcpy), use
+ the maximum size instead if the identity of the enclosing
+ object cannot be determined. */
+ if (!compute_objsize_r (base, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ /* Otherwise, use the size of the enclosing object and add
+ the offset of the member to the offset computed so far. */
+ tree offset = byte_position (field);
+ if (TREE_CODE (offset) == INTEGER_CST)
+ pref->add_offset (wi::to_offset (offset));
+ else
+ pref->add_max_offset ();
+
+ if (!pref->ref)
+ /* PREF->REF may have been already set to an SSA_NAME earlier
+ to provide better context for diagnostics. In that case,
+ leave it unchanged. */
+ pref->ref = base;
+
+ return true;
+ }
+
+ pref->ref = field;
+
+ if (!addr && POINTER_TYPE_P (TREE_TYPE (field)))
+ {
+ /* Set maximum size if the reference is to the pointer member
+ itself (as opposed to what it points to). */
+ pref->set_max_size_range ();
+ return true;
+ }
+
+ set_component_ref_size (cref, pref);
+ return true;
+}
+
/* A helper of compute_objsize_r() to determine the size from MEM_REF
MREF. Return true on success and false on failure. */
@@ -1768,10 +1900,9 @@ handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
{
gcc_assert (TREE_CODE (mref) == MEM_REF);
- ++pref->deref;
-
- if (VECTOR_TYPE_P (TREE_TYPE (mref)))
- {
+ tree mreftype = TYPE_MAIN_VARIANT (TREE_TYPE (mref));
+ if (VECTOR_TYPE_P (mreftype))
+ {
/* Hack: Handle MEM_REFs of vector types as those to complete
objects; those may be synthesized from multiple assignments
to consecutive data members (see PR 93200 and 96963).
@@ -1785,9 +1916,11 @@ handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
}
tree mrefop = TREE_OPERAND (mref, 0);
- if (!compute_objsize_r (mrefop, stmt, ostype, pref, snlim, qry))
+ if (!compute_objsize_r (mrefop, stmt, false, ostype, pref, snlim, qry))
return false;
+ ++pref->deref;
+
offset_int orng[2];
tree off = pref->eval (TREE_OPERAND (mref, 1));
range_query *const rvals = qry ? qry->rvals : NULL;
@@ -1802,168 +1935,283 @@ handle_mem_ref (tree mref, gimple *stmt, int ostype, access_ref *pref,
return true;
}
-/* Helper to compute the size of the object referenced by the PTR
- expression which must have pointer type, using Object Size type
- OSTYPE (only the least significant 2 bits are used).
- On success, sets PREF->REF to the DECL of the referenced object
- if it's unique, otherwise to null, PREF->OFFRNG to the range of
- offsets into it, and PREF->SIZRNG to the range of sizes of
- the object(s).
- SNLIM is used to avoid visiting the same PHI operand multiple
- times, and, when nonnull, RVALS to determine range information.
- Returns true on success, false when a meaningful size (or range)
- cannot be determined.
-
- The function is intended for diagnostics and should not be used
- to influence code generation or optimization. */
+/* A helper of compute_objsize_r() to determine the size from SSA_NAME
+ PTR. Return true on success and false on failure. */
static bool
-compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref,
- ssa_name_limit_t &snlim, pointer_query *qry)
+handle_ssa_name (tree ptr, bool addr, int ostype,
+ access_ref *pref, ssa_name_limit_t &snlim,
+ pointer_query *qry)
{
- STRIP_NOPS (ptr);
+ if (!snlim.next ())
+ return false;
- const bool addr = TREE_CODE (ptr) == ADDR_EXPR;
- if (addr)
+ /* Only process an SSA_NAME if the recursion limit has not yet
+ been reached. */
+ if (qry)
{
- --pref->deref;
- ptr = TREE_OPERAND (ptr, 0);
+ if (++qry->depth > qry->max_depth)
+ qry->max_depth = qry->depth;
+ if (const access_ref *cache_ref = qry->get_ref (ptr, ostype))
+ {
+ /* Add the number of DEREFerences accummulated so far. */
+ const int deref = pref->deref;
+ *pref = *cache_ref;
+ pref->deref += deref;
+ return true;
+ }
}
- if (DECL_P (ptr))
- {
- pref->ref = ptr;
-
- /* Reset the offset in case it was set by a prior call and not
- cleared by the caller. The offset is only adjusted after
- the identity of the object has been determined. */
- pref->offrng[0] = pref->offrng[1] = 0;
+ gimple *stmt = SSA_NAME_DEF_STMT (ptr);
+ if (is_gimple_call (stmt))
+ {
+ /* If STMT is a call to an allocation function get the size
+ from its argument(s). If successful, also set *PREF->REF
+ to PTR for the caller to include in diagnostics. */
+ wide_int wr[2];
+ range_query *const rvals = qry ? qry->rvals : NULL;
+ if (gimple_call_alloc_size (stmt, wr, rvals))
+ {
+ pref->ref = ptr;
+ pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
+ pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
+ /* Constrain both bounds to a valid size. */
+ offset_int maxsize = wi::to_offset (max_object_size ());
+ if (pref->sizrng[0] > maxsize)
+ pref->sizrng[0] = maxsize;
+ if (pref->sizrng[1] > maxsize)
+ pref->sizrng[1] = maxsize;
+ }
+ else
+ {
+ /* For functions known to return one of their pointer arguments
+ try to determine what the returned pointer points to, and on
+ success add OFFRNG which was set to the offset added by
+ the function (e.g., memchr) to the overall offset. */
+ bool past_end;
+ offset_int offrng[2];
+ if (tree ret = gimple_call_return_array (stmt, offrng, &past_end,
+ snlim, qry))
+ {
+ if (!compute_objsize_r (ret, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ /* Cap OFFRNG[1] to at most the remaining size of
+ the object. */
+ offset_int remrng[2];
+ remrng[1] = pref->size_remaining (remrng);
+ if (remrng[1] != 0 && !past_end)
+ /* Decrement the size for functions that never return
+ a past-the-end pointer. */
+ remrng[1] -= 1;
+
+ if (remrng[1] < offrng[1])
+ offrng[1] = remrng[1];
+ pref->add_offset (offrng[0], offrng[1]);
+ }
+ else
+ {
+ /* For other calls that might return arbitrary pointers
+ including into the middle of objects set the size
+ range to maximum, clear PREF->BASE0, and also set
+ PREF->REF to include in diagnostics. */
+ pref->set_max_size_range ();
+ pref->base0 = false;
+ pref->ref = ptr;
+ }
+ }
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- if (!addr && POINTER_TYPE_P (TREE_TYPE (ptr)))
+ if (gimple_nop_p (stmt))
+ {
+ /* For a function argument try to determine the byte size
+ of the array from the current function declaratation
+ (e.g., attribute access or related). */
+ wide_int wr[2];
+ bool static_array = false;
+ if (tree ref = gimple_parm_array_size (ptr, wr, &static_array))
{
- /* Set the maximum size if the reference is to the pointer
- itself (as opposed to what it points to), and clear
- BASE0 since the offset isn't necessarily zero-based. */
- pref->set_max_size_range ();
- pref->base0 = false;
+ pref->parmarray = !static_array;
+ pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
+ pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
+ pref->ref = ref;
+ qry->put_ref (ptr, *pref, ostype);
return true;
}
- /* Valid offsets into the object are nonnegative. */
- pref->base0 = true;
-
- if (tree size = decl_init_size (ptr, false))
- if (TREE_CODE (size) == INTEGER_CST)
- {
- pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
- return true;
- }
-
pref->set_max_size_range ();
+ pref->base0 = false;
+ pref->ref = ptr;
+ qry->put_ref (ptr, *pref, ostype);
return true;
}
- const tree_code code = TREE_CODE (ptr);
- range_query *const rvals = qry ? qry->rvals : NULL;
-
- if (code == BIT_FIELD_REF)
+ if (gimple_code (stmt) == GIMPLE_PHI)
{
- tree ref = TREE_OPERAND (ptr, 0);
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
+ /* Pass PTR to get_ref() via PREF. If all PHI arguments refer
+ to the same object the function will replace it with it. */
+ pref->ref = ptr;
+ access_ref phi_ref = *pref;
+ if (!pref->get_ref (NULL, &phi_ref, ostype, &snlim, qry))
return false;
+ *pref = phi_ref;
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- offset_int off = wi::to_offset (pref->eval (TREE_OPERAND (ptr, 2)));
- pref->add_offset (off / BITS_PER_UNIT);
+ if (!is_gimple_assign (stmt))
+ {
+ /* Clear BASE0 since the assigned pointer might point into
+ the middle of the object, set the maximum size range and,
+ if the SSA_NAME refers to a function argumnent, set
+ PREF->REF to it. */
+ pref->base0 = false;
+ pref->set_max_size_range ();
+ pref->ref = ptr;
return true;
}
- if (code == COMPONENT_REF)
+ tree_code code = gimple_assign_rhs_code (stmt);
+
+ if (code == MAX_EXPR || code == MIN_EXPR)
{
- tree ref = TREE_OPERAND (ptr, 0);
- if (TREE_CODE (TREE_TYPE (ref)) == UNION_TYPE)
- /* In accesses through union types consider the entire unions
- rather than just their members. */
- ostype = 0;
- tree field = TREE_OPERAND (ptr, 1);
+ if (!handle_min_max_size (ptr, ostype, pref, snlim, qry))
+ return false;
- if (ostype == 0)
- {
- /* In OSTYPE zero (for raw memory functions like memcpy), use
- the maximum size instead if the identity of the enclosing
- object cannot be determined. */
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
- return false;
-
- /* Otherwise, use the size of the enclosing object and add
- the offset of the member to the offset computed so far. */
- tree offset = byte_position (field);
- if (TREE_CODE (offset) == INTEGER_CST)
- pref->add_offset (wi::to_offset (offset));
- else
- pref->add_max_offset ();
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- if (!pref->ref)
- /* REF may have been already set to an SSA_NAME earlier
- to provide better context for diagnostics. In that case,
- leave it unchanged. */
- pref->ref = ref;
- return true;
- }
+ tree rhs = gimple_assign_rhs1 (stmt);
- pref->ref = field;
+ if (code == ASSERT_EXPR)
+ {
+ rhs = TREE_OPERAND (rhs, 0);
+ return compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry);
+ }
- if (!addr && POINTER_TYPE_P (TREE_TYPE (field)))
- {
- /* Set maximum size if the reference is to the pointer member
- itself (as opposed to what it points to). */
- pref->set_max_size_range ();
- return true;
- }
+ if (code == POINTER_PLUS_EXPR
+ && TREE_CODE (TREE_TYPE (rhs)) == POINTER_TYPE)
+ {
+ /* Compute the size of the object first. */
+ if (!compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry))
+ return false;
- /* SAM is set for array members that might need special treatment. */
- special_array_member sam;
- tree size = component_ref_size (ptr, &sam);
- if (sam == special_array_member::int_0)
- pref->sizrng[0] = pref->sizrng[1] = 0;
- else if (!pref->trail1special && sam == special_array_member::trail_1)
- pref->sizrng[0] = pref->sizrng[1] = 1;
- else if (size && TREE_CODE (size) == INTEGER_CST)
- pref->sizrng[0] = pref->sizrng[1] = wi::to_offset (size);
+ offset_int orng[2];
+ tree off = gimple_assign_rhs2 (stmt);
+ range_query *const rvals = qry ? qry->rvals : NULL;
+ if (get_offset_range (off, stmt, orng, rvals))
+ pref->add_offset (orng[0], orng[1]);
else
- {
- /* When the size of the member is unknown it's either a flexible
- array member or a trailing special array member (either zero
- length or one-element). Set the size to the maximum minus
- the constant size of the type. */
- pref->sizrng[0] = 0;
- pref->sizrng[1] = wi::to_offset (TYPE_MAX_VALUE (ptrdiff_type_node));
- if (tree recsize = TYPE_SIZE_UNIT (TREE_TYPE (ref)))
- if (TREE_CODE (recsize) == INTEGER_CST)
- pref->sizrng[1] -= wi::to_offset (recsize);
- }
+ pref->add_max_offset ();
+
+ qry->put_ref (ptr, *pref, ostype);
return true;
}
- if (code == ARRAY_REF)
- return handle_array_ref (ptr, stmt, addr, ostype, pref, snlim, qry);
-
- if (code == MEM_REF)
- return handle_mem_ref (ptr, stmt, ostype, pref, snlim, qry);
+ if (code == ADDR_EXPR || code == SSA_NAME)
+ {
+ if (!compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+ qry->put_ref (ptr, *pref, ostype);
+ return true;
+ }
- if (code == TARGET_MEM_REF)
+ if (ostype > 1 && POINTER_TYPE_P (TREE_TYPE (rhs)))
{
- tree ref = TREE_OPERAND (ptr, 0);
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
+ /* When determining the qualifiers follow the pointer but
+ avoid caching the result. As the pointer is added to
+ and/or dereferenced the computed size and offset need
+ not be meaningful for other queries involving the same
+ pointer. */
+ if (!compute_objsize_r (rhs, stmt, addr, ostype, pref, snlim, qry))
return false;
- /* TODO: Handle remaining operands. Until then, add maximum offset. */
- pref->ref = ptr;
- pref->add_max_offset ();
- return true;
+ rhs = pref->ref;
}
- if (code == INTEGER_CST)
+ /* (This could also be an assignment from a nonlocal pointer.) Save
+ PTR to mention in diagnostics but otherwise treat it as a pointer
+ to an unknown object. */
+ pref->ref = rhs;
+ pref->base0 = false;
+ pref->set_max_size_range ();
+ return true;
+}
+
+/* Helper to compute the size of the object referenced by the PTR
+ expression which must have pointer type, using Object Size type
+ OSTYPE (only the least significant 2 bits are used).
+ On success, sets PREF->REF to the DECL of the referenced object
+ if it's unique, otherwise to null, PREF->OFFRNG to the range of
+ offsets into it, and PREF->SIZRNG to the range of sizes of
+ the object(s).
+ ADDR is true for an enclosing ADDR_EXPR.
+ SNLIM is used to avoid visiting the same PHI operand multiple
+ times, and, when nonnull, RVALS to determine range information.
+ Returns true on success, false when a meaningful size (or range)
+ cannot be determined.
+
+ The function is intended for diagnostics and should not be used
+ to influence code generation or optimization. */
+
+static bool
+compute_objsize_r (tree ptr, gimple *stmt, bool addr, int ostype,
+ access_ref *pref, ssa_name_limit_t &snlim,
+ pointer_query *qry)
+{
+ STRIP_NOPS (ptr);
+
+ if (DECL_P (ptr))
+ return handle_decl (ptr, addr, pref);
+
+ switch (TREE_CODE (ptr))
{
+ case ADDR_EXPR:
+ {
+ tree ref = TREE_OPERAND (ptr, 0);
+ if (!compute_objsize_r (ref, stmt, true, ostype, pref, snlim, qry))
+ return false;
+
+ --pref->deref;
+ return true;
+ }
+
+ case BIT_FIELD_REF:
+ {
+ tree ref = TREE_OPERAND (ptr, 0);
+ if (!compute_objsize_r (ref, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ offset_int off = wi::to_offset (pref->eval (TREE_OPERAND (ptr, 2)));
+ pref->add_offset (off / BITS_PER_UNIT);
+ return true;
+ }
+
+ case ARRAY_REF:
+ return handle_array_ref (ptr, stmt, addr, ostype, pref, snlim, qry);
+
+ case COMPONENT_REF:
+ return handle_component_ref (ptr, stmt, addr, ostype, pref, snlim, qry);
+
+ case MEM_REF:
+ return handle_mem_ref (ptr, stmt, ostype, pref, snlim, qry);
+
+ case TARGET_MEM_REF:
+ {
+ tree ref = TREE_OPERAND (ptr, 0);
+ if (!compute_objsize_r (ref, stmt, addr, ostype, pref, snlim, qry))
+ return false;
+
+ /* TODO: Handle remaining operands. Until then, add maximum offset. */
+ pref->ref = ptr;
+ pref->add_max_offset ();
+ return true;
+ }
+
+ case INTEGER_CST:
/* Pointer constants other than null are most likely the result
of erroneous null pointer addition/subtraction. Unless zero
is a valid address set size to zero. For null pointers, set
@@ -1984,21 +2232,17 @@ compute_objsize_r (tree ptr, gimple *stmt, int ostype, access_ref *pref,
pref->sizrng[0] = pref->sizrng[1] = 0;
pref->ref = ptr;
-
return true;
- }
- if (code == STRING_CST)
- {
+ case STRING_CST:
pref->sizrng[0] = pref->sizrng[1] = TREE_STRING_LENGTH (ptr);
pref->ref = ptr;
return true;
- }
- if (code == POINTER_PLUS_EXPR)
+ case POINTER_PLUS_EXPR:
{
tree ref = TREE_OPERAND (ptr, 0);
- if (!compute_objsize_r (ref, stmt, ostype, pref, snlim, qry))
+ if (!compute_objsize_r (ref, stmt, addr, ostype, pref, snlim, qry))
return false;
/* Clear DEREF since the offset is being applied to the target
@@ -2007,200 +2251,22 @@ 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, stmt, orng, rvals))
+ if (get_offset_range (off, stmt, orng, qry->rvals))
pref->add_offset (orng[0], orng[1]);
else
pref->add_max_offset ();
return true;
}
- if (code == VIEW_CONVERT_EXPR)
- {
+ case VIEW_CONVERT_EXPR:
ptr = TREE_OPERAND (ptr, 0);
- return compute_objsize_r (ptr, stmt, ostype, pref, snlim, qry);
- }
+ return compute_objsize_r (ptr, stmt, addr, ostype, pref, snlim, qry);
- if (code == SSA_NAME)
- {
- if (!snlim.next ())
- return false;
+ case SSA_NAME:
+ return handle_ssa_name (ptr, addr, ostype, pref, snlim, qry);
- /* Only process an SSA_NAME if the recursion limit has not yet
- been reached. */
- if (qry)
- {
- if (++qry->depth)
- qry->max_depth = qry->depth;
- if (const access_ref *cache_ref = qry->get_ref (ptr))
- {
- /* If the pointer is in the cache set *PREF to what it refers
- to and return success. */
- *pref = *cache_ref;
- return true;
- }
- }
-
- stmt = SSA_NAME_DEF_STMT (ptr);
- if (is_gimple_call (stmt))
- {
- /* If STMT is a call to an allocation function get the size
- from its argument(s). If successful, also set *PREF->REF
- to PTR for the caller to include in diagnostics. */
- wide_int wr[2];
- if (gimple_call_alloc_size (stmt, wr, rvals))
- {
- pref->ref = ptr;
- pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
- pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
- /* Constrain both bounds to a valid size. */
- offset_int maxsize = wi::to_offset (max_object_size ());
- if (pref->sizrng[0] > maxsize)
- pref->sizrng[0] = maxsize;
- if (pref->sizrng[1] > maxsize)
- pref->sizrng[1] = maxsize;
- }
- else
- {
- /* For functions known to return one of their pointer arguments
- try to determine what the returned pointer points to, and on
- success add OFFRNG which was set to the offset added by
- the function (e.g., memchr) to the overall offset. */
- bool past_end;
- offset_int offrng[2];
- if (tree ret = gimple_call_return_array (stmt, offrng,
- &past_end, snlim, qry))
- {
- if (!compute_objsize_r (ret, stmt, ostype, pref, snlim, qry))
- return false;
-
- /* Cap OFFRNG[1] to at most the remaining size of
- the object. */
- offset_int remrng[2];
- remrng[1] = pref->size_remaining (remrng);
- if (remrng[1] != 0 && !past_end)
- /* Decrement the size for functions that never return
- a past-the-end pointer. */
- remrng[1] -= 1;
-
- if (remrng[1] < offrng[1])
- offrng[1] = remrng[1];
- pref->add_offset (offrng[0], offrng[1]);
- }
- else
- {
- /* For other calls that might return arbitrary pointers
- including into the middle of objects set the size
- range to maximum, clear PREF->BASE0, and also set
- PREF->REF to include in diagnostics. */
- pref->set_max_size_range ();
- pref->base0 = false;
- pref->ref = ptr;
- }
- }
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (gimple_nop_p (stmt))
- {
- /* For a function argument try to determine the byte size
- of the array from the current function declaratation
- (e.g., attribute access or related). */
- wide_int wr[2];
- bool static_array = false;
- if (tree ref = gimple_parm_array_size (ptr, wr, &static_array))
- {
- pref->parmarray = !static_array;
- pref->sizrng[0] = offset_int::from (wr[0], UNSIGNED);
- pref->sizrng[1] = offset_int::from (wr[1], UNSIGNED);
- pref->ref = ref;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- pref->set_max_size_range ();
- pref->base0 = false;
- pref->ref = ptr;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (gimple_code (stmt) == GIMPLE_PHI)
- {
- pref->ref = ptr;
- access_ref phi_ref = *pref;
- if (!pref->get_ref (NULL, &phi_ref, ostype, &snlim, qry))
- return false;
- *pref = phi_ref;
- pref->ref = ptr;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (!is_gimple_assign (stmt))
- {
- /* Clear BASE0 since the assigned pointer might point into
- the middle of the object, set the maximum size range and,
- if the SSA_NAME refers to a function argumnent, set
- PREF->REF to it. */
- pref->base0 = false;
- pref->set_max_size_range ();
- pref->ref = ptr;
- return true;
- }
-
- tree_code code = gimple_assign_rhs_code (stmt);
-
- if (code == MAX_EXPR || code == MIN_EXPR)
- {
- if (!handle_min_max_size (ptr, ostype, pref, snlim, qry))
- return false;
-
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- tree rhs = gimple_assign_rhs1 (stmt);
-
- if (code == ASSERT_EXPR)
- {
- rhs = TREE_OPERAND (rhs, 0);
- return compute_objsize_r (rhs, stmt, ostype, pref, snlim, qry);
- }
-
- if (code == POINTER_PLUS_EXPR
- && TREE_CODE (TREE_TYPE (rhs)) == POINTER_TYPE)
- {
- /* Compute the size of the object first. */
- if (!compute_objsize_r (rhs, stmt, ostype, pref, snlim, qry))
- return false;
-
- offset_int orng[2];
- tree off = gimple_assign_rhs2 (stmt);
- if (get_offset_range (off, stmt, orng, rvals))
- pref->add_offset (orng[0], orng[1]);
- else
- pref->add_max_offset ();
-
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- if (code == ADDR_EXPR || code == SSA_NAME)
- {
- if (!compute_objsize_r (rhs, stmt, ostype, pref, snlim, qry))
- return false;
- qry->put_ref (ptr, *pref);
- return true;
- }
-
- /* (This could also be an assignment from a nonlocal pointer.) Save
- PTR to mention in diagnostics but otherwise treat it as a pointer
- to an unknown object. */
- pref->ref = rhs;
- pref->base0 = false;
- pref->set_max_size_range ();
- return true;
+ default:
+ break;
}
/* Assume all other expressions point into an unknown object
@@ -2231,7 +2297,7 @@ compute_objsize (tree ptr, gimple *stmt, int ostype, access_ref *pref,
pref->sizrng[0] = pref->sizrng[1] = -1;
ssa_name_limit_t snlim;
- if (!compute_objsize_r (ptr, stmt, ostype, pref, snlim, ptr_qry))
+ if (!compute_objsize_r (ptr, stmt, false, ostype, pref, snlim, ptr_qry))
return NULL_TREE;
offset_int maxsize = pref->size_remaining ();
diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h
index fe46f711e96..a7ac7d34370 100644
--- a/gcc/pointer-query.h
+++ b/gcc/pointer-query.h
@@ -122,7 +122,7 @@ struct access_ref
/* Issue an informational message describing the target of an access
with the given mode. */
- void inform_access (access_mode) const;
+ void inform_access (access_mode, int = 1) const;
/* Reference to the accessed object(s). */
tree ref;
@@ -234,6 +234,8 @@ struct access_data
/* Read-only for functions like memcmp or strlen, write-only
for memset, read-write for memcpy or strcat. */
access_mode mode;
+ /* The object size type. */
+ int ostype;
};
enum size_range_flags
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 5/5] fix up compute_objsize: add a dump function
2021-12-04 0:00 ` Jeff Law
` (4 preceding siblings ...)
2021-12-06 17:32 ` [PATCH v2 4/5] fix up compute_objsize: refactor it into helpers Martin Sebor
@ 2021-12-06 17:32 ` Martin Sebor
2021-12-08 19:15 ` Jeff Law
5 siblings, 1 reply; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 17:32 UTC (permalink / raw)
To: Jeff Law, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 3016 bytes --]
Attached is the subset of the patch in part (5) below: Add
a new dump function. It applies on top of patch 4/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
>
[-- Attachment #2: gcc-pointer_query-refactor-5.diff --]
[-- Type: text/x-patch, Size: 4700 bytes --]
commit 2054b01fb383560b96d51fabfe9dee6dbd611f4a
Author: Martin Sebor <msebor@redhat.com>
Date: Mon Dec 6 09:52:32 2021 -0700
Add a new dump function.
gcc/ChangeLog:
* pointer-query.cc (access_ref::dump): Define new function
(pointer_query::dump): Call it.
* pointer-query.h (access_ref::dump): Declare new function.
diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
index 3f583110c71..e618c4d7276 100644
--- a/gcc/pointer-query.cc
+++ b/gcc/pointer-query.cc
@@ -1240,6 +1240,63 @@ access_ref::inform_access (access_mode mode, int ostype /* = 1 */) const
sizestr, allocfn);
}
+/* Dump *THIS to FILE. */
+
+void
+access_ref::dump (FILE *file) const
+{
+ for (int i = deref; i < 0; ++i)
+ fputc ('&', file);
+
+ for (int i = 0; i < deref; ++i)
+ fputc ('*', file);
+
+ if (gphi *phi_stmt = phi ())
+ {
+ fputs ("PHI <", file);
+ unsigned nargs = gimple_phi_num_args (phi_stmt);
+ for (unsigned i = 0; i != nargs; ++i)
+ {
+ tree arg = gimple_phi_arg_def (phi_stmt, i);
+ print_generic_expr (file, arg);
+ if (i + 1 < nargs)
+ fputs (", ", file);
+ }
+ fputc ('>', file);
+ }
+ else
+ print_generic_expr (file, ref);
+
+ if (offrng[0] != offrng[1])
+ fprintf (file, " + [%lli, %lli]",
+ (long long) offrng[0].to_shwi (),
+ (long long) offrng[1].to_shwi ());
+ else if (offrng[0] != 0)
+ fprintf (file, " %c %lli",
+ offrng[0] < 0 ? '-' : '+',
+ (long long) offrng[0].to_shwi ());
+
+ if (base0)
+ fputs (" (base0)", file);
+
+ fputs ("; size: ", file);
+ if (sizrng[0] != sizrng[1])
+ {
+ offset_int maxsize = wi::to_offset (max_object_size ());
+ if (sizrng[0] == 0 && sizrng[1] >= maxsize)
+ fputs ("unknown", file);
+ else
+ fprintf (file, "[%llu, %llu]",
+ (unsigned long long) sizrng[0].to_uhwi (),
+ (unsigned long long) sizrng[1].to_uhwi ());
+ }
+ else if (sizrng[0] != 0)
+ fprintf (file, "%llu",
+ (unsigned long long) sizrng[0].to_uhwi ());
+
+ fputc ('\n', file);
+}
+
/* Set the access to at most MAXWRITE and MAXREAD bytes, and at least 1
when MINWRITE or MINREAD, respectively, is set. */
access_data::access_data (range_query *query, gimple *stmt, access_mode mode,
@@ -1498,6 +1555,9 @@ pointer_query::flush_cache ()
void
pointer_query::dump (FILE *dump_file, bool contents /* = false */)
{
+ if (!var_cache)
+ return;
+
unsigned nused = 0, nrefs = 0;
unsigned nidxs = var_cache->indices.length ();
for (unsigned i = 0; i != nidxs; ++i)
@@ -1558,35 +1618,40 @@ pointer_query::dump (FILE *dump_file, bool contents /* = false */)
else
fprintf (dump_file, " _%u = ", ver);
- if (gphi *phi = aref.phi ())
- {
- fputs ("PHI <", dump_file);
- unsigned nargs = gimple_phi_num_args (phi);
- for (unsigned i = 0; i != nargs; ++i)
- {
- tree arg = gimple_phi_arg_def (phi, i);
- print_generic_expr (dump_file, arg);
- if (i + 1 < nargs)
- fputs (", ", dump_file);
- }
- fputc ('>', dump_file);
- }
- else
- print_generic_expr (dump_file, aref.ref);
-
- if (aref.offrng[0] != aref.offrng[1])
- fprintf (dump_file, " + [%lli, %lli]",
- (long long) aref.offrng[0].to_shwi (),
- (long long) aref.offrng[1].to_shwi ());
- else if (aref.offrng[0] != 0)
- fprintf (dump_file, " %c %lli",
- aref.offrng[0] < 0 ? '-' : '+',
- (long long) aref.offrng[0].to_shwi ());
-
- fputc ('\n', dump_file);
+ aref.dump (dump_file);
}
fputc ('\n', dump_file);
+
+ {
+ fputs ("\npointer_query cache contents (again):\n", dump_file);
+
+ tree var;
+ unsigned i;
+ FOR_EACH_SSA_NAME (i, var, cfun)
+ {
+ if (TREE_CODE (TREE_TYPE (var)) != POINTER_TYPE)
+ continue;
+
+ for (unsigned ost = 0; ost != 2; ++ost)
+ {
+ if (const access_ref *cache_ref = get_ref (var, ost))
+ {
+ unsigned ver = SSA_NAME_VERSION (var);
+ fprintf (dump_file, " %u.%u: ", ver, ost);
+ if (tree name = ssa_name (ver))
+ {
+ print_generic_expr (dump_file, name);
+ fputs (" = ", dump_file);
+ }
+ else
+ fprintf (dump_file, " _%u = ", ver);
+
+ cache_ref->dump (dump_file);
+ }
+ }
+ }
+ }
}
/* A helper of compute_objsize_r() to determine the size from an assignment
diff --git a/gcc/pointer-query.h b/gcc/pointer-query.h
index a7ac7d34370..25101b75e25 100644
--- a/gcc/pointer-query.h
+++ b/gcc/pointer-query.h
@@ -124,6 +124,9 @@ struct access_ref
with the given mode. */
void inform_access (access_mode, int = 1) const;
+ /* Dump *THIS to a file. */
+ void dump (FILE *) const;
+
/* Reference to the accessed object(s). */
tree ref;
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] fix PR 103143
2021-12-06 17:31 ` [PATCH v2] fix PR 103143 Martin Sebor
@ 2021-12-06 20:14 ` Jeff Law
2021-12-06 21:44 ` Martin Sebor
0 siblings, 1 reply; 19+ messages in thread
From: Jeff Law @ 2021-12-06 20:14 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
On 12/6/2021 10:31 AM, Martin Sebor wrote:
> I have broken up the patch into a series of six. Attached is
> part (1), the fix for the typo that causes PR 103143.
>
> 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-103413.diff
>
> commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9
> Author: Martin Sebor<msebor@redhat.com>
> Date: Sat Dec 4 16:22:07 2021 -0700
>
> Use the recursive form of compute_objsize [PR 103143].
>
> gcc/ChangeLog:
>
> PR middle-end/103143
> * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r.
>
> gcc/testsuite/ChangeLog:
> PR middle-end/103143
> * gcc.dg/Wstringop-overflow-83.c: New test.
Thanks. Presumably by going through the public API, qry->depth got
reset to 0 and/or we'd get a re-initialized snlim at each call, leading
to the infinite recursion?
OK for the trunk. Thanks for breaking this out.
jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] fix PR 103143
2021-12-06 20:14 ` Jeff Law
@ 2021-12-06 21:44 ` Martin Sebor
0 siblings, 0 replies; 19+ messages in thread
From: Martin Sebor @ 2021-12-06 21:44 UTC (permalink / raw)
To: Jeff Law, gcc-patches
On 12/6/21 1:14 PM, Jeff Law wrote:
>
>
> On 12/6/2021 10:31 AM, Martin Sebor wrote:
>> I have broken up the patch into a series of six. Attached is
>> part (1), the fix for the typo that causes PR 103143.
>>
>> 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-103413.diff
>>
>> commit 9a5bb7a2b0cdb8654061d9cba543c1408fa7adc9
>> Author: Martin Sebor<msebor@redhat.com>
>> Date: Sat Dec 4 16:22:07 2021 -0700
>>
>> Use the recursive form of compute_objsize [PR 103143].
>>
>> gcc/ChangeLog:
>>
>> PR middle-end/103143
>> * pointer-query.cc (gimple_call_return_array): Call compute_objsize_r.
>>
>> gcc/testsuite/ChangeLog:
>> PR middle-end/103143
>> * gcc.dg/Wstringop-overflow-83.c: New test.
> Thanks. Presumably by going through the public API, qry->depth got
> reset to 0 and/or we'd get a re-initialized snlim at each call, leading
> to the infinite recursion?
Yes, each call creates a new ssa_name_limit_t::visited bitmap.
It might be better to make compute_objsize_r() a member of
the same class as the bitmap and also rename it, to make it
harder to make this mistake again.
Martin
>
> OK for the trunk. Thanks for breaking this out.
>
> jeff
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data
2021-12-06 17:31 ` [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data Martin Sebor
@ 2021-12-08 18:47 ` Jeff Law
0 siblings, 0 replies; 19+ messages in thread
From: Jeff Law @ 2021-12-08 18:47 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
On 12/6/2021 10:31 AM, Martin Sebor wrote:
> Attached is the subset of the patch in part (1) below: Move
> bndrng from access_ref to access_data.
>
> 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-1.diff
>
> commit 1062c1154beeeae26e86f053946ea733ffb5d136
> Author: Martin Sebor <msebor@redhat.com>
> Date: Sat Dec 4 16:46:17 2021 -0700
>
> Move bndrng from access_ref to access_data.
>
> gcc/ChangeLog:
>
> * gimple-ssa-warn-access.cc (check_access): Adjust to member name
> change.
> (pass_waccess::check_strncmp): Same.
> * pointer-query.cc (access_ref::access_ref): Remove arguments.
> Simpilfy.
> (access_data::access_data): Define new ctors.
> (access_data::set_bound): Define new member function.
> (compute_objsize_r): Remove unnecessary code.
> * pointer-query.h (struct access_ref): Remove ctor arguments.
> (struct access_data): Declare ctor overloads.
> (access_data::dst_bndrng): New member.
> (access_data::src_bndrng): New member.
OK. Thanks for breaking this out.
jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/5] fix up compute_objsize: pass GIMPLE statement to it
2021-12-06 17:31 ` [PATCH v2 2/5] fix up compute_objsize: pass GIMPLE statement to it Martin Sebor
@ 2021-12-08 18:48 ` Jeff Law
0 siblings, 0 replies; 19+ messages in thread
From: Jeff Law @ 2021-12-08 18:48 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/5] fix up compute_objsize: refactor it into helpers
2021-12-06 17:32 ` [PATCH v2 4/5] fix up compute_objsize: refactor it into helpers Martin Sebor
@ 2021-12-08 19:12 ` Jeff Law
0 siblings, 0 replies; 19+ messages in thread
From: Jeff Law @ 2021-12-08 19:12 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 5/5] fix up compute_objsize: add a dump function
2021-12-06 17:32 ` [PATCH v2 5/5] fix up compute_objsize: add a dump function Martin Sebor
@ 2021-12-08 19:15 ` Jeff Law
0 siblings, 0 replies; 19+ messages in thread
From: Jeff Law @ 2021-12-08 19:15 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
On 12/6/2021 10:32 AM, Martin Sebor wrote:
> Attached is the subset of the patch in part (5) below: Add
> a new dump function. It applies on top of patch 4/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-5.diff
>
> commit 2054b01fb383560b96d51fabfe9dee6dbd611f4a
> Author: Martin Sebor <msebor@redhat.com>
> Date: Mon Dec 6 09:52:32 2021 -0700
>
> Add a new dump function.
>
> gcc/ChangeLog:
>
> * pointer-query.cc (access_ref::dump): Define new function
> (pointer_query::dump): Call it.
> * pointer-query.h (access_ref::dump): Declare new function.
OK. I think it's worth also noting in the ChangeLog the additional
dumping you added with the "pointer_query cache contents (again)" hunk.
Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling
2021-12-06 17:32 ` [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling Martin Sebor
@ 2021-12-08 20:08 ` Jeff Law
2021-12-09 16:57 ` Martin Sebor
0 siblings, 1 reply; 19+ messages in thread
From: Jeff Law @ 2021-12-08 20:08 UTC (permalink / raw)
To: Martin Sebor, gcc-patches
On 12/6/2021 10:32 AM, Martin Sebor wrote:
> Attached is subset of the patch in part (4) below: factor out
> PHI handling. 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-3.diff
>
> commit 6ac1d37947ad5cf07fe133faaf8414f00e0eed13
> Author: Martin Sebor <msebor@redhat.com>
> Date: Mon Dec 6 09:23:22 2021 -0700
>
> Introduce access_ref::merge_ref.
>
> gcc/ChangeLog:
>
> * pointer-query.cc (access_ref::merge_ref): Define new function.
> (access_ref::get_ref): Move code into merge_ref and call it.
> * pointer-query.h (access_ref::merge_ref): Declare new function.
OK. But it's probably worth noting that this patch does more than just
factoring out the PHI handling. It also adds the MIN/MAX bits noted in
the original cover letter. That's not inherently as bad now that this
patch isn't intermixed with the other work.
>
> diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
> index c75c4da6b60..24fbac84ec4 100644
> --- a/gcc/pointer-query.cc
> +++ b/gcc/pointer-query.cc
>
>
>
>
> @ -766,7 +818,14 @@ access_ref::get_ref (vec<access_ref> *all_refs,
>
> /* Avoid changing *THIS. */
> if (pref && pref != this)
> - *pref = phi_ref;
> + {
> + /* Keep the SSA_NAME of the PHI unchanged so that all PHI arguments
> + can be referred to later if necessary. This is useful even if
> + they all refer to the same object. */
> + tree ref = pref->ref;
> + *pref = phi_ref;
> + pref->ref = ref;
> + }
I don't see any mention of this in the ChangeLog.
So I'm fine with the patch itself. I would just ask for a better
ChangeLog. If one was to read the current ChangeLog they could easily
be led to believe this patch was just refactoring, but it brings in
other changes as well.
Thanks,
Jeff
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling
2021-12-08 20:08 ` Jeff Law
@ 2021-12-09 16:57 ` Martin Sebor
0 siblings, 0 replies; 19+ messages in thread
From: Martin Sebor @ 2021-12-09 16:57 UTC (permalink / raw)
To: Jeff Law, gcc-patches
On 12/8/21 1:08 PM, Jeff Law wrote:
>
>
> On 12/6/2021 10:32 AM, Martin Sebor wrote:
>> Attached is subset of the patch in part (4) below: factor out
>> PHI handling. 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-3.diff
>>
>> commit 6ac1d37947ad5cf07fe133faaf8414f00e0eed13
>> Author: Martin Sebor <msebor@redhat.com>
>> Date: Mon Dec 6 09:23:22 2021 -0700
>>
>> Introduce access_ref::merge_ref.
>> gcc/ChangeLog:
>> * pointer-query.cc (access_ref::merge_ref): Define new
>> function.
>> (access_ref::get_ref): Move code into merge_ref and call it.
>> * pointer-query.h (access_ref::merge_ref): Declare new
>> function.
> OK. But it's probably worth noting that this patch does more than just
> factoring out the PHI handling. It also adds the MIN/MAX bits noted in
> the original cover letter. That's not inherently as bad now that this
> patch isn't intermixed with the other work.
Thank you for the review.
The MIN_MAX change was in the original ChangeLog but I wrote this
one from scratch and neglected to mention it here. Let me add it.
>>
>> diff --git a/gcc/pointer-query.cc b/gcc/pointer-query.cc
>> index c75c4da6b60..24fbac84ec4 100644
>> --- a/gcc/pointer-query.cc
>> +++ b/gcc/pointer-query.cc
>>
>>
>>
>>
>> @ -766,7 +818,14 @@ access_ref::get_ref (vec<access_ref> *all_refs,
>> /* Avoid changing *THIS. */
>> if (pref && pref != this)
>> - *pref = phi_ref;
>> + {
>> + /* Keep the SSA_NAME of the PHI unchanged so that all PHI
>> arguments
>> + can be referred to later if necessary. This is useful even if
>> + they all refer to the same object. */
>> + tree ref = pref->ref;
>> + *pref = phi_ref;
>> + pref->ref = ref;
>> + }
> I don't see any mention of this in the ChangeLog.
This only matters for informational notes, and it's necessary
because the new merge_ref() function might replace the ref
member with that of the "merged" object. It's needed to keep
the existing behavior where we want the informational notes
printed after a warning to point to all the objects that might
be subject to the out of bounds access. This is verified by
the Wstringop-overflow-58.c and -59.c tests.
> So I'm fine with the patch itself. I would just ask for a better
> ChangeLog. If one was to read the current ChangeLog they could easily
> be led to believe this patch was just refactoring, but it brings in
> other changes as well.
It wasn't my intention to change any observable behavior with
this change, and I don't think it does.
I was going to update the ChangeLog to mention the MIN/MAX part
above but I just ended up pushing it to the main repository by
mistake.
Martin
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-12-09 16:57 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-09 2:34 [PATCH] fix up compute_objsize (including PR 103143) Martin Sebor
2021-11-15 16:49 ` PING " Martin Sebor
2021-11-22 16:41 ` PING 2 " Martin Sebor
2021-11-29 15:49 ` PING 3 " Martin Sebor
2021-12-04 0:00 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2] fix PR 103143 Martin Sebor
2021-12-06 20:14 ` Jeff Law
2021-12-06 21:44 ` Martin Sebor
2021-12-06 17:31 ` [PATCH v2 1/5] fix up compute_objsize: move bndrng into access_data Martin Sebor
2021-12-08 18:47 ` Jeff Law
2021-12-06 17:31 ` [PATCH v2 2/5] fix up compute_objsize: pass GIMPLE statement to it Martin Sebor
2021-12-08 18:48 ` Jeff Law
2021-12-06 17:32 ` [PATCH v2 3/5] fix up compute_objsize: factor out PHI handling Martin Sebor
2021-12-08 20:08 ` Jeff Law
2021-12-09 16:57 ` Martin Sebor
2021-12-06 17:32 ` [PATCH v2 4/5] fix up compute_objsize: refactor it into helpers Martin Sebor
2021-12-08 19:12 ` Jeff Law
2021-12-06 17:32 ` [PATCH v2 5/5] fix up compute_objsize: add a dump function Martin Sebor
2021-12-08 19:15 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).