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

  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(-)
  

Ping

Thanks,
Aaron

On Wed, May 31, 2023 at 9:44 PM Aaron Merey <amerey@redhat.com> wrote:
>
> 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
>
  

Ping

Thanks,
Aaron

On Thu, Jun 15, 2023 at 9:44 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Wed, May 31, 2023 at 9:44 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > 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
> >
  

Ping

Thanks,
Aaron

On Mon, Jul 3, 2023 at 1:39 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Thu, Jun 15, 2023 at 9:44 AM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Wed, May 31, 2023 at 9:44 PM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > 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
> > >
  

Aaron Merey <amerey@redhat.com> writes:

> 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.

This doesn't really explain why we need to use the unwrapping iterator
though, couldn't we just use basic_safe_range instead?  This would
remove the need for some of the updates in symfile-debug.c.

>
> 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;
> +	}

I think the more C++ way of writing this would be:

  qf.remove_if ([&] (const quick_symbol_functions_up &qf_up)
                {
                  return qf_up.get () == target;
                });

The remove function already walks the whole of `qf` to remove every item
matching qf_up, so the early return isn't actually saving anything.

> +  }
> +
>    /* 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

If my comment above about qf using basic_safe_range is correct, then
this change might not be needed.

> +{
> +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

I see what you did here, but I wonder if this is the right approach.

Rather than using basic_safe_range, maybe we should instead create
basic_safe_reverse_range, which would imply creating a
basic_safe_reverse_iterator.  The basic_safe_reverse_range would take a
"normal" forward range, but walk over it backwards.

I also think it would be super useful to have some comments about why we
can't just use the standard reverse iterators here, my initial thought
was that this should be possible, and it was only once I tried it out
that I discovered what the problems were -- I guess you already ran into
those issues, but neither the commit message, nor any comments really
explain why _this_ implementation.

Additionally, I question the need to use an unwrapping iterator here.
The comment (clearly copy & paste) says "which helped avoid touching a
lot of code when changing how objfiles are managed.", however, for this
reverse case, as far as I can tell, there's only two users, so, I'd be
tempted to say we should just update those two users to make use of
iterators...

... but both the users of progspace::objfiles_safe() actually just do:

  for (objfile *objf : current_program_space->objfiles_safe ())
    {
      if (predicate (objf))
	objf->unlink ();
    }

So I wonder if a better solution would be to offer:

  void
  progspace::unlink_objfiles_if (gdb::function_view<bool (const objfile *objfile)> predicate);

which would mean that the callers can focus on the logic of _what_ to
unlink, while progspace.c can focus on the mechanism of _how_ to unlink?

Thanks,
Andrew

>  {
>  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