Re: [PATCHv4] gdb: resolve class name via DW_AT_signature in cooked index

[Simon Marchi <simark@simark.ca>]

On 9/22/26 12:26 AM, Simon Marchi wrote:
> 
> 
> 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<gdb::task_group> phase2
>> +    = std::make_shared<gdb::task_group> ([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

I don't know if you are working on this right now, but in order to help
have this merged before the planned release on Friday, I'll prepare an
updated version of your patch where I address my own comments.  You'll
be able to decide what you keep from it.