Re: [PATCH v2 00/28] Search symbols via quick API

Simon Marchi
2025-04-23T20:11:09Z

On 2025-04-02 19:44, Tom Tromey wrote:
> This series changes how symbols are looked up in gdb.
>
> Currently, symbol lookup is done in two phases. In one phase, gdb
> searches all existing symtabs for a symbol. In another phase, gdb
> will call expand_symtabs_matching to expand some symtabs.
>
> Different spots in gdb may order these phases differently -- so some
> places will expand symtabs and then search the compunits, but other
> places will first search expanded symtabs and then use the
> expand_symtabs_matching callback to add further results.
>
> This approach has a few drawbacks.
>
> * The double search means that some discrepancies between the indexer
> and the full reader go unnoticed. This may arguably be a strength
> of the approach, though frequently a carefully crafted test case can
> show this as a bug. Essentially, though, some searches only work by
> accident.
>
> * Searching all expanded symtabs means that, as a debug session drags
> on, searches will examine many irrelevant symtabs.
>
> * In some places, the two phases use different code to perform the
> actual search, meaning that the results can depend on previous CU
> expansion decisions.
>
> * Similarly, if just a single symbol is needed, then both approaches
> are bad: expand-then-search will be unnecessarily inefficient, while
> search-then-expand approach means that the result depends on which
> CUs happen to have already been expanded.
>
> This series changes all of this. Now, all symbol lookups are done via
> the "quick" API, with the idea being that the final search of the
> expanded symtab is done via the expand_symtabs_matching callback.
>
> This fixes all the issues pointed out above:
>
> * Only relevant symtabs are searched, because the index only considers
> matching symbols.
>
> * Some discrepancies between the indexer and the full reader show up
> as symbol lookup failures. Others (if the indexer thinks a symbol
> exists but the full reader does not) will just be inefficient -- but
> we could add a verifier for this.
>
> * Lookups are no longer dependent on symtab expansion state, because
> again the indexer is pre-filtering the matches.
>
> Re-reading the series -- it's been in development for quite a while
> and I've already landed ~20 preliminary patches -- shows that there
> are still a few cleanups in here that could perhaps have gone in
> separately.
>
> Anyway, the essential change is to make the implementations of
> expand_symtabs_matching also call the callback when a symtab has
> already been expanded. After this, most of the work is cleaning up
> individual callers.
>
> This change lead to some surprising places. For example, I had to
> rewrite the .gdb_index reader, because the work done for
> "templates.exp" simply never worked in this mode -- and the test
> obscured the problems, a classic case of lookups working by accident.
>
> I regression tested each patch in this series. Furthermore I
> regression tested the series as a whole using the cc-with-debug-names
> and cc-with-gdb-index target boards. All of this was done on x86-64
> Fedora 40.
>
> I think maybe I've separately submitted "Ada import functions not in
> index" patch. Anyway it's part of this series now.
>
> Signed-off-by: Tom Tromey <tom@tromey.com>
> ---

I went over the series but not in depth. I like how it simplifies the
symbol lookup functions and how it makes things less error prone. You
can add:

Acked-By: Simon Marchi <simon.marchi@efficios.com>

Since it touches a lot of different code, I think you should merge it
soon, to avoid having to deal with more conflicts. I also have some
stuff that I will need rebase on top of this, so I would like to get to
it sooner than later.

Simon