From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Pw5VApYDsmrDszAAWB0awg (envelope-from ) for ; Tue, 22 Sep 2026 00:27:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1790051221; bh=V0esvWlq6FU5VWk7nJbNtNXiCBdLnB7CagSu3lTvEE8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=j0XljCOmS9ULs93/dS+Hbm0nS7eT9hwzgf0w4GE+qi8RtiAOZW0ZzWLko83/pnZdL APvI4wOax63nm1IsZfprmJFgZ2gDsApiVbmRYov6T3FGV9kPhWgIHfFdYr1euRDCF4 IUlYtJWm3URwUIG7HXc/TD2CTvomR1O/gyKYWVkc= Received: by simark.ca (Postfix, from userid 112) id E7B671E033; Tue, 22 Sep 2026 00:27:01 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=nUixtsrF; dkim-atps=neutral Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id B8F441E033 for ; Tue, 22 Sep 2026 00:27:00 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 52C9F4BAE7D0 for ; Tue, 22 Sep 2026 04:27:00 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 52C9F4BAE7D0 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=nUixtsrF Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 03A7B4BA903C for ; Tue, 22 Sep 2026 04:26:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 03A7B4BA903C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 03A7B4BA903C Authentication-Results: sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790051193; cv=none; b=Arbs3vpqbMRraLWwCXpcaK5vAnDnczBRJmv6qYene6QJDvhHThpw0M4ZR2NP4lWMSTSyRMEVykS6mOOPjhFs7To7sikBOmAN6ByKt4PgAvNTMMA91JivDX0GprESzbXPdxVsADjQhmk6LebCkHEq3R/1ydVmfYDhLgQz+f6o0mQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790051193; c=relaxed/simple; bh=V0esvWlq6FU5VWk7nJbNtNXiCBdLnB7CagSu3lTvEE8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=reCfmH34lKoeXpbqw866QueySoochWMm3kbJ46PRzYcRJtzhlbPq9MDE53YjqYtJ8mwSIPO+3JlUEBzSFR9DFGYAuzhwmZTVYuQ522HTrQRPdkHug5hIVt+qd3tw01uoegPZr7Rjoxwq6E8YG8JmR84sWl9MqTG69Q5+fE07iuc= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=nUixtsrF DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 03A7B4BA903C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1790051191; bh=V0esvWlq6FU5VWk7nJbNtNXiCBdLnB7CagSu3lTvEE8=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=nUixtsrFd/YmLva788NwvIk3wDchKbB9ySRsSoHGB3C6HWUbn3FETuTww/XrYQ8TK m1o7o/rXqZDNtLfHsCAJ7T+D3oAkOypEPvsD02mYyRhgiOfiO+vOfWaRl/HOSEuIwA suPOlH6u2E8FvpUkdue4gwIk+m/vn9rdN9q/kErs= Received: by simark.ca (Postfix) id A026C1E033; Tue, 22 Sep 2026 00:26:31 -0400 (EDT) Message-ID: <71fb1272-d43e-4037-bf90-7d141fb6c8c0@simark.ca> Date: Tue, 22 Sep 2026 00:26:31 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCHv4] gdb: resolve class name via DW_AT_signature in cooked index To: Andrew Burgess , gdb-patches@sourceware.org Cc: Tom Tromey References: <897f5eb957bdfd90cd3fd5efa662021ed5c2aef2.1788269262.git.aburgess@redhat.com> Content-Language: en-US From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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~public-inbox=simark.ca@sourceware.org On 2026-09-16 07:38, Andrew Burgess wrote: > gdb.dwarf2/sig-type-unnamed-class-bad-sig.exp: > This one tests some invalid DWARF, the type DIE references a > signature that doesn't exist. In this case GDB just leaves the > cooked_index_entry with a NULL name string, which means symbols > cannot be found. I think this is fine though, the DWARF is > corrupted in this case. Not super important, but is the "GDB just leaves the cooked_index_entry with a NULL name string" stale? My understanding is that the entries with a NULL name are removed from the vector, and the parent links pointing to them are removed, so there is no way to reach them anymore. > @@ -189,6 +192,52 @@ struct cooked_index_entry_name_ptr_eq > > /* See cooked-index-shard.h. */ > > +void > +cooked_index_shard::resolve_deferred_names > + (const signature_to_name_map &sig_names) > +{ > + bool need_to_cleanup_entries = false; > + for (const auto &[entry, signature] : m_deferred_names) > + { > + if (const auto it = sig_names.find (signature); > + it != sig_names.end ()) > + { > + /* Each entry should only occur once in M_DEFERRED_NAMES, > + and the entry should only be added when it has no name. */ > + gdb_assert (entry->name == nullptr); You can move this assert above the if (run it before looking up the signature). > + > + /* Patch the name. */ > + entry->name = it->second; > + } > + else > + { > + need_to_cleanup_entries = true; > + complaint (_(DWARF_ERROR_PREFIX > + "Cannot find signatured DIE %s referenced from DIE " > + "at %s [in module %s]"), > + hex_string (signature), > + sect_offset_str (entry->die_offset), > + entry->per_cu->per_bfd ()->filename ()); > + } > + } > + > + /* If we failed to resolve the name of an entry via its signature > + then remove the entry from the m_entries vector. This should be > + rare, and should only happen when we have corrupted DWARF. The > + entries still live on the obstack, so parent points are still points -> pointers? > @@ -223,6 +277,16 @@ cooked_index_shard::finalize (const parent_map_map *parent_maps) > entry->resolve_parent (new_parent); > } > > + /* Remove a parent reference if the parent has no name. This > + leaves ENTRY as an orphan, but this only happens if the DWARF > + is corrupted and we failed to find a name for the parent. We > + can safely check the parent's name at this point because all > + deferred names will have been resolved in all shards before > + finalize is called on any shard. */ > + if (const cooked_index_entry *parent = entry->get_parent (); > + parent != nullptr && parent->name == nullptr) > + entry->set_parent (nullptr); Unlikely, but Claude pointed out that we break the links to nameless parents here, but in the same loop below there is the possibility that we call full_name on the current entry: if (entry->get_parent () != nullptr) { const char *fullname = entry->full_name (&m_storage, FOR_ADA_LINKAGE_NAME); So imagine that in the vector we have in this order: - Entry - Parent entry - Grandparent entry, which is nameless The parent of "Entry" has a name, so we did not remove that link. We did not process "Parent" yet, so it still points to its parent (Grandparent). If we call full_name on "Entry", it will go up the parent chain and will hit the assert for the grandparent. It might be safer to split it into a separate step: 1. resolve deferred names 2. prune entries without name / links to parents without name 3. finalize If you want to optimize for the common case, step 1 can record if there were any problematic entry. If not, you can skip directly to step 3. That can be done by using a task_group for step 2, but only posting the tasks if necessary. If the task_group is started with no tasks, it will immediately invoke the "done" callback, which in this case would kick off step 3. I have a similar suggestion below for step 1, to avoid posting tasks unnecessarily if we know there are no deferred names to compute in a given shard. Claude also pointed out a possible race condition between cooked_index::get_main_name(), which can be called as soon as we reached the MAIN_AVAILABLE state, and the "resolve deferred names" step. get_main_name consults cooked_index_entry::name, while "resolve deferred names" sets some cooked_index_entry::name from nullptr to the actual name. It might not be an actual problem, because the cooked_index_entry for "main" is not going to be a type, so it's not part of those with a deferred name. > + > /* Note that this code must be kept in sync with > cooked_index::get_main -- if canonicalization is required > here, then a check might be required there. */ > diff --git a/gdb/dwarf2/cooked-index-shard.h b/gdb/dwarf2/cooked-index-shard.h > index 84c37958c83..191182a132b 100644 > --- a/gdb/dwarf2/cooked-index-shard.h > +++ b/gdb/dwarf2/cooked-index-shard.h > @@ -26,6 +26,7 @@ > #include "addrmap.h" > #include "gdbsupport/iterator-range.h" > #include "gdbsupport/string-set.h" > +#include "complaints.h" > > /* An index of interesting DIEs. This is "cooked", in contrast to a > mapped .debug_names or .gdb_index, which are "raw". An entry in > @@ -79,6 +80,16 @@ class cooked_index_shard > for completion, will be returned. */ > range find (const std::string &name, bool completing) const; > > + /* Record that ENTRY didn't have a name attribute, but did have a > + DW_AT_signature attribute, SIGNATURE. ENTRY will have a NULL > + name string pointer. We will patch the name of ENTRY during > + finalization once the TUs have been parsed, the correct TU will > + be found using SIGNATURE. */ > + void add_deferred_name (cooked_index_entry *entry, ULONGEST signature) > + { > + m_deferred_names.push_back ({entry, signature}); It makes no meaningful difference, but I'd write: m_deferred_names.emplace_back (entry, signature); > diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c > index 167e39ffc89..799835bca61 100644 > --- a/gdb/dwarf2/cooked-index.c > +++ b/gdb/dwarf2/cooked-index.c > @@ -58,7 +58,23 @@ cooked_index::wait (cooked_state desired_state, bool allow_quit) > if (m_state == nullptr) > return; > > - if (m_state->wait (desired_state, allow_quit)) > + bool done = m_state->wait (desired_state, allow_quit); > + > + /* Emit any cached complaints if we have finalized and we are on the > + main thread. Check for the requested state or the DONE flag > + here, we might have only asked for MAIN_AVAILABLE, but if the > + workers are quick then they might be done, in which case we > + should emit the complaints now. */ > + if (!m_finalize_complaints_emitted > + && is_main_thread () > + && (desired_state >= cooked_state::FINALIZED || done)) > + { > + m_finalize_complaints_emitted = true; > + for (auto &shard : m_shards) Can be: for (const auto &shard : m_shards) > @@ -74,31 +90,70 @@ cooked_index::set_contents () > > m_state->set (cooked_state::MAIN_AVAILABLE); > > - /* This is run after finalization is done -- but not before. If > - this task were submitted earlier, it would have to wait for > - finalization. However, that would take a slot in the global > - thread pool, and if enough such tasks were submitted at once, it > - would cause a livelock. */ > - gdb::task_group finalizers ([this] () > - { > - m_state->set (cooked_state::FINALIZED); > - m_state->write_to_cache (index_for_writing ()); > - m_state->set (cooked_state::CACHE_DONE); > - }); > - > - for (auto &shard : m_shards) > + /* Finalization is done in two phases, which we build up in reverse order. > + During the second phase we call finalize on each shard then update the > + state to FINALIZED then CACHE_DONE. */ > + std::shared_ptr phase2 > + = std::make_shared ([this] () > { > - auto this_shard = shard.get (); > + /* This is run after finalization is done -- but not before. If this > + task were submitted earlier, it would have to wait for finalization. > + However, that would take a slot in the global thread pool, and if > + enough such tasks were submitted at once, it would cause a > + livelock. */ > + m_state->set (cooked_state::FINALIZED); > + m_state->write_to_cache (index_for_writing ()); > + m_state->set (cooked_state::CACHE_DONE); > + }); > + > + /* Arrange to call finalize on each shard. */ > + for (cooked_index_shard_up &shard : m_shards) > + { > + cooked_index_shard *this_shard = shard.get (); > const parent_map_map *parent_maps = m_state->get_parent_map_map (); > - finalizers.add_task ([this, this_shard, parent_maps] () > - { > - scoped_time_it time_it ("DWARF finalize worker", > - m_state->m_per_command_time); > - this_shard->finalize (parent_maps); > - }); > + phase2->add_task ([this, this_shard, parent_maps] () > + { > + complaint_interceptor complaint_handler; > + > + scoped_time_it time_it ("DWARF finalize worker", > + m_state->m_per_command_time); > + > + this_shard->finalize (parent_maps); > + > + this_shard->merge_finalize_complaints (complaint_handler.release ()); > + }); > } > > - finalizers.start (); > + /* In the first phase we resolve any deferred cooked_index_entry names. > + These names are needed in the second phase, but due to cross shard child > + to parent references, trying to resolve deferred names in the same phase > + as the names are used would lead to data races. */ > + gdb::task_group phase1 ([phase2] () > + { > + /* This is run once after all the other phase1 tasks are done. */ > + phase2->start (); > + }); > + > + /* Arrange to call resolve_deferred_names on each shard. */ > + for (cooked_index_shard_up &shard : m_shards) > + { > + cooked_index_shard *this_shard = shard.get (); > + const signature_to_name_map *sig_name_map > + = &m_state->get_sig_name_map (); > + phase1.add_task ([this, this_shard, sig_name_map] () > + { > + complaint_interceptor complaint_handler; > + > + scoped_time_it time_it ("DWARF resolve deferred names worker", > + m_state->m_per_command_time); > + > + this_shard->resolve_deferred_names (*sig_name_map); > + > + this_shard->merge_finalize_complaints (complaint_handler.release ()); > + }); > + } > + > + phase1.start (); I would maybe have a suggestion to make this easier to follow. Instead of cramming it all in a single function, give each phase its own start_* helper method, which kicks off that parallel task. In the "done" callback of each phase, call the start_* method for the next task. You could have as many such chained tasks as you want, and it would remain clear (IMO). Also, if my understanding of task_group is correct, you could avoid posting a deferred name task when you know in advance that there is no deferred name to compute in that shard. In the common case where there are no deferred name to compute at all, the task_group will have no tasks, and it will end as soon as you call start on it. So we'll pay almost nothing for that step if it's not necessary. So, I imagine something like this: void cooked_index::set_contents () { gdb_assert (m_shards.empty ()); m_shards = m_state->release_shards (); m_state->set (cooked_state::MAIN_AVAILABLE); this->start_resolve_deferred_names (); } void cooked_index::start_resolve_deferred_names () { gdb::task_group task_group ([this] () { /* This is run once after all the other phase1 tasks are done. */ this->start_finalize (); }); /* Arrange to call resolve_deferred_names on each shard. */ for (const cooked_index_shard_up &shard : m_shards) { if (shard->m_deferred_names.empty ()) continue; task_group.add_task ([this, this_shard = shard.get (), sig_name_map = &m_state->get_sig_name_map ()] () { complaint_interceptor complaint_handler; scoped_time_it time_it ("DWARF resolve deferred names worker", m_state->m_per_command_time); this_shard->resolve_deferred_names (*sig_name_map); this_shard->merge_finalize_complaints (complaint_handler.release ()); }); } task_group.start (); } void cooked_index::start_finalize () { gdb::task_group task_group ([this] () { /* This is run after finalization is done -- but not before. If this task were submitted earlier, it would have to wait for finalization. However, that would take a slot in the global thread pool, and if enough such tasks were submitted at once, it would cause a livelock. */ m_state->set (cooked_state::FINALIZED); m_state->write_to_cache (index_for_writing ()); m_state->set (cooked_state::CACHE_DONE); }); /* Arrange to call finalize on each shard. */ for (const cooked_index_shard_up &shard : m_shards) { task_group.add_task ([this, this_shard = shard.get (), parent_maps = m_state->get_parent_map_map ()] () { complaint_interceptor complaint_handler; scoped_time_it time_it ("DWARF finalize worker", m_state->m_per_command_time); this_shard->finalize (parent_maps); this_shard->merge_finalize_complaints (complaint_handler.release ()); }); } task_group.start (); } We could even consider moving out the index_for_writing / CACHE_DONE to a separate helper method (which just runs serially), because it's conceptually a separate step than the "finalize" step. Simon