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

Simon Marchi
2025-04-23T17:23:22Z

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