From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from omta036.useast.a.cloudfilter.net (omta036.useast.a.cloudfilter.net [44.202.169.35]) by sourceware.org (Postfix) with ESMTPS id 3A4B63858C20 for ; Wed, 17 Jan 2024 18:59:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3A4B63858C20 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 3A4B63858C20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=44.202.169.35 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705517946; cv=none; b=xBzamY9+MxPywVG4/r3+ylNFt9yE3CE77PmEFswSqMiVsBftmy+dnwILE+wkDKbcEZTKQVb3XR7qfCOmNHnm3uOsXnepjgGpJd4ZfdkwTQ3QQhNQdZ9mYmtRrrBGe0I7/E7WPVQuSJyLdlKjuiigg20vPnlPDGYYBlZsV3S3tSI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1705517946; c=relaxed/simple; bh=75vCxo8PgsGV5125lqcjJj0lTasvmV2gIRfX718Dj58=; h=DKIM-Signature:From:Date:Subject:MIME-Version:Message-Id:To; b=GFfVzpEn2ONtlDbG3DUoI41nNCuvxPyY2ES8zakJ7ozDMxigA9PZrqx1SlQnvLDwwaQyY9kAi7mn9ljgne0G90qO+xyD9ubSUdOuh4cChtKtul19LEIxu3lGqWxZ2+EcDAMl2cjWj9ipdn/itFLiMncT7tnD3JLx03tg5evSJ1s= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-5009a.ext.cloudfilter.net ([10.0.29.176]) by cmsmtp with ESMTPS id Q9KdrByE28uLRQB7vrjynI; Wed, 17 Jan 2024 18:59:03 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id QB7trZw9fV9xYQB7uroPFW; Wed, 17 Jan 2024 18:59:02 +0000 X-Authority-Analysis: v=2.4 cv=NfP1akP4 c=1 sm=1 tr=0 ts=65a82376 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=OWjo9vPv0XrRhIrVQ50Ab3nP57M=:19 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=IkcTkHD0fZMA:10 a=dEuoMetlWLkA:10 a=Qbun_eYptAEA:10 a=CCpqsmhAAAAA:8 a=q92tKoACvwVzCD3sfbYA: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=a3H1SxaY2M6ssfY/7QA4Ut5994ZweeHgkBLoD0oAt8E=; b=H7LoyrcO4VkHFPJV471ew/Bmmb rrYKgwfaPscoqS2Dd+xTcEyGUThhqd4D2uIMQ3K2dk7zlx5MMxzldagKR5zNIDUJplqLwPPCQTdvb SmIeUCwxsX0jaLU1N4bFrzRHw; Received: from 97-122-68-157.hlrn.qwest.net ([97.122.68.157]:45122 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 1rQB7t-004FFp-1d for gdb-patches@sourceware.org; Wed, 17 Jan 2024 11:59:01 -0700 From: Tom Tromey Date: Wed, 17 Jan 2024 11:58:52 -0700 Subject: [PATCH 5/7] Correctly handle DIE parent computations MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20240117-die-map-madness-v1-5-42fb435ad1ed@tromey.com> References: <20240117-die-map-madness-v1-0-42fb435ad1ed@tromey.com> In-Reply-To: <20240117-die-map-madness-v1-0-42fb435ad1ed@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.68.157 X-Source-L: No X-Exim-ID: 1rQB7t-004FFp-1d X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-68-157.hlrn.qwest.net ([192.168.0.21]) [97.122.68.157]:45122 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: MS4xfImSf9y5dgLPVvYCtJfixiLyWfiv2/Ps5wWezRqJ2wt8o/hSDGOW99ise1PLW6uiD5YI/GG4tkvRGMLZC76xJxvpEWSdjdWo4zNJPhvwfL4fR8Xu86Ia h/lM4d/tO+vtj9/Emdkw+JjbkaMDXM7XSFoXh9068H08EsxGGVU2C75DtlSiFtVbPb3iVSE4862T0P7/Lg7eNxM7O5ecoSgswzg= X-Spam-Status: No, score=-3022.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS,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: 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 | 13 ++++++++--- gdb/dwarf2/cooked-index.h | 45 ++++++++++++++++++++++++++++-------- gdb/dwarf2/read.c | 59 ++++++++++++++++++++--------------------------- 3 files changed, 70 insertions(+), 47 deletions(-) diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c index e4f112b6ee0..93370f96bec 100644 --- a/gdb/dwarf2/cooked-index.c +++ b/gdb/dwarf2/cooked-index.c @@ -319,7 +319,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) { @@ -360,6 +360,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); @@ -479,7 +486,7 @@ cooked_index::wait (cooked_state desired_state, bool allow_quit) } void -cooked_index::set_contents (vec_type &&vec) +cooked_index::set_contents (vec_type &&vec, const parent_map_map *parent_maps) { gdb_assert (m_vector.empty ()); m_vector = std::move (vec); @@ -502,7 +509,7 @@ cooked_index::set_contents (vec_type &&vec) 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 442ee9e14fd..d8e6fe476a4 100644 --- a/gdb/dwarf2/cooked-index.h +++ b/gdb/dwarf2/cooked-index.h @@ -392,9 +392,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; @@ -459,6 +459,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. */ @@ -474,6 +487,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; }; @@ -544,13 +560,17 @@ class cooked_index_worker unit_iterator end); /* 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; @@ -567,6 +587,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; @@ -654,8 +678,9 @@ class cooked_index : public dwarf_scanner_base void start_reading (); /* Called by cooked_index_worker to set the contents of this index - and transition to the MAIN_AVAILABLE state. */ - void set_contents (vec_type &&vec); + and transition to the MAIN_AVAILABLE state. PARENT_MAPS is used + when resolving pending parent links. */ + void set_contents (vec_type &&vec, const parent_map_map *parent_maps); /* A range over a vector of subranges. */ using range = range_chain; diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 585c15212f8..494f2b5625c 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 ()) { } @@ -4538,18 +4539,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. @@ -4866,7 +4857,8 @@ cooked_index_worker::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 @@ -4876,7 +4868,10 @@ cooked_index_worker::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); @@ -4884,11 +4879,13 @@ cooked_index_worker::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)); + table->set_contents (std::move (indexes), &m_all_parents_map); } void @@ -16327,13 +16324,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; @@ -16457,7 +16458,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; @@ -16534,13 +16535,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, @@ -16632,13 +16630,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); - } } /* An implementation of quick_symbol_functions for the cooked DWARF -- 2.43.0