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

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

[Adhemerval Zanella]

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)));
 }
 
 /* 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 ());
+
   global_symbol_searcher spec2 (kind, regexp);
   spec2.set_symbol_type_regexp (type_regexp);
   spec2.set_exclude_minsyms (true);
+  spec2.set_scope_filter (&module_names);
   std::vector<symbol_search> symbols = spec2.search ();
 
   /* Now iterate over all MODULES, checking to see which items from
diff --git a/gdb/symtab.h b/gdb/symtab.h
index bd719af6669..0bce7c6a3a2 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -30,6 +30,7 @@
 #include "gdbsupport/gdb_regex.h"
 #include "gdbsupport/enum-flags.h"
 #include "gdbsupport/function-view.h"
+#include "gdbsupport/unordered_set.h"
 #include "gdbsupport/iteration-status.h"
 #include <optional>
 #include <string_view>
@@ -2645,6 +2646,14 @@ class global_symbol_searcher
     m_max_search_results = max_search_results;
   }
 
+  /* Only consider symbols whose name is qualified by one of SCOPES, that
+     is, starts with one of them followed by "::".  SCOPES must outlive
+     the search.  */
+  void set_scope_filter (const gdb::unordered_set<std::string_view> *scopes)
+  {
+    m_scope_filter = scopes;
+  }
+
   /* Search the symbols from all objfiles in the current program space
      looking for matches as defined by the current state of this object.
 
@@ -2678,6 +2687,13 @@ class global_symbol_searcher
      of SIZE_MAX, there is no "unlimited".  */
   size_t m_max_search_results = SIZE_MAX;
 
+  /* If not nullptr, the scopes symbol names must be qualified by, see
+     set_scope_filter.  */
+  const gdb::unordered_set<std::string_view> *m_scope_filter = nullptr;
+
+  /* Return true if NAME passes M_SCOPE_FILTER.  */
+  bool scope_filter_matches (const char *name) const;
+
   /* Compile M_SYMBOL_NAME_REGEXP, if set.
 
      This function normalizes the spacing when it detects a C++ operator in the
@@ -2691,12 +2707,14 @@ class global_symbol_searcher
   bool symtab_matches_filenames (symtab *symtab) const;
 
   /* Return true if SYM is of kind M_KIND, is in a file matching
-     M_FILENAMES, and matches NAME_REGEX and TYPE_REGEX.  */
+     M_FILENAMES, passes M_SCOPE_FILTER, and matches NAME_REGEX and
+     TYPE_REGEX.  */
   bool symbol_matches (const symbol *sym,
 		       const std::optional<compiled_regex> &name_regex,
 		       const std::optional<compiled_regex> &type_regex) const;
 
-  /* Return true if MSYMBOL is of kind M_KIND and matches NAME_REGEX.
+  /* Return true if MSYMBOL is of kind M_KIND, passes M_SCOPE_FILTER, and
+     matches NAME_REGEX.
 
      This function also filters out the minimal symbols artificially created by
      GDB.  */
diff --git a/gdb/testsuite/gdb.fortran/info-modules.exp b/gdb/testsuite/gdb.fortran/info-modules.exp
index f94df7f1655..af31cf7c799 100644
--- a/gdb/testsuite/gdb.fortran/info-modules.exp
+++ b/gdb/testsuite/gdb.fortran/info-modules.exp
@@ -160,3 +160,16 @@ GDBInfoModuleSymbols::check_entry "${srcfile}" "mod1" "21" \
     "${real4} mod1::mod1_var_1;"
 GDBInfoModuleSymbols::check_no_entry "${srcfile}" ".*"
 GDBInfoModuleSymbols::check_no_entry "${srcfile2}" ".*"
+
+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}" ".*"
+
+gdb_test "info module functions -m nomatch" \
+    "^All functions in all modules matching regular expression \"nomatch\":"
+gdb_test "info module variables -m nomatch" \
+    "^All variables in all modules matching regular expression \"nomatch\":"
diff --git a/gdb/testsuite/gdb.mi/mi-fortran-modules.exp b/gdb/testsuite/gdb.mi/mi-fortran-modules.exp
index 51e810c9271..69f9124a61f 100644
--- a/gdb/testsuite/gdb.mi/mi-fortran-modules.exp
+++ b/gdb/testsuite/gdb.mi/mi-fortran-modules.exp
@@ -158,3 +158,16 @@ gdb_test_multiple $cmd $test -prompt $mi_gdb_prompt$ {
 	}
     }
 }
+
+# Check that a --module regexp matching no modules gives an empty
+# result, and no error.
+mi_gdb_test "108-symbol-info-module-functions --module nomatch" \
+    "108\\^done,symbols=\\\[\\\]" \
+    "-symbol-info-module-functions --module nomatch"
+
+# Check that it still honours Fortran's case insensitivity.
+set mod1_check_all_re \
+    "\{module=\"mod1\",files=\\\[\{filename=\"\[^\"\]+$srcfile2\",fullname=\"\[^\"\]+$srcfile2\",symbols=\\\[\{line=\"21\",name=\"mod1::check_all\",type=\"void \\(void\\)\",description=\"void mod1::check_all\\(void\\);\"\}\\\]\}\\\]\}"
+mi_gdb_test "109-symbol-info-module-functions --module mod1 --name CHECK_ALL" \
+    "109\\^done,symbols=\\\[${mod1_check_all_re}(,${mod1_check_all_re})*\\\]" \
+    "-symbol-info-module-functions --module mod1 --name CHECK_ALL"
-- 
2.53.0

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

[Andrew Burgess]

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