diff mbox series

[2/4,v2] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator

Message ID 20231028002008.1105723-3-amerey@redhat.com
State New
Headers
Series On-demand debuginfo downloading |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Aaron Merey Oct. 28, 2023, 12:20 a.m. UTC 
  v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html

v2 removes unwrapping_reverse_objfile_iterator and adds
basic_safe_reverse_range and basic_safe_reverse_iterator.

Commit message:

This patch changes progspace objfile_list insertion so that separate
debug objfiles are placed into the list after the parent objfile,
instead of before.  Additionally qf_require_partial_symbols now returns
a safe_range.

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 need to delete a
.gdb_index quick_symbol_functions from a parent objfile while looping
the objfile's list of quick_symbol_functions becasue the separate
debug objfile has just been downloaded.  The use of a safe_range
prevents this removal from causing iterator invalidation.

gdb might also download a debuginfo file during symtab expansion.
In this case an objfile will be added to the current progspace's
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.

To facilitate the safe deletion of objfiles, this patch also adds
basic_safe_reverse_range and basic_safe_reverse_iterator.  This allows
objfiles to be removed from the objfiles_list in a loop without iterator
invalidation.

If a forward safe iterator were to be used, the deletion of an
objfile could invalidate the safe iterator's reference to the next
objfile in the objfiles_list.  This can happen when the deletion
of an objfile causes the deletion of a separate debug objfile that
happens to the be next element in the objfiles_list.

The standard reverse iterator is not suitable for safe objfile deletion.
In order to safely delete the first objfile in the objfiles_list, the
standard reverse iterator's underlying begin iterator would have to be
decremented, resulting in undefined behavior.

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/jit.c                               |   7 +-
 gdb/objfiles.c                          |   8 +-
 gdb/objfiles.h                          |   8 +-
 gdb/progspace.c                         |  19 ++++-
 gdb/progspace.h                         |  31 ++++---
 gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
 gdbsupport/safe-iterator.h              | 106 ++++++++++++++++++++++++
 7 files changed, 154 insertions(+), 27 deletions(-)

Comments

Aaron Merey Nov. 12, 2023, 8:20 p.m. UTC | #1 
Ping

Thanks,
Aaron

On Fri, Oct 27, 2023 at 8:20 PM Aaron Merey <amerey@redhat.com> wrote:
>
> v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html
>
> v2 removes unwrapping_reverse_objfile_iterator and adds
> basic_safe_reverse_range and basic_safe_reverse_iterator.
>
> Commit message:
>
> This patch changes progspace objfile_list insertion so that separate
> debug objfiles are placed into the list after the parent objfile,
> instead of before.  Additionally qf_require_partial_symbols now returns
> a safe_range.
>
> 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 need to delete a
> .gdb_index quick_symbol_functions from a parent objfile while looping
> the objfile's list of quick_symbol_functions becasue the separate
> debug objfile has just been downloaded.  The use of a safe_range
> prevents this removal from causing iterator invalidation.
>
> gdb might also download a debuginfo file during symtab expansion.
> In this case an objfile will be added to the current progspace's
> 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.
>
> To facilitate the safe deletion of objfiles, this patch also adds
> basic_safe_reverse_range and basic_safe_reverse_iterator.  This allows
> objfiles to be removed from the objfiles_list in a loop without iterator
> invalidation.
>
> If a forward safe iterator were to be used, the deletion of an
> objfile could invalidate the safe iterator's reference to the next
> objfile in the objfiles_list.  This can happen when the deletion
> of an objfile causes the deletion of a separate debug objfile that
> happens to the be next element in the objfiles_list.
>
> The standard reverse iterator is not suitable for safe objfile deletion.
> In order to safely delete the first objfile in the objfiles_list, the
> standard reverse iterator's underlying begin iterator would have to be
> decremented, resulting in undefined behavior.
>
> 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/jit.c                               |   7 +-
>  gdb/objfiles.c                          |   8 +-
>  gdb/objfiles.h                          |   8 +-
>  gdb/progspace.c                         |  19 ++++-
>  gdb/progspace.h                         |  31 ++++---
>  gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
>  gdbsupport/safe-iterator.h              | 106 ++++++++++++++++++++++++
>  7 files changed, 154 insertions(+), 27 deletions(-)
>
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 9e8325ab803..a39fdc5a96d 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -1240,11 +1240,10 @@ jit_breakpoint_re_set (void)
>  static void
>  jit_inferior_exit_hook (struct inferior *inf)
>  {
> -  for (objfile *objf : current_program_space->objfiles_safe ())
> +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
>      {
> -      if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
> -       objf->unlink ();
> -    }
> +      return (objf->jited_data != nullptr) && (objf->jited_data->addr != 0);
> +    });
>  }
>
>  void
> diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> index 8f085b1bb7c..9822c179962 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -793,14 +793,12 @@ have_full_symbols (void)
>  void
>  objfile_purge_solibs (void)
>  {
> -  for (objfile *objf : current_program_space->objfiles_safe ())
> +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
>      {
>        /* We assume that the solib package has been purged already, or will
>          be soon.  */
> -
> -      if (!(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED))
> -       objf->unlink ();
> -    }
> +      return !(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED);
> +    });
>  }
>
>
> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index 4b8aa9bfcec..c20b63ceadf 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -698,13 +698,17 @@ struct objfile
>
>  private:
>
> +  using qf_list = std::forward_list<quick_symbol_functions_up>;
> +  using qf_range = iterator_range<qf_list::iterator>;
> +  using qf_safe_range = basic_safe_range<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 (qf_range (qf.begin (), qf.end ()));
>    }
>
>  public:
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 839707e9d71..c0fca1dace7 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -143,19 +143,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));
>      }
>  }
>
> @@ -184,6 +184,17 @@ program_space::remove_objfile (struct objfile *objfile)
>
>  /* See progspace.h.  */
>
> +void
> +program_space::unlink_objfiles_if
> +  (gdb::function_view<bool (const objfile *objfile)> predicate)
> +{
> +  for (auto &it : objfiles_safe ())
> +    if (predicate (it.get ()))
> +      it->unlink ();
> +}
> +
> +/* See progspace.h.  */
> +
>  struct objfile *
>  program_space::objfile_for_address (CORE_ADDR address)
>  {
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index a22e427400e..17bb1710ccf 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -214,28 +214,32 @@ struct program_space
>         unwrapping_objfile_iterator (objfiles_list.end ()));
>    }
>
> -  using objfiles_safe_range = basic_safe_range<objfiles_range>;
> +  using objfiles_safe_range = iterator_range<objfile_list::iterator>;
> +  using objfiles_safe_reverse_range
> +    = basic_safe_reverse_range<objfiles_safe_range>;
>
>    /* An iterable object that can be used to iterate over all objfiles.
>       The basic use is in a foreach, like:
>
>       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 ()
> +     This variant uses a basic_safe_reverse_iterator so that objfiles
> +     can be deleted during iteration.
> +
> +     The use of a reverse iterator helps ensure that separate debug
> +     objfiles are deleted before their parent objfile.  This prevents
> +     iterator invalidation 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_safe_range (objfiles_list.begin (), objfiles_list.end ()));
>    }
>
> -  /* 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);
> @@ -250,6 +254,11 @@ struct program_space
>    /* Free all the objfiles associated with this program space.  */
>    void free_all_objfiles ();
>
> +  /* Unlink all objfiles associated with this program space for which
> +     PREDICATE evaluates to true.  */
> +  void unlink_objfiles_if
> +    (gdb::function_view<bool (const objfile *objfile)> predicate);
> +
>    /* Return the objfile containing ADDRESS, or nullptr if the address
>       is outside all objfiles in this progspace.  */
>    struct objfile *objfile_for_address (CORE_ADDR address);
> 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" \
> diff --git a/gdbsupport/safe-iterator.h b/gdbsupport/safe-iterator.h
> index ccd772ca2a5..9f57c1543cf 100644
> --- a/gdbsupport/safe-iterator.h
> +++ b/gdbsupport/safe-iterator.h
> @@ -136,4 +136,110 @@ class basic_safe_range
>    Range m_range;
>  };
>
> +/* A reverse basic_safe_iterator.  See basic_safe_iterator for intended use.  */
> +
> +template<typename Iterator>
> +class basic_safe_reverse_iterator
> +{
> +public:
> +  typedef basic_safe_reverse_iterator self_type;
> +  typedef typename Iterator::value_type value_type;
> +  typedef typename Iterator::reference reference;
> +  typedef typename Iterator::pointer pointer;
> +  typedef typename Iterator::iterator_category iterator_category;
> +  typedef typename Iterator::difference_type difference_type;
> +
> +  /* Construct the iterator using ARG, and construct the end iterator
> +     using ARG2.  */
> +  template<typename Arg>
> +  explicit basic_safe_reverse_iterator (Arg &&arg, Arg &&arg2)
> +    : m_begin (std::forward<Arg> (arg)),
> +      m_end (std::forward<Arg> (arg2)),
> +      m_it (m_end),
> +      m_next (m_end)
> +  {
> +    /* M_IT and M_NEXT are initialized as one-past-end.  Set M_IT to point
> +       to the last element and set M_NEXT to point to the second last element,
> +       if such elements exist.  */
> +    if (m_it != m_begin)
> +      {
> +       --m_it;
> +
> +       if (m_it != m_begin)
> +         {
> +           --m_next;
> +           --m_next;
> +         }
> +      }
> +  }
> +
> +  typename gdb::invoke_result<decltype(&Iterator::operator*), Iterator>::type
> +    operator* () const
> +  { return *m_it; }
> +
> +  self_type &operator++ ()
> +  {
> +    m_it = m_next;
> +
> +    if (m_it != m_end)
> +      {
> +       /* Use M_BEGIN only if we sure that it is valid.  */
> +       if (m_it == m_begin)
> +         m_next = m_end;
> +       else
> +         --m_next;
> +      }
> +
> +    return *this;
> +  }
> +
> +  bool operator== (const self_type &other) const
> +  { return m_it == other.m_it; }
> +
> +  bool operator!= (const self_type &other) const
> +  { return m_it != other.m_it; }
> +
> +private:
> +  /* The first element.  */
> +  Iterator m_begin {};
> +
> +  /* A one-past-end iterator.  */
> +  Iterator m_end {};
> +
> +  /* The current element.  */
> +  Iterator m_it {};
> +
> +  /* The next element.  Always one element ahead of M_IT.  */
> +  Iterator m_next {};
> +};
> +
> +/* A range adapter that wraps a forward range, and then returns
> +   safe reverse iterators wrapping the original range's iterators.  */
> +
> +template<typename Range>
> +class basic_safe_reverse_range
> +{
> +public:
> +
> +  typedef basic_safe_reverse_iterator<typename Range::iterator> iterator;
> +
> +  explicit basic_safe_reverse_range (Range range)
> +    : m_range (range)
> +  {
> +  }
> +
> +  iterator begin ()
> +  {
> +    return iterator (m_range.begin (), m_range.end ());
> +  }
> +
> +  iterator end ()
> +  {
> +    return iterator (m_range.end (), m_range.end ());
> +  }
> +
> +private:
> +
> +  Range m_range;
> +};
>  #endif /* COMMON_SAFE_ITERATOR_H */
> --
> 2.41.0
>
Aaron Merey Nov. 20, 2023, 6:39 p.m. UTC | #2 
Ping

Thanks,
Aaron

On Sun, Nov 12, 2023 at 3:20 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Fri, Oct 27, 2023 at 8:20 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html
> >
> > v2 removes unwrapping_reverse_objfile_iterator and adds
> > basic_safe_reverse_range and basic_safe_reverse_iterator.
> >
> > Commit message:
> >
> > This patch changes progspace objfile_list insertion so that separate
> > debug objfiles are placed into the list after the parent objfile,
> > instead of before.  Additionally qf_require_partial_symbols now returns
> > a safe_range.
> >
> > 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 need to delete a
> > .gdb_index quick_symbol_functions from a parent objfile while looping
> > the objfile's list of quick_symbol_functions becasue the separate
> > debug objfile has just been downloaded.  The use of a safe_range
> > prevents this removal from causing iterator invalidation.
> >
> > gdb might also download a debuginfo file during symtab expansion.
> > In this case an objfile will be added to the current progspace's
> > 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.
> >
> > To facilitate the safe deletion of objfiles, this patch also adds
> > basic_safe_reverse_range and basic_safe_reverse_iterator.  This allows
> > objfiles to be removed from the objfiles_list in a loop without iterator
> > invalidation.
> >
> > If a forward safe iterator were to be used, the deletion of an
> > objfile could invalidate the safe iterator's reference to the next
> > objfile in the objfiles_list.  This can happen when the deletion
> > of an objfile causes the deletion of a separate debug objfile that
> > happens to the be next element in the objfiles_list.
> >
> > The standard reverse iterator is not suitable for safe objfile deletion.
> > In order to safely delete the first objfile in the objfiles_list, the
> > standard reverse iterator's underlying begin iterator would have to be
> > decremented, resulting in undefined behavior.
> >
> > 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/jit.c                               |   7 +-
> >  gdb/objfiles.c                          |   8 +-
> >  gdb/objfiles.h                          |   8 +-
> >  gdb/progspace.c                         |  19 ++++-
> >  gdb/progspace.h                         |  31 ++++---
> >  gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
> >  gdbsupport/safe-iterator.h              | 106 ++++++++++++++++++++++++
> >  7 files changed, 154 insertions(+), 27 deletions(-)
> >
> > diff --git a/gdb/jit.c b/gdb/jit.c
> > index 9e8325ab803..a39fdc5a96d 100644
> > --- a/gdb/jit.c
> > +++ b/gdb/jit.c
> > @@ -1240,11 +1240,10 @@ jit_breakpoint_re_set (void)
> >  static void
> >  jit_inferior_exit_hook (struct inferior *inf)
> >  {
> > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> >      {
> > -      if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
> > -       objf->unlink ();
> > -    }
> > +      return (objf->jited_data != nullptr) && (objf->jited_data->addr != 0);
> > +    });
> >  }
> >
> >  void
> > diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> > index 8f085b1bb7c..9822c179962 100644
> > --- a/gdb/objfiles.c
> > +++ b/gdb/objfiles.c
> > @@ -793,14 +793,12 @@ have_full_symbols (void)
> >  void
> >  objfile_purge_solibs (void)
> >  {
> > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> >      {
> >        /* We assume that the solib package has been purged already, or will
> >          be soon.  */
> > -
> > -      if (!(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED))
> > -       objf->unlink ();
> > -    }
> > +      return !(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED);
> > +    });
> >  }
> >
> >
> > diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> > index 4b8aa9bfcec..c20b63ceadf 100644
> > --- a/gdb/objfiles.h
> > +++ b/gdb/objfiles.h
> > @@ -698,13 +698,17 @@ struct objfile
> >
> >  private:
> >
> > +  using qf_list = std::forward_list<quick_symbol_functions_up>;
> > +  using qf_range = iterator_range<qf_list::iterator>;
> > +  using qf_safe_range = basic_safe_range<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 (qf_range (qf.begin (), qf.end ()));
> >    }
> >
> >  public:
> > diff --git a/gdb/progspace.c b/gdb/progspace.c
> > index 839707e9d71..c0fca1dace7 100644
> > --- a/gdb/progspace.c
> > +++ b/gdb/progspace.c
> > @@ -143,19 +143,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));
> >      }
> >  }
> >
> > @@ -184,6 +184,17 @@ program_space::remove_objfile (struct objfile *objfile)
> >
> >  /* See progspace.h.  */
> >
> > +void
> > +program_space::unlink_objfiles_if
> > +  (gdb::function_view<bool (const objfile *objfile)> predicate)
> > +{
> > +  for (auto &it : objfiles_safe ())
> > +    if (predicate (it.get ()))
> > +      it->unlink ();
> > +}
> > +
> > +/* See progspace.h.  */
> > +
> >  struct objfile *
> >  program_space::objfile_for_address (CORE_ADDR address)
> >  {
> > diff --git a/gdb/progspace.h b/gdb/progspace.h
> > index a22e427400e..17bb1710ccf 100644
> > --- a/gdb/progspace.h
> > +++ b/gdb/progspace.h
> > @@ -214,28 +214,32 @@ struct program_space
> >         unwrapping_objfile_iterator (objfiles_list.end ()));
> >    }
> >
> > -  using objfiles_safe_range = basic_safe_range<objfiles_range>;
> > +  using objfiles_safe_range = iterator_range<objfile_list::iterator>;
> > +  using objfiles_safe_reverse_range
> > +    = basic_safe_reverse_range<objfiles_safe_range>;
> >
> >    /* An iterable object that can be used to iterate over all objfiles.
> >       The basic use is in a foreach, like:
> >
> >       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 ()
> > +     This variant uses a basic_safe_reverse_iterator so that objfiles
> > +     can be deleted during iteration.
> > +
> > +     The use of a reverse iterator helps ensure that separate debug
> > +     objfiles are deleted before their parent objfile.  This prevents
> > +     iterator invalidation 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_safe_range (objfiles_list.begin (), objfiles_list.end ()));
> >    }
> >
> > -  /* 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);
> > @@ -250,6 +254,11 @@ struct program_space
> >    /* Free all the objfiles associated with this program space.  */
> >    void free_all_objfiles ();
> >
> > +  /* Unlink all objfiles associated with this program space for which
> > +     PREDICATE evaluates to true.  */
> > +  void unlink_objfiles_if
> > +    (gdb::function_view<bool (const objfile *objfile)> predicate);
> > +
> >    /* Return the objfile containing ADDRESS, or nullptr if the address
> >       is outside all objfiles in this progspace.  */
> >    struct objfile *objfile_for_address (CORE_ADDR address);
> > 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" \
> > diff --git a/gdbsupport/safe-iterator.h b/gdbsupport/safe-iterator.h
> > index ccd772ca2a5..9f57c1543cf 100644
> > --- a/gdbsupport/safe-iterator.h
> > +++ b/gdbsupport/safe-iterator.h
> > @@ -136,4 +136,110 @@ class basic_safe_range
> >    Range m_range;
> >  };
> >
> > +/* A reverse basic_safe_iterator.  See basic_safe_iterator for intended use.  */
> > +
> > +template<typename Iterator>
> > +class basic_safe_reverse_iterator
> > +{
> > +public:
> > +  typedef basic_safe_reverse_iterator self_type;
> > +  typedef typename Iterator::value_type value_type;
> > +  typedef typename Iterator::reference reference;
> > +  typedef typename Iterator::pointer pointer;
> > +  typedef typename Iterator::iterator_category iterator_category;
> > +  typedef typename Iterator::difference_type difference_type;
> > +
> > +  /* Construct the iterator using ARG, and construct the end iterator
> > +     using ARG2.  */
> > +  template<typename Arg>
> > +  explicit basic_safe_reverse_iterator (Arg &&arg, Arg &&arg2)
> > +    : m_begin (std::forward<Arg> (arg)),
> > +      m_end (std::forward<Arg> (arg2)),
> > +      m_it (m_end),
> > +      m_next (m_end)
> > +  {
> > +    /* M_IT and M_NEXT are initialized as one-past-end.  Set M_IT to point
> > +       to the last element and set M_NEXT to point to the second last element,
> > +       if such elements exist.  */
> > +    if (m_it != m_begin)
> > +      {
> > +       --m_it;
> > +
> > +       if (m_it != m_begin)
> > +         {
> > +           --m_next;
> > +           --m_next;
> > +         }
> > +      }
> > +  }
> > +
> > +  typename gdb::invoke_result<decltype(&Iterator::operator*), Iterator>::type
> > +    operator* () const
> > +  { return *m_it; }
> > +
> > +  self_type &operator++ ()
> > +  {
> > +    m_it = m_next;
> > +
> > +    if (m_it != m_end)
> > +      {
> > +       /* Use M_BEGIN only if we sure that it is valid.  */
> > +       if (m_it == m_begin)
> > +         m_next = m_end;
> > +       else
> > +         --m_next;
> > +      }
> > +
> > +    return *this;
> > +  }
> > +
> > +  bool operator== (const self_type &other) const
> > +  { return m_it == other.m_it; }
> > +
> > +  bool operator!= (const self_type &other) const
> > +  { return m_it != other.m_it; }
> > +
> > +private:
> > +  /* The first element.  */
> > +  Iterator m_begin {};
> > +
> > +  /* A one-past-end iterator.  */
> > +  Iterator m_end {};
> > +
> > +  /* The current element.  */
> > +  Iterator m_it {};
> > +
> > +  /* The next element.  Always one element ahead of M_IT.  */
> > +  Iterator m_next {};
> > +};
> > +
> > +/* A range adapter that wraps a forward range, and then returns
> > +   safe reverse iterators wrapping the original range's iterators.  */
> > +
> > +template<typename Range>
> > +class basic_safe_reverse_range
> > +{
> > +public:
> > +
> > +  typedef basic_safe_reverse_iterator<typename Range::iterator> iterator;
> > +
> > +  explicit basic_safe_reverse_range (Range range)
> > +    : m_range (range)
> > +  {
> > +  }
> > +
> > +  iterator begin ()
> > +  {
> > +    return iterator (m_range.begin (), m_range.end ());
> > +  }
> > +
> > +  iterator end ()
> > +  {
> > +    return iterator (m_range.end (), m_range.end ());
> > +  }
> > +
> > +private:
> > +
> > +  Range m_range;
> > +};
> >  #endif /* COMMON_SAFE_ITERATOR_H */
> > --
> > 2.41.0
> >
Aaron Merey Nov. 30, 2023, 4:30 p.m. UTC | #3 
Ping

Thanks,
Aaron

On Mon, Nov 20, 2023 at 1:39 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Sun, Nov 12, 2023 at 3:20 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Fri, Oct 27, 2023 at 8:20 PM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html
> > >
> > > v2 removes unwrapping_reverse_objfile_iterator and adds
> > > basic_safe_reverse_range and basic_safe_reverse_iterator.
> > >
> > > Commit message:
> > >
> > > This patch changes progspace objfile_list insertion so that separate
> > > debug objfiles are placed into the list after the parent objfile,
> > > instead of before.  Additionally qf_require_partial_symbols now returns
> > > a safe_range.
> > >
> > > 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 need to delete a
> > > .gdb_index quick_symbol_functions from a parent objfile while looping
> > > the objfile's list of quick_symbol_functions becasue the separate
> > > debug objfile has just been downloaded.  The use of a safe_range
> > > prevents this removal from causing iterator invalidation.
> > >
> > > gdb might also download a debuginfo file during symtab expansion.
> > > In this case an objfile will be added to the current progspace's
> > > 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.
> > >
> > > To facilitate the safe deletion of objfiles, this patch also adds
> > > basic_safe_reverse_range and basic_safe_reverse_iterator.  This allows
> > > objfiles to be removed from the objfiles_list in a loop without iterator
> > > invalidation.
> > >
> > > If a forward safe iterator were to be used, the deletion of an
> > > objfile could invalidate the safe iterator's reference to the next
> > > objfile in the objfiles_list.  This can happen when the deletion
> > > of an objfile causes the deletion of a separate debug objfile that
> > > happens to the be next element in the objfiles_list.
> > >
> > > The standard reverse iterator is not suitable for safe objfile deletion.
> > > In order to safely delete the first objfile in the objfiles_list, the
> > > standard reverse iterator's underlying begin iterator would have to be
> > > decremented, resulting in undefined behavior.
> > >
> > > 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/jit.c                               |   7 +-
> > >  gdb/objfiles.c                          |   8 +-
> > >  gdb/objfiles.h                          |   8 +-
> > >  gdb/progspace.c                         |  19 ++++-
> > >  gdb/progspace.h                         |  31 ++++---
> > >  gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
> > >  gdbsupport/safe-iterator.h              | 106 ++++++++++++++++++++++++
> > >  7 files changed, 154 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/gdb/jit.c b/gdb/jit.c
> > > index 9e8325ab803..a39fdc5a96d 100644
> > > --- a/gdb/jit.c
> > > +++ b/gdb/jit.c
> > > @@ -1240,11 +1240,10 @@ jit_breakpoint_re_set (void)
> > >  static void
> > >  jit_inferior_exit_hook (struct inferior *inf)
> > >  {
> > > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> > >      {
> > > -      if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
> > > -       objf->unlink ();
> > > -    }
> > > +      return (objf->jited_data != nullptr) && (objf->jited_data->addr != 0);
> > > +    });
> > >  }
> > >
> > >  void
> > > diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> > > index 8f085b1bb7c..9822c179962 100644
> > > --- a/gdb/objfiles.c
> > > +++ b/gdb/objfiles.c
> > > @@ -793,14 +793,12 @@ have_full_symbols (void)
> > >  void
> > >  objfile_purge_solibs (void)
> > >  {
> > > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> > >      {
> > >        /* We assume that the solib package has been purged already, or will
> > >          be soon.  */
> > > -
> > > -      if (!(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED))
> > > -       objf->unlink ();
> > > -    }
> > > +      return !(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED);
> > > +    });
> > >  }
> > >
> > >
> > > diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> > > index 4b8aa9bfcec..c20b63ceadf 100644
> > > --- a/gdb/objfiles.h
> > > +++ b/gdb/objfiles.h
> > > @@ -698,13 +698,17 @@ struct objfile
> > >
> > >  private:
> > >
> > > +  using qf_list = std::forward_list<quick_symbol_functions_up>;
> > > +  using qf_range = iterator_range<qf_list::iterator>;
> > > +  using qf_safe_range = basic_safe_range<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 (qf_range (qf.begin (), qf.end ()));
> > >    }
> > >
> > >  public:
> > > diff --git a/gdb/progspace.c b/gdb/progspace.c
> > > index 839707e9d71..c0fca1dace7 100644
> > > --- a/gdb/progspace.c
> > > +++ b/gdb/progspace.c
> > > @@ -143,19 +143,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));
> > >      }
> > >  }
> > >
> > > @@ -184,6 +184,17 @@ program_space::remove_objfile (struct objfile *objfile)
> > >
> > >  /* See progspace.h.  */
> > >
> > > +void
> > > +program_space::unlink_objfiles_if
> > > +  (gdb::function_view<bool (const objfile *objfile)> predicate)
> > > +{
> > > +  for (auto &it : objfiles_safe ())
> > > +    if (predicate (it.get ()))
> > > +      it->unlink ();
> > > +}
> > > +
> > > +/* See progspace.h.  */
> > > +
> > >  struct objfile *
> > >  program_space::objfile_for_address (CORE_ADDR address)
> > >  {
> > > diff --git a/gdb/progspace.h b/gdb/progspace.h
> > > index a22e427400e..17bb1710ccf 100644
> > > --- a/gdb/progspace.h
> > > +++ b/gdb/progspace.h
> > > @@ -214,28 +214,32 @@ struct program_space
> > >         unwrapping_objfile_iterator (objfiles_list.end ()));
> > >    }
> > >
> > > -  using objfiles_safe_range = basic_safe_range<objfiles_range>;
> > > +  using objfiles_safe_range = iterator_range<objfile_list::iterator>;
> > > +  using objfiles_safe_reverse_range
> > > +    = basic_safe_reverse_range<objfiles_safe_range>;
> > >
> > >    /* An iterable object that can be used to iterate over all objfiles.
> > >       The basic use is in a foreach, like:
> > >
> > >       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 ()
> > > +     This variant uses a basic_safe_reverse_iterator so that objfiles
> > > +     can be deleted during iteration.
> > > +
> > > +     The use of a reverse iterator helps ensure that separate debug
> > > +     objfiles are deleted before their parent objfile.  This prevents
> > > +     iterator invalidation 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_safe_range (objfiles_list.begin (), objfiles_list.end ()));
> > >    }
> > >
> > > -  /* 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);
> > > @@ -250,6 +254,11 @@ struct program_space
> > >    /* Free all the objfiles associated with this program space.  */
> > >    void free_all_objfiles ();
> > >
> > > +  /* Unlink all objfiles associated with this program space for which
> > > +     PREDICATE evaluates to true.  */
> > > +  void unlink_objfiles_if
> > > +    (gdb::function_view<bool (const objfile *objfile)> predicate);
> > > +
> > >    /* Return the objfile containing ADDRESS, or nullptr if the address
> > >       is outside all objfiles in this progspace.  */
> > >    struct objfile *objfile_for_address (CORE_ADDR address);
> > > 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" \
> > > diff --git a/gdbsupport/safe-iterator.h b/gdbsupport/safe-iterator.h
> > > index ccd772ca2a5..9f57c1543cf 100644
> > > --- a/gdbsupport/safe-iterator.h
> > > +++ b/gdbsupport/safe-iterator.h
> > > @@ -136,4 +136,110 @@ class basic_safe_range
> > >    Range m_range;
> > >  };
> > >
> > > +/* A reverse basic_safe_iterator.  See basic_safe_iterator for intended use.  */
> > > +
> > > +template<typename Iterator>
> > > +class basic_safe_reverse_iterator
> > > +{
> > > +public:
> > > +  typedef basic_safe_reverse_iterator self_type;
> > > +  typedef typename Iterator::value_type value_type;
> > > +  typedef typename Iterator::reference reference;
> > > +  typedef typename Iterator::pointer pointer;
> > > +  typedef typename Iterator::iterator_category iterator_category;
> > > +  typedef typename Iterator::difference_type difference_type;
> > > +
> > > +  /* Construct the iterator using ARG, and construct the end iterator
> > > +     using ARG2.  */
> > > +  template<typename Arg>
> > > +  explicit basic_safe_reverse_iterator (Arg &&arg, Arg &&arg2)
> > > +    : m_begin (std::forward<Arg> (arg)),
> > > +      m_end (std::forward<Arg> (arg2)),
> > > +      m_it (m_end),
> > > +      m_next (m_end)
> > > +  {
> > > +    /* M_IT and M_NEXT are initialized as one-past-end.  Set M_IT to point
> > > +       to the last element and set M_NEXT to point to the second last element,
> > > +       if such elements exist.  */
> > > +    if (m_it != m_begin)
> > > +      {
> > > +       --m_it;
> > > +
> > > +       if (m_it != m_begin)
> > > +         {
> > > +           --m_next;
> > > +           --m_next;
> > > +         }
> > > +      }
> > > +  }
> > > +
> > > +  typename gdb::invoke_result<decltype(&Iterator::operator*), Iterator>::type
> > > +    operator* () const
> > > +  { return *m_it; }
> > > +
> > > +  self_type &operator++ ()
> > > +  {
> > > +    m_it = m_next;
> > > +
> > > +    if (m_it != m_end)
> > > +      {
> > > +       /* Use M_BEGIN only if we sure that it is valid.  */
> > > +       if (m_it == m_begin)
> > > +         m_next = m_end;
> > > +       else
> > > +         --m_next;
> > > +      }
> > > +
> > > +    return *this;
> > > +  }
> > > +
> > > +  bool operator== (const self_type &other) const
> > > +  { return m_it == other.m_it; }
> > > +
> > > +  bool operator!= (const self_type &other) const
> > > +  { return m_it != other.m_it; }
> > > +
> > > +private:
> > > +  /* The first element.  */
> > > +  Iterator m_begin {};
> > > +
> > > +  /* A one-past-end iterator.  */
> > > +  Iterator m_end {};
> > > +
> > > +  /* The current element.  */
> > > +  Iterator m_it {};
> > > +
> > > +  /* The next element.  Always one element ahead of M_IT.  */
> > > +  Iterator m_next {};
> > > +};
> > > +
> > > +/* A range adapter that wraps a forward range, and then returns
> > > +   safe reverse iterators wrapping the original range's iterators.  */
> > > +
> > > +template<typename Range>
> > > +class basic_safe_reverse_range
> > > +{
> > > +public:
> > > +
> > > +  typedef basic_safe_reverse_iterator<typename Range::iterator> iterator;
> > > +
> > > +  explicit basic_safe_reverse_range (Range range)
> > > +    : m_range (range)
> > > +  {
> > > +  }
> > > +
> > > +  iterator begin ()
> > > +  {
> > > +    return iterator (m_range.begin (), m_range.end ());
> > > +  }
> > > +
> > > +  iterator end ()
> > > +  {
> > > +    return iterator (m_range.end (), m_range.end ());
> > > +  }
> > > +
> > > +private:
> > > +
> > > +  Range m_range;
> > > +};
> > >  #endif /* COMMON_SAFE_ITERATOR_H */
> > > --
> > > 2.41.0
> > >
Aaron Merey Dec. 12, 2023, 3:01 p.m. UTC | #4 
Ping

Thanks,
Aaron

On Thu, Nov 30, 2023 at 11:30 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Mon, Nov 20, 2023 at 1:39 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Sun, Nov 12, 2023 at 3:20 PM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > Ping
> > >
> > > Thanks,
> > > Aaron
> > >
> > > On Fri, Oct 27, 2023 at 8:20 PM Aaron Merey <amerey@redhat.com> wrote:
> > > >
> > > > v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html
> > > >
> > > > v2 removes unwrapping_reverse_objfile_iterator and adds
> > > > basic_safe_reverse_range and basic_safe_reverse_iterator.
> > > >
> > > > Commit message:
> > > >
> > > > This patch changes progspace objfile_list insertion so that separate
> > > > debug objfiles are placed into the list after the parent objfile,
> > > > instead of before.  Additionally qf_require_partial_symbols now returns
> > > > a safe_range.
> > > >
> > > > 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 need to delete a
> > > > .gdb_index quick_symbol_functions from a parent objfile while looping
> > > > the objfile's list of quick_symbol_functions becasue the separate
> > > > debug objfile has just been downloaded.  The use of a safe_range
> > > > prevents this removal from causing iterator invalidation.
> > > >
> > > > gdb might also download a debuginfo file during symtab expansion.
> > > > In this case an objfile will be added to the current progspace's
> > > > 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.
> > > >
> > > > To facilitate the safe deletion of objfiles, this patch also adds
> > > > basic_safe_reverse_range and basic_safe_reverse_iterator.  This allows
> > > > objfiles to be removed from the objfiles_list in a loop without iterator
> > > > invalidation.
> > > >
> > > > If a forward safe iterator were to be used, the deletion of an
> > > > objfile could invalidate the safe iterator's reference to the next
> > > > objfile in the objfiles_list.  This can happen when the deletion
> > > > of an objfile causes the deletion of a separate debug objfile that
> > > > happens to the be next element in the objfiles_list.
> > > >
> > > > The standard reverse iterator is not suitable for safe objfile deletion.
> > > > In order to safely delete the first objfile in the objfiles_list, the
> > > > standard reverse iterator's underlying begin iterator would have to be
> > > > decremented, resulting in undefined behavior.
> > > >
> > > > 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/jit.c                               |   7 +-
> > > >  gdb/objfiles.c                          |   8 +-
> > > >  gdb/objfiles.h                          |   8 +-
> > > >  gdb/progspace.c                         |  19 ++++-
> > > >  gdb/progspace.h                         |  31 ++++---
> > > >  gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
> > > >  gdbsupport/safe-iterator.h              | 106 ++++++++++++++++++++++++
> > > >  7 files changed, 154 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/gdb/jit.c b/gdb/jit.c
> > > > index 9e8325ab803..a39fdc5a96d 100644
> > > > --- a/gdb/jit.c
> > > > +++ b/gdb/jit.c
> > > > @@ -1240,11 +1240,10 @@ jit_breakpoint_re_set (void)
> > > >  static void
> > > >  jit_inferior_exit_hook (struct inferior *inf)
> > > >  {
> > > > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > > > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> > > >      {
> > > > -      if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
> > > > -       objf->unlink ();
> > > > -    }
> > > > +      return (objf->jited_data != nullptr) && (objf->jited_data->addr != 0);
> > > > +    });
> > > >  }
> > > >
> > > >  void
> > > > diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> > > > index 8f085b1bb7c..9822c179962 100644
> > > > --- a/gdb/objfiles.c
> > > > +++ b/gdb/objfiles.c
> > > > @@ -793,14 +793,12 @@ have_full_symbols (void)
> > > >  void
> > > >  objfile_purge_solibs (void)
> > > >  {
> > > > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > > > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> > > >      {
> > > >        /* We assume that the solib package has been purged already, or will
> > > >          be soon.  */
> > > > -
> > > > -      if (!(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED))
> > > > -       objf->unlink ();
> > > > -    }
> > > > +      return !(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED);
> > > > +    });
> > > >  }
> > > >
> > > >
> > > > diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> > > > index 4b8aa9bfcec..c20b63ceadf 100644
> > > > --- a/gdb/objfiles.h
> > > > +++ b/gdb/objfiles.h
> > > > @@ -698,13 +698,17 @@ struct objfile
> > > >
> > > >  private:
> > > >
> > > > +  using qf_list = std::forward_list<quick_symbol_functions_up>;
> > > > +  using qf_range = iterator_range<qf_list::iterator>;
> > > > +  using qf_safe_range = basic_safe_range<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 (qf_range (qf.begin (), qf.end ()));
> > > >    }
> > > >
> > > >  public:
> > > > diff --git a/gdb/progspace.c b/gdb/progspace.c
> > > > index 839707e9d71..c0fca1dace7 100644
> > > > --- a/gdb/progspace.c
> > > > +++ b/gdb/progspace.c
> > > > @@ -143,19 +143,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));
> > > >      }
> > > >  }
> > > >
> > > > @@ -184,6 +184,17 @@ program_space::remove_objfile (struct objfile *objfile)
> > > >
> > > >  /* See progspace.h.  */
> > > >
> > > > +void
> > > > +program_space::unlink_objfiles_if
> > > > +  (gdb::function_view<bool (const objfile *objfile)> predicate)
> > > > +{
> > > > +  for (auto &it : objfiles_safe ())
> > > > +    if (predicate (it.get ()))
> > > > +      it->unlink ();
> > > > +}
> > > > +
> > > > +/* See progspace.h.  */
> > > > +
> > > >  struct objfile *
> > > >  program_space::objfile_for_address (CORE_ADDR address)
> > > >  {
> > > > diff --git a/gdb/progspace.h b/gdb/progspace.h
> > > > index a22e427400e..17bb1710ccf 100644
> > > > --- a/gdb/progspace.h
> > > > +++ b/gdb/progspace.h
> > > > @@ -214,28 +214,32 @@ struct program_space
> > > >         unwrapping_objfile_iterator (objfiles_list.end ()));
> > > >    }
> > > >
> > > > -  using objfiles_safe_range = basic_safe_range<objfiles_range>;
> > > > +  using objfiles_safe_range = iterator_range<objfile_list::iterator>;
> > > > +  using objfiles_safe_reverse_range
> > > > +    = basic_safe_reverse_range<objfiles_safe_range>;
> > > >
> > > >    /* An iterable object that can be used to iterate over all objfiles.
> > > >       The basic use is in a foreach, like:
> > > >
> > > >       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 ()
> > > > +     This variant uses a basic_safe_reverse_iterator so that objfiles
> > > > +     can be deleted during iteration.
> > > > +
> > > > +     The use of a reverse iterator helps ensure that separate debug
> > > > +     objfiles are deleted before their parent objfile.  This prevents
> > > > +     iterator invalidation 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_safe_range (objfiles_list.begin (), objfiles_list.end ()));
> > > >    }
> > > >
> > > > -  /* 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);
> > > > @@ -250,6 +254,11 @@ struct program_space
> > > >    /* Free all the objfiles associated with this program space.  */
> > > >    void free_all_objfiles ();
> > > >
> > > > +  /* Unlink all objfiles associated with this program space for which
> > > > +     PREDICATE evaluates to true.  */
> > > > +  void unlink_objfiles_if
> > > > +    (gdb::function_view<bool (const objfile *objfile)> predicate);
> > > > +
> > > >    /* Return the objfile containing ADDRESS, or nullptr if the address
> > > >       is outside all objfiles in this progspace.  */
> > > >    struct objfile *objfile_for_address (CORE_ADDR address);
> > > > 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" \
> > > > diff --git a/gdbsupport/safe-iterator.h b/gdbsupport/safe-iterator.h
> > > > index ccd772ca2a5..9f57c1543cf 100644
> > > > --- a/gdbsupport/safe-iterator.h
> > > > +++ b/gdbsupport/safe-iterator.h
> > > > @@ -136,4 +136,110 @@ class basic_safe_range
> > > >    Range m_range;
> > > >  };
> > > >
> > > > +/* A reverse basic_safe_iterator.  See basic_safe_iterator for intended use.  */
> > > > +
> > > > +template<typename Iterator>
> > > > +class basic_safe_reverse_iterator
> > > > +{
> > > > +public:
> > > > +  typedef basic_safe_reverse_iterator self_type;
> > > > +  typedef typename Iterator::value_type value_type;
> > > > +  typedef typename Iterator::reference reference;
> > > > +  typedef typename Iterator::pointer pointer;
> > > > +  typedef typename Iterator::iterator_category iterator_category;
> > > > +  typedef typename Iterator::difference_type difference_type;
> > > > +
> > > > +  /* Construct the iterator using ARG, and construct the end iterator
> > > > +     using ARG2.  */
> > > > +  template<typename Arg>
> > > > +  explicit basic_safe_reverse_iterator (Arg &&arg, Arg &&arg2)
> > > > +    : m_begin (std::forward<Arg> (arg)),
> > > > +      m_end (std::forward<Arg> (arg2)),
> > > > +      m_it (m_end),
> > > > +      m_next (m_end)
> > > > +  {
> > > > +    /* M_IT and M_NEXT are initialized as one-past-end.  Set M_IT to point
> > > > +       to the last element and set M_NEXT to point to the second last element,
> > > > +       if such elements exist.  */
> > > > +    if (m_it != m_begin)
> > > > +      {
> > > > +       --m_it;
> > > > +
> > > > +       if (m_it != m_begin)
> > > > +         {
> > > > +           --m_next;
> > > > +           --m_next;
> > > > +         }
> > > > +      }
> > > > +  }
> > > > +
> > > > +  typename gdb::invoke_result<decltype(&Iterator::operator*), Iterator>::type
> > > > +    operator* () const
> > > > +  { return *m_it; }
> > > > +
> > > > +  self_type &operator++ ()
> > > > +  {
> > > > +    m_it = m_next;
> > > > +
> > > > +    if (m_it != m_end)
> > > > +      {
> > > > +       /* Use M_BEGIN only if we sure that it is valid.  */
> > > > +       if (m_it == m_begin)
> > > > +         m_next = m_end;
> > > > +       else
> > > > +         --m_next;
> > > > +      }
> > > > +
> > > > +    return *this;
> > > > +  }
> > > > +
> > > > +  bool operator== (const self_type &other) const
> > > > +  { return m_it == other.m_it; }
> > > > +
> > > > +  bool operator!= (const self_type &other) const
> > > > +  { return m_it != other.m_it; }
> > > > +
> > > > +private:
> > > > +  /* The first element.  */
> > > > +  Iterator m_begin {};
> > > > +
> > > > +  /* A one-past-end iterator.  */
> > > > +  Iterator m_end {};
> > > > +
> > > > +  /* The current element.  */
> > > > +  Iterator m_it {};
> > > > +
> > > > +  /* The next element.  Always one element ahead of M_IT.  */
> > > > +  Iterator m_next {};
> > > > +};
> > > > +
> > > > +/* A range adapter that wraps a forward range, and then returns
> > > > +   safe reverse iterators wrapping the original range's iterators.  */
> > > > +
> > > > +template<typename Range>
> > > > +class basic_safe_reverse_range
> > > > +{
> > > > +public:
> > > > +
> > > > +  typedef basic_safe_reverse_iterator<typename Range::iterator> iterator;
> > > > +
> > > > +  explicit basic_safe_reverse_range (Range range)
> > > > +    : m_range (range)
> > > > +  {
> > > > +  }
> > > > +
> > > > +  iterator begin ()
> > > > +  {
> > > > +    return iterator (m_range.begin (), m_range.end ());
> > > > +  }
> > > > +
> > > > +  iterator end ()
> > > > +  {
> > > > +    return iterator (m_range.end (), m_range.end ());
> > > > +  }
> > > > +
> > > > +private:
> > > > +
> > > > +  Range m_range;
> > > > +};
> > > >  #endif /* COMMON_SAFE_ITERATOR_H */
> > > > --
> > > > 2.41.0
> > > >
Aaron Merey Dec. 20, 2023, 2:57 p.m. UTC | #5 
Ping

Thanks,
Aaron

On Tue, Dec 12, 2023 at 10:01 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Thu, Nov 30, 2023 at 11:30 AM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Mon, Nov 20, 2023 at 1:39 PM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > Ping
> > >
> > > Thanks,
> > > Aaron
> > >
> > > On Sun, Nov 12, 2023 at 3:20 PM Aaron Merey <amerey@redhat.com> wrote:
> > > >
> > > > Ping
> > > >
> > > > Thanks,
> > > > Aaron
> > > >
> > > > On Fri, Oct 27, 2023 at 8:20 PM Aaron Merey <amerey@redhat.com> wrote:
> > > > >
> > > > > v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html
> > > > >
> > > > > v2 removes unwrapping_reverse_objfile_iterator and adds
> > > > > basic_safe_reverse_range and basic_safe_reverse_iterator.
> > > > >
> > > > > Commit message:
> > > > >
> > > > > This patch changes progspace objfile_list insertion so that separate
> > > > > debug objfiles are placed into the list after the parent objfile,
> > > > > instead of before.  Additionally qf_require_partial_symbols now returns
> > > > > a safe_range.
> > > > >
> > > > > 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 need to delete a
> > > > > .gdb_index quick_symbol_functions from a parent objfile while looping
> > > > > the objfile's list of quick_symbol_functions becasue the separate
> > > > > debug objfile has just been downloaded.  The use of a safe_range
> > > > > prevents this removal from causing iterator invalidation.
> > > > >
> > > > > gdb might also download a debuginfo file during symtab expansion.
> > > > > In this case an objfile will be added to the current progspace's
> > > > > 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.
> > > > >
> > > > > To facilitate the safe deletion of objfiles, this patch also adds
> > > > > basic_safe_reverse_range and basic_safe_reverse_iterator.  This allows
> > > > > objfiles to be removed from the objfiles_list in a loop without iterator
> > > > > invalidation.
> > > > >
> > > > > If a forward safe iterator were to be used, the deletion of an
> > > > > objfile could invalidate the safe iterator's reference to the next
> > > > > objfile in the objfiles_list.  This can happen when the deletion
> > > > > of an objfile causes the deletion of a separate debug objfile that
> > > > > happens to the be next element in the objfiles_list.
> > > > >
> > > > > The standard reverse iterator is not suitable for safe objfile deletion.
> > > > > In order to safely delete the first objfile in the objfiles_list, the
> > > > > standard reverse iterator's underlying begin iterator would have to be
> > > > > decremented, resulting in undefined behavior.
> > > > >
> > > > > 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/jit.c                               |   7 +-
> > > > >  gdb/objfiles.c                          |   8 +-
> > > > >  gdb/objfiles.h                          |   8 +-
> > > > >  gdb/progspace.c                         |  19 ++++-
> > > > >  gdb/progspace.h                         |  31 ++++---
> > > > >  gdb/testsuite/gdb.python/py-objfile.exp |   2 +-
> > > > >  gdbsupport/safe-iterator.h              | 106 ++++++++++++++++++++++++
> > > > >  7 files changed, 154 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/gdb/jit.c b/gdb/jit.c
> > > > > index 9e8325ab803..a39fdc5a96d 100644
> > > > > --- a/gdb/jit.c
> > > > > +++ b/gdb/jit.c
> > > > > @@ -1240,11 +1240,10 @@ jit_breakpoint_re_set (void)
> > > > >  static void
> > > > >  jit_inferior_exit_hook (struct inferior *inf)
> > > > >  {
> > > > > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > > > > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> > > > >      {
> > > > > -      if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
> > > > > -       objf->unlink ();
> > > > > -    }
> > > > > +      return (objf->jited_data != nullptr) && (objf->jited_data->addr != 0);
> > > > > +    });
> > > > >  }
> > > > >
> > > > >  void
> > > > > diff --git a/gdb/objfiles.c b/gdb/objfiles.c
> > > > > index 8f085b1bb7c..9822c179962 100644
> > > > > --- a/gdb/objfiles.c
> > > > > +++ b/gdb/objfiles.c
> > > > > @@ -793,14 +793,12 @@ have_full_symbols (void)
> > > > >  void
> > > > >  objfile_purge_solibs (void)
> > > > >  {
> > > > > -  for (objfile *objf : current_program_space->objfiles_safe ())
> > > > > +  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
> > > > >      {
> > > > >        /* We assume that the solib package has been purged already, or will
> > > > >          be soon.  */
> > > > > -
> > > > > -      if (!(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED))
> > > > > -       objf->unlink ();
> > > > > -    }
> > > > > +      return !(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED);
> > > > > +    });
> > > > >  }
> > > > >
> > > > >
> > > > > diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> > > > > index 4b8aa9bfcec..c20b63ceadf 100644
> > > > > --- a/gdb/objfiles.h
> > > > > +++ b/gdb/objfiles.h
> > > > > @@ -698,13 +698,17 @@ struct objfile
> > > > >
> > > > >  private:
> > > > >
> > > > > +  using qf_list = std::forward_list<quick_symbol_functions_up>;
> > > > > +  using qf_range = iterator_range<qf_list::iterator>;
> > > > > +  using qf_safe_range = basic_safe_range<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 (qf_range (qf.begin (), qf.end ()));
> > > > >    }
> > > > >
> > > > >  public:
> > > > > diff --git a/gdb/progspace.c b/gdb/progspace.c
> > > > > index 839707e9d71..c0fca1dace7 100644
> > > > > --- a/gdb/progspace.c
> > > > > +++ b/gdb/progspace.c
> > > > > @@ -143,19 +143,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));
> > > > >      }
> > > > >  }
> > > > >
> > > > > @@ -184,6 +184,17 @@ program_space::remove_objfile (struct objfile *objfile)
> > > > >
> > > > >  /* See progspace.h.  */
> > > > >
> > > > > +void
> > > > > +program_space::unlink_objfiles_if
> > > > > +  (gdb::function_view<bool (const objfile *objfile)> predicate)
> > > > > +{
> > > > > +  for (auto &it : objfiles_safe ())
> > > > > +    if (predicate (it.get ()))
> > > > > +      it->unlink ();
> > > > > +}
> > > > > +
> > > > > +/* See progspace.h.  */
> > > > > +
> > > > >  struct objfile *
> > > > >  program_space::objfile_for_address (CORE_ADDR address)
> > > > >  {
> > > > > diff --git a/gdb/progspace.h b/gdb/progspace.h
> > > > > index a22e427400e..17bb1710ccf 100644
> > > > > --- a/gdb/progspace.h
> > > > > +++ b/gdb/progspace.h
> > > > > @@ -214,28 +214,32 @@ struct program_space
> > > > >         unwrapping_objfile_iterator (objfiles_list.end ()));
> > > > >    }
> > > > >
> > > > > -  using objfiles_safe_range = basic_safe_range<objfiles_range>;
> > > > > +  using objfiles_safe_range = iterator_range<objfile_list::iterator>;
> > > > > +  using objfiles_safe_reverse_range
> > > > > +    = basic_safe_reverse_range<objfiles_safe_range>;
> > > > >
> > > > >    /* An iterable object that can be used to iterate over all objfiles.
> > > > >       The basic use is in a foreach, like:
> > > > >
> > > > >       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 ()
> > > > > +     This variant uses a basic_safe_reverse_iterator so that objfiles
> > > > > +     can be deleted during iteration.
> > > > > +
> > > > > +     The use of a reverse iterator helps ensure that separate debug
> > > > > +     objfiles are deleted before their parent objfile.  This prevents
> > > > > +     iterator invalidation 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_safe_range (objfiles_list.begin (), objfiles_list.end ()));
> > > > >    }
> > > > >
> > > > > -  /* 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);
> > > > > @@ -250,6 +254,11 @@ struct program_space
> > > > >    /* Free all the objfiles associated with this program space.  */
> > > > >    void free_all_objfiles ();
> > > > >
> > > > > +  /* Unlink all objfiles associated with this program space for which
> > > > > +     PREDICATE evaluates to true.  */
> > > > > +  void unlink_objfiles_if
> > > > > +    (gdb::function_view<bool (const objfile *objfile)> predicate);
> > > > > +
> > > > >    /* Return the objfile containing ADDRESS, or nullptr if the address
> > > > >       is outside all objfiles in this progspace.  */
> > > > >    struct objfile *objfile_for_address (CORE_ADDR address);
> > > > > 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" \
> > > > > diff --git a/gdbsupport/safe-iterator.h b/gdbsupport/safe-iterator.h
> > > > > index ccd772ca2a5..9f57c1543cf 100644
> > > > > --- a/gdbsupport/safe-iterator.h
> > > > > +++ b/gdbsupport/safe-iterator.h
> > > > > @@ -136,4 +136,110 @@ class basic_safe_range
> > > > >    Range m_range;
> > > > >  };
> > > > >
> > > > > +/* A reverse basic_safe_iterator.  See basic_safe_iterator for intended use.  */
> > > > > +
> > > > > +template<typename Iterator>
> > > > > +class basic_safe_reverse_iterator
> > > > > +{
> > > > > +public:
> > > > > +  typedef basic_safe_reverse_iterator self_type;
> > > > > +  typedef typename Iterator::value_type value_type;
> > > > > +  typedef typename Iterator::reference reference;
> > > > > +  typedef typename Iterator::pointer pointer;
> > > > > +  typedef typename Iterator::iterator_category iterator_category;
> > > > > +  typedef typename Iterator::difference_type difference_type;
> > > > > +
> > > > > +  /* Construct the iterator using ARG, and construct the end iterator
> > > > > +     using ARG2.  */
> > > > > +  template<typename Arg>
> > > > > +  explicit basic_safe_reverse_iterator (Arg &&arg, Arg &&arg2)
> > > > > +    : m_begin (std::forward<Arg> (arg)),
> > > > > +      m_end (std::forward<Arg> (arg2)),
> > > > > +      m_it (m_end),
> > > > > +      m_next (m_end)
> > > > > +  {
> > > > > +    /* M_IT and M_NEXT are initialized as one-past-end.  Set M_IT to point
> > > > > +       to the last element and set M_NEXT to point to the second last element,
> > > > > +       if such elements exist.  */
> > > > > +    if (m_it != m_begin)
> > > > > +      {
> > > > > +       --m_it;
> > > > > +
> > > > > +       if (m_it != m_begin)
> > > > > +         {
> > > > > +           --m_next;
> > > > > +           --m_next;
> > > > > +         }
> > > > > +      }
> > > > > +  }
> > > > > +
> > > > > +  typename gdb::invoke_result<decltype(&Iterator::operator*), Iterator>::type
> > > > > +    operator* () const
> > > > > +  { return *m_it; }
> > > > > +
> > > > > +  self_type &operator++ ()
> > > > > +  {
> > > > > +    m_it = m_next;
> > > > > +
> > > > > +    if (m_it != m_end)
> > > > > +      {
> > > > > +       /* Use M_BEGIN only if we sure that it is valid.  */
> > > > > +       if (m_it == m_begin)
> > > > > +         m_next = m_end;
> > > > > +       else
> > > > > +         --m_next;
> > > > > +      }
> > > > > +
> > > > > +    return *this;
> > > > > +  }
> > > > > +
> > > > > +  bool operator== (const self_type &other) const
> > > > > +  { return m_it == other.m_it; }
> > > > > +
> > > > > +  bool operator!= (const self_type &other) const
> > > > > +  { return m_it != other.m_it; }
> > > > > +
> > > > > +private:
> > > > > +  /* The first element.  */
> > > > > +  Iterator m_begin {};
> > > > > +
> > > > > +  /* A one-past-end iterator.  */
> > > > > +  Iterator m_end {};
> > > > > +
> > > > > +  /* The current element.  */
> > > > > +  Iterator m_it {};
> > > > > +
> > > > > +  /* The next element.  Always one element ahead of M_IT.  */
> > > > > +  Iterator m_next {};
> > > > > +};
> > > > > +
> > > > > +/* A range adapter that wraps a forward range, and then returns
> > > > > +   safe reverse iterators wrapping the original range's iterators.  */
> > > > > +
> > > > > +template<typename Range>
> > > > > +class basic_safe_reverse_range
> > > > > +{
> > > > > +public:
> > > > > +
> > > > > +  typedef basic_safe_reverse_iterator<typename Range::iterator> iterator;
> > > > > +
> > > > > +  explicit basic_safe_reverse_range (Range range)
> > > > > +    : m_range (range)
> > > > > +  {
> > > > > +  }
> > > > > +
> > > > > +  iterator begin ()
> > > > > +  {
> > > > > +    return iterator (m_range.begin (), m_range.end ());
> > > > > +  }
> > > > > +
> > > > > +  iterator end ()
> > > > > +  {
> > > > > +    return iterator (m_range.end (), m_range.end ());
> > > > > +  }
> > > > > +
> > > > > +private:
> > > > > +
> > > > > +  Range m_range;
> > > > > +};
> > > > >  #endif /* COMMON_SAFE_ITERATOR_H */
> > > > > --
> > > > > 2.41.0
> > > > >
Thiago Jung Bauermann Dec. 26, 2023, 5:09 p.m. UTC | #6 
Aaron Merey <amerey@redhat.com> writes:

> v1: https://sourceware.org/pipermail/gdb-patches/2023-June/199984.html
>
> v2 removes unwrapping_reverse_objfile_iterator and adds
> basic_safe_reverse_range and basic_safe_reverse_iterator.
>
> Commit message:
>
> This patch changes progspace objfile_list insertion so that separate
> debug objfiles are placed into the list after the parent objfile,
> instead of before.  Additionally qf_require_partial_symbols now returns
> a safe_range.
>
> 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 need to delete a
> .gdb_index quick_symbol_functions from a parent objfile while looping
> the objfile's list of quick_symbol_functions becasue the separate

*because

> debug objfile has just been downloaded.  The use of a safe_range
> prevents this removal from causing iterator invalidation.

<snip>

> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index a22e427400e..17bb1710ccf 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -214,28 +214,32 @@ struct program_space
>         unwrapping_objfile_iterator (objfiles_list.end ()));
>    }
>  
> -  using objfiles_safe_range = basic_safe_range<objfiles_range>;
> +  using objfiles_safe_range = iterator_range<objfile_list::iterator>;

Does objfile_safe_range still need the safe qualifier? Or should it
be called objfiles_range now?

> +  using objfiles_safe_reverse_range
> +    = basic_safe_reverse_range<objfiles_safe_range>;

<snip>

> 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

Does the fact that this testcase needed change mean that there was an
API break? In my opinion it doesn't and this patch is ok. I think it
would be too much to guarantee a specific ordering of the returned
objfiles.

But I'm mentioning it anyway because perhaps someone has a different
view?
diff mbox series

Patch

diff --git a/gdb/jit.c b/gdb/jit.c
index 9e8325ab803..a39fdc5a96d 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -1240,11 +1240,10 @@  jit_breakpoint_re_set (void)
 static void
 jit_inferior_exit_hook (struct inferior *inf)
 {
-  for (objfile *objf : current_program_space->objfiles_safe ())
+  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
     {
-      if (objf->jited_data != nullptr && objf->jited_data->addr != 0)
-	objf->unlink ();
-    }
+      return (objf->jited_data != nullptr) && (objf->jited_data->addr != 0);
+    });
 }
 
 void
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 8f085b1bb7c..9822c179962 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -793,14 +793,12 @@  have_full_symbols (void)
 void
 objfile_purge_solibs (void)
 {
-  for (objfile *objf : current_program_space->objfiles_safe ())
+  current_program_space->unlink_objfiles_if ([&] (const objfile *objf)
     {
       /* We assume that the solib package has been purged already, or will
 	 be soon.  */
-
-      if (!(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED))
-	objf->unlink ();
-    }
+      return !(objf->flags & OBJF_USERLOADED) && (objf->flags & OBJF_SHARED);
+    });
 }
 
 
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 4b8aa9bfcec..c20b63ceadf 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -698,13 +698,17 @@  struct objfile
 
 private:
 
+  using qf_list = std::forward_list<quick_symbol_functions_up>;
+  using qf_range = iterator_range<qf_list::iterator>;
+  using qf_safe_range = basic_safe_range<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 (qf_range (qf.begin (), qf.end ()));
   }
 
 public:
diff --git a/gdb/progspace.c b/gdb/progspace.c
index 839707e9d71..c0fca1dace7 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -143,19 +143,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));
     }
 }
 
@@ -184,6 +184,17 @@  program_space::remove_objfile (struct objfile *objfile)
 
 /* See progspace.h.  */
 
+void
+program_space::unlink_objfiles_if
+  (gdb::function_view<bool (const objfile *objfile)> predicate)
+{
+  for (auto &it : objfiles_safe ())
+    if (predicate (it.get ()))
+      it->unlink ();
+}
+
+/* See progspace.h.  */
+
 struct objfile *
 program_space::objfile_for_address (CORE_ADDR address)
 {
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a22e427400e..17bb1710ccf 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -214,28 +214,32 @@  struct program_space
        unwrapping_objfile_iterator (objfiles_list.end ()));
   }
 
-  using objfiles_safe_range = basic_safe_range<objfiles_range>;
+  using objfiles_safe_range = iterator_range<objfile_list::iterator>;
+  using objfiles_safe_reverse_range
+    = basic_safe_reverse_range<objfiles_safe_range>;
 
   /* An iterable object that can be used to iterate over all objfiles.
      The basic use is in a foreach, like:
 
      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 ()
+     This variant uses a basic_safe_reverse_iterator so that objfiles
+     can be deleted during iteration.
+
+     The use of a reverse iterator helps ensure that separate debug
+     objfiles are deleted before their parent objfile.  This prevents
+     iterator invalidation 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_safe_range (objfiles_list.begin (), objfiles_list.end ()));
   }
 
-  /* 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);
@@ -250,6 +254,11 @@  struct program_space
   /* Free all the objfiles associated with this program space.  */
   void free_all_objfiles ();
 
+  /* Unlink all objfiles associated with this program space for which
+     PREDICATE evaluates to true.  */
+  void unlink_objfiles_if
+    (gdb::function_view<bool (const objfile *objfile)> predicate);
+
   /* Return the objfile containing ADDRESS, or nullptr if the address
      is outside all objfiles in this progspace.  */
   struct objfile *objfile_for_address (CORE_ADDR address);
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" \
diff --git a/gdbsupport/safe-iterator.h b/gdbsupport/safe-iterator.h
index ccd772ca2a5..9f57c1543cf 100644
--- a/gdbsupport/safe-iterator.h
+++ b/gdbsupport/safe-iterator.h
@@ -136,4 +136,110 @@  class basic_safe_range
   Range m_range;
 };
 
+/* A reverse basic_safe_iterator.  See basic_safe_iterator for intended use.  */
+
+template<typename Iterator>
+class basic_safe_reverse_iterator
+{
+public:
+  typedef basic_safe_reverse_iterator self_type;
+  typedef typename Iterator::value_type value_type;
+  typedef typename Iterator::reference reference;
+  typedef typename Iterator::pointer pointer;
+  typedef typename Iterator::iterator_category iterator_category;
+  typedef typename Iterator::difference_type difference_type;
+
+  /* Construct the iterator using ARG, and construct the end iterator
+     using ARG2.  */
+  template<typename Arg>
+  explicit basic_safe_reverse_iterator (Arg &&arg, Arg &&arg2)
+    : m_begin (std::forward<Arg> (arg)),
+      m_end (std::forward<Arg> (arg2)),
+      m_it (m_end),
+      m_next (m_end)
+  {
+    /* M_IT and M_NEXT are initialized as one-past-end.  Set M_IT to point
+       to the last element and set M_NEXT to point to the second last element,
+       if such elements exist.  */
+    if (m_it != m_begin)
+      {
+	--m_it;
+
+	if (m_it != m_begin)
+	  {
+	    --m_next;
+	    --m_next;
+	  }
+      }
+  }
+
+  typename gdb::invoke_result<decltype(&Iterator::operator*), Iterator>::type
+    operator* () const
+  { return *m_it; }
+
+  self_type &operator++ ()
+  {
+    m_it = m_next;
+
+    if (m_it != m_end)
+      {
+	/* Use M_BEGIN only if we sure that it is valid.  */
+	if (m_it == m_begin)
+	  m_next = m_end;
+	else
+	  --m_next;
+      }
+
+    return *this;
+  }
+
+  bool operator== (const self_type &other) const
+  { return m_it == other.m_it; }
+
+  bool operator!= (const self_type &other) const
+  { return m_it != other.m_it; }
+
+private:
+  /* The first element.  */
+  Iterator m_begin {};
+
+  /* A one-past-end iterator.  */
+  Iterator m_end {};
+
+  /* The current element.  */
+  Iterator m_it {};
+
+  /* The next element.  Always one element ahead of M_IT.  */
+  Iterator m_next {};
+};
+
+/* A range adapter that wraps a forward range, and then returns
+   safe reverse iterators wrapping the original range's iterators.  */
+
+template<typename Range>
+class basic_safe_reverse_range
+{
+public:
+
+  typedef basic_safe_reverse_iterator<typename Range::iterator> iterator;
+
+  explicit basic_safe_reverse_range (Range range)
+    : m_range (range)
+  {
+  }
+
+  iterator begin ()
+  {
+    return iterator (m_range.begin (), m_range.end ());
+  }
+
+  iterator end ()
+  {
+    return iterator (m_range.end (), m_range.end ());
+  }
+
+private:
+
+  Range m_range;
+};
 #endif /* COMMON_SAFE_ITERATOR_H */