Re: [PATCH 1/2] gdb: split cooked index finalization into separate steps

[Simon Marchi <simon.marchi@efficios.com>]

On 9/24/26 6:16 AM, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> This patch refactors the existing cooked index "finalize" step into
>> multiple steps.  This is a preparatory patch for the following patch,
>> which will add some more steps.  I believe that using this pattern will
>> help keep the code readable and easy to understand, despite the
>> increasing amount of work done.
>>
>> The following patch introduces a step that needs to run after the
>> deferred parents have been resolved, but before the name
>> canonicalization happens, so factor out the deferred parents code into a
>> step of its own.
>>
>> The code that remains in cooked_index_shard::finalize is all about name
>> canonicalization (if we consider the Ada stuff to be a special case of
>> that), so rename that step to cooked_index_shard::canonicalize_names().
>>
>> For clarity, move the "write to cache" code out of the "done" callback
>> to make it clear that it is logically a separate step.
>>
>> So as of this patch, we have the steps:
>>
>>  - resolve deferred parents
>>  - canonicalize names
>>  - write to cache
>>
>> The term "finalization" now refers to all these steps.  Or, depending on
>> your definition, it could refer to all but "write to cache", since the
>> index reaches the FINALIZED state (i.e. becomes fully usable) just
>> before that one.
>>
>> Here are the implementation details:
>>
>>  - The first step is called by cooked_index::set_contents(), then each
>>    step is responsible for calling the next one (thus forming a chain),
>>    until we are done.
>>
>>  - Steps that involve per-shard parallel processing use gdb::task_group,
>>    the "done" callback of the task_group starts the next task.
>>
>>    A special case of that is: if some steps are able to detect ahead of
>>    time that it's unnecessary to run the step for a given shard, they
>>    can omit adding a task for that shard.
>>
>>    An even more special case of that is: if no tasks are added to a task
>>    group, the "done" callback for that task group is invoked
>>    immediately when the group is started.  This allows easily and
>>    cheaply skipping entire steps when they are not needed.
>>
>>    This is used in this patch for the new "resolve deferred parents"
>>    step.  Each shard records whether it has at least one deferred parent
>>    during the initial scan.  If a shard has none (which is a common
>>    case), then cooked_index_shard::resolve_deferred_parents would
>>    iterate the index entries for nothing.  The step therefore omits
>>    adding a task for that shard in that case.  If no shards have any
>>    deferred parents that require resolving, we go directly to the next
>>    step with very little cost.
>>
>> During review, Claude pointed out that my change actually fixes a
>> potential problem.  The current code resolves the deferred parent of an
>> entry in the same loop where it potentially calls "full_name" for that
>> entry (to create the special Ada entries).  "full_name" walks up the
>> parent chain, and thus requires all grand-parents to be resolved.
>> Depending on the order of DIEs in the file, it might not be true.
>> Having the "resolve deferred parents" step run completely before the
>> name canonicalization step avoids this.
>>
>> I built a gdb.dwarf2 test case around the DWARF structure it proposed to
>> hit the bug.  It is perhaps not something we expect from real-world
>> producers, but it's also not completely unthinkable.  It hits this
>> assert when the patch is not applied:
>>
>>     gdb/dwarf2/cooked-index-entry.h:217: internal-error: get_parent: Assertion `(flags & IS_PARENT_DEFERRED) == 0' failed.
>>
>> ... and passes with it.
> 
> Simon,
> 
> Thanks for doing this.  My reworked series (unposted) had a patch just
> like this, but your addition of the new test is a bonus I didn't have,
> so I think yours is better.  I only had one minor suggestion from my
> take on this change, see below...
> 
>> +
>> +void
>> +cooked_index_shard::canonicalize_names ()
>>  {
>>    gdb::unordered_set<const cooked_index_entry *,
>>  		     cooked_index_entry_name_ptr_hash,
>> @@ -216,13 +236,6 @@ cooked_index_shard::finalize (const parent_map_map *parent_maps)
>>  
>>    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);
>> -	}
>> -
> 
> I replaced this block with:
> 
>   /* Deferred parents should not reach this point.  */
>   gdb_assert ((entry->flags & IS_PARENT_DEFERRED) == 0);
> 
> I think the assert is worth keeping.  With that:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Good idea, applied locally.

Simon