[PING*2][PATCH 1/3 v3] gdb/progspace: Add reverse safe iterator
Aaron Merey
amerey@redhat.com
Fri May 31 20:58:00 GMT 2024
Ping
Thanks,
Aaron
On Fri, May 17, 2024 at 10:09 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Fri, May 3, 2024 at 6:59 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > This is a reposting of v3 from
> > https://sourceware.org/pipermail/gdb-patches/2024-January/205955.html
> >
> > 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 because 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 | 14 +--
> > gdb/objfiles.h | 17 +++-
> > gdb/progspace.c | 19 +++-
> > gdb/progspace.h | 31 ++++---
> > gdb/symfile-debug.c | 34 +++----
> > gdb/testsuite/gdb.python/py-objfile.exp | 2 +-
> > gdbsupport/safe-iterator.h | 115 ++++++++++++++++++++++++
> > 8 files changed, 195 insertions(+), 44 deletions(-)
> >
> > diff --git a/gdb/jit.c b/gdb/jit.c
> > index 797be95a8da..90f9159fa9d 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 ba88ed1bc41..70017c7fb32 100644
> > --- a/gdb/objfiles.c
> > +++ b/gdb/objfiles.c
> > @@ -469,6 +469,12 @@ objfile::unlink ()
> > current_program_space->remove_objfile (this);
> > }
> >
> > +qf_safe_range
> > +objfile::qf_safe ()
> > +{
> > + return qf_safe_range (qf_range (qf.begin (), qf.end ()));
> > +}
> > +
> > /* Free all separate debug objfile of OBJFILE, but don't free OBJFILE
> > itself. */
> >
> > @@ -792,14 +798,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 8b8b7182e87..1c8f79a2826 100644
> > --- a/gdb/objfiles.h
> > +++ b/gdb/objfiles.h
> > @@ -370,6 +370,12 @@ class separate_debug_iterator
> >
> > typedef iterator_range<separate_debug_iterator> separate_debug_range;
> >
> > +/* See objfile::qf_safe. */
> > +
> > +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>;
> > +
> > /* Sections in an objfile. The section offsets are stored in the
> > OBJFILE. */
> >
> > @@ -767,8 +773,15 @@ struct objfile
> > const struct sym_fns *sf = nullptr;
> >
> > /* The "quick" (aka partial) symbol functions for this symbol
> > - reader. */
> > - std::forward_list<quick_symbol_functions_up> qf;
> > + reader. Many quick_symbol_functions methods may result
> > + in the deletion of a quick_symbol_functions from this
> > + qf_list. It is recommended that qf_safe be used to iterate
> > + over the qf_list. */
> > + qf_list qf;
> > +
> > + /* Returns an iterable object that allows for safe deletion during
> > + iteration. See gdbsupport/safe-iterator.h. */
> > + qf_safe_range qf_safe ();
> >
> > /* Per objfile data-pointers required by other GDB modules. */
> >
> > diff --git a/gdb/progspace.c b/gdb/progspace.c
> > index 0deca3f2376..b1b2dfb30c8 100644
> > --- a/gdb/progspace.c
> > +++ b/gdb/progspace.c
> > @@ -140,19 +140,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));
> > }
> > }
> >
> > @@ -181,6 +181,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 bbf54efa07a..4d5ac991849 100644
> > --- a/gdb/progspace.h
> > +++ b/gdb/progspace.h
> > @@ -250,28 +250,32 @@ struct program_space
> > unwrapping_objfile_iterator (objfiles_list.end ()));
> > }
> >
> > - using objfiles_safe_range = basic_safe_range<objfiles_range>;
> > + using objfiles_it_range = iterator_range<objfile_list::iterator>;
> > + using objfiles_safe_reverse_range
> > + = basic_safe_reverse_range<objfiles_it_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_it_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);
> > @@ -286,6 +290,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/symfile-debug.c b/gdb/symfile-debug.c
> > index 86c7010b2d2..330a520dca6 100644
> > --- a/gdb/symfile-debug.c
> > +++ b/gdb/symfile-debug.c
> > @@ -84,7 +84,7 @@ objfile::has_partial_symbols ()
> > them, then that is an indication that they are in fact available. Without
> > this function the symbols may have been already read in but they also may
> > not be present in this objfile. */
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > retval = iter->has_symbols (this);
> > if (retval)
> > @@ -107,7 +107,7 @@ objfile::has_unexpanded_symtabs ()
> > objfile_debug_name (this));
> >
> > bool result = false;
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > if (iter->has_unexpanded_symtabs (this))
> > {
> > @@ -132,7 +132,7 @@ objfile::find_last_source_symtab ()
> > gdb_printf (gdb_stdlog, "qf->find_last_source_symtab (%s)\n",
> > objfile_debug_name (this));
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > retval = iter->find_last_source_symtab (this);
> > if (retval != nullptr)
> > @@ -165,7 +165,7 @@ objfile::forget_cached_source_info ()
> > }
> > }
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->forget_cached_source_info (this);
> > }
> >
> > @@ -212,7 +212,7 @@ objfile::map_symtabs_matching_filename
> > return result;
> > };
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > if (!iter->expand_symtabs_matching (this,
> > match_one_filename,
> > @@ -275,7 +275,7 @@ objfile::lookup_symbol (block_enum kind, const lookup_name_info &name,
> > return true;
> > };
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > if (!iter->expand_symtabs_matching (this,
> > nullptr,
> > @@ -305,7 +305,7 @@ objfile::print_stats (bool print_bcache)
> > gdb_printf (gdb_stdlog, "qf->print_stats (%s, %d)\n",
> > objfile_debug_name (this), print_bcache);
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->print_stats (this, print_bcache);
> > }
> >
> > @@ -316,7 +316,7 @@ objfile::dump ()
> > gdb_printf (gdb_stdlog, "qf->dump (%s)\n",
> > objfile_debug_name (this));
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->dump (this);
> > }
> >
> > @@ -331,7 +331,7 @@ objfile::expand_symtabs_for_function (const char *func_name)
> > lookup_name_info base_lookup (func_name, symbol_name_match_type::FULL);
> > lookup_name_info lookup_name = base_lookup.make_ignore_params ();
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->expand_symtabs_matching (this,
> > nullptr,
> > &lookup_name,
> > @@ -349,7 +349,7 @@ objfile::expand_all_symtabs ()
> > gdb_printf (gdb_stdlog, "qf->expand_all_symtabs (%s)\n",
> > objfile_debug_name (this));
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->expand_all_symtabs (this);
> > }
> >
> > @@ -367,7 +367,7 @@ objfile::expand_symtabs_with_fullname (const char *fullname)
> > return filename_cmp (basenames ? basename : fullname, filename) == 0;
> > };
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->expand_symtabs_matching (this,
> > file_matcher,
> > nullptr,
> > @@ -399,7 +399,7 @@ objfile::expand_symtabs_matching
> > host_address_to_string (&expansion_notify),
> > domain_name (domain).c_str ());
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > if (!iter->expand_symtabs_matching (this, file_matcher, lookup_name,
> > symbol_matcher, expansion_notify,
> > search_flags, domain))
> > @@ -424,7 +424,7 @@ objfile::find_pc_sect_compunit_symtab (struct bound_minimal_symbol msymbol,
> > host_address_to_string (section),
> > warn_if_readin);
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > retval = iter->find_pc_sect_compunit_symtab (this, msymbol, pc, section,
> > warn_if_readin);
> > @@ -452,7 +452,7 @@ objfile::map_symbol_filenames (gdb::function_view<symbol_filename_ftype> fun,
> > objfile_debug_name (this),
> > need_fullname);
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->map_symbol_filenames (this, fun, need_fullname);
> > }
> >
> > @@ -464,7 +464,7 @@ objfile::compute_main_name ()
> > "qf->compute_main_name (%s)\n",
> > objfile_debug_name (this));
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->compute_main_name (this);
> > }
> >
> > @@ -478,7 +478,7 @@ objfile::find_compunit_symtab_by_address (CORE_ADDR address)
> > hex_string (address));
> >
> > struct compunit_symtab *result = NULL;
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > result = iter->find_compunit_symtab_by_address (this, address);
> > if (result != nullptr)
> > @@ -503,7 +503,7 @@ objfile::lookup_global_symbol_language (const char *name,
> > enum language result = language_unknown;
> > *symbol_found_p = false;
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > result = iter->lookup_global_symbol_language (this, name, domain,
> > symbol_found_p);
> > diff --git a/gdb/testsuite/gdb.python/py-objfile.exp b/gdb/testsuite/gdb.python/py-objfile.exp
> > index 2f5b7752b43..8460c300de0 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 f69f3896758..be23cf7920a 100644
> > --- a/gdbsupport/safe-iterator.h
> > +++ b/gdbsupport/safe-iterator.h
> > @@ -136,4 +136,119 @@ 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. ARG and ARG2 should be forward iterators, typically
> > + from begin and end methods, respectively.
> > +
> > + For example if ARG1 is created with container.begin and ARG2 is
> > + is created with container.end, then the basic_safe_reverse_iterator
> > + will traverse from the last element in the container to the first
> > + element in the container. */
> > + 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 std::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;
> > +
> > + /* RANGE must be a forward range. basic_safe_reverse_iterators
> > + will be used to traverse the forward range from the last element
> > + to the first. */
> > + 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.43.0
> >
More information about the Gdb-patches
mailing list