[v2,3/5] fix up compute_objsize: factor out PHI handling
Commit Message
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
>
Comments
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
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
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.
@@ -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);
@@ -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;