Re: [PATCH] gdb: Restrict module drill-down searches to matched modules

[Andrew Burgess <aburgess@redhat.com>]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> search_module_symbols (used by -symbol-info-module-{functions,variables}
> and the 'info module' subcommands) performs a second global_symbol_searcher
> search for *all* functions or variables in the program with no module
> filter.  This expands every compunit, and only afterwards keeps the
> symbols whose qualified name starts with a matched module prefix.
>
> The modules are already known from the first search, so restrict the
> second one to the symbols qualified by one of them.  Their names are
> collected in a set, and the new global_symbol_searcher::set_scope_filter
> makes the search skip every name that is not prefixed by one of them.
> It is checked while expanding symtabs, so that the compunits of the
> other modules are not expanded at all, and while collecting symbols
> and minimal symbols.
>
> Handing the searcher a '^\(mod1\|mod2\|...\)::' regexp instead would
> restrict the search in the same way, but gdb matches regexps with
> libiberty's backtracking matcher, which tries every alternative for each
> name; with a few thousand modules that costs more than the compunit
> expansion it avoids.
>
> Also return early when no module matched, skipping the second search
> entirely.
>
> On a 2000-module synthetic Fortran benchmark, cold medians:
>
>   -symbol-info-module-functions --module <M>:  0.97 s -> 0.05 s
>   -symbol-info-module-variables --module <M>:  0.34 s -> 0.06 s
>
> with a regexp matching 100 of the modules:
>
>   -symbol-info-module-variables --module <RE>: 0.38 s -> 0.08 s
>
> and the searches that match every module are not penalised:
>
>   -symbol-info-module-variables:               1.36 s -> 1.28 s
>   -symbol-info-module-functions:               1.86 s -> 1.16 s
>   -symbol-info-module-variables --name grid_:  0.39 s -> 0.39 s
>
> Change-Id: I9d1554fce539f89b6d1f09802b6d24469bec49cf
> ---
>  gdb/symtab.c                                | 40 +++++++++++++++++++--
>  gdb/symtab.h                                | 22 ++++++++++--
>  gdb/testsuite/gdb.fortran/info-modules.exp  | 13 +++++++
>  gdb/testsuite/gdb.mi/mi-fortran-modules.exp | 13 +++++++
>  4 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index b082a488b88..9551f3cdc74 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4783,6 +4783,9 @@ global_symbol_searcher::symbol_matches
>    if (!name_matches (name_regex, sym->natural_name ()))
>      return false;
>  
> +  if (!scope_filter_matches (sym->natural_name ()))
> +    return false;
> +
>    switch (m_kind)
>      {
>      case symbol_search_kind::VARIABLE:
> @@ -4826,7 +4829,25 @@ global_symbol_searcher::msymbol_matches
>    if (!is_suitable_msymbol (msymbol))
>      return false;
>  
> -  return name_matches (name_regex, msymbol->natural_name ());
> +  if (!name_matches (name_regex, msymbol->natural_name ()))
> +    return false;
> +
> +  return scope_filter_matches (msymbol->natural_name ());
> +}
> +
> +/* See symtab.h.  */
> +
> +bool
> +global_symbol_searcher::scope_filter_matches (const char *name) const
> +{
> +  if (m_scope_filter == nullptr)
> +    return true;
> +
> +  const std::string_view full_name (name);
> +  const size_t sep = full_name.find ("::");
> +
> +  return (sep != std::string_view::npos
> +	  && m_scope_filter->contains (full_name.substr (0, sep)));
>  }

Unfortunately, I this doesn't work right now.  If you take your new
test and add a 'clean_restart' call before it, like this:

  clean_restart $testfile

  GDBInfoModuleSymbols::run_command "info module functions -m mod1 SUB_M1_A"
  GDBInfoModuleSymbols::check_header \
      "All functions matching regular expression \"SUB_M1_A\", in all modules matching regular expression \"mod1\":"
  GDBInfoModuleSymbols::check_entry "${srcfile}" "mod1" "25" \
      "void mod1::sub_m1_a\\(${integer4}\\);"
  GDBInfoModuleSymbols::check_no_entry "${srcfile}" ".*"
  GDBInfoModuleSymbols::check_no_entry "${srcfile2}" ".*"

Then run, you'll find that you no longer list the expected functions.
The old code would though.

The problem is that the NAME passed to scoped_filter_matches is only
qualified once the containing symtab has been expanded.  Before then it
is not qualified, and the `full_name.find ("::")` will not find the ::
string and so every function symbol will be rejected.

I tracked the bug down, but I've run out of time right now to think
about what the solution is.  But you should for sure update your tests
to cover this case.

>  
>  /* See symtab.h.  */
> @@ -4851,7 +4872,8 @@ global_symbol_searcher::expand_symtabs
>       &lookup_name_info::match_any (),
>       [&] (const char *symname)
>       {
> -       return name_matches (name_regex, symname);
> +       return (name_matches (name_regex, symname)
> +	       && scope_filter_matches (symname));
>       },
>       NULL,
>       SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
> @@ -6723,12 +6745,24 @@ search_module_symbols (const char *module_regexp, const char *regexp,
>    spec1.set_exclude_minsyms (true);
>    std::vector<symbol_search> modules = spec1.search ();
>  
> +  if (modules.empty ())
> +    return results;
> +
>    /* Now search for all symbols of the required KIND matching the required
>       regular expressions.  We figure out which ones are in which modules
> -     below.  */
> +     below.
> +
> +     Only symbols of the modules found above can be part of the result,
> +     so restrict the search to them.  This avoids expanding the symtabs of
> +     every other module.  */
> +  gdb::unordered_set<std::string_view> module_names;
> +  for (const symbol_search &p : modules)
> +    module_names.insert (p.symbol->print_name ());

I think this use of `print_name` is wrong.  I think `natural_name` would
be the correct choice.  The comment on `print_name` says:

  /* Returns a version of the name of a symbol that is
     suitable for output.  In C++ this is the "demangled" form of the
     name if demangle is on and the "mangled" form of the name if
     demangle is off.  In other languages this is just the symbol name.
     The result should never be NULL.  Don't use this for internal
     purposes (e.g. storing in a hashtable): it's only suitable for output.  */

It specifically calls out this exact use case as a place where
print_name should not be used.

You should be able to test this change by setting 'set print demangle
off' and running your existing module lookup tests again, but that still
doesn't work as there are two additional uses of `print_name` later in
this function that cause this to break.  It would be good if you could
create a new commit, before this one, that fixes the existing bug and
adds a suitable test.

Then this commit would come after adding the new filter changes.

Thanks,
Andrew