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
commit 2054b01fb383560b96d51fabfe9dee6dbd611f4a
Author: Martin Sebor <mse...@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;
