Alex Coplan <alex.cop...@arm.com> writes:
> Hi,
>
> The fix for PR113089 introduced range-based for loops over the
> debug_insn_uses of an RTL-SSA set_info, but in the case that we reset a
> debug insn, the use would get removed from the use list, and thus we
> would end up using an invalidated iterator in the next iteration of the
> loop. In practice this means we end up terminating the loop
> prematurely, and hence ICE as in PR113089 since there are debug uses
> that we failed to fix up.
>
> This patch fixes that by introducing a general mechanism to avoid this
> sort of problem. We introduce a safe_iterator to iterator-utils.h which
> wraps an iterator, and also holds the end iterator value. It then
> pre-computes the next iterator value at all iterations, so it doesn't
> matter if the original iterator got invalidated during the loop body, we
> can still move safely to the next iteration.
>
> We introduce an iterate_safely helper which effectively adapts a
> container such as iterator_range into a container of safe_iterators over
> the original iterator type.
>
> We then use iterate_safely around all loops over debug_insn_uses () in
> the aarch64 ldp/stp pass to fix PR113616. While doing this, I
> remembered that cleanup_tombstones () had the same problem. I
> previously worked around this locally by manually maintaining the next
> nondebug insn, so this patch also refactors that loop to use the new
> iterate_safely helper.
>
> While doing that I noticed that a couple of cases in cleanup_tombstones
> could be converted from using dyn_cast<set_info *> to as_a<set_info *>,
> which should be safe because there are no clobbers of mem in RTL-SSA, so
> all defs of memory should be set_infos.
>
> Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
>
> Thanks,
> Alex
>
> gcc/ChangeLog:
>
> PR target/113616
> * config/aarch64/aarch64-ldp-fusion.cc (fixup_debug_uses_trailing_add):
> Use iterate_safely when iterating over debug uses.
> (fixup_debug_uses): Likewise.
> (ldp_bb_info::cleanup_tombstones): Use iterate_safely to iterate
> over nondebug insns instead of manually maintaining the next insn.
> * iterator-utils.h (class safe_iterator): New.
> (iterate_safely): New.
>
> gcc/testsuite/ChangeLog:
>
> PR target/113616
> * gcc.c-torture/compile/pr113616.c: New test.
OK, thanks.
Richard
> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> index 932a6398ae3..22ed95eb743 100644
> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> @@ -1480,7 +1480,7 @@ fixup_debug_uses_trailing_add (obstack_watermark
> &attempt,
> def_info *def = defs[0];
>
> if (auto set = safe_dyn_cast<set_info *> (def->prev_def ()))
> - for (auto use : set->debug_insn_uses ())
> + for (auto use : iterate_safely (set->debug_insn_uses ()))
> if (*use->insn () > *pair_dst)
> // DEF is getting re-ordered above USE, fix up USE accordingly.
> fixup_debug_use (attempt, use, def, base, wb_offset);
> @@ -1544,13 +1544,16 @@ fixup_debug_uses (obstack_watermark &attempt,
> auto def = memory_access (insns[0]->defs ());
> auto last_def = memory_access (insns[1]->defs ());
> for (; def != last_def; def = def->next_def ())
> - for (auto use : as_a<set_info *> (def)->debug_insn_uses ())
> - {
> - if (dump_file)
> - fprintf (dump_file, " i%d: resetting debug use of mem\n",
> - use->insn ()->uid ());
> - reset_debug_use (use);
> - }
> + {
> + auto set = as_a<set_info *> (def);
> + for (auto use : iterate_safely (set->debug_insn_uses ()))
> + {
> + if (dump_file)
> + fprintf (dump_file, " i%d: resetting debug use of mem\n",
> + use->insn ()->uid ());
> + reset_debug_use (use);
> + }
> + }
> }
>
> // Now let's take care of register uses, starting with debug uses
> @@ -1577,7 +1580,7 @@ fixup_debug_uses (obstack_watermark &attempt,
>
> // Now that we've characterized the defs involved, go through the
> // debug uses and determine how to update them (if needed).
> - for (auto use : set->debug_insn_uses ())
> + for (auto use : iterate_safely (set->debug_insn_uses ()))
> {
> if (*pair_dst < *use->insn () && defs[1])
> // We're re-ordering defs[1] above a previous use of the
> @@ -1609,7 +1612,7 @@ fixup_debug_uses (obstack_watermark &attempt,
>
> // We have a def in insns[1] which isn't def'd by the first insn.
> // Look to the previous def and see if it has any debug uses.
> - for (auto use : prev_set->debug_insn_uses ())
> + for (auto use : iterate_safely (prev_set->debug_insn_uses ()))
> if (*pair_dst < *use->insn ())
> // We're ordering DEF above a previous use of the same register.
> update_debug_use (use, def, writeback_pat);
> @@ -1622,7 +1625,8 @@ fixup_debug_uses (obstack_watermark &attempt,
> // second writeback def which need re-parenting: do that.
> auto def = find_access (insns[1]->defs (), base_regno);
> gcc_assert (def);
> - for (auto use : as_a<set_info *> (def)->debug_insn_uses ())
> + auto set = as_a<set_info *> (def);
> + for (auto use : iterate_safely (set->debug_insn_uses ()))
> {
> insn_change change (use->insn ());
> change.new_uses = check_remove_regno_access (attempt,
> @@ -2921,26 +2925,16 @@ ldp_bb_info::cleanup_tombstones ()
> if (!m_emitted_tombstone)
> return;
>
> - insn_info *insn = m_bb->head_insn ();
> - while (insn)
> + for (auto insn : iterate_safely (m_bb->nondebug_insns ()))
> {
> - insn_info *next = insn->next_nondebug_insn ();
> if (!insn->is_real ()
> || !bitmap_bit_p (&m_tombstone_bitmap, insn->uid ()))
> - {
> - insn = next;
> - continue;
> - }
> + continue;
>
> - auto def = memory_access (insn->defs ());
> - auto set = dyn_cast<set_info *> (def);
> - if (set && set->has_any_uses ())
> + auto set = as_a<set_info *> (memory_access (insn->defs ()));
> + if (set->has_any_uses ())
> {
> - def_info *prev_def = def->prev_def ();
> - auto prev_set = dyn_cast<set_info *> (prev_def);
> - if (!prev_set)
> - gcc_unreachable ();
> -
> + auto prev_set = as_a<set_info *> (set->prev_def ());
> while (set->first_use ())
> crtl->ssa->reparent_use (set->first_use (), prev_set);
> }
> @@ -2948,7 +2942,6 @@ ldp_bb_info::cleanup_tombstones ()
> // Now set has no uses, we can delete it.
> insn_change change (insn, insn_change::DELETE);
> crtl->ssa->change_insn (change);
> - insn = next;
> }
> }
>
> diff --git a/gcc/iterator-utils.h b/gcc/iterator-utils.h
> index a3f7dd5384d..af1463b0cfb 100644
> --- a/gcc/iterator-utils.h
> +++ b/gcc/iterator-utils.h
> @@ -200,4 +200,77 @@ list_iterator<T, Next>::operator++ (int)
> return ret;
> }
>
> +// An iterator that pre-computes the next value if we haven't already got to
> the
> +// end. This is useful in cases where a standard iterator would get
> invalidated
> +// (e.g. elements getting removed from a container) during the body of a
> loop.
> +template<typename T>
> +class safe_iterator
> +{
> + T m_current;
> + const T m_end;
> + T m_next;
> +
> + T get_next ()
> + {
> + if (m_current != m_end)
> + {
> + // FIXME: we should use std::next here but that would mean having
> + // #include <iterator> everywhere that iterator-utils.h is included.
> + //
> + // For now we just implement it directly.
> + T iter = m_current;
> + return ++iter;
> + }
> + return m_end;
> + }
> +
> + void advance ()
> + {
> + m_current = m_next;
> + if (m_next != m_end)
> + ++m_next;
> + }
> +
> +public:
> + bool operator== (const safe_iterator &other) const
> + {
> + return m_current == other.m_current;
> + }
> +
> + bool operator!= (const safe_iterator &other) const
> + {
> + return m_current != other.m_current;
> + }
> +
> + typename T::value_type operator*() const { return *m_current; }
> +
> + safe_iterator &operator++ ()
> + {
> + advance ();
> + return *this;
> + }
> +
> + safe_iterator operator++ (int)
> + {
> + auto ret = *this;
> + advance ();
> + return ret;
> + }
> +
> + safe_iterator (T iter, T end)
> + : m_current (iter), m_end (end), m_next (get_next ()) {}
> +};
> +
> +// Convert a container RANGE into a container of safe_iterators.
> +template<typename Container>
> +inline
> +iterator_range<safe_iterator<typename Container::const_iterator>>
> +iterate_safely (Container range)
> +{
> + return {
> + { range.begin (), range.end () },
> + { range.end (), range.end () }
> + };
> +}
> +
> #endif
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr113616.c
> b/gcc/testsuite/gcc.c-torture/compile/pr113616.c
> new file mode 100644
> index 00000000000..04c38eadffb
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr113616.c
> @@ -0,0 +1,19 @@
> +// { dg-do compile }
> +// { dg-options "-g" }
> +struct A { struct A *a; } foo ();
> +struct B { long b; };
> +struct C { struct B c; struct A d; } *e;
> +
> +void
> +bar (void)
> +{
> + int f;
> + struct C *g;
> + struct A *h;
> + for (g = 0, g = e ? (void *) e - (char) (__SIZE_TYPE__) &g->d : 0, h = g ?
> (&g->d)->a : 0; g;
> + g = 0, g = h ? (void *) h - (char) (__SIZE_TYPE__) &g->d : 0, h = h ?
> h->a : 0)
> + {
> + f = (int) (__SIZE_TYPE__) g;
> + foo (((struct B *) g)->b);
> + }
> +}
