[PATCH 4/6] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator
Aaron Merey
amerey@redhat.com
Thu Jun 15 13:44:32 GMT 2023
Ping
Thanks,
Aaron
On Wed, May 31, 2023 at 9:44 PM Aaron Merey <amerey@redhat.com> wrote:
>
> To facilitate the deletion of objfiles, progspace objects use a safe
> iterator that holds a reference to the next objfile in the progspace's
> objfile_list. This allows objfiles to be deleted in a loop without
> invalidating the loop's iterator. progspace also uses an unwrapping
> iterator over std::unique_ptr<objfile> that automatically deferences
> the unique_ptr.
>
> This patch changes the objfile safe iterator to be a reverse safe
> iterator. It changes the unwrapping iterator into a template. It
> also modifies 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 download a debuginfo
> file during symtab expansion. In this case an objfile could be added
> to an 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.
>
> The unwrapping iterator is changed into a template in order to
> use it with objfile qf_require_partial_symbols, which is now also
> uses with a safe iterator. This is because after a separate debug
> objfile is downloaded on-demand, we want to remove any .gdb_index
> quick_symbol_functions from the parent objfile during iteration over
> the parent's quick_symbol_functions. The newly downloaded separate
> debug objfile contains the index and all of the related symbols
> so the .gdb_index should not be associated with the parent objfile
> any longer.
>
> Finally a safe reverse iterator is now used during progspace objfile
> deletion in order to prevent iterator invalidation during the loop
> in which objfiles are deleted. This could happen during forward
> iteration over objfiles_list when a separate debug objfile immediately
> follows it's parent objfile in the list (which is now possible since
> objfiles are inserted into the list after their parent). Deletion
> of the parent would cause deletion of the separate debug objfile,
> which would invalidate the safe forward iterator's reference to the
> next objfile in the list. A safe reverse iterator deletes separate
> debug objfiles before their parent, so the iterator's reference to
> the next objfile always stays valid.
>
> 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/objfiles.h | 22 +++-
> gdb/progspace.c | 8 +-
> gdb/progspace.h | 159 +++++++++++++++++++-----
> gdb/symfile-debug.c | 136 ++++++++++----------
> gdb/testsuite/gdb.python/py-objfile.exp | 2 +-
> 5 files changed, 218 insertions(+), 109 deletions(-)
>
> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index 189856f0a51..bb7b0a4579d 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -613,6 +613,17 @@ struct objfile
> /* See quick_symbol_functions. */
> void require_partial_symbols (bool verbose);
>
> + /* Remove TARGET from this objfile's collection of quick_symbol_functions. */
> + void remove_partial_symbol (quick_symbol_functions *target)
> + {
> + for (quick_symbol_functions_up &qf_up : qf)
> + if (qf_up.get () == target)
> + {
> + qf.remove (qf_up);
> + return;
> + }
> + }
> +
> /* Return the relocation offset applied to SECTION. */
> CORE_ADDR section_offset (bfd_section *section) const
> {
> @@ -699,13 +710,20 @@ struct objfile
>
> private:
>
> + using qf_list = std::forward_list<quick_symbol_functions_up>;
> + using unwrapping_qf_range = iterator_range<unwrapping_iterator<qf_list::iterator>>;
> + using qf_safe_range = basic_safe_range<unwrapping_qf_range>;
> +
> /* Ensure that partial symbols have been read and return the "quick" (aka
> partial) symbol functions for this symbol reader. */
> - const std::forward_list<quick_symbol_functions_up> &
> + qf_safe_range
> qf_require_partial_symbols ()
> {
> this->require_partial_symbols (true);
> - return qf;
> + return qf_safe_range
> + (unwrapping_qf_range
> + (unwrapping_iterator<qf_list::iterator> (qf.begin ()),
> + unwrapping_iterator<qf_list::iterator> (qf.end ())));
> }
>
> public:
> diff --git a/gdb/progspace.c b/gdb/progspace.c
> index 32bdfebcf7c..1ed75eef2f9 100644
> --- a/gdb/progspace.c
> +++ b/gdb/progspace.c
> @@ -139,19 +139,19 @@ program_space::free_all_objfiles ()
>
> void
> program_space::add_objfile (std::unique_ptr<objfile> &&objfile,
> - struct objfile *before)
> + struct objfile *after)
> {
> - if (before == nullptr)
> + if (after == nullptr)
> objfiles_list.push_back (std::move (objfile));
> else
> {
> auto iter = std::find_if (objfiles_list.begin (), objfiles_list.end (),
> [=] (const std::unique_ptr<::objfile> &objf)
> {
> - return objf.get () == before;
> + return objf.get () == after;
> });
> gdb_assert (iter != objfiles_list.end ());
> - objfiles_list.insert (iter, std::move (objfile));
> + objfiles_list.insert (++iter, std::move (objfile));
> }
> }
>
> diff --git a/gdb/progspace.h b/gdb/progspace.h
> index 85215f0e2f1..6e33e48c88e 100644
> --- a/gdb/progspace.h
> +++ b/gdb/progspace.h
> @@ -40,56 +40,141 @@ struct address_space;
> struct program_space;
> struct so_list;
>
> +/* An iterator that wraps an iterator over std::unique_ptr, and dereferences
> + the returned object. This is useful for iterating over a list of shared
> + pointers and returning raw pointers -- which helped avoid touching a lot
> + of code when changing how objfiles are managed. */
> +
> +template<typename UniquePtrIter>
> +class unwrapping_iterator
> +{
> +public:
> + typedef unwrapping_iterator self_type;
> + typedef typename UniquePtrIter::value_type::pointer value_type;
> + typedef typename UniquePtrIter::reference reference;
> + typedef typename UniquePtrIter::pointer pointer;
> + typedef typename UniquePtrIter::iterator_category iterator_category;
> + typedef typename UniquePtrIter::difference_type difference_type;
> +
> + unwrapping_iterator (UniquePtrIter iter)
> + : m_iter (std::move (iter))
> + {
> + }
> +
> + value_type operator* () const
> + {
> + return m_iter->get ();
> + }
> +
> + unwrapping_iterator operator++ ()
> + {
> + ++m_iter;
> + return *this;
> + }
> +
> + bool operator!= (const unwrapping_iterator &other) const
> + {
> + return m_iter != other.m_iter;
> + }
> +
> +private:
> + /* The underlying iterator. */
> + UniquePtrIter m_iter;
> +};
> +
> typedef std::list<std::unique_ptr<objfile>> objfile_list;
>
> -/* An iterator that wraps an iterator over std::unique_ptr<objfile>,
> - and dereferences the returned object. This is useful for iterating
> - over a list of shared pointers and returning raw pointers -- which
> - helped avoid touching a lot of code when changing how objfiles are
> - managed. */
> +/* An reverse iterator that wraps an iterator over objfile_list, and
> + dereferences the returned object. This is useful for reverse iterating
> + over a list of shared pointers and returning raw pointers -- which helped
> + avoid touching a lot of code when changing how objfiles are managed. */
>
> -class unwrapping_objfile_iterator
> +class unwrapping_reverse_objfile_iterator
> {
> public:
> -
> - typedef unwrapping_objfile_iterator self_type;
> + typedef unwrapping_reverse_objfile_iterator self_type;
> typedef typename ::objfile *value_type;
> typedef typename ::objfile &reference;
> typedef typename ::objfile **pointer;
> typedef typename objfile_list::iterator::iterator_category iterator_category;
> typedef typename objfile_list::iterator::difference_type difference_type;
>
> - unwrapping_objfile_iterator (objfile_list::iterator iter)
> - : m_iter (std::move (iter))
> - {
> - }
> -
> - objfile *operator* () const
> + value_type operator* () const
> {
> return m_iter->get ();
> }
>
> - unwrapping_objfile_iterator operator++ ()
> + unwrapping_reverse_objfile_iterator operator++ ()
> {
> - ++m_iter;
> + if (m_iter != m_begin)
> + --m_iter;
> + else
> + {
> + /* We can't decrement M_ITER since it is the begin iterator of the
> + objfile list. Set M_ITER to the list's end iterator to indicate
> + this is now one-past-the-end. */
> + m_iter = m_end;
> +
> + /* Overwrite M_BEGIN to avoid possibly copying an invalid iterator. */
> + m_begin = m_end;
> + }
> +
> return *this;
> }
>
> - bool operator!= (const unwrapping_objfile_iterator &other) const
> + bool operator!= (const unwrapping_reverse_objfile_iterator &other) const
> {
> return m_iter != other.m_iter;
> }
>
> + /* Return an unwrapping reverse iterator starting at the last element of
> + OBJF_LIST. */
> + static unwrapping_reverse_objfile_iterator begin (objfile_list &objf_list)
> + {
> + auto begin = objf_list.begin ();
> + auto end = objf_list.end ();
> + auto rev_begin = objf_list.end ();
> +
> + /* Start REV_BEGIN on the last objfile in OBJF_LIST. */
> + if (begin != end)
> + --rev_begin;
> +
> + return unwrapping_reverse_objfile_iterator (rev_begin, begin, end);
> + }
> +
> + /* Return a one-past-the-end unwrapping reverse iterator. */
> + static unwrapping_reverse_objfile_iterator end (objfile_list &objf_list)
> + {
> + return unwrapping_reverse_objfile_iterator (objf_list.end (),
> + objf_list.end (),
> + objf_list.end ());
> + }
> +
> private:
> + /* begin and end methods should be used to create these objects. */
> + unwrapping_reverse_objfile_iterator (objfile_list::iterator iter,
> + objfile_list::iterator begin,
> + objfile_list::iterator end)
> + : m_iter (std::move (iter)), m_begin (std::move (begin)),
> + m_end (std::move (end))
> + {
> + }
>
> - /* The underlying iterator. */
> - objfile_list::iterator m_iter;
> -};
> + /* The underlying iterator. */
> + objfile_list::iterator m_iter;
>
> + /* The underlying iterator pointing to the first objfile in the sequence. Used
> + to track when to stop decrementing M_ITER. */
> + objfile_list::iterator m_begin;
>
> -/* A range that returns unwrapping_objfile_iterators. */
> + /* The underlying iterator's one-past-the-end. */
> + objfile_list::iterator m_end;
> +};
>
> -using unwrapping_objfile_range = iterator_range<unwrapping_objfile_iterator>;
> +/* A range that returns unwrapping_iterators. */
> +
> +using unwrapping_objfile_range
> + = iterator_range<unwrapping_iterator<objfile_list::iterator>>;
>
> /* A program space represents a symbolic view of an address space.
> Roughly speaking, it holds all the data associated with a
> @@ -209,11 +294,12 @@ struct program_space
> objfiles_range objfiles ()
> {
> return objfiles_range
> - (unwrapping_objfile_iterator (objfiles_list.begin ()),
> - unwrapping_objfile_iterator (objfiles_list.end ()));
> + (unwrapping_iterator<objfile_list::iterator> (objfiles_list.begin ()),
> + unwrapping_iterator<objfile_list::iterator> (objfiles_list.end ()));
> }
>
> - using objfiles_safe_range = basic_safe_range<objfiles_range>;
> + using objfiles_reverse_range = iterator_range<unwrapping_reverse_objfile_iterator>;
> + using objfiles_safe_reverse_range = basic_safe_range<objfiles_reverse_range>;
>
> /* An iterable object that can be used to iterate over all objfiles.
> The basic use is in a foreach, like:
> @@ -221,20 +307,25 @@ struct program_space
> 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 ()
> + deleted during iteration.
> +
> + The use of a reverse iterator helps ensure that separate debug
> + objfiles are deleted before their parent objfile. This prevents
> + the invalidation of an iterator 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_reverse_range
> + (unwrapping_reverse_objfile_iterator::begin (objfiles_list),
> + unwrapping_reverse_objfile_iterator::end (objfiles_list)));
> }
>
> - /* 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);
> diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
> index 9db5c47a8ce..784b81b5ca6 100644
> --- a/gdb/symfile-debug.c
> +++ b/gdb/symfile-debug.c
> @@ -109,9 +109,9 @@ objfile::has_unexpanded_symtabs ()
> objfile_debug_name (this));
>
> bool result = false;
> - for (const auto &iter : qf_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - if (iter->has_unexpanded_symtabs (this))
> + if (qf->has_unexpanded_symtabs (this))
> {
> result = true;
> break;
> @@ -134,9 +134,9 @@ 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_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - retval = iter->find_last_source_symtab (this);
> + retval = qf->find_last_source_symtab (this);
> if (retval != nullptr)
> break;
> }
> @@ -167,8 +167,8 @@ objfile::forget_cached_source_info ()
> }
> }
>
> - for (const auto &iter : qf_require_partial_symbols ())
> - iter->forget_cached_source_info (this);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->forget_cached_source_info (this);
> }
>
> bool
> @@ -214,17 +214,17 @@ objfile::map_symtabs_matching_filename
> return result;
> };
>
> - for (const auto &iter : qf_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - if (!iter->expand_symtabs_matching (this,
> - match_one_filename,
> - nullptr,
> - nullptr,
> - on_expansion,
> - (SEARCH_GLOBAL_BLOCK
> - | SEARCH_STATIC_BLOCK),
> - UNDEF_DOMAIN,
> - ALL_DOMAIN))
> + if (!qf->expand_symtabs_matching (this,
> + match_one_filename,
> + nullptr,
> + nullptr,
> + on_expansion,
> + (SEARCH_GLOBAL_BLOCK
> + | SEARCH_STATIC_BLOCK),
> + UNDEF_DOMAIN,
> + ALL_DOMAIN))
> {
> retval = false;
> break;
> @@ -283,18 +283,18 @@ objfile::lookup_symbol (block_enum kind, const char *name, domain_enum domain)
> return true;
> };
>
> - for (const auto &iter : qf_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - if (!iter->expand_symtabs_matching (this,
> - nullptr,
> - &lookup_name,
> - nullptr,
> - search_one_symtab,
> - kind == GLOBAL_BLOCK
> - ? SEARCH_GLOBAL_BLOCK
> - : SEARCH_STATIC_BLOCK,
> - domain,
> - ALL_DOMAIN))
> + if (!qf->expand_symtabs_matching (this,
> + nullptr,
> + &lookup_name,
> + nullptr,
> + search_one_symtab,
> + kind == GLOBAL_BLOCK
> + ? SEARCH_GLOBAL_BLOCK
> + : SEARCH_STATIC_BLOCK,
> + domain,
> + ALL_DOMAIN))
> break;
> }
>
> @@ -314,8 +314,8 @@ 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_require_partial_symbols ())
> - iter->print_stats (this, print_bcache);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->print_stats (this, print_bcache);
> }
>
> void
> @@ -340,16 +340,16 @@ 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_require_partial_symbols ())
> - iter->expand_symtabs_matching (this,
> - nullptr,
> - &lookup_name,
> - nullptr,
> - nullptr,
> - (SEARCH_GLOBAL_BLOCK
> - | SEARCH_STATIC_BLOCK),
> - VAR_DOMAIN,
> - ALL_DOMAIN);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->expand_symtabs_matching (this,
> + nullptr,
> + &lookup_name,
> + nullptr,
> + nullptr,
> + (SEARCH_GLOBAL_BLOCK
> + | SEARCH_STATIC_BLOCK),
> + VAR_DOMAIN,
> + ALL_DOMAIN);
> }
>
> void
> @@ -359,8 +359,8 @@ objfile::expand_all_symtabs ()
> gdb_printf (gdb_stdlog, "qf->expand_all_symtabs (%s)\n",
> objfile_debug_name (this));
>
> - for (const auto &iter : qf_require_partial_symbols ())
> - iter->expand_all_symtabs (this);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->expand_all_symtabs (this);
> }
>
> void
> @@ -377,16 +377,16 @@ objfile::expand_symtabs_with_fullname (const char *fullname)
> return filename_cmp (basenames ? basename : fullname, filename) == 0;
> };
>
> - for (const auto &iter : qf_require_partial_symbols ())
> - iter->expand_symtabs_matching (this,
> - file_matcher,
> - nullptr,
> - nullptr,
> - nullptr,
> - (SEARCH_GLOBAL_BLOCK
> - | SEARCH_STATIC_BLOCK),
> - UNDEF_DOMAIN,
> - ALL_DOMAIN);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->expand_symtabs_matching (this,
> + file_matcher,
> + nullptr,
> + nullptr,
> + nullptr,
> + (SEARCH_GLOBAL_BLOCK
> + | SEARCH_STATIC_BLOCK),
> + UNDEF_DOMAIN,
> + ALL_DOMAIN);
> }
>
> void
> @@ -402,9 +402,9 @@ objfile::expand_matching_symbols
> domain_name (domain), global,
> host_address_to_string (ordered_compare));
>
> - for (const auto &iter : qf_require_partial_symbols ())
> - iter->expand_matching_symbols (this, name, domain, global,
> - ordered_compare);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->expand_matching_symbols (this, name, domain, global,
> + ordered_compare);
> }
>
> bool
> @@ -429,10 +429,10 @@ objfile::expand_symtabs_matching
> host_address_to_string (&expansion_notify),
> search_domain_name (kind));
>
> - for (const auto &iter : qf_require_partial_symbols ())
> - if (!iter->expand_symtabs_matching (this, file_matcher, lookup_name,
> - symbol_matcher, expansion_notify,
> - search_flags, domain, kind))
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + if (!qf->expand_symtabs_matching (this, file_matcher, lookup_name,
> + symbol_matcher, expansion_notify,
> + search_flags, domain, kind))
> return false;
> return true;
> }
> @@ -454,10 +454,10 @@ objfile::find_pc_sect_compunit_symtab (struct bound_minimal_symbol msymbol,
> host_address_to_string (section),
> warn_if_readin);
>
> - for (const auto &iter : qf_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - retval = iter->find_pc_sect_compunit_symtab (this, msymbol, pc, section,
> - warn_if_readin);
> + retval = qf->find_pc_sect_compunit_symtab (this, msymbol, pc, section,
> + warn_if_readin);
> if (retval != nullptr)
> break;
> }
> @@ -482,8 +482,8 @@ objfile::map_symbol_filenames (gdb::function_view<symbol_filename_ftype> fun,
> objfile_debug_name (this),
> need_fullname);
>
> - for (const auto &iter : qf_require_partial_symbols ())
> - iter->map_symbol_filenames (this, fun, need_fullname);
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> + qf->map_symbol_filenames (this, fun, need_fullname);
> }
>
> struct compunit_symtab *
> @@ -496,9 +496,9 @@ objfile::find_compunit_symtab_by_address (CORE_ADDR address)
> hex_string (address));
>
> struct compunit_symtab *result = NULL;
> - for (const auto &iter : qf_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - result = iter->find_compunit_symtab_by_address (this, address);
> + result = qf->find_compunit_symtab_by_address (this, address);
> if (result != nullptr)
> break;
> }
> @@ -521,10 +521,10 @@ objfile::lookup_global_symbol_language (const char *name,
> enum language result = language_unknown;
> *symbol_found_p = false;
>
> - for (const auto &iter : qf_require_partial_symbols ())
> + for (quick_symbol_functions *qf : qf_require_partial_symbols ())
> {
> - result = iter->lookup_global_symbol_language (this, name, domain,
> - symbol_found_p);
> + result = qf->lookup_global_symbol_language (this, name, domain,
> + symbol_found_p);
> if (*symbol_found_p)
> break;
> }
> 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" \
> --
> 2.40.1
>
More information about the Gdb-patches
mailing list