From: Aaron Merey <amerey@redhat.com>
To: gdb-patches@sourceware.org
Cc: Andrew Burgess <aburgess@redhat.com>
Subject: [PING*4][PATCH 2/4 v2] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator
Date: Tue, 12 Dec 2023 10:01:14 -0500 [thread overview]
Message-ID: <CAJDtP-QpYoCHuBSYjts=EgJSmi2JnE6iijw+n6yHQkbfob5P1A@mail.gmail.com> (raw)
In-Reply-To: <CAJDtP-R5FrUni2iDawov1k_BMfsyUVUtkaVXQ+WOECKGBrWFkw@mail.gmail.com>
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
> > > >
next prev parent reply other threads:[~2023-12-12 15:01 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-28 0:20 [PATCH 0/4] On-demand debuginfo downloading Aaron Merey
2023-10-28 0:20 ` [PATCH 1/4 v7] gdb: Buffer output streams during events that might download debuginfo Aaron Merey
2023-11-12 20:20 ` Aaron Merey
2023-11-20 18:38 ` [PING*2][PATCH " Aaron Merey
2023-11-30 16:29 ` [PING*3][PATCH " Aaron Merey
2023-12-12 15:00 ` [PING*4][PATCH " Aaron Merey
2023-12-20 14:57 ` [PING*5][PATCH " Aaron Merey
2023-12-26 16:28 ` [PATCH " Thiago Jung Bauermann
2024-01-17 17:49 ` Aaron Merey
2024-01-17 18:05 ` Andrew Burgess
2023-10-28 0:20 ` [PATCH 2/4 v2] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator Aaron Merey
2023-11-12 20:20 ` Aaron Merey
2023-11-20 18:39 ` [PING*2][PATCH " Aaron Merey
2023-11-30 16:30 ` [PING*3][PATCH " Aaron Merey
2023-12-12 15:01 ` Aaron Merey [this message]
2023-12-20 14:57 ` [PING*5][PATCH " Aaron Merey
2023-12-26 17:09 ` [PATCH " Thiago Jung Bauermann
2023-10-28 0:20 ` [PATCH 3/4 v4] gdb/debuginfod: Support on-demand debuginfo downloading Aaron Merey
2023-11-12 20:20 ` Aaron Merey
2023-11-20 18:39 ` [PING*2][PATCH " Aaron Merey
2023-11-30 16:30 ` [PING*3][PATCH " Aaron Merey
2023-12-12 15:01 ` [PING*4][PATCH " Aaron Merey
2023-12-20 14:57 ` [PING*5][PATCH " Aaron Merey
2023-12-26 18:35 ` [PATCH " Thiago Jung Bauermann
2023-10-28 0:20 ` [PATCH 4/4 v5] gdb/debuginfod: Add .debug_line downloading Aaron Merey
2023-11-12 20:21 ` Aaron Merey
2023-11-20 18:40 ` [PING*2][PATCH " Aaron Merey
2023-11-30 16:30 ` [PING*3][PATCH " Aaron Merey
2023-12-12 15:08 ` [PING*4][PATCH " Aaron Merey
2023-12-20 14:58 ` [PING*5][PATCH " Aaron Merey
2023-12-27 0:30 ` [PATCH " Thiago Jung Bauermann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJDtP-QpYoCHuBSYjts=EgJSmi2JnE6iijw+n6yHQkbfob5P1A@mail.gmail.com' \
--to=amerey@redhat.com \
--cc=aburgess@redhat.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).