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
>
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
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.
@@ -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
@@ -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;