[PATCH 4/6] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator

[Aaron Merey <amerey@redhat.com>]

To facilitate the deletion of objfiles, progspace objects use a safe
iterator that holds a reference to the next objfile in the progspace's
objfile_list.  This allows objfiles to be deleted in a loop without
invalidating the loop's iterator.  progspace also uses an unwrapping
iterator over std::unique_ptr<objfile> that automatically deferences
the unique_ptr.

This patch changes the objfile safe iterator to be a reverse safe
iterator.  It changes the unwrapping iterator into a template.  It
also modifies objfile_list insertion so that separate debug objfiles
are placed into the list after the parent objfile, instead of before.

These changes are intended to prepare gdb for on-demand debuginfo
downloading and the downloading of .gdb_index sections.

With on-demand downloading enabled, gdb might download a debuginfo
file during symtab expansion.  In this case an objfile could be added
to an objfiles_list during iteration over the list (for example, in
iterate_over_symtabs).  We want these loops to also iterate over newly
downloaded objfiles. So objfiles need to be inserted into objfiles_list
after their parent since it is during the search of the parent
objfile for some symbol or filename that the separate debug objfile
might be downloaded.

The unwrapping iterator is changed into a template in order to
use it with objfile qf_require_partial_symbols, which is now also
uses with a safe iterator.  This is because after a separate debug
objfile is downloaded on-demand, we want to remove any .gdb_index
quick_symbol_functions from the parent objfile during iteration over
the parent's quick_symbol_functions.  The newly downloaded separate
debug objfile contains the index and all of the related symbols
so the .gdb_index should not be associated with the parent objfile
any longer.

Finally a safe reverse iterator is now used during progspace objfile
deletion in order to prevent iterator invalidation during the loop
in which objfiles are deleted.  This could happen during forward
iteration over objfiles_list when a separate debug objfile immediately
follows it's parent objfile in the list (which is now possible since
objfiles are inserted into the list after their parent).  Deletion
of the parent would cause deletion of the separate debug objfile,
which would invalidate the safe forward iterator's reference to the
next objfile in the list.  A safe reverse iterator deletes separate
debug objfiles before their parent, so the iterator's reference to
the next objfile always stays valid.

A small change was also made to a testcase in py-objfile.exp to
account for the new placement of separate debug objfiles in
objfiles_list.
---
 gdb/objfiles.h                          |  22 +++-
 gdb/progspace.c                         |   8 +-
 gdb/progspace.h                         | 159 +++++++++++++++++++-----
 gdb/symfile-debug.c                     | 136 ++++++++++----------
 gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
 5 files changed, 218 insertions(+), 109 deletions(-)

diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 189856f0a51..bb7b0a4579d 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -613,6 +613,17 @@ struct objfile
   /* See quick_symbol_functions.  */
   void require_partial_symbols (bool verbose);
 
+  /* Remove TARGET from this objfile's collection of quick_symbol_functions.  */
+  void remove_partial_symbol (quick_symbol_functions *target)
+  {
+    for (quick_symbol_functions_up &qf_up : qf)
+      if (qf_up.get () == target)
+	{
+	  qf.remove (qf_up);
+	  return;
+	}
+  }
+
   /* Return the relocation offset applied to SECTION.  */
   CORE_ADDR section_offset (bfd_section *section) const
   {
@@ -699,13 +710,20 @@ struct objfile
 
 private:
 
+  using qf_list = std::forward_list<quick_symbol_functions_up>;
+  using unwrapping_qf_range = iterator_range<unwrapping_iterator<qf_list::iterator>>;
+  using qf_safe_range = basic_safe_range<unwrapping_qf_range>;
+
   /* Ensure that partial symbols have been read and return the "quick" (aka
      partial) symbol functions for this symbol reader.  */
-  const std::forward_list<quick_symbol_functions_up> &
+  qf_safe_range
   qf_require_partial_symbols ()
   {
     this->require_partial_symbols (true);
-    return qf;
+    return qf_safe_range
+      (unwrapping_qf_range
+	(unwrapping_iterator<qf_list::iterator> (qf.begin ()),
+	 unwrapping_iterator<qf_list::iterator> (qf.end ())));
   }
 
 public:
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 32bdfebcf7c..1ed75eef2f9 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -139,19 +139,19 @@ program_space::free_all_objfiles ()
 
 void
 program_space::add_objfile (std::unique_ptr<objfile> &&objfile,
-			    struct objfile *before)
+			    struct objfile *after)
 {
-  if (before == nullptr)
+  if (after == nullptr)
     objfiles_list.push_back (std::move (objfile));
   else
     {
       auto iter = std::find_if (objfiles_list.begin (), objfiles_list.end (),
 				[=] (const std::unique_ptr<::objfile> &objf)
 				{
-				  return objf.get () == before;
+				  return objf.get () == after;
 				});
       gdb_assert (iter != objfiles_list.end ());
-      objfiles_list.insert (iter, std::move (objfile));
+      objfiles_list.insert (++iter, std::move (objfile));
     }
 }
 
diff --git a/gdb/progspace.h b/gdb/progspace.h
index 85215f0e2f1..6e33e48c88e 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -40,56 +40,141 @@ struct address_space;
 struct program_space;
 struct so_list;
 
+/* An iterator that wraps an iterator over std::unique_ptr, and dereferences
+   the returned object.  This is useful for iterating over a list of shared
+   pointers and returning raw pointers -- which helped avoid touching a lot
+   of code when changing how objfiles are managed.  */
+
+template<typename UniquePtrIter>
+class unwrapping_iterator
+{
+public:
+  typedef unwrapping_iterator self_type;
+  typedef typename UniquePtrIter::value_type::pointer value_type;
+  typedef typename UniquePtrIter::reference  reference;
+  typedef typename UniquePtrIter::pointer pointer;
+  typedef typename UniquePtrIter::iterator_category iterator_category;
+  typedef typename UniquePtrIter::difference_type difference_type;
+
+  unwrapping_iterator (UniquePtrIter iter)
+    : m_iter (std::move (iter))
+  {
+  }
+
+  value_type operator* () const
+  {
+    return m_iter->get ();
+  }
+
+  unwrapping_iterator operator++ ()
+  {
+    ++m_iter;
+    return *this;
+  }
+
+  bool operator!= (const unwrapping_iterator &other) const
+  {
+    return m_iter != other.m_iter;
+  }
+
+private:
+  /* The underlying iterator.  */
+  UniquePtrIter m_iter;
+};
+
 typedef std::list<std::unique_ptr<objfile>> objfile_list;
 
-/* An iterator that wraps an iterator over std::unique_ptr<objfile>,
-   and dereferences the returned object.  This is useful for iterating
-   over a list of shared pointers and returning raw pointers -- which
-   helped avoid touching a lot of code when changing how objfiles are
-   managed.  */
+/* An reverse iterator that wraps an iterator over objfile_list, and
+   dereferences the returned object.  This is useful for reverse iterating
+   over a list of shared pointers and returning raw pointers -- which helped
+   avoid touching a lot of code when changing how objfiles are managed.  */
 
-class unwrapping_objfile_iterator
+class unwrapping_reverse_objfile_iterator
 {
 public:
-
-  typedef unwrapping_objfile_iterator self_type;
+  typedef unwrapping_reverse_objfile_iterator self_type;
   typedef typename ::objfile *value_type;
   typedef typename ::objfile &reference;
   typedef typename ::objfile **pointer;
   typedef typename objfile_list::iterator::iterator_category iterator_category;
   typedef typename objfile_list::iterator::difference_type difference_type;
 
-  unwrapping_objfile_iterator (objfile_list::iterator iter)
-    : m_iter (std::move (iter))
-  {
-  }
-
-  objfile *operator* () const
+  value_type operator* () const
   {
     return m_iter->get ();
   }
 
-  unwrapping_objfile_iterator operator++ ()
+  unwrapping_reverse_objfile_iterator operator++ ()
   {
-    ++m_iter;
+    if (m_iter != m_begin)
+      --m_iter;
+    else
+      {
+	/* We can't decrement M_ITER since it is the begin iterator of the
+	   objfile list.  Set M_ITER to the list's end iterator to indicate
+	   this is now one-past-the-end.  */
+	m_iter = m_end;
+
+	/* Overwrite M_BEGIN to avoid possibly copying an invalid iterator.  */
+	m_begin = m_end;
+      }
+
     return *this;
   }
 
-  bool operator!= (const unwrapping_objfile_iterator &other) const
+  bool operator!= (const unwrapping_reverse_objfile_iterator &other) const
   {
     return m_iter != other.m_iter;
   }
 
+  /* Return an unwrapping reverse iterator starting at the last element of
+     OBJF_LIST.  */
+  static unwrapping_reverse_objfile_iterator begin (objfile_list &objf_list)
+  {
+    auto begin = objf_list.begin ();
+    auto end = objf_list.end ();
+    auto rev_begin = objf_list.end ();
+
+    /* Start REV_BEGIN on the last objfile in OBJF_LIST.  */
+    if (begin != end)
+      --rev_begin;
+
+    return unwrapping_reverse_objfile_iterator (rev_begin, begin, end);
+  }
+
+  /* Return a one-past-the-end unwrapping reverse iterator.  */
+  static unwrapping_reverse_objfile_iterator end (objfile_list &objf_list)
+  {
+    return unwrapping_reverse_objfile_iterator (objf_list.end (),
+						objf_list.end (),
+						objf_list.end ());
+  }
+
 private:
+  /* begin and end methods should be used to create these objects.  */
+  unwrapping_reverse_objfile_iterator (objfile_list::iterator iter,
+				       objfile_list::iterator begin,
+				       objfile_list::iterator end)
+    : m_iter (std::move (iter)), m_begin (std::move (begin)),
+      m_end (std::move (end))
+  {
+  }
 
-  /* The underlying iterator.  */
-  objfile_list::iterator m_iter;
-};
+ /* The underlying iterator.  */
+ objfile_list::iterator m_iter;
 
+ /* The underlying iterator pointing to the first objfile in the sequence.  Used
+    to track when to stop decrementing M_ITER.  */
+ objfile_list::iterator m_begin;
 
-/* A range that returns unwrapping_objfile_iterators.  */
+  /* The underlying iterator's one-past-the-end.  */
+ objfile_list::iterator m_end;
+};
 
-using unwrapping_objfile_range = iterator_range<unwrapping_objfile_iterator>;
+/* A range that returns unwrapping_iterators.  */
+
+using unwrapping_objfile_range
+  = iterator_range<unwrapping_iterator<objfile_list::iterator>>;
 
 /* A program space represents a symbolic view of an address space.
    Roughly speaking, it holds all the data associated with a
@@ -209,11 +294,12 @@ struct program_space
   objfiles_range objfiles ()
   {
     return objfiles_range
-      (unwrapping_objfile_iterator (objfiles_list.begin ()),
-       unwrapping_objfile_iterator (objfiles_list.end ()));
+      (unwrapping_iterator<objfile_list::iterator> (objfiles_list.begin ()),
+       unwrapping_iterator<objfile_list::iterator> (objfiles_list.end ()));
   }
 
-  using objfiles_safe_range = basic_safe_range<objfiles_range>;
+  using objfiles_reverse_range = iterator_range<unwrapping_reverse_objfile_iterator>;
+  using objfiles_safe_reverse_range = basic_safe_range<objfiles_reverse_range>;
 
   /* An iterable object that can be used to iterate over all objfiles.
      The basic use is in a foreach, like:
@@ -221,20 +307,25 @@ struct program_space
      for (objfile *objf : pspace->objfiles_safe ()) { ... }
 
      This variant uses a basic_safe_iterator so that objfiles can be
-     deleted during iteration.  */
-  objfiles_safe_range objfiles_safe ()
+     deleted during iteration.
+
+     The use of a reverse iterator helps ensure that separate debug
+     objfiles are deleted before their parent objfile.  This prevents
+     the invalidation of an iterator due to the deletion of a parent
+     objfile.  */
+  objfiles_safe_reverse_range objfiles_safe ()
   {
-    return objfiles_safe_range
-      (objfiles_range
-	 (unwrapping_objfile_iterator (objfiles_list.begin ()),
-	  unwrapping_objfile_iterator (objfiles_list.end ())));
+    return objfiles_safe_reverse_range
+      (objfiles_reverse_range
+	(unwrapping_reverse_objfile_iterator::begin (objfiles_list),
+	 unwrapping_reverse_objfile_iterator::end (objfiles_list)));
   }
 
-  /* Add OBJFILE to the list of objfiles, putting it just before
-     BEFORE.  If BEFORE is nullptr, it will go at the end of the
+  /* Add OBJFILE to the list of objfiles, putting it just after
+     AFTER.  If AFTER is nullptr, it will go at the end of the
      list.  */
   void add_objfile (std::unique_ptr<objfile> &&objfile,
-		    struct objfile *before);
+		    struct objfile *after);
 
   /* Remove OBJFILE from the list of objfiles.  */
   void remove_objfile (struct objfile *objfile);
diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
index 9db5c47a8ce..784b81b5ca6 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -109,9 +109,9 @@ objfile::has_unexpanded_symtabs ()
 		objfile_debug_name (this));
 
   bool result = false;
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      if (iter->has_unexpanded_symtabs (this))
+      if (qf->has_unexpanded_symtabs (this))
 	{
 	  result = true;
 	  break;
@@ -134,9 +134,9 @@ objfile::find_last_source_symtab ()
     gdb_printf (gdb_stdlog, "qf->find_last_source_symtab (%s)\n",
 		objfile_debug_name (this));
 
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      retval = iter->find_last_source_symtab (this);
+      retval = qf->find_last_source_symtab (this);
       if (retval != nullptr)
 	break;
     }
@@ -167,8 +167,8 @@ objfile::forget_cached_source_info ()
 	}
     }
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->forget_cached_source_info (this);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->forget_cached_source_info (this);
 }
 
 bool
@@ -214,17 +214,17 @@ objfile::map_symtabs_matching_filename
     return result;
   };
 
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      if (!iter->expand_symtabs_matching (this,
-					  match_one_filename,
-					  nullptr,
-					  nullptr,
-					  on_expansion,
-					  (SEARCH_GLOBAL_BLOCK
-					   | SEARCH_STATIC_BLOCK),
-					  UNDEF_DOMAIN,
-					  ALL_DOMAIN))
+      if (!qf->expand_symtabs_matching (this,
+					match_one_filename,
+					nullptr,
+					nullptr,
+					on_expansion,
+					(SEARCH_GLOBAL_BLOCK
+					 | SEARCH_STATIC_BLOCK),
+					UNDEF_DOMAIN,
+					ALL_DOMAIN))
 	{
 	  retval = false;
 	  break;
@@ -283,18 +283,18 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
     return true;
   };
 
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      if (!iter->expand_symtabs_matching (this,
-					  nullptr,
-					  &lookup_name,
-					  nullptr,
-					  search_one_symtab,
-					  kind == GLOBAL_BLOCK
-					  ? SEARCH_GLOBAL_BLOCK
-					  : SEARCH_STATIC_BLOCK,
-					  domain,
-					  ALL_DOMAIN))
+      if (!qf->expand_symtabs_matching (this,
+					nullptr,
+					&lookup_name,
+					nullptr,
+					search_one_symtab,
+					kind == GLOBAL_BLOCK
+					? SEARCH_GLOBAL_BLOCK
+					: SEARCH_STATIC_BLOCK,
+					domain,
+					ALL_DOMAIN))
 	break;
     }
 
@@ -314,8 +314,8 @@ objfile::print_stats (bool print_bcache)
     gdb_printf (gdb_stdlog, "qf->print_stats (%s, %d)\n",
 		objfile_debug_name (this), print_bcache);
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->print_stats (this, print_bcache);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->print_stats (this, print_bcache);
 }
 
 void
@@ -340,16 +340,16 @@ objfile::expand_symtabs_for_function (const char *func_name)
   lookup_name_info base_lookup (func_name, symbol_name_match_type::FULL);
   lookup_name_info lookup_name = base_lookup.make_ignore_params ();
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->expand_symtabs_matching (this,
-				   nullptr,
-				   &lookup_name,
-				   nullptr,
-				   nullptr,
-				   (SEARCH_GLOBAL_BLOCK
-				    | SEARCH_STATIC_BLOCK),
-				   VAR_DOMAIN,
-				   ALL_DOMAIN);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->expand_symtabs_matching (this,
+				 nullptr,
+				 &lookup_name,
+				 nullptr,
+				 nullptr,
+				 (SEARCH_GLOBAL_BLOCK
+				  | SEARCH_STATIC_BLOCK),
+				 VAR_DOMAIN,
+				 ALL_DOMAIN);
 }
 
 void
@@ -359,8 +359,8 @@ objfile::expand_all_symtabs ()
     gdb_printf (gdb_stdlog, "qf->expand_all_symtabs (%s)\n",
 		objfile_debug_name (this));
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->expand_all_symtabs (this);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->expand_all_symtabs (this);
 }
 
 void
@@ -377,16 +377,16 @@ objfile::expand_symtabs_with_fullname (const char *fullname)
     return filename_cmp (basenames ? basename : fullname, filename) == 0;
   };
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->expand_symtabs_matching (this,
-				   file_matcher,
-				   nullptr,
-				   nullptr,
-				   nullptr,
-				   (SEARCH_GLOBAL_BLOCK
-				    | SEARCH_STATIC_BLOCK),
-				   UNDEF_DOMAIN,
-				   ALL_DOMAIN);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->expand_symtabs_matching (this,
+				 file_matcher,
+				 nullptr,
+				 nullptr,
+				 nullptr,
+				 (SEARCH_GLOBAL_BLOCK
+				  | SEARCH_STATIC_BLOCK),
+				 UNDEF_DOMAIN,
+				 ALL_DOMAIN);
 }
 
 void
@@ -402,9 +402,9 @@ objfile::expand_matching_symbols
 		domain_name (domain), global,
 		host_address_to_string (ordered_compare));
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->expand_matching_symbols (this, name, domain, global,
-				   ordered_compare);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->expand_matching_symbols (this, name, domain, global,
+				 ordered_compare);
 }
 
 bool
@@ -429,10 +429,10 @@ objfile::expand_symtabs_matching
 		host_address_to_string (&expansion_notify),
 		search_domain_name (kind));
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    if (!iter->expand_symtabs_matching (this, file_matcher, lookup_name,
-					symbol_matcher, expansion_notify,
-					search_flags, domain, kind))
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    if (!qf->expand_symtabs_matching (this, file_matcher, lookup_name,
+				      symbol_matcher, expansion_notify,
+				      search_flags, domain, kind))
       return false;
   return true;
 }
@@ -454,10 +454,10 @@ objfile::find_pc_sect_compunit_symtab (struct bound_minimal_symbol msymbol,
 		host_address_to_string (section),
 		warn_if_readin);
 
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      retval = iter->find_pc_sect_compunit_symtab (this, msymbol, pc, section,
-						   warn_if_readin);
+      retval = qf->find_pc_sect_compunit_symtab (this, msymbol, pc, section,
+						 warn_if_readin);
       if (retval != nullptr)
 	break;
     }
@@ -482,8 +482,8 @@ objfile::map_symbol_filenames (gdb::function_view<symbol_filename_ftype> fun,
 		objfile_debug_name (this),
 		need_fullname);
 
-  for (const auto &iter : qf_require_partial_symbols ())
-    iter->map_symbol_filenames (this, fun, need_fullname);
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
+    qf->map_symbol_filenames (this, fun, need_fullname);
 }
 
 struct compunit_symtab *
@@ -496,9 +496,9 @@ objfile::find_compunit_symtab_by_address (CORE_ADDR address)
 		hex_string (address));
 
   struct compunit_symtab *result = NULL;
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      result = iter->find_compunit_symtab_by_address (this, address);
+      result = qf->find_compunit_symtab_by_address (this, address);
       if (result != nullptr)
 	break;
     }
@@ -521,10 +521,10 @@ objfile::lookup_global_symbol_language (const char *name,
   enum language result = language_unknown;
   *symbol_found_p = false;
 
-  for (const auto &iter : qf_require_partial_symbols ())
+  for (quick_symbol_functions *qf : qf_require_partial_symbols ())
     {
-      result = iter->lookup_global_symbol_language (this, name, domain,
-						    symbol_found_p);
+      result = qf->lookup_global_symbol_language (this, name, domain,
+						  symbol_found_p);
       if (*symbol_found_p)
 	break;
     }
diff --git a/gdb/testsuite/gdb.python/py-objfile.exp b/gdb/testsuite/gdb.python/py-objfile.exp
index 61b9942de79..0bf49976b73 100644
--- a/gdb/testsuite/gdb.python/py-objfile.exp
+++ b/gdb/testsuite/gdb.python/py-objfile.exp
@@ -135,7 +135,7 @@ gdb_test "p main" "= {<text variable, no debug info>} $hex <main>" \
 gdb_py_test_silent_cmd "python objfile.add_separate_debug_file(\"${binfile}\")" \
     "Add separate debug file file" 1
 
-gdb_py_test_silent_cmd "python sep_objfile = gdb.objfiles()\[0\]" \
+gdb_py_test_silent_cmd "python sep_objfile = gdb.objfiles()\[1\]" \
     "Get separate debug info objfile" 1
 
 gdb_test "python print (sep_objfile.owner.filename)" "${testfile}2" \
-- 
2.40.1