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

[Andrew Burgess <aburgess@redhat.com>]

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>

Thanks,
Andrew