[PING*2][PATCH 5/7 v2] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator

[Aaron Merey <amerey@redhat.com>]

Ping

Thanks,
Aaron

On Tue, Sep 19, 2023 at 10:35 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Wed, Aug 16, 2023 at 12:43 AM 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 804c832f47d..091e0b4a771 100644
> > --- a/gdb/jit.c
> > +++ b/gdb/jit.c
> > @@ -1239,11 +1239,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 5ba5f0a616d..f862182458d 100644
> > --- a/gdb/objfiles.c
> > +++ b/gdb/objfiles.c
> > @@ -794,14 +794,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 189856f0a51..4a396af17da 100644
> > --- a/gdb/objfiles.h
> > +++ b/gdb/objfiles.h
> > @@ -699,13 +699,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 5cf8334ee67..2ab1a799fe7 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));
> >      }
> >  }
> >
> > @@ -180,6 +180,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 ee12d89c173..a730c1334eb 100644
> > --- a/gdb/progspace.h
> > +++ b/gdb/progspace.h
> > @@ -213,28 +213,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);
> > @@ -249,6 +253,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
> >