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

[Guinevere Larsen <guinevere@redhat.com>]

Hi Tromey,

I am having an issue with the quick_fns interface, and while it isn't 
affected by this series directly (as I originally thought it could be), 
I would appreciate if you could share your expertise on this area. The 
TL;DR of the issue is: the quick API seems to be caching something and 
searching objfile B for a symbol can return a symbol with details from 
objfile A (that was loaded first). I tried looking around for where the 
cache could be hidden, but couldn't find it. Do you know if where the 
cache is, if it really exists?

Some more context: Now that we have a way to get all solibs (and thus 
objfiles) that belong to one namespace, so I'm trying to implement 
search for a symbol inside a specific namespace. To do this, I iterate 
through objfiles in the selected namespace and use 
lookup_symbol_in_objfile on all of them. The first issue I ran into was 
the gdb_bfd_cache making the second objfile's symbols not be read, which 
I already managed to fix. I know we have correct reading of symbols and 
addresses if I follow the dwarf reading, so if symbol lookup was 
correct, everything should be working.

Yet, the symbols found through the quick_fns all seem to point to the 
same inferior address (I'm unsure if they are different symbols, or the 
same symbol returned through dynamic allocation so pointing to different 
addresses), meaning that we still effectively only find the first one 
and return it. I can verify it by either taking the address directly, or 
setting the variable in one namespace and reading it in another.

If you'd like to play around with my changes, I pushed them to the 
users/guinevere/symbol_in_linker_namespace branch, and you can use the 
test gdb.base/dlmopen-ns-ids to search for the variable gdb_dlmopen_glob 
in different namespaces. (gdb.base/dlmopen.exp may not work because I 
haven't looked into LOC_UNRESOLVED variables yet, only LOC_STATIC).

-- 
Cheers,
Guinevere Larsen
She/Her/Hers


On 4/2/25 8:44 PM, 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>
> ---
> Changes in v2:
> - Rebased for all the other recent DWARF changes
> - Link to v1: https://inbox.sourceware.org/gdb-patches/20250311-search-in-psyms-v1-0-d73d9be20983@tromey.com
>
> ---
> Tom Tromey (28):
>        Add another minor hack to cooked_index_entry::full_name
>        Change ada_decode to preserve upper-case in some situations
>        Emit some type declarations in .gdb_index
>        Ada import functions not in index
>        Fix index's handling of DW_TAG_imported_declaration
>        Put all CTF symbols in global scope
>        Restore "ingestion" of .debug_str when writing .debug_names
>        Entries from anon-struct.exp not in cooked index
>        Remove dwarf2_per_cu_data::mark
>        Have expand_symtabs_matching work for already-expanded CUs
>        Rewrite the .gdb_index reader
>        Convert default_collect_symbol_completion_matches_break_on
>        Convert gdbpy_lookup_static_symbols
>        Convert ada_add_global_exceptions
>        Convert ada_language_defn::collect_symbol_completion_matches
>        Convert ada-lang.c:map_matching_symbols
>        Remove expand_symtabs_matching
>        Simplify basic_lookup_transparent_type
>        Remove objfile::expand_symtabs_for_function
>        Convert linespec.c:iterate_over_all_matching_symtabs
>        Simplify block_lookup_symbol_primary
>        Pass lookup_name_info to block_lookup_symbol_primary
>        Simplify block_lookup_symbol
>        Add best_symbol_tracker
>        Convert lookup_symbol_via_quick_fns
>        Convert lookup_symbol_in_objfile
>        Make dw_expand_symtabs_matching_file_matcher static
>        Remove enter_symbol_lookup
>
>   gdb/ada-lang.c                            |  221 ++---
>   gdb/ada-lang.h                            |   15 +-
>   gdb/block.c                               |   57 +-
>   gdb/block.h                               |   27 +-
>   gdb/cp-support.c                          |   28 +-
>   gdb/ctfread.c                             |    6 +-
>   gdb/dwarf2/abbrev.c                       |    7 +-
>   gdb/dwarf2/abbrev.h                       |    8 +
>   gdb/dwarf2/cooked-index-entry.c           |    7 +
>   gdb/dwarf2/cooked-index-shard.c           |   13 +-
>   gdb/dwarf2/cooked-index-shard.h           |    7 +
>   gdb/dwarf2/cooked-index-worker.h          |   17 +-
>   gdb/dwarf2/cooked-indexer.c               |   37 +-
>   gdb/dwarf2/index-write.c                  |  103 ++-
>   gdb/dwarf2/read-gdb-index.c               | 1268 ++++++-----------------------
>   gdb/dwarf2/read-gdb-index.h               |   14 +
>   gdb/dwarf2/read.c                         |  176 ++--
>   gdb/dwarf2/read.h                         |   48 +-
>   gdb/dwarf2/tag.h                          |   12 +
>   gdb/linespec.c                            |   17 +-
>   gdb/objfiles.h                            |    4 -
>   gdb/psymtab.c                             |    3 -
>   gdb/python/py-symbol.c                    |   29 +-
>   gdb/symfile-debug.c                       |   22 -
>   gdb/symfile.c                             |   25 -
>   gdb/symfile.h                             |    9 -
>   gdb/symtab.c                              |  223 ++---
>   gdb/symtab.h                              |    2 +-
>   gdb/testsuite/gdb.ada/import.exp          |   28 +-
>   gdb/testsuite/gdb.ctf/cross-tu-cyclic.exp |    4 +-
>   30 files changed, 861 insertions(+), 1576 deletions(-)
> ---
> base-commit: 60ac9c60fe5b6dc5a59a30a971c3fad020fecf45
> change-id: 20250311-search-in-psyms-3eb1049177e0
>
> Best regards,