From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 494173858CDB for ; Thu, 1 Jun 2023 01:44:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 494173858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1685583853; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=biSw1HBoSxNV9fqZoQ/BjiZ3HsnP3HCJ8/JBZPhAK7M=; b=gC1LMvaTWFWfvzfK8JfauQHcZg2iHU84ZtXHv7DCHlm7+NvLjKP05SQF1V5QWrH9PoyDnX qv7ogZhK3RkiI4SrHnjxR+KUX0x0fAAvgbAgq4GwqPY0d+yIbXj7Lhip/hqwxExHLOsRKF Z6EvOzzrytBEOyPh5faS1k57bFstPlc= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-651-PEqwB50qPCeD7ZCYPZjmyw-1; Wed, 31 May 2023 21:44:12 -0400 X-MC-Unique: PEqwB50qPCeD7ZCYPZjmyw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AC523185A78B for ; Thu, 1 Jun 2023 01:44:12 +0000 (UTC) Received: from localhost.localdomain (unknown [10.22.32.3]) by smtp.corp.redhat.com (Postfix) with ESMTP id 488B3492B00; Thu, 1 Jun 2023 01:44:12 +0000 (UTC) From: Aaron Merey To: gdb-patches@sourceware.org Cc: aburgess@redhat.com, Aaron Merey Subject: [PATCH 4/6] gdb/progspace: Add reverse safe iterator and template for unwrapping iterator Date: Wed, 31 May 2023 21:43:45 -0400 Message-Id: <20230601014347.3367489-5-amerey@redhat.com> In-Reply-To: <20230601014347.3367489-1-amerey@redhat.com> References: <20230601014347.3367489-1-amerey@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII"; x-default=true X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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; + using unwrapping_qf_range = iterator_range>; + using qf_safe_range = basic_safe_range; + /* Ensure that partial symbols have been read and return the "quick" (aka partial) symbol functions for this symbol reader. */ - const std::forward_list & + qf_safe_range qf_require_partial_symbols () { this->require_partial_symbols (true); - return qf; + return qf_safe_range + (unwrapping_qf_range + (unwrapping_iterator (qf.begin ()), + unwrapping_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, - 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 +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> objfile_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. */ +/* 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; +/* A range that returns unwrapping_iterators. */ + +using unwrapping_objfile_range + = iterator_range>; /* 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 (objfiles_list.begin ()), + unwrapping_iterator (objfiles_list.end ())); } - using objfiles_safe_range = basic_safe_range; + using objfiles_reverse_range = iterator_range; + using objfiles_safe_reverse_range = basic_safe_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, - 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 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" "= {} $hex
" \ 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