[2/4,v3] gdb/progspace: Add reverse safe iterator
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
Commit Message
v3 tweaks qf iteration in symfile-debug.c to account for the removal of
qf_require_partial_symbols.
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.
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 qf 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 | 10 +++
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 | 106 ++++++++++++++++++++++++
8 files changed, 181 insertions(+), 42 deletions(-)
Comments
Aaron Merey <amerey@redhat.com> writes:
> v3 tweaks qf iteration in symfile-debug.c to account for the removal of
> qf_require_partial_symbols.
>
> 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.
>
> 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 qf 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 | 10 +++
> 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 | 106 ++++++++++++++++++++++++
> 8 files changed, 181 insertions(+), 42 deletions(-)
>
> diff --git a/gdb/jit.c b/gdb/jit.c
> index 85a10be3055..10ee5f9b749 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..d53dfaca90b 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -470,6 +470,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. */
>
> @@ -793,14 +799,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 621342d22b7..34b3dac0b26 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. */
>
> @@ -764,6 +770,10 @@ struct objfile
> reader. */
> std::forward_list<quick_symbol_functions_up> qf;
I think this should be updated to use qf_list.
>
> + /* Returns iterable object that allows for safe deletion during
> + iteration. */
> + qf_safe_range qf_safe ();
> +
> /* Per objfile data-pointers required by other GDB modules. */
>
> registry<objfile> registry_fields;
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 0fc1fdd57ab..c4cc2e2e8af 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -141,19 +141,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));
> }
> }
>
> @@ -182,6 +182,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 163cbf6a0f8..f23a57dc842 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 32c3ed9d349..c684f9cdd43 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -85,7 +85,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 ())
I read the commit message a couple of times, but I'm still not 100% sure
I understand what's happening here. Is the change from qf -> qf_safe()
because calling (in this case has_symbols()) might download the full
debug info, and in doing do, might delete a previously downloaded
.debug_info section?
If this is the case then is it true that we will almost never want to
iterate over `qf` directly as most quick_symbol_functions methods might
result in this download/delete behaviour?
If I'm right, then I think it might be useful to place a warning in the
comment before `qf` explaining this.
> {
> retval = iter->has_symbols (this);
> if (retval)
> @@ -108,7 +108,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))
> {
> @@ -133,7 +133,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)
> @@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
> }
> }
>
> - for (const auto &iter : qf)
> + for (const auto &iter : qf_safe ())
> iter->forget_cached_source_info (this);
> }
>
> @@ -213,7 +213,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,
> @@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
> return true;
> };
>
> - for (const auto &iter : qf)
> + for (const auto &iter : qf_safe ())
> {
> if (!iter->expand_symtabs_matching (this,
> nullptr,
> @@ -309,7 +309,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);
> }
>
> @@ -320,7 +320,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);
> }
>
> @@ -335,7 +335,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,
> @@ -354,7 +354,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);
> }
>
> @@ -372,7 +372,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,
> @@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
> host_address_to_string (&expansion_notify),
> search_domain_name (kind));
>
> - 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, kind))
> @@ -431,7 +431,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);
> @@ -459,7 +459,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);
> }
>
> @@ -471,7 +471,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);
> }
>
> @@ -485,7 +485,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)
> @@ -510,7 +510,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 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 b4891328f1a..8bb80729bbd 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. */
Maybe this is a standard thing, and I'm just showing my lack of
experience, but, given the existence of methods like rbegin() and
rend(), I think the comments here should be doing more to explain what's
expected of this input.
Plus this comment seems confusing, though I can sort of see what it's
trying to say.
We're creating a *_iterator object, which itself contains other
iterators. We need to be super clear which particular "iterator" we're
talking about in the comments.
Also, I think we need to be explicit that for this reverse iterator, the
first argument to the constructor is going to be the LAST element that
is iterated over. One might be tempted to think that a safe reverse iterator
should be initialised with rbegin() and rend(), which isn't going to be
correct (unless I'm not understand, please correct me if so).
> + 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. */
I think the fact that this takes a FORWARD range is really important,
and we should do more to advertise this. E.g. I think there should be a
comment on the constructor saying that RANGE is a forward range over
which we will iterate backwards.
Thanks,
Andrew
> +
> +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.43.0
Hi Andrew,
Thanks for the review.
On Thu, Jan 18, 2024 at 7:03 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> Aaron Merey <amerey@redhat.com> writes:
> > +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. */
> >
> > @@ -764,6 +770,10 @@ struct objfile
> > reader. */
> > std::forward_list<quick_symbol_functions_up> qf;
>
> I think this should be updated to use qf_list.
Done.
> > --- a/gdb/symfile-debug.c
> > +++ b/gdb/symfile-debug.c
> > @@ -85,7 +85,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 ())
>
> I read the commit message a couple of times, but I'm still not 100% sure
> I understand what's happening here. Is the change from qf -> qf_safe()
> because calling (in this case has_symbols()) might download the full
> debug info, and in doing do, might delete a previously downloaded
> .debug_info section?
quick_functions_up made from a separately downloaded .gdb_index is at
first associated with the parent objfile (i.e. the objfile of the shared
library containing a .gnu_debuglink that refers to the separate debug info
which actually contains this .gdb_index section).
When the separate debuginfo is downloaded (possibly during iteration
over the parent's qf_list), we need to remove the gdb_index qf from
the parent's qf_list. This prevents gdb from later attempting to read
the parent objfile for debug info that is actually in the corresponding
separate debug objfile.
> If this is the case then is it true that we will almost never want to
> iterate over `qf` directly as most quick_symbol_functions methods might
> result in this download/delete behaviour?
>
> If I'm right, then I think it might be useful to place a warning in the
> comment before `qf` explaining this.
Done.
> > +/* 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. */
>
> Maybe this is a standard thing, and I'm just showing my lack of
> experience, but, given the existence of methods like rbegin() and
> rend(), I think the comments here should be doing more to explain what's
> expected of this input.
>
> Plus this comment seems confusing, though I can sort of see what it's
> trying to say.
>
> We're creating a *_iterator object, which itself contains other
> iterators. We need to be super clear which particular "iterator" we're
> talking about in the comments.
>
> Also, I think we need to be explicit that for this reverse iterator, the
> first argument to the constructor is going to be the LAST element that
> is iterated over. One might be tempted to think that a safe reverse iterator
> should be initialised with rbegin() and rend(), which isn't going to be
> correct (unless I'm not understand, please correct me if so).
That's right, the arguments to basic_safe_reverse_iterator should be
forward iterators. This will not work with reverse iterators.
I added more information to this comment to hopefully clear up any
confusion.
> > +
> > +/* A range adapter that wraps a forward range, and then returns
> > + safe reverse iterators wrapping the original range's iterators. */
>
> I think the fact that this takes a FORWARD range is really important,
> and we should do more to advertise this. E.g. I think there should be a
> comment on the constructor saying that RANGE is a forward range over
> which we will iterate backwards.
>
Done. The updated patch is included below.
Aaron
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 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 85a10be3055..10ee5f9b749 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..d53dfaca90b 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -470,6 +470,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. */
@@ -793,14 +799,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 621342d22b7..b917886988d 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. */
@@ -761,8 +767,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 0fc1fdd57ab..c4cc2e2e8af 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -141,19 +141,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));
}
}
@@ -182,6 +182,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 163cbf6a0f8..f23a57dc842 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 32c3ed9d349..c684f9cdd43 100644
--- a/gdb/symfile-debug.c
+++ b/gdb/symfile-debug.c
@@ -85,7 +85,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)
@@ -108,7 +108,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))
{
@@ -133,7 +133,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)
@@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
}
}
- for (const auto &iter : qf)
+ for (const auto &iter : qf_safe ())
iter->forget_cached_source_info (this);
}
@@ -213,7 +213,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,
@@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
return true;
};
- for (const auto &iter : qf)
+ for (const auto &iter : qf_safe ())
{
if (!iter->expand_symtabs_matching (this,
nullptr,
@@ -309,7 +309,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);
}
@@ -320,7 +320,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);
}
@@ -335,7 +335,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,
@@ -354,7 +354,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);
}
@@ -372,7 +372,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,
@@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
host_address_to_string (&expansion_notify),
search_domain_name (kind));
- 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, kind))
@@ -431,7 +431,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);
@@ -459,7 +459,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);
}
@@ -471,7 +471,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);
}
@@ -485,7 +485,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)
@@ -510,7 +510,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 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 b4891328f1a..d1b9ce21ffc 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 */
Ping
Thanks,
Aaron
On Fri, Jan 19, 2024 at 12:09 AM Aaron Merey <amerey@redhat.com> wrote:
>
> Hi Andrew,
>
> Thanks for the review.
>
> On Thu, Jan 18, 2024 at 7:03 AM Andrew Burgess <aburgess@redhat.com> wrote:
> >
> > Aaron Merey <amerey@redhat.com> writes:
> > > +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. */
> > >
> > > @@ -764,6 +770,10 @@ struct objfile
> > > reader. */
> > > std::forward_list<quick_symbol_functions_up> qf;
> >
> > I think this should be updated to use qf_list.
>
> Done.
>
> > > --- a/gdb/symfile-debug.c
> > > +++ b/gdb/symfile-debug.c
> > > @@ -85,7 +85,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 ())
> >
> > I read the commit message a couple of times, but I'm still not 100% sure
> > I understand what's happening here. Is the change from qf -> qf_safe()
> > because calling (in this case has_symbols()) might download the full
> > debug info, and in doing do, might delete a previously downloaded
> > .debug_info section?
>
> quick_functions_up made from a separately downloaded .gdb_index is at
> first associated with the parent objfile (i.e. the objfile of the shared
> library containing a .gnu_debuglink that refers to the separate debug info
> which actually contains this .gdb_index section).
>
> When the separate debuginfo is downloaded (possibly during iteration
> over the parent's qf_list), we need to remove the gdb_index qf from
> the parent's qf_list. This prevents gdb from later attempting to read
> the parent objfile for debug info that is actually in the corresponding
> separate debug objfile.
>
> > If this is the case then is it true that we will almost never want to
> > iterate over `qf` directly as most quick_symbol_functions methods might
> > result in this download/delete behaviour?
> >
> > If I'm right, then I think it might be useful to place a warning in the
> > comment before `qf` explaining this.
>
> Done.
>
> > > +/* 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. */
> >
> > Maybe this is a standard thing, and I'm just showing my lack of
> > experience, but, given the existence of methods like rbegin() and
> > rend(), I think the comments here should be doing more to explain what's
> > expected of this input.
> >
> > Plus this comment seems confusing, though I can sort of see what it's
> > trying to say.
> >
> > We're creating a *_iterator object, which itself contains other
> > iterators. We need to be super clear which particular "iterator" we're
> > talking about in the comments.
> >
> > Also, I think we need to be explicit that for this reverse iterator, the
> > first argument to the constructor is going to be the LAST element that
> > is iterated over. One might be tempted to think that a safe reverse iterator
> > should be initialised with rbegin() and rend(), which isn't going to be
> > correct (unless I'm not understand, please correct me if so).
>
> That's right, the arguments to basic_safe_reverse_iterator should be
> forward iterators. This will not work with reverse iterators.
>
> I added more information to this comment to hopefully clear up any
> confusion.
>
> > > +
> > > +/* A range adapter that wraps a forward range, and then returns
> > > + safe reverse iterators wrapping the original range's iterators. */
> >
> > I think the fact that this takes a FORWARD range is really important,
> > and we should do more to advertise this. E.g. I think there should be a
> > comment on the constructor saying that RANGE is a forward range over
> > which we will iterate backwards.
> >
>
> Done. The updated patch is included below.
>
> Aaron
>
> 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 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 85a10be3055..10ee5f9b749 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..d53dfaca90b 100644
> --- a/gdb/objfiles.c
> +++ b/gdb/objfiles.c
> @@ -470,6 +470,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. */
>
> @@ -793,14 +799,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 621342d22b7..b917886988d 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. */
>
> @@ -761,8 +767,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 0fc1fdd57ab..c4cc2e2e8af 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -141,19 +141,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));
> }
> }
>
> @@ -182,6 +182,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 163cbf6a0f8..f23a57dc842 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 32c3ed9d349..c684f9cdd43 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -85,7 +85,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)
> @@ -108,7 +108,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))
> {
> @@ -133,7 +133,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)
> @@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
> }
> }
>
> - for (const auto &iter : qf)
> + for (const auto &iter : qf_safe ())
> iter->forget_cached_source_info (this);
> }
>
> @@ -213,7 +213,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,
> @@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
> return true;
> };
>
> - for (const auto &iter : qf)
> + for (const auto &iter : qf_safe ())
> {
> if (!iter->expand_symtabs_matching (this,
> nullptr,
> @@ -309,7 +309,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);
> }
>
> @@ -320,7 +320,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);
> }
>
> @@ -335,7 +335,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,
> @@ -354,7 +354,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);
> }
>
> @@ -372,7 +372,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,
> @@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
> host_address_to_string (&expansion_notify),
> search_domain_name (kind));
>
> - 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, kind))
> @@ -431,7 +431,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);
> @@ -459,7 +459,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);
> }
>
> @@ -471,7 +471,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);
> }
>
> @@ -485,7 +485,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)
> @@ -510,7 +510,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 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 b4891328f1a..d1b9ce21ffc 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
>
Ping. Ready to be pushed to master?
Aaron
On Wed, Feb 7, 2024 at 4:24 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping
>
> Thanks,
> Aaron
>
> On Fri, Jan 19, 2024 at 12:09 AM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Hi Andrew,
> >
> > Thanks for the review.
> >
> > On Thu, Jan 18, 2024 at 7:03 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > >
> > > Aaron Merey <amerey@redhat.com> writes:
> > > > +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. */
> > > >
> > > > @@ -764,6 +770,10 @@ struct objfile
> > > > reader. */
> > > > std::forward_list<quick_symbol_functions_up> qf;
> > >
> > > I think this should be updated to use qf_list.
> >
> > Done.
> >
> > > > --- a/gdb/symfile-debug.c
> > > > +++ b/gdb/symfile-debug.c
> > > > @@ -85,7 +85,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 ())
> > >
> > > I read the commit message a couple of times, but I'm still not 100% sure
> > > I understand what's happening here. Is the change from qf -> qf_safe()
> > > because calling (in this case has_symbols()) might download the full
> > > debug info, and in doing do, might delete a previously downloaded
> > > .debug_info section?
> >
> > quick_functions_up made from a separately downloaded .gdb_index is at
> > first associated with the parent objfile (i.e. the objfile of the shared
> > library containing a .gnu_debuglink that refers to the separate debug info
> > which actually contains this .gdb_index section).
> >
> > When the separate debuginfo is downloaded (possibly during iteration
> > over the parent's qf_list), we need to remove the gdb_index qf from
> > the parent's qf_list. This prevents gdb from later attempting to read
> > the parent objfile for debug info that is actually in the corresponding
> > separate debug objfile.
> >
> > > If this is the case then is it true that we will almost never want to
> > > iterate over `qf` directly as most quick_symbol_functions methods might
> > > result in this download/delete behaviour?
> > >
> > > If I'm right, then I think it might be useful to place a warning in the
> > > comment before `qf` explaining this.
> >
> > Done.
> >
> > > > +/* 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. */
> > >
> > > Maybe this is a standard thing, and I'm just showing my lack of
> > > experience, but, given the existence of methods like rbegin() and
> > > rend(), I think the comments here should be doing more to explain what's
> > > expected of this input.
> > >
> > > Plus this comment seems confusing, though I can sort of see what it's
> > > trying to say.
> > >
> > > We're creating a *_iterator object, which itself contains other
> > > iterators. We need to be super clear which particular "iterator" we're
> > > talking about in the comments.
> > >
> > > Also, I think we need to be explicit that for this reverse iterator, the
> > > first argument to the constructor is going to be the LAST element that
> > > is iterated over. One might be tempted to think that a safe reverse iterator
> > > should be initialised with rbegin() and rend(), which isn't going to be
> > > correct (unless I'm not understand, please correct me if so).
> >
> > That's right, the arguments to basic_safe_reverse_iterator should be
> > forward iterators. This will not work with reverse iterators.
> >
> > I added more information to this comment to hopefully clear up any
> > confusion.
> >
> > > > +
> > > > +/* A range adapter that wraps a forward range, and then returns
> > > > + safe reverse iterators wrapping the original range's iterators. */
> > >
> > > I think the fact that this takes a FORWARD range is really important,
> > > and we should do more to advertise this. E.g. I think there should be a
> > > comment on the constructor saying that RANGE is a forward range over
> > > which we will iterate backwards.
> > >
> >
> > Done. The updated patch is included below.
> >
> > Aaron
> >
> > 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 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 85a10be3055..10ee5f9b749 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..d53dfaca90b 100644
> > --- a/gdb/objfiles.c
> > +++ b/gdb/objfiles.c
> > @@ -470,6 +470,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. */
> >
> > @@ -793,14 +799,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 621342d22b7..b917886988d 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. */
> >
> > @@ -761,8 +767,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 0fc1fdd57ab..c4cc2e2e8af 100644
> > --- a/gdb/progspace.c
> > +++ b/gdb/progspace.c
> > @@ -141,19 +141,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));
> > }
> > }
> >
> > @@ -182,6 +182,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 163cbf6a0f8..f23a57dc842 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 32c3ed9d349..c684f9cdd43 100644
> > --- a/gdb/symfile-debug.c
> > +++ b/gdb/symfile-debug.c
> > @@ -85,7 +85,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)
> > @@ -108,7 +108,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))
> > {
> > @@ -133,7 +133,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)
> > @@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
> > }
> > }
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > iter->forget_cached_source_info (this);
> > }
> >
> > @@ -213,7 +213,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,
> > @@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
> > return true;
> > };
> >
> > - for (const auto &iter : qf)
> > + for (const auto &iter : qf_safe ())
> > {
> > if (!iter->expand_symtabs_matching (this,
> > nullptr,
> > @@ -309,7 +309,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);
> > }
> >
> > @@ -320,7 +320,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);
> > }
> >
> > @@ -335,7 +335,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,
> > @@ -354,7 +354,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);
> > }
> >
> > @@ -372,7 +372,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,
> > @@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
> > host_address_to_string (&expansion_notify),
> > search_domain_name (kind));
> >
> > - 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, kind))
> > @@ -431,7 +431,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);
> > @@ -459,7 +459,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);
> > }
> >
> > @@ -471,7 +471,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);
> > }
> >
> > @@ -485,7 +485,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)
> > @@ -510,7 +510,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 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 b4891328f1a..d1b9ce21ffc 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
> >
Ping. Ready to be pushed to master?
Thanks,
Aaron
On Thu, Feb 22, 2024 at 5:22 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping. Ready to be pushed to master?
>
> Aaron
>
> On Wed, Feb 7, 2024 at 4:24 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Fri, Jan 19, 2024 at 12:09 AM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > Hi Andrew,
> > >
> > > Thanks for the review.
> > >
> > > On Thu, Jan 18, 2024 at 7:03 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > > >
> > > > Aaron Merey <amerey@redhat.com> writes:
> > > > > +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. */
> > > > >
> > > > > @@ -764,6 +770,10 @@ struct objfile
> > > > > reader. */
> > > > > std::forward_list<quick_symbol_functions_up> qf;
> > > >
> > > > I think this should be updated to use qf_list.
> > >
> > > Done.
> > >
> > > > > --- a/gdb/symfile-debug.c
> > > > > +++ b/gdb/symfile-debug.c
> > > > > @@ -85,7 +85,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 ())
> > > >
> > > > I read the commit message a couple of times, but I'm still not 100% sure
> > > > I understand what's happening here. Is the change from qf -> qf_safe()
> > > > because calling (in this case has_symbols()) might download the full
> > > > debug info, and in doing do, might delete a previously downloaded
> > > > .debug_info section?
> > >
> > > quick_functions_up made from a separately downloaded .gdb_index is at
> > > first associated with the parent objfile (i.e. the objfile of the shared
> > > library containing a .gnu_debuglink that refers to the separate debug info
> > > which actually contains this .gdb_index section).
> > >
> > > When the separate debuginfo is downloaded (possibly during iteration
> > > over the parent's qf_list), we need to remove the gdb_index qf from
> > > the parent's qf_list. This prevents gdb from later attempting to read
> > > the parent objfile for debug info that is actually in the corresponding
> > > separate debug objfile.
> > >
> > > > If this is the case then is it true that we will almost never want to
> > > > iterate over `qf` directly as most quick_symbol_functions methods might
> > > > result in this download/delete behaviour?
> > > >
> > > > If I'm right, then I think it might be useful to place a warning in the
> > > > comment before `qf` explaining this.
> > >
> > > Done.
> > >
> > > > > +/* 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. */
> > > >
> > > > Maybe this is a standard thing, and I'm just showing my lack of
> > > > experience, but, given the existence of methods like rbegin() and
> > > > rend(), I think the comments here should be doing more to explain what's
> > > > expected of this input.
> > > >
> > > > Plus this comment seems confusing, though I can sort of see what it's
> > > > trying to say.
> > > >
> > > > We're creating a *_iterator object, which itself contains other
> > > > iterators. We need to be super clear which particular "iterator" we're
> > > > talking about in the comments.
> > > >
> > > > Also, I think we need to be explicit that for this reverse iterator, the
> > > > first argument to the constructor is going to be the LAST element that
> > > > is iterated over. One might be tempted to think that a safe reverse iterator
> > > > should be initialised with rbegin() and rend(), which isn't going to be
> > > > correct (unless I'm not understand, please correct me if so).
> > >
> > > That's right, the arguments to basic_safe_reverse_iterator should be
> > > forward iterators. This will not work with reverse iterators.
> > >
> > > I added more information to this comment to hopefully clear up any
> > > confusion.
> > >
> > > > > +
> > > > > +/* A range adapter that wraps a forward range, and then returns
> > > > > + safe reverse iterators wrapping the original range's iterators. */
> > > >
> > > > I think the fact that this takes a FORWARD range is really important,
> > > > and we should do more to advertise this. E.g. I think there should be a
> > > > comment on the constructor saying that RANGE is a forward range over
> > > > which we will iterate backwards.
> > > >
> > >
> > > Done. The updated patch is included below.
> > >
> > > Aaron
> > >
> > > 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 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 85a10be3055..10ee5f9b749 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..d53dfaca90b 100644
> > > --- a/gdb/objfiles.c
> > > +++ b/gdb/objfiles.c
> > > @@ -470,6 +470,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. */
> > >
> > > @@ -793,14 +799,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 621342d22b7..b917886988d 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. */
> > >
> > > @@ -761,8 +767,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 0fc1fdd57ab..c4cc2e2e8af 100644
> > > --- a/gdb/progspace.c
> > > +++ b/gdb/progspace.c
> > > @@ -141,19 +141,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));
> > > }
> > > }
> > >
> > > @@ -182,6 +182,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 163cbf6a0f8..f23a57dc842 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 32c3ed9d349..c684f9cdd43 100644
> > > --- a/gdb/symfile-debug.c
> > > +++ b/gdb/symfile-debug.c
> > > @@ -85,7 +85,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)
> > > @@ -108,7 +108,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))
> > > {
> > > @@ -133,7 +133,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)
> > > @@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
> > > }
> > > }
> > >
> > > - for (const auto &iter : qf)
> > > + for (const auto &iter : qf_safe ())
> > > iter->forget_cached_source_info (this);
> > > }
> > >
> > > @@ -213,7 +213,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,
> > > @@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
> > > return true;
> > > };
> > >
> > > - for (const auto &iter : qf)
> > > + for (const auto &iter : qf_safe ())
> > > {
> > > if (!iter->expand_symtabs_matching (this,
> > > nullptr,
> > > @@ -309,7 +309,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);
> > > }
> > >
> > > @@ -320,7 +320,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);
> > > }
> > >
> > > @@ -335,7 +335,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,
> > > @@ -354,7 +354,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);
> > > }
> > >
> > > @@ -372,7 +372,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,
> > > @@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
> > > host_address_to_string (&expansion_notify),
> > > search_domain_name (kind));
> > >
> > > - 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, kind))
> > > @@ -431,7 +431,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);
> > > @@ -459,7 +459,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);
> > > }
> > >
> > > @@ -471,7 +471,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);
> > > }
> > >
> > > @@ -485,7 +485,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)
> > > @@ -510,7 +510,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 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 b4891328f1a..d1b9ce21ffc 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
> > >
Ping. Ready to be pushed to master?
Aaron
On Thu, Feb 22, 2024 at 5:22 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Ping. Ready to be pushed to master?
>
> Aaron
>
> On Wed, Feb 7, 2024 at 4:24 PM Aaron Merey <amerey@redhat.com> wrote:
> >
> > Ping
> >
> > Thanks,
> > Aaron
> >
> > On Fri, Jan 19, 2024 at 12:09 AM Aaron Merey <amerey@redhat.com> wrote:
> > >
> > > Hi Andrew,
> > >
> > > Thanks for the review.
> > >
> > > On Thu, Jan 18, 2024 at 7:03 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > > >
> > > > Aaron Merey <amerey@redhat.com> writes:
> > > > > +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. */
> > > > >
> > > > > @@ -764,6 +770,10 @@ struct objfile
> > > > > reader. */
> > > > > std::forward_list<quick_symbol_functions_up> qf;
> > > >
> > > > I think this should be updated to use qf_list.
> > >
> > > Done.
> > >
> > > > > --- a/gdb/symfile-debug.c
> > > > > +++ b/gdb/symfile-debug.c
> > > > > @@ -85,7 +85,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 ())
> > > >
> > > > I read the commit message a couple of times, but I'm still not 100% sure
> > > > I understand what's happening here. Is the change from qf -> qf_safe()
> > > > because calling (in this case has_symbols()) might download the full
> > > > debug info, and in doing do, might delete a previously downloaded
> > > > .debug_info section?
> > >
> > > quick_functions_up made from a separately downloaded .gdb_index is at
> > > first associated with the parent objfile (i.e. the objfile of the shared
> > > library containing a .gnu_debuglink that refers to the separate debug info
> > > which actually contains this .gdb_index section).
> > >
> > > When the separate debuginfo is downloaded (possibly during iteration
> > > over the parent's qf_list), we need to remove the gdb_index qf from
> > > the parent's qf_list. This prevents gdb from later attempting to read
> > > the parent objfile for debug info that is actually in the corresponding
> > > separate debug objfile.
> > >
> > > > If this is the case then is it true that we will almost never want to
> > > > iterate over `qf` directly as most quick_symbol_functions methods might
> > > > result in this download/delete behaviour?
> > > >
> > > > If I'm right, then I think it might be useful to place a warning in the
> > > > comment before `qf` explaining this.
> > >
> > > Done.
> > >
> > > > > +/* 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. */
> > > >
> > > > Maybe this is a standard thing, and I'm just showing my lack of
> > > > experience, but, given the existence of methods like rbegin() and
> > > > rend(), I think the comments here should be doing more to explain what's
> > > > expected of this input.
> > > >
> > > > Plus this comment seems confusing, though I can sort of see what it's
> > > > trying to say.
> > > >
> > > > We're creating a *_iterator object, which itself contains other
> > > > iterators. We need to be super clear which particular "iterator" we're
> > > > talking about in the comments.
> > > >
> > > > Also, I think we need to be explicit that for this reverse iterator, the
> > > > first argument to the constructor is going to be the LAST element that
> > > > is iterated over. One might be tempted to think that a safe reverse iterator
> > > > should be initialised with rbegin() and rend(), which isn't going to be
> > > > correct (unless I'm not understand, please correct me if so).
> > >
> > > That's right, the arguments to basic_safe_reverse_iterator should be
> > > forward iterators. This will not work with reverse iterators.
> > >
> > > I added more information to this comment to hopefully clear up any
> > > confusion.
> > >
> > > > > +
> > > > > +/* A range adapter that wraps a forward range, and then returns
> > > > > + safe reverse iterators wrapping the original range's iterators. */
> > > >
> > > > I think the fact that this takes a FORWARD range is really important,
> > > > and we should do more to advertise this. E.g. I think there should be a
> > > > comment on the constructor saying that RANGE is a forward range over
> > > > which we will iterate backwards.
> > > >
> > >
> > > Done. The updated patch is included below.
> > >
> > > Aaron
> > >
> > > 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 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 85a10be3055..10ee5f9b749 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..d53dfaca90b 100644
> > > --- a/gdb/objfiles.c
> > > +++ b/gdb/objfiles.c
> > > @@ -470,6 +470,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. */
> > >
> > > @@ -793,14 +799,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 621342d22b7..b917886988d 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. */
> > >
> > > @@ -761,8 +767,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 0fc1fdd57ab..c4cc2e2e8af 100644
> > > --- a/gdb/progspace.c
> > > +++ b/gdb/progspace.c
> > > @@ -141,19 +141,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));
> > > }
> > > }
> > >
> > > @@ -182,6 +182,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 163cbf6a0f8..f23a57dc842 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 32c3ed9d349..c684f9cdd43 100644
> > > --- a/gdb/symfile-debug.c
> > > +++ b/gdb/symfile-debug.c
> > > @@ -85,7 +85,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)
> > > @@ -108,7 +108,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))
> > > {
> > > @@ -133,7 +133,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)
> > > @@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
> > > }
> > > }
> > >
> > > - for (const auto &iter : qf)
> > > + for (const auto &iter : qf_safe ())
> > > iter->forget_cached_source_info (this);
> > > }
> > >
> > > @@ -213,7 +213,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,
> > > @@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
> > > return true;
> > > };
> > >
> > > - for (const auto &iter : qf)
> > > + for (const auto &iter : qf_safe ())
> > > {
> > > if (!iter->expand_symtabs_matching (this,
> > > nullptr,
> > > @@ -309,7 +309,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);
> > > }
> > >
> > > @@ -320,7 +320,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);
> > > }
> > >
> > > @@ -335,7 +335,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,
> > > @@ -354,7 +354,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);
> > > }
> > >
> > > @@ -372,7 +372,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,
> > > @@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
> > > host_address_to_string (&expansion_notify),
> > > search_domain_name (kind));
> > >
> > > - 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, kind))
> > > @@ -431,7 +431,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);
> > > @@ -459,7 +459,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);
> > > }
> > >
> > > @@ -471,7 +471,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);
> > > }
> > >
> > > @@ -485,7 +485,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)
> > > @@ -510,7 +510,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 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 b4891328f1a..d1b9ce21ffc 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
> > >
@@ -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
@@ -470,6 +470,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. */
@@ -793,14 +799,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);
+ });
}
@@ -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. */
@@ -764,6 +770,10 @@ struct objfile
reader. */
std::forward_list<quick_symbol_functions_up> qf;
+ /* Returns iterable object that allows for safe deletion during
+ iteration. */
+ qf_safe_range qf_safe ();
+
/* Per objfile data-pointers required by other GDB modules. */
registry<objfile> registry_fields;
@@ -141,19 +141,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));
}
}
@@ -182,6 +182,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)
{
@@ -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);
@@ -85,7 +85,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)
@@ -108,7 +108,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))
{
@@ -133,7 +133,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)
@@ -166,7 +166,7 @@ objfile::forget_cached_source_info ()
}
}
- for (const auto &iter : qf)
+ for (const auto &iter : qf_safe ())
iter->forget_cached_source_info (this);
}
@@ -213,7 +213,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,
@@ -278,7 +278,7 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
return true;
};
- for (const auto &iter : qf)
+ for (const auto &iter : qf_safe ())
{
if (!iter->expand_symtabs_matching (this,
nullptr,
@@ -309,7 +309,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);
}
@@ -320,7 +320,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);
}
@@ -335,7 +335,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,
@@ -354,7 +354,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);
}
@@ -372,7 +372,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,
@@ -406,7 +406,7 @@ objfile::expand_symtabs_matching
host_address_to_string (&expansion_notify),
search_domain_name (kind));
- 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, kind))
@@ -431,7 +431,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);
@@ -459,7 +459,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);
}
@@ -471,7 +471,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);
}
@@ -485,7 +485,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)
@@ -510,7 +510,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);
@@ -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" \
@@ -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 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;
+
+ 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 */