| Message ID | ZbdnuM9BwxTpA9MS@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Ensure iterator validity when updating debug uses [PR113616] | expand |
Alex Coplan <alex.coplan@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); > + } > +}
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); + } +}