From patchwork Mon Apr 8 21:11:41 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom Tromey X-Patchwork-Id: 88207 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0E2FE385E448 for ; Mon, 8 Apr 2024 21:13:26 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from omta038.useast.a.cloudfilter.net (omta038.useast.a.cloudfilter.net [44.202.169.37]) by sourceware.org (Postfix) with ESMTPS id 37A06385840D for ; Mon, 8 Apr 2024 21:11:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 37A06385840D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 37A06385840D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=44.202.169.37 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712610709; cv=none; b=m/fGm1ZuvIlZ/toqAAwSi2iAKay4DfKuHAxmhOxYCwGP7ZHcp6u20mCjkaQo9nqDeT3capdTcos8UwRxJh3djXaUA1C5frQgoeKo3MB9HFFYuPwh78qUic7vgLmX43Cox4WU3RCdE7Ov0x6SypGKhx6a0mSsM3cYrfDfICTQ0Bs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712610709; c=relaxed/simple; bh=2ELopapbz9pTAaFJ97ajhMFZJCZd1PwX3QnoQALksNs=; h=DKIM-Signature:From:Date:Subject:MIME-Version:Message-Id:To; b=lblq9Zo/0QrgYNJHmI3yUE6evFLlSs7LRr7RJVoVBcPwKfD7ebBHsjT30NAyexSFln+zjLBWQBcbopcVGZ4Pz1wbfLOI5Hc2oZRgiGLptJN/7n6Flv7ornc6RDgQdzAvlhtumsEfPvtMc6iSkA7jlhnx4AcOZPzauHMg5iRTsvU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-5004a.ext.cloudfilter.net ([10.0.29.221]) by cmsmtp with ESMTPS id tr6drAsoPQr4StwHHrdAZP; Mon, 08 Apr 2024 21:11:43 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id twHFrmjrEGDo0twHGrQR0u; Mon, 08 Apr 2024 21:11:42 +0000 X-Authority-Analysis: v=2.4 cv=I+uuR8gg c=1 sm=1 tr=0 ts=66145d8e a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=IkcTkHD0fZMA:10 a=raytVjVEu-sA:10 a=Qbun_eYptAEA:10 a=CCpqsmhAAAAA:8 a=Q-bJrc6Cx20q2ANe5GgA:9 a=QEXdDO2ut3YA:10 a=ul9cdbp4aOFLsgKbc677:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=To:In-Reply-To:References:Message-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:Subject:Date:From:Sender:Reply-To:Cc:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=92bsv54c4K5cgtfOel9VTZEjs0AGq5qrDqB6C98UKkA=; b=eWBjv9oHlFtb50eOjd7cPsAwEb F+XbnkNF0nk/92qF2VhtzSetwfNUBWZ8Mw3UZP5tMFwunjheK36wiZsnMUCcBBKns+hqIVUMH0cmu QVmrTOk/8ZV4dPEQaMj0H62xc; Received: from 97-122-82-115.hlrn.qwest.net ([97.122.82.115]:54230 helo=[192.168.0.21]) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96.2) (envelope-from ) id 1rtwHF-001uXf-2A for gdb-patches@sourceware.org; Mon, 08 Apr 2024 15:11:41 -0600 From: Tom Tromey Date: Mon, 08 Apr 2024 15:11:41 -0600 Subject: [PATCH v2 5/7] Correctly handle DIE parent computations MIME-Version: 1.0 Message-Id: <20240408-die-map-madness-v2-5-6741626d544d@tromey.com> References: <20240408-die-map-madness-v2-0-6741626d544d@tromey.com> In-Reply-To: <20240408-die-map-madness-v2-0-6741626d544d@tromey.com> To: gdb-patches@sourceware.org X-Mailer: b4 0.12.4 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 97.122.82.115 X-Source-L: No X-Exim-ID: 1rtwHF-001uXf-2A X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-82-115.hlrn.qwest.net ([192.168.0.21]) [97.122.82.115]:54230 X-Source-Auth: tom+tromey.com X-Email-Count: 9 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfCkz0l6unhmQSu02spPNnEGwQkRu5zBzgqamxtTUZ03VsCPfGDRSjrmZsXRLSTEiGdr2iKDyD4TtXHcSVxWn252jLLapO8A5JzcrwRKjsg3I04RNGPv9 a5S1Y4uJDwJpjGzqSumoE5JSwgjDSplM19OBAyXHx9ITW/qkTWWH5way8aQq0XgdS50dWPxygZVtyEyG3AcUWreZ4pj8lZPIyPo= X-Spam-Status: No, score=-3021.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Tom de Vries pointed out that the combination of sharding, multi-threading, and per-CU "racing" means that sometimes a cross-CU DIE reference might not be correctly resolved. However, it's important to handle this correctly, due to some unfortunate aspects of DWARF. This patch implements this by arranging to preserve each worker's DIE map through the end of index finalization. The extra data is discarded when finalization is done. This approach also allows the parent name resolution to be sharded, by integrating it into the existing entry finalization loop. In an earlier review, I remarked that addrmap couldn't be used here. However, I was mistaken. A *mutable* addrmap cannot be used, as those are based on splay trees and restructure the tree even during lookups (and thus aren't thread-safe). A fixed addrmap, on the other hand, is just a vector and is thread-safe. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30846 --- gdb/dwarf2/cooked-index.c | 14 +++++++--- gdb/dwarf2/cooked-index.h | 49 +++++++++++++++++++++++++++-------- gdb/dwarf2/read-debug-names.c | 7 +++-- gdb/dwarf2/read.c | 60 +++++++++++++++++++------------------------ 4 files changed, 80 insertions(+), 50 deletions(-) diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c index 66822b575ca..eb1b1df8d74 100644 --- a/gdb/dwarf2/cooked-index.c +++ b/gdb/dwarf2/cooked-index.c @@ -330,7 +330,7 @@ cooked_index_shard::handle_gnat_encoded_entry (cooked_index_entry *entry, /* See cooked-index.h. */ void -cooked_index_shard::finalize () +cooked_index_shard::finalize (const parent_map_map *parent_maps) { auto hash_name_ptr = [] (const void *p) { @@ -371,6 +371,13 @@ cooked_index_shard::finalize () for (cooked_index_entry *entry : m_entries) { + if ((entry->flags & IS_PARENT_DEFERRED) != 0) + { + const cooked_index_entry *new_parent + = parent_maps->find (entry->get_deferred_parent ()); + entry->resolve_parent (new_parent); + } + /* Note that this code must be kept in sync with language_requires_canonicalization. */ gdb_assert (entry->canonical == nullptr); @@ -630,7 +637,8 @@ cooked_index::wait (cooked_state desired_state, bool allow_quit) } void -cooked_index::set_contents (vec_type &&vec, deferred_warnings *warn) +cooked_index::set_contents (vec_type &&vec, deferred_warnings *warn, + const parent_map_map *parent_maps) { gdb_assert (m_vector.empty ()); m_vector = std::move (vec); @@ -652,7 +660,7 @@ cooked_index::set_contents (vec_type &&vec, deferred_warnings *warn) for (auto &idx : m_vector) { auto this_index = idx.get (); - finalizers.add_task ([=] () { this_index->finalize (); }); + finalizers.add_task ([=] () { this_index->finalize (parent_maps); }); } finalizers.start (); diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h index 6400750b587..d245570e34d 100644 --- a/gdb/dwarf2/cooked-index.h +++ b/gdb/dwarf2/cooked-index.h @@ -351,9 +351,9 @@ class cooked_index_shard /* Finalize the index. This should be called a single time, when the index has been fully populated. It enters all the entries - into the internal table. This may be invoked in a worker - thread. */ - void finalize (); + into the internal table and fixes up all missing parent links. + This may be invoked in a worker thread. */ + void finalize (const parent_map_map *parent_maps); /* Storage for the entries. */ auto_obstack m_storage; @@ -419,6 +419,19 @@ class cooked_index_storage return &m_addrmap; } + /* Return the parent_map that is currently being created. */ + parent_map *get_parent_map () + { + return &m_parent_map; + } + + /* Return the parent_map that is currently being created. Ownership + is passed to the caller. */ + parent_map release_parent_map () + { + return std::move (m_parent_map); + } + private: /* Hash function for a cutu_reader. */ @@ -434,6 +447,9 @@ class cooked_index_storage /* The index shard that is being constructed. */ std::unique_ptr m_index; + /* Parent map for each CU that is read. */ + parent_map m_parent_map; + /* A writeable addrmap being constructed by this scanner. */ addrmap_mutable m_addrmap; }; @@ -508,13 +524,17 @@ class cooked_index_worker { } /* Each thread returns a tuple holding a cooked index, any collected - complaints, and a vector of errors that should be printed. The - latter is done because GDB's I/O system is not thread-safe. - run_on_main_thread could be used, but that would mean the - messages are printed after the prompt, which looks weird. */ + complaints, a vector of errors that should be printed, and a + vector of parent maps. + + The errors are retained because GDB's I/O system is not + thread-safe. run_on_main_thread could be used, but that would + mean the messages are printed after the prompt, which looks + weird. */ using result_type = std::tuple, complaint_collection, - std::vector>; + std::vector, + parent_map>; /* The per-objfile object. */ dwarf2_per_objfile *m_per_objfile; @@ -527,6 +547,10 @@ class cooked_index_worker This is enforced in the cooked_index_worker constructor. */ deferred_warnings m_warnings; + /* A map of all parent maps. Used during finalization to fix up + parent relationships. */ + parent_map_map m_all_parents_map; + #if CXX_STD_THREAD /* Current state of this object. */ cooked_state m_state = cooked_state::INITIAL; @@ -618,9 +642,12 @@ class cooked_index : public dwarf_scanner_base /* Called by cooked_index_worker to set the contents of this index and transition to the MAIN_AVAILABLE state. WARN is used to - collect any warnings that may arise when writing to the - cache. */ - void set_contents (vec_type &&vec, deferred_warnings *warn); + collect any warnings that may arise when writing to the cache. + PARENT_MAPS is used when resolving pending parent links. + PARENT_MAPS may be NULL if there are no IS_PARENT_DEFERRED + entries in VEC. */ + void set_contents (vec_type &&vec, deferred_warnings *warn, + const parent_map_map *parent_maps); /* A range over a vector of subranges. */ using range = range_chain; diff --git a/gdb/dwarf2/read-debug-names.c b/gdb/dwarf2/read-debug-names.c index a3547cde7aa..eebe5a91095 100644 --- a/gdb/dwarf2/read-debug-names.c +++ b/gdb/dwarf2/read-debug-names.c @@ -345,13 +345,16 @@ cooked_index_debug_names::do_reading () = create_quick_file_names_table (per_bfd->all_units.size ()); m_results.emplace_back (nullptr, complaint_handler.release (), - std::move (exceptions)); + std::move (exceptions), + parent_map ()); std::vector> indexes; indexes.push_back (std::move (m_map.shard)); cooked_index *table = (gdb::checked_static_cast (per_bfd->index_table.get ())); - table->set_contents (std::move (indexes), &m_warnings); + /* Note that this code never uses IS_PARENT_DEFERRED, so it is safe + to pass nullptr here. */ + table->set_contents (std::move (indexes), &m_warnings, nullptr); bfd_thread_cleanup (); } diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 6332ffb5767..2a723bb947f 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -4463,7 +4463,8 @@ class cooked_indexer enum language language) : m_index_storage (storage), m_per_cu (per_cu), - m_language (language) + m_language (language), + m_die_range_map (storage->get_parent_map ()) { } @@ -4539,18 +4540,8 @@ class cooked_indexer /* The language that we're assuming when reading. */ enum language m_language; - /* Map from DIE ranges to newly-created entries. See - m_deferred_entries to understand this. */ - parent_map m_die_range_map; - - /* The generated DWARF can sometimes have the declaration for a - method in a class (or perhaps namespace) scope, with the - definition appearing outside this scope... just one of the many - bad things about DWARF. In order to handle this situation, we - defer certain entries until the end of scanning, at which point - we'll know the containing context of all the DIEs that we might - have scanned. This vector stores these deferred entries. */ - std::vector m_deferred_entries; + /* Map from DIE ranges to newly-created entries. */ + parent_map *m_die_range_map; }; /* Subroutine of dwarf2_build_psymtabs_hard to simplify it. @@ -4893,7 +4884,8 @@ cooked_index_debug_info::process_cus (size_t task_number, unit_iterator first, m_results[task_number] = result_type (thread_storage.release (), complaint_handler.release (), - std::move (errors)); + std::move (errors), + std::move (thread_storage.release_parent_map ())); } void @@ -4903,7 +4895,10 @@ cooked_index_debug_info::done_reading () can only be dealt with on the main thread. */ std::vector> indexes; for (auto &one_result : m_results) - indexes.push_back (std::move (std::get<0> (one_result))); + { + indexes.push_back (std::move (std::get<0> (one_result))); + m_all_parents_map.add_map (std::get<3> (one_result)); + } /* This has to wait until we read the CUs, we need the list of DWOs. */ process_skeletonless_type_units (m_per_objfile, &m_index_storage); @@ -4911,11 +4906,14 @@ cooked_index_debug_info::done_reading () indexes.push_back (m_index_storage.release ()); indexes.shrink_to_fit (); + m_all_parents_map.add_map (m_index_storage.release_parent_map ()); + dwarf2_per_bfd *per_bfd = m_per_objfile->per_bfd; cooked_index *table = (gdb::checked_static_cast (per_bfd->index_table.get ())); - table->set_contents (std::move (indexes), &m_warnings); + table->set_contents (std::move (indexes), &m_warnings, + &m_all_parents_map); } void @@ -16259,13 +16257,17 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu, if (*parent_entry == nullptr) { + /* We only perform immediate lookups of parents for DIEs + from earlier in this CU. This avoids any problem + with a NULL result when when we see a reference to a + DIE in another CU that we may or may not have + imported locally. */ parent_map::addr_type addr = parent_map::form_addr (origin_offset, origin_is_dwz); - if (new_reader->cu == reader->cu - && new_info_ptr > watermark_ptr) + if (new_reader->cu != reader->cu || new_info_ptr > watermark_ptr) *maybe_defer = addr; else - *parent_entry = m_die_range_map.find (addr); + *parent_entry = m_die_range_map->find (addr); } unsigned int bytes_read; @@ -16396,7 +16398,7 @@ cooked_indexer::recurse (cutu_reader *reader, parent_map::addr_type end = parent_map::form_addr (sect_offset (info_ptr - 1 - reader->buffer), reader->cu->per_cu->is_dwz); - m_die_range_map.add_entry (start, end, parent_entry); + m_die_range_map->add_entry (start, end, parent_entry); } return info_ptr; @@ -16481,13 +16483,10 @@ cooked_indexer::index_dies (cutu_reader *reader, if (name != nullptr) { if (defer != 0) - { - this_entry - = m_index_storage->add (this_die, abbrev->tag, - flags | IS_PARENT_DEFERRED, name, - defer, m_per_cu); - m_deferred_entries.push_back (this_entry); - } + this_entry + = m_index_storage->add (this_die, abbrev->tag, + flags | IS_PARENT_DEFERRED, name, + defer, m_per_cu); else this_entry = m_index_storage->add (this_die, abbrev->tag, flags, name, @@ -16579,13 +16578,6 @@ cooked_indexer::make_index (cutu_reader *reader) if (!reader->comp_unit_die->has_children) return; index_dies (reader, reader->info_ptr, nullptr, false); - - for (const auto &entry : m_deferred_entries) - { - const cooked_index_entry *parent - = m_die_range_map.find (entry->get_deferred_parent ()); - entry->resolve_parent (parent); - } } dwarf2_per_cu_data *