[gcc r14-8496] aarch64: Ensure iterator validity when updating debug uses [PR113616]

[gcc r14-8496] aarch64: Ensure iterator validity when updating debug uses [PR113616]

[Alex Coplan]

https://gcc.gnu.org/g:d41a1873f334cf29b9a595bb03c27bff2be17319

commit r14-8496-gd41a1873f334cf29b9a595bb03c27bff2be17319
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Mon Jan 29 13:28:04 2024 +0000

    aarch64: Ensure iterator validity when updating debug uses [PR113616]
    
    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.
    
    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.

Diff:
---
 gcc/config/aarch64/aarch64-ldp-fusion.cc       | 47 +++++++----------
 gcc/iterator-utils.h                           | 73 ++++++++++++++++++++++++++
 gcc/testsuite/gcc.c-torture/compile/pr113616.c | 19 +++++++
 3 files changed, 112 insertions(+), 27 deletions(-)

diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
index 932a6398ae30..22ed95eb743c 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 a3f7dd5384d9..af1463b0cfbf 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 000000000000..04c38eadffb5
--- /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);
+    }
+}