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

[Simon Marchi <simark@simark.ca>]

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