Re: [PATCH v2 11/28] Rewrite the .gdb_index reader

[Simon Marchi <simark@simark.ca>]

On 2025-04-02 19:45, Tom Tromey wrote:
> This patch rewrites the .gdb_index reader to create the same data
> structures that are created by the cooked indexer and the .debug_names
> reader.
> 
> This is done in support of this series; but also because, from what I
> can tell, the "templates.exp" change didn't really work properly with
> this reader.
> 
> In addition to fixing that problem, this patch removes a lot of code.
> 
> Implementing this required a couple of hacks, as .gdb_index does not
> contain all the information that's used by the cooked index
> implementation.
> 
> * The index-searching code likes to differentiate between the various
>   DWARF tags when matching, but .gdb_index lumps many things into a
>   single "other" category.  To handle this, we introduce a phony tag
>   that's used so that the match method can match on multiple domains.
> 
> * Similarly, .gdb_index doesn't distinguish between the type and
>   struct domains, so another phony tag is used for this.
> 
> * Support for older versions of .gdb_index is removed entirely.
> 
> * The reader must attempt to guess the language of various symbols.
>   This is somewhat finicky.  "Plain" (unqualified) symbols are marked
>   as language_unknown and then a couple of hacks are used to handle
>   these -- one in expand_symtabs_matching and another when recognizing
>   "main".
> 
> For what it's worth, I consider .gdb_index to be near the end of its
> life.  While .debug_names is not perfect -- we found a number of bugs
> in the standard while implementing it -- it is better than .gdb_index
> and also better documented.
> 
> After this patch, we could conceivably remove dwarf_scanner_base.
> However, I have not done this.
> 
> Finally, this patch also changes this reader to dump the content of
> the index, as the other DWARF readers do.  This can be handy when
> debugging gdb.

I'm not knowledgeable enough with the .gdb_index quirks to have an
informed opinion about this.  I am happy with this change because it
brings it in line with the other two we already have (.debug_info and
.debug_names).

I noted some random comments below.

> diff --git a/gdb/dwarf2/cooked-index-shard.c b/gdb/dwarf2/cooked-index-shard.c
> index 29a8aea513786e4c1c1ed77dee8610fc329d1c8a..888a0fa345b9124e2814aa722e71a9d2fd5adf8e 100644
> --- a/gdb/dwarf2/cooked-index-shard.c
> +++ b/gdb/dwarf2/cooked-index-shard.c
> @@ -86,7 +86,16 @@ cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
>       implicit "main" discovery.  */
>    if ((flags & IS_MAIN) != 0)
>      m_main = result;
> -  else if ((flags & IS_PARENT_DEFERRED) == 0
> +  /* The language check here is subtle: it exists solely to work
> +     around a bug in .gdb_index.  That index does not record
> +     languages, but it might emit an entry for "main".  However,
> +     recognizing this "main" as being the main program would be wrong
> +     -- for example, an Ada program has a C "main" but this is not the
> +     desired target of the "start" command.  Requiring the language to
> +     be set here avoids over-eagerly setting the "main" when using
> +     .gdb_index.  Should .gdb_index ever be removed (PR symtab/31363),
> +     the language_unknown check here could also be removed.  */
> +  else if (lang != language_unknown

Did you remove the IS_PARENT_DEFERRED check on purpose?

> @@ -203,10 +209,10 @@ enum class cooked_state
>  /* An object of this type controls the scanning of the DWARF.  It
>     schedules the worker tasks and tracks the current state.  Once
>     scanning is done, this object is discarded.
> -
> +   

Trailing spaces ^.

> +/* This is like a cooked index, but as it has been ingested from
> +   .gdb_index, it can't be used to write out an index.  */
> +
> +class cooked_gdb_index : public cooked_index

Technically, I think it should be "This is a cooked index as ingested
from .gdb_index".  You know, the "is-a" relationship.  Currently, the
base class is the "from .debug_info" case and we have some derived
classes for the indexes.  I feel like we should (eventually) move to
having an abstract base class and add a derived class for the "from
.debug_info" case.

> +	  /* Note that this assumes the final component ends in \0.  */
> +	  cooked_index_entry *entry = result.add (per_cu->sect_off, tag,
> +						  flags, this_lang,
> +						  components.back ().data (),
> +						  nullptr, per_cu);
> +	  /* Don't bother pushing if we do not need a panrent.  */

panrent -> parent

Simon