On December 12, 2017 9:50:38 PM GMT+01:00, David Malcolm <dmalc...@redhat.com>
wrote:
>PR tree-optimization/83312 reports a false positive from
>-Warray-bounds.
>The root cause is that VRP both:
>
>(a) updates a GIMPLE_COND to be always false, and
>
>(b) updates an ARRAY_REF in the now-unreachable other path to use an
> ASSERT_EXPR with a negative index:
> def_stmt j_6 = ASSERT_EXPR <j_9, j_9 < 0>;
>
>When vrp_prop::check_all_array_refs () runs, the CFG hasn't yet been
>updated to take account of (a), and so a false positive is emitted
>when (b) is encountered.
>
>This patch fixes the false warning by converting
> vrp_prop::check_all_array_refs
>from a simple iteration over all BBs to use a new dom_walker subclass,
>using the "skip_unreachable_blocks = true" mechanism to avoid analyzing
>(b).
>
>There didn't seem to be a pre-existing way to determine the unique
>out-edge after a GIMPLE_COND (if it has a constant cond), so I added
>a new gimple_cond_get_unique_successor_edge function. Similarly,
>something similar may apply for switches, so I put in a
>gimple_get_unique_successor_edge (though I wasn't able to create a
>reproducer that used a switch).
>
>Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
>
>OK for trunk?
I don't like the GIMPLE.c bits a lot. Can't you use the existing known taken
edge helper (too lazy to look up from my phone...) basically splitting out some
code from cond processing in evrp for example?
The Dom walk itself looks like a good solution to me.
Richard.
>gcc/ChangeLog:
> PR tree-optimization/83312
> * domwalk.h (dom_walker::dom_walker): Fix typo in comment.
> * gimple.c: Include "tree-cfg.h".
> (gimple_get_unique_successor_edge): New function.
> (gimple_cond_get_unique_successor_edge): New function.
> * gimple.h (gimple_get_unique_successor_edge): New decl.
> (gimple_cond_get_unique_successor_edge): New decl.
> * tree-vrp.c (class check_array_bounds_dom_walker): New subclass
> of dom_walker.
> (vrp_prop::check_all_array_refs): Reimplement as...
> (check_array_bounds_dom_walker::before_dom_children): ...this new
> vfunc. Replace linear search through BB block list, excluding
> those with non-executable in-edges, with dominator walk.
>
>gcc/testsuite/ChangeLog:
> PR tree-optimization/83312
> * gcc.dg/pr83312.c: New test case.
>---
> gcc/domwalk.h | 2 +-
> gcc/gimple.c | 36 +++++++++++++++++++
> gcc/gimple.h | 2 ++
> gcc/testsuite/gcc.dg/pr83312.c | 30 ++++++++++++++++
>gcc/tree-vrp.c | 80
>+++++++++++++++++++++++++++---------------
> 5 files changed, 120 insertions(+), 30 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr83312.c
>
>diff --git a/gcc/domwalk.h b/gcc/domwalk.h
>index 6ac93eb..c7e3450 100644
>--- a/gcc/domwalk.h
>+++ b/gcc/domwalk.h
>@@ -32,7 +32,7 @@ class dom_walker
> public:
> static const edge STOP;
>
>- /* Use SKIP_UNREACHBLE_BLOCKS = true when your client can discover
>+ /* Use SKIP_UNREACHABLE_BLOCKS = true when your client can discover
> that some edges are not executable.
>
> If a client can discover that a COND, SWITCH or GOTO has a static
>diff --git a/gcc/gimple.c b/gcc/gimple.c
>index c986a73..e22fcda 100644
>--- a/gcc/gimple.c
>+++ b/gcc/gimple.c
>@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see
> #include "stringpool.h"
> #include "attribs.h"
> #include "asan.h"
>+#include "tree-cfg.h"
>
>
>/* All the tuples have their operand vector (if present) at the very
>bottom
>@@ -3087,6 +3088,41 @@ gimple_inexpensive_call_p (gcall *stmt)
> return false;
> }
>
>+/* If STMT terminates its basic block, determine if it has a uniquely
>+ valid successor edge and if so return it.
>+
>+ Otherwise, return NULL. */
>+
>+edge
>+gimple_get_unique_successor_edge (const gimple *stmt)
>+{
>+ switch (gimple_code (stmt))
>+ {
>+ case GIMPLE_COND:
>+ return gimple_cond_get_unique_successor_edge
>+ (as_a <const gcond *> (stmt));
>+ default:
>+ return NULL;
>+ }
>+}
>+
>+/* Determine if COND has a uniquely valid successor edge and if so
>return it.
>+
>+ Otherwise, return NULL. */
>+
>+edge
>+gimple_cond_get_unique_successor_edge (const gcond *cond)
>+{
>+ edge te, fe;
>+ extract_true_false_edges_from_block (gimple_bb (cond), &te, &fe);
>+ if (gimple_cond_true_p (cond))
>+ return te;
>+ else if (gimple_cond_false_p (cond))
>+ return fe;
>+ else
>+ return NULL;
>+}
>+
> #if CHECKING_P
>
> namespace selftest {
>diff --git a/gcc/gimple.h b/gcc/gimple.h
>index 0fcdd05..ab4cb8b 100644
>--- a/gcc/gimple.h
>+++ b/gcc/gimple.h
>@@ -1529,6 +1529,8 @@ extern void gimple_seq_discard (gimple_seq);
>extern void maybe_remove_unused_call_args (struct function *, gimple
>*);
> extern bool gimple_inexpensive_call_p (gcall *);
> extern bool stmt_can_terminate_bb_p (gimple *);
>+extern edge gimple_get_unique_successor_edge (const gimple *stmt);
>+extern edge gimple_cond_get_unique_successor_edge (const gcond *cond);
>
>/* Formal (expression) temporary table handling: multiple occurrences
>of
> the same scalar expression are evaluated into the same temporary. */
>diff --git a/gcc/testsuite/gcc.dg/pr83312.c
>b/gcc/testsuite/gcc.dg/pr83312.c
>new file mode 100644
>index 0000000..2eb241d
>--- /dev/null
>+++ b/gcc/testsuite/gcc.dg/pr83312.c
>@@ -0,0 +1,30 @@
>+/* { dg-options "-O2 -Warray-bounds" } */
>+
>+struct ptlrpcd_ctl {
>+ char pc_name[20];
>+};
>+struct ptlrpcd {
>+ struct ptlrpcd_ctl pd_threads[6];
>+};
>+struct ptlrpcd *ptlrpcd_init_pd;
>+static void ptlrpcd_ctl_init(struct ptlrpcd_ctl *pc, int index) {
>+ if (index < 0)
>+ __builtin_snprintf(pc->pc_name, sizeof(pc->pc_name),
>"ptlrpcd_rcv");
>+ else
>+ __builtin_snprintf(pc->pc_name, sizeof(pc->pc_name), "ptlrpcd_%d",
>index);
>+}
>+int ptlrpcd_init_ncpts;
>+static int ptlrpcd_init(int nthreads) {
>+ int j;
>+ if (ptlrpcd_init_ncpts) {
>+ ptlrpcd_ctl_init(&ptlrpcd_init_pd->pd_threads[0], -1);
>+ for (j = 1; j < nthreads; j++)
>+ ptlrpcd_ctl_init(&ptlrpcd_init_pd->pd_threads[j], j);
>+ }
>+ return 0;
>+}
>+int ptlrpcd_init_groupsize;
>+void ptlrpcd_addref(void) {
>+ ptlrpcd_init(ptlrpcd_init_groupsize);
>+}
>+
>diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
>index a86b382..67bc118 100644
>--- a/gcc/tree-vrp.c
>+++ b/gcc/tree-vrp.c
>@@ -5000,44 +5000,66 @@ check_array_bounds (tree *tp, int
>*walk_subtree, void *data)
> return NULL_TREE;
> }
>
>-/* Walk over all statements of all reachable BBs and call
>check_array_bounds
>- on them. */
>+/* A dom_walker subclass for use by vrp_prop::check_all_array_refs,
>+ to walk over all statements of all reachable BBs and call
>+ check_array_bounds on them. */
>
>-void
>-vrp_prop::check_all_array_refs ()
>+class check_array_bounds_dom_walker : public dom_walker
> {
>- basic_block bb;
>- gimple_stmt_iterator si;
>+ public:
>+ check_array_bounds_dom_walker (vrp_prop *prop)
>+ : dom_walker (CDI_DOMINATORS, true), m_prop (prop) {}
>+ ~check_array_bounds_dom_walker () {}
>
>- FOR_EACH_BB_FN (bb, cfun)
>- {
>- edge_iterator ei;
>- edge e;
>- bool executable = false;
>+ edge before_dom_children (basic_block) FINAL OVERRIDE;
>
>- /* Skip blocks that were found to be unreachable. */
>- FOR_EACH_EDGE (e, ei, bb->preds)
>- executable |= !!(e->flags & EDGE_EXECUTABLE);
>- if (!executable)
>- continue;
>+ private:
>+ vrp_prop *m_prop;
>+};
>
>- for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
>- {
>- gimple *stmt = gsi_stmt (si);
>- struct walk_stmt_info wi;
>- if (!gimple_has_location (stmt)
>- || is_gimple_debug (stmt))
>- continue;
>+/* Implementation of dom_walker::before_dom_children.
>+
>+ Walk over all statements of BB and call check_array_bounds on them,
>+ and determine if there's a unique successor edge. */
>
>- memset (&wi, 0, sizeof (wi));
>+edge
>+check_array_bounds_dom_walker::before_dom_children (basic_block bb)
>+{
>+ gimple_stmt_iterator si;
>+ for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
>+ {
>+ gimple *stmt = gsi_stmt (si);
>+ struct walk_stmt_info wi;
>+ if (!gimple_has_location (stmt)
>+ || is_gimple_debug (stmt))
>+ continue;
>
>- wi.info = this;
>+ memset (&wi, 0, sizeof (wi));
>
>- walk_gimple_op (gsi_stmt (si),
>- check_array_bounds,
>- &wi);
>- }
>+ wi.info = m_prop;
>+
>+ walk_gimple_op (stmt, check_array_bounds, &wi);
> }
>+
>+ /* Determine if there's a unique successor edge, and if so, return
>+ that back to dom_walker, ensuring that we don't visit blocks that
>+ became unreachable during the VRP propagation
>+ (PR tree-optimization/83312). */
>+ gimple *last = last_stmt (bb);
>+ if (last)
>+ return gimple_get_unique_successor_edge (last);
>+
>+ return NULL;
>+}
>+
>+/* Walk over all statements of all reachable BBs and call
>check_array_bounds
>+ on them. */
>+
>+void
>+vrp_prop::check_all_array_refs ()
>+{
>+ check_array_bounds_dom_walker w (this);
>+ w.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
> }
>
> /* Return true if all imm uses of VAR are either in STMT, or
