| Message ID | cd074c78-6efe-49c8-7b66-1c6a71adfca2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fix PR 103143 | expand |
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.
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;