[PATCH 0/9] Cleanups in global_symbol_searcher

[PATCH 0/9] Cleanups in global_symbol_searcher

[Simon Marchi]

I had a look at global_symbol_searcher while reviewing a recent patch,
and I had a bit of trouble understanding how it worked.  I made a few
changes to try to make it clearer.

Simon Marchi (9):
  gdb: add symbol_search_kind enum for global_symbol_searcher
  gdb: factor out regexp compilation from global_symbol_searcher::search
  gdb: simplify insertion in
    global_symbol_searcher::add_matching_symbols
  gdb: use a switch on m_kind in add_matching_symbols
  gdb: make global_symbol_searcher::is_suitable_msymbol non-static
  gdb: update comment of global_symbol_searcher::expand_symtabs
  gdb: add matching helpers to global_symbol_searcher
  gdb: pass result containers by reference in global_symbol_searcher
  gdb: fix some comments in global_symbol_searcher

 gdb/mi/mi-symbol-cmds.c |  36 ++--
 gdb/python/python.c     |   2 +-
 gdb/symtab.c            | 458 +++++++++++++++++++++++-----------------
 gdb/symtab.h            | 105 +++++----
 4 files changed, 348 insertions(+), 253 deletions(-)


base-commit: ccf0f68e72ea0a5628097b980a04e1a8262781df
-- 
2.55.0

[PATCH 1/9] gdb: add symbol_search_kind enum for global_symbol_searcher

[Simon Marchi]

While reading the global_symbol_searcher code, something that was not
clear to me was that despite accepting a domain_search_flags,  searches
are mutually exclusive between types, variables and functions.

One snippet of code that really confused me was (now removed by a
previous commit):

		  if ((kind & SEARCH_FUNCTION_DOMAIN) != 0
		      ? (find_compunit_symtab_for_pc
			 (msymbol->value_address (objfile)) == NULL)
		      : (lookup_symbol_in_objfile_from_linkage_name
			 (objfile, msymbol->linkage_name (),
			  SEARCH_VFT)
			 .symbol == NULL))

I kept thinking: what if you search for both functions and variables,
this will take the "function" path, this can't work.  The answer is that
it's not supposed to happen.

This patch adds the symbol_search_kind enum and uses it in
global_symbol_searcher (and a few caller functions), making it clear
that you search just one kind at a time.

The new enum has a certain overlap with enum domain_search_flag, but I
think that having the more scoped enum makes global_symbol_searcher
easier to understand, by specifying clearly what it needs to handle.

Change-Id: I4a3f4752c7be51a4da5831f9780d21e6e7271b38
---
 gdb/mi/mi-symbol-cmds.c |  36 +++++++------
 gdb/python/python.c     |   2 +-
 gdb/symtab.c            | 112 ++++++++++++++++++++++------------------
 gdb/symtab.h            |  33 ++++++++----
 4 files changed, 106 insertions(+), 77 deletions(-)

diff --git a/gdb/mi/mi-symbol-cmds.c b/gdb/mi/mi-symbol-cmds.c
index 58f2feef5533..30b8ec495e8f 100644
--- a/gdb/mi/mi-symbol-cmds.c
+++ b/gdb/mi/mi-symbol-cmds.c
@@ -71,7 +71,7 @@ mi_cmd_symbol_list_lines (const char *command, const char *const *argv,
    results.  */
 
 static void
-output_debug_symbol (ui_out *uiout, domain_search_flags kind,
+output_debug_symbol (ui_out *uiout, symbol_search_kind kind,
 		     struct symbol *sym, int block)
 {
   ui_out_emit_tuple tuple_emitter (uiout);
@@ -80,7 +80,8 @@ output_debug_symbol (ui_out *uiout, domain_search_flags kind,
     uiout->field_unsigned ("line", sym->line ());
   uiout->field_string ("name", sym->print_name ());
 
-  if ((kind & (SEARCH_FUNCTION_DOMAIN | SEARCH_VAR_DOMAIN)) != 0)
+  if (kind == symbol_search_kind::FUNCTION
+      || kind == symbol_search_kind::VARIABLE)
     {
       string_file tmp_stream;
       type_print (sym->type (), "", &tmp_stream, -1);
@@ -111,7 +112,7 @@ output_nondebug_symbol (ui_out *uiout, const bound_minimal_symbol &msymbol)
    and then prints the matching [m]symbols in an MI structured format.  */
 
 static void
-mi_symbol_info (domain_search_flags kind, const char *name_regexp,
+mi_symbol_info (symbol_search_kind kind, const char *name_regexp,
 		const char *type_regexp, bool exclude_minsyms,
 		size_t max_results)
 {
@@ -188,7 +189,7 @@ parse_max_results_option (const char *arg)
    Processes command line options from ARGV and ARGC.  */
 
 static void
-mi_info_functions_or_variables (domain_search_flags kind,
+mi_info_functions_or_variables (symbol_search_kind kind,
 				const char *const *argv, int argc)
 {
   size_t max_results = SIZE_MAX;
@@ -215,7 +216,7 @@ mi_info_functions_or_variables (domain_search_flags kind,
   while (1)
     {
       const char *cmd_string
-	= ((kind == SEARCH_FUNCTION_DOMAIN)
+	= ((kind == symbol_search_kind::FUNCTION)
 	   ? "-symbol-info-functions" : "-symbol-info-variables");
       int opt = mi_getopt (cmd_string, argc, argv, opts, &oind, &oarg);
       if (opt < 0)
@@ -251,7 +252,7 @@ using module_symbol_search_iterator
 static module_symbol_search_iterator
 output_module_symbols_in_single_module_and_file
 	(struct ui_out *uiout, module_symbol_search_iterator iter,
-	 const module_symbol_search_iterator end, domain_search_flags kind)
+	 const module_symbol_search_iterator end, symbol_search_kind kind)
 {
   /* The symbol for the module in which the first result resides.  */
   const symbol *first_module_symbol = iter->first.symbol;
@@ -286,7 +287,7 @@ output_module_symbols_in_single_module_and_file
 static module_symbol_search_iterator
 output_module_symbols_in_single_module
 	(struct ui_out *uiout, module_symbol_search_iterator iter,
-	 const module_symbol_search_iterator end, domain_search_flags kind)
+	 const module_symbol_search_iterator end, symbol_search_kind kind)
 {
   gdb_assert (iter->first.symbol != nullptr);
   gdb_assert (iter->second.symbol != nullptr);
@@ -314,7 +315,7 @@ output_module_symbols_in_single_module
    command line options passed to the MI command.  */
 
 static void
-mi_info_module_functions_or_variables (domain_search_flags kind,
+mi_info_module_functions_or_variables (symbol_search_kind kind,
 				       const char *const *argv, int argc)
 {
   const char *module_regexp = nullptr;
@@ -341,7 +342,7 @@ mi_info_module_functions_or_variables (domain_search_flags kind,
   while (1)
     {
       const char *cmd_string
-	= ((kind == SEARCH_FUNCTION_DOMAIN)
+	= ((kind == symbol_search_kind::FUNCTION)
 	   ? "-symbol-info-module-functions"
 	   : "-symbol-info-module-variables");
       int opt = mi_getopt (cmd_string, argc, argv, opts, &oind, &oarg);
@@ -383,7 +384,7 @@ void
 mi_cmd_symbol_info_functions (const char *command, const char *const *argv,
 			      int argc)
 {
-  mi_info_functions_or_variables (SEARCH_FUNCTION_DOMAIN, argv, argc);
+  mi_info_functions_or_variables (symbol_search_kind::FUNCTION, argv, argc);
 }
 
 /* Implement -symbol-info-module-functions command.  */
@@ -392,7 +393,8 @@ void
 mi_cmd_symbol_info_module_functions (const char *command,
 				     const char *const *argv, int argc)
 {
-  mi_info_module_functions_or_variables (SEARCH_FUNCTION_DOMAIN, argv, argc);
+  mi_info_module_functions_or_variables (symbol_search_kind::FUNCTION,
+					 argv, argc);
 }
 
 /* Implement -symbol-info-module-variables command.  */
@@ -401,7 +403,8 @@ void
 mi_cmd_symbol_info_module_variables (const char *command,
 				     const char *const *argv, int argc)
 {
-  mi_info_module_functions_or_variables (SEARCH_VAR_DOMAIN, argv, argc);
+  mi_info_module_functions_or_variables (symbol_search_kind::VARIABLE,
+					 argv, argc);
 }
 
 /* Implement -symbol-inf-modules command.  */
@@ -444,7 +447,8 @@ mi_cmd_symbol_info_modules (const char *command, const char *const *argv,
 	}
     }
 
-  mi_symbol_info (SEARCH_MODULE_DOMAIN, regexp, nullptr, true, max_results);
+  mi_symbol_info (symbol_search_kind::MODULE, regexp, nullptr, true,
+		  max_results);
 }
 
 /* Implement -symbol-info-types command.  */
@@ -487,8 +491,8 @@ mi_cmd_symbol_info_types (const char *command, const char *const *argv,
 	}
     }
 
-  mi_symbol_info (SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN, regexp, nullptr,
-		  true, max_results);
+  mi_symbol_info (symbol_search_kind::TYPE, regexp, nullptr, true,
+		  max_results);
 }
 
 /* Implement -symbol-info-variables command.  */
@@ -497,5 +501,5 @@ void
 mi_cmd_symbol_info_variables (const char *command, const char *const *argv,
 			      int argc)
 {
-  mi_info_functions_or_variables (SEARCH_VAR_DOMAIN, argv, argc);
+  mi_info_functions_or_variables (symbol_search_kind::VARIABLE, argv, argc);
 }
diff --git a/gdb/python/python.c b/gdb/python/python.c
index d51cd83966be..467317b7996e 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -848,7 +848,7 @@ gdbpy_rbreak (PyObject *self, PyObject *args, PyObject *kw)
       minsyms_p = cmp;
     }
 
-  global_symbol_searcher spec (SEARCH_FUNCTION_DOMAIN, regex);
+  global_symbol_searcher spec (symbol_search_kind::FUNCTION, regex);
 
   /* The "symtabs" keyword is any Python iterable object that returns
      a gdb.Symtab on each iteration.  If specified, iterate through
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 27070f74f83e..708df078e2ea 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4698,11 +4698,31 @@ treg_matches_sym_type_name (const compiled_regex &treg,
   return treg.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
 }
 
+/* Return the domain search flags matching symbols of kind KIND.  */
+
+static domain_search_flags
+to_search_flags (symbol_search_kind kind)
+{
+  switch (kind)
+    {
+    case symbol_search_kind::VARIABLE:
+      return SEARCH_VAR_DOMAIN;
+    case symbol_search_kind::FUNCTION:
+      return SEARCH_FUNCTION_DOMAIN;
+    case symbol_search_kind::TYPE:
+      return SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN;
+    case symbol_search_kind::MODULE:
+      return SEARCH_MODULE_DOMAIN;
+    }
+
+  gdb_assert_not_reached ("invalid symbol_search_kind");
+}
+
 /* See symtab.h.  */
 
 bool
 global_symbol_searcher::is_suitable_msymbol
-	(const domain_search_flags kind, const minimal_symbol *msymbol)
+	(symbol_search_kind kind, const minimal_symbol *msymbol)
 {
   switch (msymbol->type ())
     {
@@ -4710,12 +4730,12 @@ global_symbol_searcher::is_suitable_msymbol
     case mst_bss:
     case mst_file_data:
     case mst_file_bss:
-      return (kind & SEARCH_VAR_DOMAIN) != 0;
+      return kind == symbol_search_kind::VARIABLE;
     case mst_text:
     case mst_file_text:
     case mst_solib_trampoline:
     case mst_text_gnu_ifunc:
-      return (kind & SEARCH_FUNCTION_DOMAIN) != 0;
+      return kind == symbol_search_kind::FUNCTION;
     default:
       return false;
     }
@@ -4727,7 +4747,6 @@ bool
 global_symbol_searcher::expand_symtabs
 	(objfile *objfile, const std::optional<compiled_regex> &preg) const
 {
-  domain_search_flags kind = m_kind;
   bool found_func_msymbol_without_debug_info = false;
 
   auto do_file_match = [&] (const char *filename, bool basenames)
@@ -4748,7 +4767,7 @@ global_symbol_searcher::expand_symtabs
      },
      NULL,
      SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
-     kind);
+     to_search_flags (m_kind));
 
   /* Here, we search through the minimal symbol tables for functions that
      match, and force their symbols to be read.  The symbol will then be found
@@ -4764,7 +4783,7 @@ global_symbol_searcher::expand_symtabs
      symtab.  When no file names were given the caller unconditionally rescans
      the minimal symbols for SEARCH_VAR_DOMAIN.  */
   if (m_filenames.empty ()
-      && (kind & SEARCH_FUNCTION_DOMAIN) != 0)
+      && m_kind == symbol_search_kind::FUNCTION)
     {
       for (minimal_symbol *msymbol : objfile->msymbols ())
 	{
@@ -4773,7 +4792,7 @@ global_symbol_searcher::expand_symtabs
 	  if (msymbol->created_by_gdb)
 	    continue;
 
-	  if (is_suitable_msymbol (kind, msymbol))
+	  if (is_suitable_msymbol (m_kind, msymbol))
 	    {
 	      if (!preg.has_value ()
 		  || preg->exec (msymbol->natural_name (), 0,
@@ -4804,7 +4823,7 @@ global_symbol_searcher::add_matching_symbols
 	 const std::optional<compiled_regex> &treg,
 	 std::set<symbol_search> *result_set) const
 {
-  domain_search_flags kind = m_kind;
+  domain_search_flags domain = to_search_flags (m_kind);
 
   /* Add matching symbols (if not already present).  */
   for (compunit_symtab &cust : objfile->compunits ())
@@ -4832,7 +4851,7 @@ global_symbol_searcher::add_matching_symbols
 					 m_filenames, false))))
 		continue;
 
-	      if (!sym->matches (kind))
+	      if (!sym->matches (domain))
 		continue;
 
 	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
@@ -4845,7 +4864,7 @@ global_symbol_searcher::add_matching_symbols
 		   && !treg_matches_sym_type_name (*treg, sym)))
 		continue;
 
-	      if ((kind & SEARCH_VAR_DOMAIN) != 0)
+	      if (m_kind == symbol_search_kind::VARIABLE)
 		{
 		  if (sym->loc_class () == LOC_UNRESOLVED
 		      /* LOC_CONST can be used for more than
@@ -4882,8 +4901,6 @@ global_symbol_searcher::add_matching_msymbols
 	(objfile *objfile, const std::optional<compiled_regex> &preg,
 	 std::vector<symbol_search> *results) const
 {
-  domain_search_flags kind = m_kind;
-
   for (minimal_symbol *msymbol : objfile->msymbols ())
     {
       QUIT;
@@ -4891,7 +4908,7 @@ global_symbol_searcher::add_matching_msymbols
       if (msymbol->created_by_gdb)
 	continue;
 
-      if (is_suitable_msymbol (kind, msymbol))
+      if (is_suitable_msymbol (m_kind, msymbol))
 	{
 	  if (!preg.has_value ()
 	      || preg->exec (msymbol->natural_name (), 0,
@@ -4899,7 +4916,7 @@ global_symbol_searcher::add_matching_msymbols
 	    {
 	      /* For functions we can do a quick check of whether the
 		 symbol might be found via find_pc_symtab.  */
-	      if ((kind & SEARCH_FUNCTION_DOMAIN) == 0
+	      if (m_kind != symbol_search_kind::FUNCTION
 		  || (find_compunit_symtab_for_pc
 		      (msymbol->value_address (objfile)) == NULL))
 		{
@@ -5006,12 +5023,12 @@ global_symbol_searcher::search () const
      minimal symbol, as we assume that a minimal symbol does not have a
      type.  */
   if ((found_func_msymbol_without_debug_info
-       || (m_filenames.empty () && (m_kind & SEARCH_VAR_DOMAIN) != 0))
+       || (m_filenames.empty () && m_kind == symbol_search_kind::VARIABLE))
       && !m_exclude_minsyms
       && !treg.has_value ())
     {
-      gdb_assert ((m_kind & (SEARCH_VAR_DOMAIN | SEARCH_FUNCTION_DOMAIN))
-		  != 0);
+      gdb_assert (m_kind == symbol_search_kind::VARIABLE
+		  || m_kind == symbol_search_kind::FUNCTION);
       for (objfile &objfile : current_program_space->objfiles ())
 	if (!add_matching_msymbols (&objfile, preg, &result))
 	  break;
@@ -5143,7 +5160,7 @@ print_msymbol_info (bound_minimal_symbol msymbol)
 
 static void
 symtab_symbol_info (bool quiet, bool exclude_minsyms,
-		    const char *regexp, domain_enum kind,
+		    const char *regexp, symbol_search_kind kind,
 		    const char *t_regexp, int from_tty)
 {
   const char *last_filename = "";
@@ -5152,11 +5169,7 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
   if (regexp != nullptr && *regexp == '\0')
     regexp = nullptr;
 
-  domain_search_flags flags = to_search_flags (kind);
-  if (kind == TYPE_DOMAIN)
-    flags |= SEARCH_STRUCT_DOMAIN;
-
-  global_symbol_searcher spec (flags, regexp);
+  global_symbol_searcher spec (kind, regexp);
   spec.set_symbol_type_regexp (t_regexp);
   spec.set_exclude_minsyms (exclude_minsyms);
   std::vector<symbol_search> symbols = spec.search ();
@@ -5166,20 +5179,20 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
       const char *classname;
       switch (kind)
 	{
-	case VAR_DOMAIN:
+	case symbol_search_kind::VARIABLE:
 	  classname = "variable";
 	  break;
-	case FUNCTION_DOMAIN:
+	case symbol_search_kind::FUNCTION:
 	  classname = "function";
 	  break;
-	case TYPE_DOMAIN:
+	case symbol_search_kind::TYPE:
 	  classname = "type";
 	  break;
-	case MODULE_DOMAIN:
+	case symbol_search_kind::MODULE:
 	  classname = "module";
 	  break;
 	default:
-	  gdb_assert_not_reached ("invalid domain enum");
+	  gdb_assert_not_reached ("invalid symbol_search_kind");
 	}
 
       if (regexp != NULL)
@@ -5304,7 +5317,7 @@ info_variables_command (const char *args, int from_tty)
     args = nullptr;
 
   symtab_symbol_info
-    (opts.quiet, opts.exclude_minsyms, args, VAR_DOMAIN,
+    (opts.quiet, opts.exclude_minsyms, args, symbol_search_kind::VARIABLE,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
      from_tty);
 }
@@ -5323,7 +5336,7 @@ info_functions_command (const char *args, int from_tty)
     args = nullptr;
 
   symtab_symbol_info
-    (opts.quiet, opts.exclude_minsyms, args, FUNCTION_DOMAIN,
+    (opts.quiet, opts.exclude_minsyms, args, symbol_search_kind::FUNCTION,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
      from_tty);
 }
@@ -5366,8 +5379,8 @@ info_types_command (const char *args, int from_tty)
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
   if (args != nullptr && *args == '\0')
     args = nullptr;
-  symtab_symbol_info (opts.quiet, false, args, TYPE_DOMAIN, nullptr,
-		      from_tty);
+  symtab_symbol_info (opts.quiet, false, args, symbol_search_kind::TYPE,
+		      nullptr, from_tty);
 }
 
 /* Command completer for 'info types' command.  */
@@ -5399,8 +5412,8 @@ info_modules_command (const char *args, int from_tty)
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
   if (args != nullptr && *args == '\0')
     args = nullptr;
-  symtab_symbol_info (opts.quiet, true, args, MODULE_DOMAIN, nullptr,
-		      from_tty);
+  symtab_symbol_info (opts.quiet, true, args, symbol_search_kind::MODULE,
+		      nullptr, from_tty);
 }
 
 /* Implement the 'info main' command.  */
@@ -5441,7 +5454,7 @@ rbreak_command (const char *regexp, int from_tty)
      because the std::move nullifies file_name.  */
   bool file_name_p = file_name != nullptr;
 
-  global_symbol_searcher spec (SEARCH_FUNCTION_DOMAIN, regexp);
+  global_symbol_searcher spec (symbol_search_kind::FUNCTION, regexp);
   if (file_name_p)
     spec.add_filename (std::move (file_name));
   std::vector<symbol_search> symbols = spec.search ();
@@ -6654,12 +6667,12 @@ static struct cmd_list_element *info_module_cmdlist = NULL;
 
 std::vector<module_symbol_search>
 search_module_symbols (const char *module_regexp, const char *regexp,
-		       const char *type_regexp, domain_search_flags kind)
+		       const char *type_regexp, symbol_search_kind kind)
 {
   std::vector<module_symbol_search> results;
 
   /* Search for all modules matching MODULE_REGEXP.  */
-  global_symbol_searcher spec1 (SEARCH_MODULE_DOMAIN, module_regexp);
+  global_symbol_searcher spec1 (symbol_search_kind::MODULE, module_regexp);
   spec1.set_exclude_minsyms (true);
   std::vector<symbol_search> modules = spec1.search ();
 
@@ -6705,9 +6718,10 @@ search_module_symbols (const char *module_regexp, const char *regexp,
 static void
 info_module_subcommand (bool quiet, const char *module_regexp,
 			const char *regexp, const char *type_regexp,
-			domain_search_flags kind)
+			symbol_search_kind kind)
 {
-  gdb_assert (kind == SEARCH_FUNCTION_DOMAIN || kind == SEARCH_VAR_DOMAIN);
+  gdb_assert (kind == symbol_search_kind::FUNCTION
+	      || kind == symbol_search_kind::VARIABLE);
 
   /* Print a header line.  Don't build the header line bit by bit as this
      prevents internationalisation.  */
@@ -6718,12 +6732,12 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	  if (type_regexp == nullptr)
 	    {
 	      if (regexp == nullptr)
-		gdb_printf ((kind == SEARCH_VAR_DOMAIN
+		gdb_printf ((kind == symbol_search_kind::VARIABLE
 			     ? _("All variables in all modules:")
 			     : _("All functions in all modules:")));
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression"
 			" \"%s\" in all modules:")
 		    : _("All functions matching regular expression"
@@ -6734,7 +6748,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	    {
 	      if (regexp == nullptr)
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables with type matching regular "
 			"expression \"%s\" in all modules:")
 		    : _("All functions with type matching regular "
@@ -6742,7 +6756,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 		   type_regexp);
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression "
 			"\"%s\",\n\twith type matching regular "
 			"expression \"%s\" in all modules:")
@@ -6758,7 +6772,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	    {
 	      if (regexp == nullptr)
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables in all modules matching regular "
 			"expression \"%s\":")
 		    : _("All functions in all modules matching regular "
@@ -6766,7 +6780,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 		   module_regexp);
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression "
 			"\"%s\",\n\tin all modules matching regular "
 			"expression \"%s\":")
@@ -6779,7 +6793,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	    {
 	      if (regexp == nullptr)
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables with type matching regular "
 			"expression \"%s\"\n\tin all modules matching "
 			"regular expression \"%s\":")
@@ -6789,7 +6803,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 		   type_regexp, module_regexp);
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression "
 			"\"%s\",\n\twith type matching regular expression "
 			"\"%s\",\n\tin all modules matching regular "
@@ -6901,7 +6915,7 @@ info_module_functions_command (const char *args, int from_tty)
     (opts.quiet,
      opts.module_regexp.empty () ? nullptr : opts.module_regexp.c_str (), args,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
-     SEARCH_FUNCTION_DOMAIN);
+     symbol_search_kind::FUNCTION);
 }
 
 /* Implements the 'info module variables' command.  */
@@ -6920,7 +6934,7 @@ info_module_variables_command (const char *args, int from_tty)
     (opts.quiet,
      opts.module_regexp.empty () ? nullptr : opts.module_regexp.c_str (), args,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
-     SEARCH_VAR_DOMAIN);
+     symbol_search_kind::VARIABLE);
 }
 
 /* Command completer for 'info module ...' sub-commands.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 6443216fd7f0..a35449fbeeda 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2586,6 +2586,23 @@ struct symbol_search
 				  const symbol_search &sym_b);
 };
 
+/* The kind of symbols searched for by global_symbol_searcher.  */
+
+enum class symbol_search_kind
+{
+  /* Variables, excluding enum constants.  */
+  VARIABLE,
+
+  /* Functions.  */
+  FUNCTION,
+
+  /* Type names.  */
+  TYPE,
+
+  /* Fortran modules.  */
+  MODULE,
+};
+
 /* In order to search for global symbols of a particular kind matching
    particular regular expressions, create an instance of this structure and
    call the SEARCH member function.  */
@@ -2594,7 +2611,7 @@ class global_symbol_searcher
 public:
 
   /* Constructor.  */
-  global_symbol_searcher (domain_search_flags kind,
+  global_symbol_searcher (symbol_search_kind kind,
 			  const char *symbol_name_regexp)
     : m_kind (kind),
       m_symbol_name_regexp (symbol_name_regexp)
@@ -2635,14 +2652,8 @@ class global_symbol_searcher
   /* The set of source files to search in for matching symbols.  */
   std::vector<gdb::unique_xmalloc_ptr<char>> m_filenames;
 
-  /* The kind of symbols are we searching for.
-     VARIABLES_DOMAIN - Search all symbols, excluding functions, type
-			names, and constants (enums).
-     FUNCTIONS_DOMAIN - Search all functions..
-     TYPES_DOMAIN     - Search all type names.
-     MODULES_DOMAIN   - Search all Fortran modules.
-     ALL_DOMAIN       - Not valid for this function.  */
-  domain_search_flags m_kind;
+  /* The kind of symbols we are searching for.  */
+  symbol_search_kind m_kind;
 
   /* Regular expression to match against the symbol name.  */
   const char *m_symbol_name_regexp = nullptr;
@@ -2686,7 +2697,7 @@ class global_symbol_searcher
 			      std::vector<symbol_search> *results) const;
 
   /* Return true if MSYMBOL is of type KIND.  */
-  static bool is_suitable_msymbol (const domain_search_flags kind,
+  static bool is_suitable_msymbol (symbol_search_kind kind,
 				   const minimal_symbol *msymbol);
 };
 
@@ -2703,7 +2714,7 @@ using module_symbol_search = std::pair<symbol_search, symbol_search>;
    within the module.  */
 extern std::vector<module_symbol_search> search_module_symbols
 	(const char *module_regexp, const char *regexp,
-	 const char *type_regexp, domain_search_flags kind);
+	 const char *type_regexp, symbol_search_kind kind);
 
 /* Convert a global or static symbol SYM (based on BLOCK, which should be
    either GLOBAL_BLOCK or STATIC_BLOCK) into a string for use in 'info'
-- 
2.55.0

RE: [PATCH 1/9] gdb: add symbol_search_kind enum for global_symbol_searcher

[Rohr, Stephan]

Hi Simon,

see my feedback inlined below.

Thanks
Stephan

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 1/9] gdb: add symbol_search_kind enum for
> global_symbol_searcher
> 
> While reading the global_symbol_searcher code, something that was not
> clear to me was that despite accepting a domain_search_flags,  searches

Double spaces in ",  searches".

> are mutually exclusive between types, variables and functions.
> 
> One snippet of code that really confused me was (now removed by a
> previous commit):
> 

Should we mention the previous commit " gdb: Skip useless minsym scan in
variable-only symbol searches" explicitly? It may help future readers of the
patch.

> 		  if ((kind & SEARCH_FUNCTION_DOMAIN) != 0
> 		      ? (find_compunit_symtab_for_pc
> 			 (msymbol->value_address (objfile)) == NULL)
> 		      : (lookup_symbol_in_objfile_from_linkage_name
> 			 (objfile, msymbol->linkage_name (),
> 			  SEARCH_VFT)
> 			 .symbol == NULL))
> 
> I kept thinking: what if you search for both functions and variables,
> this will take the "function" path, this can't work.  The answer is that
> it's not supposed to happen.
> 
> This patch adds the symbol_search_kind enum and uses it in
> global_symbol_searcher (and a few caller functions), making it clear
> that you search just one kind at a time.
> 
> The new enum has a certain overlap with enum domain_search_flag, but I
> think that having the more scoped enum makes global_symbol_searcher
> easier to understand, by specifying clearly what it needs to handle.
> 
> Change-Id: I4a3f4752c7be51a4da5831f9780d21e6e7271b38
> ---
>  gdb/mi/mi-symbol-cmds.c |  36 +++++++------
>  gdb/python/python.c     |   2 +-
>  gdb/symtab.c            | 112 ++++++++++++++++++++++------------------
>  gdb/symtab.h            |  33 ++++++++----
>  4 files changed, 106 insertions(+), 77 deletions(-)
> 
> diff --git a/gdb/mi/mi-symbol-cmds.c b/gdb/mi/mi-symbol-cmds.c
> index 58f2feef5533..30b8ec495e8f 100644
> --- a/gdb/mi/mi-symbol-cmds.c
> +++ b/gdb/mi/mi-symbol-cmds.c
> @@ -71,7 +71,7 @@ mi_cmd_symbol_list_lines (const char *command, const
> char *const *argv,
>     results.  */
> 
>  static void
> -output_debug_symbol (ui_out *uiout, domain_search_flags kind,
> +output_debug_symbol (ui_out *uiout, symbol_search_kind kind,
>  		     struct symbol *sym, int block)
>  {
>    ui_out_emit_tuple tuple_emitter (uiout);
> @@ -80,7 +80,8 @@ output_debug_symbol (ui_out *uiout,
> domain_search_flags kind,
>      uiout->field_unsigned ("line", sym->line ());
>    uiout->field_string ("name", sym->print_name ());
> 
> -  if ((kind & (SEARCH_FUNCTION_DOMAIN | SEARCH_VAR_DOMAIN)) != 0)
> +  if (kind == symbol_search_kind::FUNCTION
> +      || kind == symbol_search_kind::VARIABLE)
>      {
>        string_file tmp_stream;
>        type_print (sym->type (), "", &tmp_stream, -1);
> @@ -111,7 +112,7 @@ output_nondebug_symbol (ui_out *uiout, const
> bound_minimal_symbol &msymbol)
>     and then prints the matching [m]symbols in an MI structured format.  */
> 
>  static void
> -mi_symbol_info (domain_search_flags kind, const char *name_regexp,
> +mi_symbol_info (symbol_search_kind kind, const char *name_regexp,
>  		const char *type_regexp, bool exclude_minsyms,
>  		size_t max_results)
>  {
> @@ -188,7 +189,7 @@ parse_max_results_option (const char *arg)
>     Processes command line options from ARGV and ARGC.  */
> 
>  static void
> -mi_info_functions_or_variables (domain_search_flags kind,
> +mi_info_functions_or_variables (symbol_search_kind kind,
>  				const char *const *argv, int argc)
>  {
>    size_t max_results = SIZE_MAX;
> @@ -215,7 +216,7 @@ mi_info_functions_or_variables
> (domain_search_flags kind,
>    while (1)
>      {
>        const char *cmd_string
> -	= ((kind == SEARCH_FUNCTION_DOMAIN)
> +	= ((kind == symbol_search_kind::FUNCTION)
>  	   ? "-symbol-info-functions" : "-symbol-info-variables");
>        int opt = mi_getopt (cmd_string, argc, argv, opts, &oind, &oarg);
>        if (opt < 0)
> @@ -251,7 +252,7 @@ using module_symbol_search_iterator
>  static module_symbol_search_iterator
>  output_module_symbols_in_single_module_and_file
>  	(struct ui_out *uiout, module_symbol_search_iterator iter,
> -	 const module_symbol_search_iterator end, domain_search_flags
> kind)
> +	 const module_symbol_search_iterator end, symbol_search_kind
> kind)
>  {
>    /* The symbol for the module in which the first result resides.  */
>    const symbol *first_module_symbol = iter->first.symbol;
> @@ -286,7 +287,7 @@ output_module_symbols_in_single_module_and_file
>  static module_symbol_search_iterator
>  output_module_symbols_in_single_module
>  	(struct ui_out *uiout, module_symbol_search_iterator iter,
> -	 const module_symbol_search_iterator end, domain_search_flags
> kind)
> +	 const module_symbol_search_iterator end, symbol_search_kind
> kind)
>  {
>    gdb_assert (iter->first.symbol != nullptr);
>    gdb_assert (iter->second.symbol != nullptr);
> @@ -314,7 +315,7 @@ output_module_symbols_in_single_module
>     command line options passed to the MI command.  */
> 
>  static void
> -mi_info_module_functions_or_variables (domain_search_flags kind,
> +mi_info_module_functions_or_variables (symbol_search_kind kind,
>  				       const char *const *argv, int argc)
>  {
>    const char *module_regexp = nullptr;
> @@ -341,7 +342,7 @@ mi_info_module_functions_or_variables
> (domain_search_flags kind,
>    while (1)
>      {
>        const char *cmd_string
> -	= ((kind == SEARCH_FUNCTION_DOMAIN)
> +	= ((kind == symbol_search_kind::FUNCTION)
>  	   ? "-symbol-info-module-functions"
>  	   : "-symbol-info-module-variables");
>        int opt = mi_getopt (cmd_string, argc, argv, opts, &oind, &oarg);
> @@ -383,7 +384,7 @@ void
>  mi_cmd_symbol_info_functions (const char *command, const char *const
> *argv,
>  			      int argc)
>  {
> -  mi_info_functions_or_variables (SEARCH_FUNCTION_DOMAIN, argv, argc);
> +  mi_info_functions_or_variables (symbol_search_kind::FUNCTION, argv,
> argc);
>  }
> 
>  /* Implement -symbol-info-module-functions command.  */
> @@ -392,7 +393,8 @@ void
>  mi_cmd_symbol_info_module_functions (const char *command,
>  				     const char *const *argv, int argc)
>  {
> -  mi_info_module_functions_or_variables (SEARCH_FUNCTION_DOMAIN,
> argv, argc);
> +  mi_info_module_functions_or_variables (symbol_search_kind::FUNCTION,
> +					 argv, argc);
>  }
> 
>  /* Implement -symbol-info-module-variables command.  */
> @@ -401,7 +403,8 @@ void
>  mi_cmd_symbol_info_module_variables (const char *command,
>  				     const char *const *argv, int argc)
>  {
> -  mi_info_module_functions_or_variables (SEARCH_VAR_DOMAIN, argv,
> argc);
> +  mi_info_module_functions_or_variables (symbol_search_kind::VARIABLE,
> +					 argv, argc);
>  }
> 
>  /* Implement -symbol-inf-modules command.  */
> @@ -444,7 +447,8 @@ mi_cmd_symbol_info_modules (const char
> *command, const char *const *argv,
>  	}
>      }
> 
> -  mi_symbol_info (SEARCH_MODULE_DOMAIN, regexp, nullptr, true,
> max_results);
> +  mi_symbol_info (symbol_search_kind::MODULE, regexp, nullptr, true,
> +		  max_results);
>  }
> 
>  /* Implement -symbol-info-types command.  */
> @@ -487,8 +491,8 @@ mi_cmd_symbol_info_types (const char *command,
> const char *const *argv,
>  	}
>      }
> 
> -  mi_symbol_info (SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN,
> regexp, nullptr,
> -		  true, max_results);
> +  mi_symbol_info (symbol_search_kind::TYPE, regexp, nullptr, true,
> +		  max_results);
>  }
> 
>  /* Implement -symbol-info-variables command.  */
> @@ -497,5 +501,5 @@ void
>  mi_cmd_symbol_info_variables (const char *command, const char *const
> *argv,
>  			      int argc)
>  {
> -  mi_info_functions_or_variables (SEARCH_VAR_DOMAIN, argv, argc);
> +  mi_info_functions_or_variables (symbol_search_kind::VARIABLE, argv,
> argc);
>  }
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index d51cd83966be..467317b7996e 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -848,7 +848,7 @@ gdbpy_rbreak (PyObject *self, PyObject *args,
> PyObject *kw)
>        minsyms_p = cmp;
>      }
> 
> -  global_symbol_searcher spec (SEARCH_FUNCTION_DOMAIN, regex);
> +  global_symbol_searcher spec (symbol_search_kind::FUNCTION, regex);
> 
>    /* The "symtabs" keyword is any Python iterable object that returns
>       a gdb.Symtab on each iteration.  If specified, iterate through
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 27070f74f83e..708df078e2ea 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4698,11 +4698,31 @@ treg_matches_sym_type_name (const
> compiled_regex &treg,
>    return treg.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
>  }
> 
> +/* Return the domain search flags matching symbols of kind KIND.  */
> +
> +static domain_search_flags
> +to_search_flags (symbol_search_kind kind)

Same name as " to_search_flags (domain_enum domain)" in symtab.h.
Not sure if this might be confusing.  I don't have a strong opinion here.

> +{
> +  switch (kind)
> +    {
> +    case symbol_search_kind::VARIABLE:
> +      return SEARCH_VAR_DOMAIN;
> +    case symbol_search_kind::FUNCTION:
> +      return SEARCH_FUNCTION_DOMAIN;
> +    case symbol_search_kind::TYPE:

Should we add a comment why the " SEARCH_STRUCT_DOMAIN" is
included?  I think this might be helpful for future readers, e.g.
"Some languages have a tag namespace.  Search the "STRUCT_DOMAIN", too."

> +      return SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN;
> +    case symbol_search_kind::MODULE:
> +      return SEARCH_MODULE_DOMAIN;
> +    }
> +
> +  gdb_assert_not_reached ("invalid symbol_search_kind");
> +}
> +
>  /* See symtab.h.  */
> 
>  bool
>  global_symbol_searcher::is_suitable_msymbol
> -	(const domain_search_flags kind, const minimal_symbol *msymbol)
> +	(symbol_search_kind kind, const minimal_symbol *msymbol)
>  {
>    switch (msymbol->type ())
>      {
> @@ -4710,12 +4730,12 @@ global_symbol_searcher::is_suitable_msymbol
>      case mst_bss:
>      case mst_file_data:
>      case mst_file_bss:
> -      return (kind & SEARCH_VAR_DOMAIN) != 0;
> +      return kind == symbol_search_kind::VARIABLE;
>      case mst_text:
>      case mst_file_text:
>      case mst_solib_trampoline:
>      case mst_text_gnu_ifunc:
> -      return (kind & SEARCH_FUNCTION_DOMAIN) != 0;
> +      return kind == symbol_search_kind::FUNCTION;
>      default:
>        return false;
>      }
> @@ -4727,7 +4747,6 @@ bool
>  global_symbol_searcher::expand_symtabs
>  	(objfile *objfile, const std::optional<compiled_regex> &preg) const
>  {
> -  domain_search_flags kind = m_kind;
>    bool found_func_msymbol_without_debug_info = false;
> 
>    auto do_file_match = [&] (const char *filename, bool basenames)
> @@ -4748,7 +4767,7 @@ global_symbol_searcher::expand_symtabs
>       },
>       NULL,
>       SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
> -     kind);
> +     to_search_flags (m_kind));
> 
>    /* Here, we search through the minimal symbol tables for functions that
>       match, and force their symbols to be read.  The symbol will then be found
> @@ -4764,7 +4783,7 @@ global_symbol_searcher::expand_symtabs
>       symtab.  When no file names were given the caller unconditionally rescans
>       the minimal symbols for SEARCH_VAR_DOMAIN.  */
>    if (m_filenames.empty ()
> -      && (kind & SEARCH_FUNCTION_DOMAIN) != 0)
> +      && m_kind == symbol_search_kind::FUNCTION)
>      {
>        for (minimal_symbol *msymbol : objfile->msymbols ())
>  	{
> @@ -4773,7 +4792,7 @@ global_symbol_searcher::expand_symtabs
>  	  if (msymbol->created_by_gdb)
>  	    continue;
> 
> -	  if (is_suitable_msymbol (kind, msymbol))
> +	  if (is_suitable_msymbol (m_kind, msymbol))
>  	    {
>  	      if (!preg.has_value ()
>  		  || preg->exec (msymbol->natural_name (), 0,
> @@ -4804,7 +4823,7 @@ global_symbol_searcher::add_matching_symbols
>  	 const std::optional<compiled_regex> &treg,
>  	 std::set<symbol_search> *result_set) const
>  {
> -  domain_search_flags kind = m_kind;
> +  domain_search_flags domain = to_search_flags (m_kind);
> 
>    /* Add matching symbols (if not already present).  */
>    for (compunit_symtab &cust : objfile->compunits ())
> @@ -4832,7 +4851,7 @@ global_symbol_searcher::add_matching_symbols
>  					 m_filenames, false))))
>  		continue;
> 
> -	      if (!sym->matches (kind))
> +	      if (!sym->matches (domain))
>  		continue;
> 
>  	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
> @@ -4845,7 +4864,7 @@ global_symbol_searcher::add_matching_symbols
>  		   && !treg_matches_sym_type_name (*treg, sym)))
>  		continue;
> 
> -	      if ((kind & SEARCH_VAR_DOMAIN) != 0)
> +	      if (m_kind == symbol_search_kind::VARIABLE)
>  		{
>  		  if (sym->loc_class () == LOC_UNRESOLVED
>  		      /* LOC_CONST can be used for more than
> @@ -4882,8 +4901,6 @@
> global_symbol_searcher::add_matching_msymbols
>  	(objfile *objfile, const std::optional<compiled_regex> &preg,
>  	 std::vector<symbol_search> *results) const
>  {
> -  domain_search_flags kind = m_kind;
> -
>    for (minimal_symbol *msymbol : objfile->msymbols ())
>      {
>        QUIT;
> @@ -4891,7 +4908,7 @@
> global_symbol_searcher::add_matching_msymbols
>        if (msymbol->created_by_gdb)
>  	continue;
> 
> -      if (is_suitable_msymbol (kind, msymbol))
> +      if (is_suitable_msymbol (m_kind, msymbol))
>  	{
>  	  if (!preg.has_value ()
>  	      || preg->exec (msymbol->natural_name (), 0,
> @@ -4899,7 +4916,7 @@
> global_symbol_searcher::add_matching_msymbols
>  	    {
>  	      /* For functions we can do a quick check of whether the
>  		 symbol might be found via find_pc_symtab.  */
> -	      if ((kind & SEARCH_FUNCTION_DOMAIN) == 0
> +	      if (m_kind != symbol_search_kind::FUNCTION
>  		  || (find_compunit_symtab_for_pc
>  		      (msymbol->value_address (objfile)) == NULL))
>  		{
> @@ -5006,12 +5023,12 @@ global_symbol_searcher::search () const
>       minimal symbol, as we assume that a minimal symbol does not have a
>       type.  */
>    if ((found_func_msymbol_without_debug_info
> -       || (m_filenames.empty () && (m_kind & SEARCH_VAR_DOMAIN) != 0))
> +       || (m_filenames.empty () && m_kind ==
> symbol_search_kind::VARIABLE))
>        && !m_exclude_minsyms
>        && !treg.has_value ())
>      {
> -      gdb_assert ((m_kind & (SEARCH_VAR_DOMAIN |
> SEARCH_FUNCTION_DOMAIN))
> -		  != 0);
> +      gdb_assert (m_kind == symbol_search_kind::VARIABLE
> +		  || m_kind == symbol_search_kind::FUNCTION);
>        for (objfile &objfile : current_program_space->objfiles ())
>  	if (!add_matching_msymbols (&objfile, preg, &result))
>  	  break;
> @@ -5143,7 +5160,7 @@ print_msymbol_info (bound_minimal_symbol
> msymbol)
> 
>  static void
>  symtab_symbol_info (bool quiet, bool exclude_minsyms,
> -		    const char *regexp, domain_enum kind,
> +		    const char *regexp, symbol_search_kind kind,
>  		    const char *t_regexp, int from_tty)
>  {
>    const char *last_filename = "";
> @@ -5152,11 +5169,7 @@ symtab_symbol_info (bool quiet, bool
> exclude_minsyms,
>    if (regexp != nullptr && *regexp == '\0')
>      regexp = nullptr;
> 
> -  domain_search_flags flags = to_search_flags (kind);
> -  if (kind == TYPE_DOMAIN)
> -    flags |= SEARCH_STRUCT_DOMAIN;
> -
> -  global_symbol_searcher spec (flags, regexp);
> +  global_symbol_searcher spec (kind, regexp);
>    spec.set_symbol_type_regexp (t_regexp);
>    spec.set_exclude_minsyms (exclude_minsyms);
>    std::vector<symbol_search> symbols = spec.search ();
> @@ -5166,20 +5179,20 @@ symtab_symbol_info (bool quiet, bool
> exclude_minsyms,
>        const char *classname;
>        switch (kind)
>  	{
> -	case VAR_DOMAIN:
> +	case symbol_search_kind::VARIABLE:
>  	  classname = "variable";
>  	  break;
> -	case FUNCTION_DOMAIN:
> +	case symbol_search_kind::FUNCTION:
>  	  classname = "function";
>  	  break;
> -	case TYPE_DOMAIN:
> +	case symbol_search_kind::TYPE:
>  	  classname = "type";
>  	  break;
> -	case MODULE_DOMAIN:
> +	case symbol_search_kind::MODULE:
>  	  classname = "module";
>  	  break;
>  	default:
> -	  gdb_assert_not_reached ("invalid domain enum");
> +	  gdb_assert_not_reached ("invalid symbol_search_kind");

I think we could drop the "default" case as part of this refactoring.  All
cases are handled; we lose compile time warnings for unhandled cases with
the "default" case.

>  	}
> 
>        if (regexp != NULL)
> @@ -5304,7 +5317,7 @@ info_variables_command (const char *args, int
> from_tty)
>      args = nullptr;
> 
>    symtab_symbol_info
> -    (opts.quiet, opts.exclude_minsyms, args, VAR_DOMAIN,
> +    (opts.quiet, opts.exclude_minsyms, args, symbol_search_kind::VARIABLE,
>       opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
>       from_tty);
>  }
> @@ -5323,7 +5336,7 @@ info_functions_command (const char *args, int
> from_tty)
>      args = nullptr;
> 
>    symtab_symbol_info
> -    (opts.quiet, opts.exclude_minsyms, args, FUNCTION_DOMAIN,
> +    (opts.quiet, opts.exclude_minsyms, args, symbol_search_kind::FUNCTION,
>       opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
>       from_tty);
>  }
> @@ -5366,8 +5379,8 @@ info_types_command (const char *args, int
> from_tty)
>      (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
>    if (args != nullptr && *args == '\0')
>      args = nullptr;
> -  symtab_symbol_info (opts.quiet, false, args, TYPE_DOMAIN, nullptr,
> -		      from_tty);
> +  symtab_symbol_info (opts.quiet, false, args, symbol_search_kind::TYPE,
> +		      nullptr, from_tty);
>  }
> 
>  /* Command completer for 'info types' command.  */
> @@ -5399,8 +5412,8 @@ info_modules_command (const char *args, int
> from_tty)
>      (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
>    if (args != nullptr && *args == '\0')
>      args = nullptr;
> -  symtab_symbol_info (opts.quiet, true, args, MODULE_DOMAIN, nullptr,
> -		      from_tty);
> +  symtab_symbol_info (opts.quiet, true, args, symbol_search_kind::MODULE,
> +		      nullptr, from_tty);
>  }
> 
>  /* Implement the 'info main' command.  */
> @@ -5441,7 +5454,7 @@ rbreak_command (const char *regexp, int
> from_tty)
>       because the std::move nullifies file_name.  */
>    bool file_name_p = file_name != nullptr;
> 
> -  global_symbol_searcher spec (SEARCH_FUNCTION_DOMAIN, regexp);
> +  global_symbol_searcher spec (symbol_search_kind::FUNCTION, regexp);
>    if (file_name_p)
>      spec.add_filename (std::move (file_name));
>    std::vector<symbol_search> symbols = spec.search ();
> @@ -6654,12 +6667,12 @@ static struct cmd_list_element
> *info_module_cmdlist = NULL;
> 
>  std::vector<module_symbol_search>
>  search_module_symbols (const char *module_regexp, const char *regexp,
> -		       const char *type_regexp, domain_search_flags kind)
> +		       const char *type_regexp, symbol_search_kind kind)
>  {
>    std::vector<module_symbol_search> results;
> 
>    /* Search for all modules matching MODULE_REGEXP.  */
> -  global_symbol_searcher spec1 (SEARCH_MODULE_DOMAIN,
> module_regexp);
> +  global_symbol_searcher spec1 (symbol_search_kind::MODULE,
> module_regexp);
>    spec1.set_exclude_minsyms (true);
>    std::vector<symbol_search> modules = spec1.search ();
> 
> @@ -6705,9 +6718,10 @@ search_module_symbols (const char
> *module_regexp, const char *regexp,
>  static void
>  info_module_subcommand (bool quiet, const char *module_regexp,
>  			const char *regexp, const char *type_regexp,
> -			domain_search_flags kind)
> +			symbol_search_kind kind)
>  {
> -  gdb_assert (kind == SEARCH_FUNCTION_DOMAIN || kind ==
> SEARCH_VAR_DOMAIN);
> +  gdb_assert (kind == symbol_search_kind::FUNCTION
> +	      || kind == symbol_search_kind::VARIABLE);
> 
>    /* Print a header line.  Don't build the header line bit by bit as this
>       prevents internationalisation.  */
> @@ -6718,12 +6732,12 @@ info_module_subcommand (bool quiet, const
> char *module_regexp,
>  	  if (type_regexp == nullptr)
>  	    {
>  	      if (regexp == nullptr)
> -		gdb_printf ((kind == SEARCH_VAR_DOMAIN
> +		gdb_printf ((kind == symbol_search_kind::VARIABLE
>  			     ? _("All variables in all modules:")
>  			     : _("All functions in all modules:")));
>  	      else
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables matching regular expression"
>  			" \"%s\" in all modules:")
>  		    : _("All functions matching regular expression"
> @@ -6734,7 +6748,7 @@ info_module_subcommand (bool quiet, const char
> *module_regexp,
>  	    {
>  	      if (regexp == nullptr)
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables with type matching regular "
>  			"expression \"%s\" in all modules:")
>  		    : _("All functions with type matching regular "
> @@ -6742,7 +6756,7 @@ info_module_subcommand (bool quiet, const char
> *module_regexp,
>  		   type_regexp);
>  	      else
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables matching regular expression "
>  			"\"%s\",\n\twith type matching regular "
>  			"expression \"%s\" in all modules:")
> @@ -6758,7 +6772,7 @@ info_module_subcommand (bool quiet, const char
> *module_regexp,
>  	    {
>  	      if (regexp == nullptr)
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables in all modules matching regular "
>  			"expression \"%s\":")
>  		    : _("All functions in all modules matching regular "
> @@ -6766,7 +6780,7 @@ info_module_subcommand (bool quiet, const char
> *module_regexp,
>  		   module_regexp);
>  	      else
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables matching regular expression "
>  			"\"%s\",\n\tin all modules matching regular "
>  			"expression \"%s\":")
> @@ -6779,7 +6793,7 @@ info_module_subcommand (bool quiet, const char
> *module_regexp,
>  	    {
>  	      if (regexp == nullptr)
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables with type matching regular "
>  			"expression \"%s\"\n\tin all modules matching "
>  			"regular expression \"%s\":")
> @@ -6789,7 +6803,7 @@ info_module_subcommand (bool quiet, const char
> *module_regexp,
>  		   type_regexp, module_regexp);
>  	      else
>  		gdb_printf
> -		  ((kind == SEARCH_VAR_DOMAIN
> +		  ((kind == symbol_search_kind::VARIABLE
>  		    ? _("All variables matching regular expression "
>  			"\"%s\",\n\twith type matching regular expression "
>  			"\"%s\",\n\tin all modules matching regular "
> @@ -6901,7 +6915,7 @@ info_module_functions_command (const char
> *args, int from_tty)
>      (opts.quiet,
>       opts.module_regexp.empty () ? nullptr : opts.module_regexp.c_str (), args,
>       opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
> -     SEARCH_FUNCTION_DOMAIN);
> +     symbol_search_kind::FUNCTION);
>  }
> 
>  /* Implements the 'info module variables' command.  */
> @@ -6920,7 +6934,7 @@ info_module_variables_command (const char
> *args, int from_tty)
>      (opts.quiet,
>       opts.module_regexp.empty () ? nullptr : opts.module_regexp.c_str (), args,
>       opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
> -     SEARCH_VAR_DOMAIN);
> +     symbol_search_kind::VARIABLE);
>  }
> 
>  /* Command completer for 'info module ...' sub-commands.  */
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 6443216fd7f0..a35449fbeeda 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2586,6 +2586,23 @@ struct symbol_search
>  				  const symbol_search &sym_b);
>  };
> 
> +/* The kind of symbols searched for by global_symbol_searcher.  */
> +
> +enum class symbol_search_kind
> +{
> +  /* Variables, excluding enum constants.  */
> +  VARIABLE,
> +
> +  /* Functions.  */
> +  FUNCTION,
> +
> +  /* Type names.  */

IIUC, this covers "SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN" ?
Should the comment be extended to state this explicitly, e.g., "Type names,
including struct, union and enum tags." ?

> +  TYPE,
> +
> +  /* Fortran modules.  */
> +  MODULE,
> +};
> +
>  /* In order to search for global symbols of a particular kind matching
>     particular regular expressions, create an instance of this structure and
>     call the SEARCH member function.  */
> @@ -2594,7 +2611,7 @@ class global_symbol_searcher
>  public:
> 
>    /* Constructor.  */
> -  global_symbol_searcher (domain_search_flags kind,
> +  global_symbol_searcher (symbol_search_kind kind,
>  			  const char *symbol_name_regexp)
>      : m_kind (kind),
>        m_symbol_name_regexp (symbol_name_regexp)
> @@ -2635,14 +2652,8 @@ class global_symbol_searcher
>    /* The set of source files to search in for matching symbols.  */
>    std::vector<gdb::unique_xmalloc_ptr<char>> m_filenames;
> 
> -  /* The kind of symbols are we searching for.
> -     VARIABLES_DOMAIN - Search all symbols, excluding functions, type
> -			names, and constants (enums).
> -     FUNCTIONS_DOMAIN - Search all functions..
> -     TYPES_DOMAIN     - Search all type names.
> -     MODULES_DOMAIN   - Search all Fortran modules.
> -     ALL_DOMAIN       - Not valid for this function.  */
> -  domain_search_flags m_kind;
> +  /* The kind of symbols we are searching for.  */
> +  symbol_search_kind m_kind;
> 
>    /* Regular expression to match against the symbol name.  */
>    const char *m_symbol_name_regexp = nullptr;
> @@ -2686,7 +2697,7 @@ class global_symbol_searcher
>  			      std::vector<symbol_search> *results) const;
> 
>    /* Return true if MSYMBOL is of type KIND.  */
> -  static bool is_suitable_msymbol (const domain_search_flags kind,
> +  static bool is_suitable_msymbol (symbol_search_kind kind,
>  				   const minimal_symbol *msymbol);
>  };
> 
> @@ -2703,7 +2714,7 @@ using module_symbol_search =
> std::pair<symbol_search, symbol_search>;
>     within the module.  */
>  extern std::vector<module_symbol_search> search_module_symbols
>  	(const char *module_regexp, const char *regexp,
> -	 const char *type_regexp, domain_search_flags kind);
> +	 const char *type_regexp, symbol_search_kind kind);
> 
>  /* Convert a global or static symbol SYM (based on BLOCK, which should be
>     either GLOBAL_BLOCK or STATIC_BLOCK) into a string for use in 'info'
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 1/9] gdb: add symbol_search_kind enum for global_symbol_searcher

[Simon Marchi]

On 2026-09-30 05:31, Rohr, Stephan wrote:
> Hi Simon,
> 
> see my feedback inlined below.
> 
> Thanks
> Stephan
> 
>> -----Original Message-----
>> From: Simon Marchi <simon.marchi@efficios.com>
>> Sent: Tuesday, 29 September 2026 21:39
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 1/9] gdb: add symbol_search_kind enum for
>> global_symbol_searcher
>>
>> While reading the global_symbol_searcher code, something that was not
>> clear to me was that despite accepting a domain_search_flags,  searches
> 
> Double spaces in ",  searches".

Done.

>> are mutually exclusive between types, variables and functions.
>>
>> One snippet of code that really confused me was (now removed by a
>> previous commit):
>>
> 
> Should we mention the previous commit " gdb: Skip useless minsym scan in
> variable-only symbol searches" explicitly? It may help future readers of the
> patch.

Done.

>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 27070f74f83e..708df078e2ea 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4698,11 +4698,31 @@ treg_matches_sym_type_name (const
>> compiled_regex &treg,
>>    return treg.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
>>  }
>>
>> +/* Return the domain search flags matching symbols of kind KIND.  */
>> +
>> +static domain_search_flags
>> +to_search_flags (symbol_search_kind kind)
> 
> Same name as " to_search_flags (domain_enum domain)" in symtab.h.
> Not sure if this might be confusing.  I don't have a strong opinion here.

Yes, that's the intent, two overloads that do the same job but with
different input.

>> +{
>> +  switch (kind)
>> +    {
>> +    case symbol_search_kind::VARIABLE:
>> +      return SEARCH_VAR_DOMAIN;
>> +    case symbol_search_kind::FUNCTION:
>> +      return SEARCH_FUNCTION_DOMAIN;
>> +    case symbol_search_kind::TYPE:
> 
> Should we add a comment why the " SEARCH_STRUCT_DOMAIN" is
> included?  I think this might be helpful for future readers, e.g.
> "Some languages have a tag namespace.  Search the "STRUCT_DOMAIN", too."

I hesitated to put that clarification on the enumerator itself, but then
thought that the code here would make it clear that
symbol_search_kind::TYPE includes both the bare types and the "struct
domain" types.  given your comment, I would add that clarification on
the enumerator:

  /* Type names, including those with a struct/union/enum prefix (STRUCT
     domain).  */
  TYPE,

>> @@ -5166,20 +5179,20 @@ symtab_symbol_info (bool quiet, bool
>> exclude_minsyms,
>>        const char *classname;
>>        switch (kind)
>>  	{
>> -	case VAR_DOMAIN:
>> +	case symbol_search_kind::VARIABLE:
>>  	  classname = "variable";
>>  	  break;
>> -	case FUNCTION_DOMAIN:
>> +	case symbol_search_kind::FUNCTION:
>>  	  classname = "function";
>>  	  break;
>> -	case TYPE_DOMAIN:
>> +	case symbol_search_kind::TYPE:
>>  	  classname = "type";
>>  	  break;
>> -	case MODULE_DOMAIN:
>> +	case symbol_search_kind::MODULE:
>>  	  classname = "module";
>>  	  break;
>>  	default:
>> -	  gdb_assert_not_reached ("invalid domain enum");
>> +	  gdb_assert_not_reached ("invalid symbol_search_kind");
> 
> I think we could drop the "default" case as part of this refactoring.  All
> cases are handled; we lose compile time warnings for unhandled cases with
> the "default" case.

Agreed that we can drop the default.  Note that we don't build with
-Wswitch, but we specifically enable it locally here and there.

>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index 6443216fd7f0..a35449fbeeda 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2586,6 +2586,23 @@ struct symbol_search
>>  				  const symbol_search &sym_b);
>>  };
>>
>> +/* The kind of symbols searched for by global_symbol_searcher.  */
>> +
>> +enum class symbol_search_kind
>> +{
>> +  /* Variables, excluding enum constants.  */
>> +  VARIABLE,
>> +
>> +  /* Functions.  */
>> +  FUNCTION,
>> +
>> +  /* Type names.  */
> 
> IIUC, this covers "SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN" ?
> Should the comment be extended to state this explicitly, e.g., "Type names,
> including struct, union and enum tags." ?

Done, as per the comment above.

Simon

[PATCH 2/9] gdb: factor out regexp compilation from global_symbol_searcher::search

[Simon Marchi]

Factor out the scopes that compile the regexps, in order to make
global_symbol_searcher::search itself simpler to read.

Also, I found the names "preg" and "treg" very unclear, so replace them
with "name_regex" and "type_regex".

Change-Id: Ie4485b2eb593967069ac635c933df817bc5304e6
---
 gdb/symtab.c | 171 +++++++++++++++++++++++++++++----------------------
 gdb/symtab.h |  43 +++++++------
 2 files changed, 122 insertions(+), 92 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 708df078e2ea..c4f76a494abd 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4745,7 +4745,8 @@ global_symbol_searcher::is_suitable_msymbol
 
 bool
 global_symbol_searcher::expand_symtabs
-	(objfile *objfile, const std::optional<compiled_regex> &preg) const
+	(objfile *objfile,
+	 const std::optional<compiled_regex> &name_regex) const
 {
   bool found_func_msymbol_without_debug_info = false;
 
@@ -4762,8 +4763,8 @@ global_symbol_searcher::expand_symtabs
      &lookup_name_info::match_any (),
      [&] (const char *symname)
      {
-       return (!preg.has_value ()
-	       || preg->exec (symname, 0, NULL, 0) == 0);
+       return (!name_regex.has_value ()
+	       || name_regex->exec (symname, 0, NULL, 0) == 0);
      },
      NULL,
      SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
@@ -4794,9 +4795,9 @@ global_symbol_searcher::expand_symtabs
 
 	  if (is_suitable_msymbol (m_kind, msymbol))
 	    {
-	      if (!preg.has_value ()
-		  || preg->exec (msymbol->natural_name (), 0,
-				 NULL, 0) == 0)
+	      if (!name_regex.has_value ()
+		  || name_regex->exec (msymbol->natural_name (), 0,
+				       NULL, 0) == 0)
 		{
 		  /* An important side-effect of this lookup function is
 		     to expand the symbol table if msymbol is found, later
@@ -4819,8 +4820,8 @@ global_symbol_searcher::expand_symtabs
 bool
 global_symbol_searcher::add_matching_symbols
 	(objfile *objfile,
-	 const std::optional<compiled_regex> &preg,
-	 const std::optional<compiled_regex> &treg,
+	 const std::optional<compiled_regex> &name_regex,
+	 const std::optional<compiled_regex> &type_regex,
 	 std::set<symbol_search> *result_set) const
 {
   domain_search_flags domain = to_search_flags (m_kind);
@@ -4854,14 +4855,15 @@ global_symbol_searcher::add_matching_symbols
 	      if (!sym->matches (domain))
 		continue;
 
-	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
-						   nullptr, 0) != 0)
+	      if (name_regex.has_value ()
+		  && name_regex->exec (sym->natural_name (), 0,
+				       nullptr, 0) != 0)
 		continue;
 
 	      if (((sym->domain () == VAR_DOMAIN
 		    || sym->domain () == FUNCTION_DOMAIN)
-		   && treg.has_value ()
-		   && !treg_matches_sym_type_name (*treg, sym)))
+		   && type_regex.has_value ()
+		   && !treg_matches_sym_type_name (*type_regex, sym)))
 		continue;
 
 	      if (m_kind == symbol_search_kind::VARIABLE)
@@ -4898,7 +4900,7 @@ global_symbol_searcher::add_matching_symbols
 
 bool
 global_symbol_searcher::add_matching_msymbols
-	(objfile *objfile, const std::optional<compiled_regex> &preg,
+	(objfile *objfile, const std::optional<compiled_regex> &name_regex,
 	 std::vector<symbol_search> *results) const
 {
   for (minimal_symbol *msymbol : objfile->msymbols ())
@@ -4910,9 +4912,9 @@ global_symbol_searcher::add_matching_msymbols
 
       if (is_suitable_msymbol (m_kind, msymbol))
 	{
-	  if (!preg.has_value ()
-	      || preg->exec (msymbol->natural_name (), 0,
-			     NULL, 0) == 0)
+	  if (!name_regex.has_value ()
+	      || name_regex->exec (msymbol->natural_name (), 0,
+				   NULL, 0) == 0)
 	    {
 	      /* For functions we can do a quick check of whether the
 		 symbol might be found via find_pc_symtab.  */
@@ -4938,65 +4940,84 @@ global_symbol_searcher::add_matching_msymbols
   return true;
 }
 
+/* Return the regcomp flags to use for the symbol name and symbol type
+   regexps.  */
+
+static int
+search_regex_cflags ()
+{
+  return REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE : 0);
+}
+
+/* See symtab.h.  */
+
+std::optional<compiled_regex>
+global_symbol_searcher::compile_name_regex () const
+{
+  if (m_symbol_name_regexp == nullptr)
+    return {};
+
+  const char *symbol_name_regexp = m_symbol_name_regexp;
+  std::string symbol_name_regexp_holder;
+
+  /* Make sure spacing is right for C++ operators.
+     This is just a courtesy to make the matching less sensitive
+     to how many spaces the user leaves between 'operator'
+     and <TYPENAME> or <OPERATOR>.  */
+  const char *op_end;
+  const char *opname = operator_chars (symbol_name_regexp, &op_end);
+
+  if (*opname)
+    {
+      /* -1 means ok; otherwise number of spaces needed.  */
+      int fix = -1;
+
+      if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
+	{
+	  /* There should 1 space between 'operator' and 'TYPENAME'.  */
+	  if (opname[-1] != ' ' || opname[-2] == ' ')
+	    fix = 1;
+	}
+      else
+	{
+	  /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
+	  if (opname[-1] == ' ')
+	    fix = 0;
+	}
+      /* If wrong number of spaces, fix it.  */
+      if (fix >= 0)
+	{
+	  symbol_name_regexp_holder
+	    = string_printf ("operator%.*s%s", fix, " ", opname);
+	  symbol_name_regexp = symbol_name_regexp_holder.c_str ();
+	}
+    }
+
+  return std::optional<compiled_regex> (std::in_place, symbol_name_regexp,
+					search_regex_cflags (),
+					_("Invalid regexp"));
+}
+
+/* See symtab.h.  */
+
+std::optional<compiled_regex>
+global_symbol_searcher::compile_type_regex () const
+{
+  if (m_symbol_type_regexp == nullptr)
+    return {};
+
+  return std::optional<compiled_regex> (std::in_place, m_symbol_type_regexp,
+					search_regex_cflags (),
+					_("Invalid regexp"));
+}
+
 /* See symtab.h.  */
 
 std::vector<symbol_search>
 global_symbol_searcher::search () const
 {
-  std::optional<compiled_regex> preg;
-  std::optional<compiled_regex> treg;
-
-  if (m_symbol_name_regexp != NULL)
-    {
-      const char *symbol_name_regexp = m_symbol_name_regexp;
-      std::string symbol_name_regexp_holder;
-
-      /* Make sure spacing is right for C++ operators.
-	 This is just a courtesy to make the matching less sensitive
-	 to how many spaces the user leaves between 'operator'
-	 and <TYPENAME> or <OPERATOR>.  */
-      const char *op_end;
-      const char *opname = operator_chars (symbol_name_regexp, &op_end);
-
-      if (*opname)
-	{
-	  int fix = -1;		/* -1 means ok; otherwise number of
-				    spaces needed.  */
-
-	  if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
-	    {
-	      /* There should 1 space between 'operator' and 'TYPENAME'.  */
-	      if (opname[-1] != ' ' || opname[-2] == ' ')
-		fix = 1;
-	    }
-	  else
-	    {
-	      /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
-	      if (opname[-1] == ' ')
-		fix = 0;
-	    }
-	  /* If wrong number of spaces, fix it.  */
-	  if (fix >= 0)
-	    {
-	      symbol_name_regexp_holder
-		= string_printf ("operator%.*s%s", fix, " ", opname);
-	      symbol_name_regexp = symbol_name_regexp_holder.c_str ();
-	    }
-	}
-
-      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
-				? REG_ICASE : 0);
-      preg.emplace (symbol_name_regexp, cflags,
-		    _("Invalid regexp"));
-    }
-
-  if (m_symbol_type_regexp != NULL)
-    {
-      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
-				? REG_ICASE : 0);
-      treg.emplace (m_symbol_type_regexp, cflags,
-		    _("Invalid regexp"));
-    }
+  std::optional<compiled_regex> name_regex = compile_name_regex ();
+  std::optional<compiled_regex> type_regex = compile_type_regex ();
 
   bool found_func_msymbol_without_debug_info = false;
   std::set<symbol_search> result_set;
@@ -5004,13 +5025,15 @@ global_symbol_searcher::search () const
     {
       /* Expand symtabs within objfile that possibly contain matching
 	 symbols.  */
-      found_func_msymbol_without_debug_info |= expand_symtabs (&objfile, preg);
+      found_func_msymbol_without_debug_info
+	|= expand_symtabs (&objfile, name_regex);
 
       /* Find matching symbols within OBJFILE and add them in to the
 	 RESULT_SET set.  Use a set here so that we can easily detect
 	 duplicates as we go, and can therefore track how many unique
 	 matches we have found so far.  */
-      if (!add_matching_symbols (&objfile, preg, treg, &result_set))
+      if (!add_matching_symbols (&objfile, name_regex, type_regex,
+				 &result_set))
 	break;
     }
 
@@ -5025,12 +5048,12 @@ global_symbol_searcher::search () const
   if ((found_func_msymbol_without_debug_info
        || (m_filenames.empty () && m_kind == symbol_search_kind::VARIABLE))
       && !m_exclude_minsyms
-      && !treg.has_value ())
+      && !type_regex.has_value ())
     {
       gdb_assert (m_kind == symbol_search_kind::VARIABLE
 		  || m_kind == symbol_search_kind::FUNCTION);
       for (objfile &objfile : current_program_space->objfiles ())
-	if (!add_matching_msymbols (&objfile, preg, &result))
+	if (!add_matching_msymbols (&objfile, name_regex, &result))
 	  break;
     }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index a35449fbeeda..1675b126b19d 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2669,31 +2669,38 @@ class global_symbol_searcher
      of SIZE_MAX, there is no "unlimited".  */
   size_t m_max_search_results = SIZE_MAX;
 
-  /* Expand symtabs in OBJFILE that match PREG, are of type M_KIND.  Return
-     true if any msymbols were seen that we should later consider adding to
-     the results list.  */
-  bool expand_symtabs (objfile *objfile,
-		       const std::optional<compiled_regex> &preg) const;
+  /* Compile M_SYMBOL_NAME_REGEXP, if set.  */
+  std::optional<compiled_regex> compile_name_regex () const;
 
-  /* Add symbols from symtabs in OBJFILE that match PREG, and TREG, and are
-     of type M_KIND, to the results set RESULTS_SET.  Return false if we
-     stop adding results early due to having already found too many results
-     (based on M_MAX_SEARCH_RESULTS limit), otherwise return true.
+  /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
+  std::optional<compiled_regex> compile_type_regex () const;
+
+  /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type M_KIND.
+     Return true if any msymbols were seen that we should later consider
+     adding to the results list.  */
+  bool expand_symtabs (objfile *objfile,
+		       const std::optional<compiled_regex> &name_regex) const;
+
+  /* Add symbols from symtabs in OBJFILE that match NAME_REGEX, and
+     TYPE_REGEX, and are of type M_KIND, to the results set RESULTS_SET.
+     Return false if we stop adding results early due to having already
+     found too many results (based on M_MAX_SEARCH_RESULTS limit),
+     otherwise return true.
      Returning true does not indicate that any results were added, just
      that we didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
   bool add_matching_symbols (objfile *objfile,
-			     const std::optional<compiled_regex> &preg,
-			     const std::optional<compiled_regex> &treg,
+			     const std::optional<compiled_regex> &name_regex,
+			     const std::optional<compiled_regex> &type_regex,
 			     std::set<symbol_search> *result_set) const;
 
-  /* Add msymbols from OBJFILE that match PREG and M_KIND, to the results
-     vector RESULTS.  Return false if we stop adding results early due to
-     having already found too many results (based on max search results
-     limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning true
-     does not indicate that any results were added, just that we didn't
-     _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
+  /* Add msymbols from OBJFILE that match NAME_REGEX and M_KIND, to the
+     results vector RESULTS.  Return false if we stop adding results early
+     due to having already found too many results (based on max search
+     results limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning
+     true does not indicate that any results were added, just that we
+     didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
   bool add_matching_msymbols (objfile *objfile,
-			      const std::optional<compiled_regex> &preg,
+			      const std::optional<compiled_regex> &name_regex,
 			      std::vector<symbol_search> *results) const;
 
   /* Return true if MSYMBOL is of type KIND.  */
-- 
2.55.0

RE: [PATCH 2/9] gdb: factor out regexp compilation from global_symbol_searcher::search

[Rohr, Stephan]

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 2/9] gdb: factor out regexp compilation from
> global_symbol_searcher::search
> 
> Factor out the scopes that compile the regexps, in order to make
> global_symbol_searcher::search itself simpler to read.
> 
> Also, I found the names "preg" and "treg" very unclear, so replace them
> with "name_regex" and "type_regex".
> 
> Change-Id: Ie4485b2eb593967069ac635c933df817bc5304e6
> ---
>  gdb/symtab.c | 171 +++++++++++++++++++++++++++++----------------------
>  gdb/symtab.h |  43 +++++++------
>  2 files changed, 122 insertions(+), 92 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 708df078e2ea..c4f76a494abd 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4745,7 +4745,8 @@ global_symbol_searcher::is_suitable_msymbol
> 
>  bool
>  global_symbol_searcher::expand_symtabs
> -	(objfile *objfile, const std::optional<compiled_regex> &preg) const
> +	(objfile *objfile,
> +	 const std::optional<compiled_regex> &name_regex) const
>  {
>    bool found_func_msymbol_without_debug_info = false;
> 
> @@ -4762,8 +4763,8 @@ global_symbol_searcher::expand_symtabs
>       &lookup_name_info::match_any (),
>       [&] (const char *symname)
>       {
> -       return (!preg.has_value ()
> -	       || preg->exec (symname, 0, NULL, 0) == 0);
> +       return (!name_regex.has_value ()
> +	       || name_regex->exec (symname, 0, NULL, 0) == 0);
>       },
>       NULL,
>       SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
> @@ -4794,9 +4795,9 @@ global_symbol_searcher::expand_symtabs
> 
>  	  if (is_suitable_msymbol (m_kind, msymbol))
>  	    {
> -	      if (!preg.has_value ()
> -		  || preg->exec (msymbol->natural_name (), 0,
> -				 NULL, 0) == 0)
> +	      if (!name_regex.has_value ()
> +		  || name_regex->exec (msymbol->natural_name (), 0,
> +				       NULL, 0) == 0)
>  		{
>  		  /* An important side-effect of this lookup function is
>  		     to expand the symbol table if msymbol is found, later
> @@ -4819,8 +4820,8 @@ global_symbol_searcher::expand_symtabs
>  bool
>  global_symbol_searcher::add_matching_symbols
>  	(objfile *objfile,
> -	 const std::optional<compiled_regex> &preg,
> -	 const std::optional<compiled_regex> &treg,
> +	 const std::optional<compiled_regex> &name_regex,
> +	 const std::optional<compiled_regex> &type_regex,
>  	 std::set<symbol_search> *result_set) const
>  {
>    domain_search_flags domain = to_search_flags (m_kind);
> @@ -4854,14 +4855,15 @@
> global_symbol_searcher::add_matching_symbols
>  	      if (!sym->matches (domain))
>  		continue;
> 
> -	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
> -						   nullptr, 0) != 0)
> +	      if (name_regex.has_value ()
> +		  && name_regex->exec (sym->natural_name (), 0,
> +				       nullptr, 0) != 0)
>  		continue;
> 
>  	      if (((sym->domain () == VAR_DOMAIN
>  		    || sym->domain () == FUNCTION_DOMAIN)
> -		   && treg.has_value ()
> -		   && !treg_matches_sym_type_name (*treg, sym)))
> +		   && type_regex.has_value ()
> +		   && !treg_matches_sym_type_name (*type_regex, sym)))

Should we also rename " treg_matches_sym_type_name" as part of this
refactoring?   Also, the debug prints from " treg_matches_sym_type_name".

>  		continue;
> 
>  	      if (m_kind == symbol_search_kind::VARIABLE)
> @@ -4898,7 +4900,7 @@ global_symbol_searcher::add_matching_symbols
> 
>  bool
>  global_symbol_searcher::add_matching_msymbols
> -	(objfile *objfile, const std::optional<compiled_regex> &preg,
> +	(objfile *objfile, const std::optional<compiled_regex> &name_regex,
>  	 std::vector<symbol_search> *results) const
>  {
>    for (minimal_symbol *msymbol : objfile->msymbols ())
> @@ -4910,9 +4912,9 @@
> global_symbol_searcher::add_matching_msymbols
> 
>        if (is_suitable_msymbol (m_kind, msymbol))
>  	{
> -	  if (!preg.has_value ()
> -	      || preg->exec (msymbol->natural_name (), 0,
> -			     NULL, 0) == 0)
> +	  if (!name_regex.has_value ()
> +	      || name_regex->exec (msymbol->natural_name (), 0,
> +				   NULL, 0) == 0)
>  	    {
>  	      /* For functions we can do a quick check of whether the
>  		 symbol might be found via find_pc_symtab.  */
> @@ -4938,65 +4940,84 @@
> global_symbol_searcher::add_matching_msymbols
>    return true;
>  }
> 
> +/* Return the regcomp flags to use for the symbol name and symbol type
> +   regexps.  */
> +
> +static int
> +search_regex_cflags ()
> +{
> +  return REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE :
> 0);
> +}
> +
> +/* See symtab.h.  */
> +
> +std::optional<compiled_regex>
> +global_symbol_searcher::compile_name_regex () const
> +{
> +  if (m_symbol_name_regexp == nullptr)
> +    return {};
> +
> +  const char *symbol_name_regexp = m_symbol_name_regexp;
> +  std::string symbol_name_regexp_holder;
> +
> +  /* Make sure spacing is right for C++ operators.
> +     This is just a courtesy to make the matching less sensitive
> +     to how many spaces the user leaves between 'operator'
> +     and <TYPENAME> or <OPERATOR>.  */
> +  const char *op_end;
> +  const char *opname = operator_chars (symbol_name_regexp, &op_end);
> +
> +  if (*opname)
> +    {
> +      /* -1 means ok; otherwise number of spaces needed.  */
> +      int fix = -1;
> +
> +      if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
> +	{
> +	  /* There should 1 space between 'operator' and 'TYPENAME'.  */
> +	  if (opname[-1] != ' ' || opname[-2] == ' ')
> +	    fix = 1;
> +	}
> +      else
> +	{
> +	  /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
> +	  if (opname[-1] == ' ')
> +	    fix = 0;
> +	}
> +      /* If wrong number of spaces, fix it.  */
> +      if (fix >= 0)
> +	{
> +	  symbol_name_regexp_holder
> +	    = string_printf ("operator%.*s%s", fix, " ", opname);
> +	  symbol_name_regexp = symbol_name_regexp_holder.c_str ();
> +	}
> +    }
> +
> +  return std::optional<compiled_regex> (std::in_place, symbol_name_regexp,
> +					search_regex_cflags (),
> +					_("Invalid regexp"));
> +}
> +
> +/* See symtab.h.  */
> +
> +std::optional<compiled_regex>
> +global_symbol_searcher::compile_type_regex () const
> +{
> +  if (m_symbol_type_regexp == nullptr)
> +    return {};
> +
> +  return std::optional<compiled_regex> (std::in_place,
> m_symbol_type_regexp,
> +					search_regex_cflags (),
> +					_("Invalid regexp"));
> +}
> +
>  /* See symtab.h.  */
> 
>  std::vector<symbol_search>
>  global_symbol_searcher::search () const
>  {
> -  std::optional<compiled_regex> preg;
> -  std::optional<compiled_regex> treg;
> -
> -  if (m_symbol_name_regexp != NULL)
> -    {
> -      const char *symbol_name_regexp = m_symbol_name_regexp;
> -      std::string symbol_name_regexp_holder;
> -
> -      /* Make sure spacing is right for C++ operators.
> -	 This is just a courtesy to make the matching less sensitive
> -	 to how many spaces the user leaves between 'operator'
> -	 and <TYPENAME> or <OPERATOR>.  */
> -      const char *op_end;
> -      const char *opname = operator_chars (symbol_name_regexp, &op_end);
> -
> -      if (*opname)
> -	{
> -	  int fix = -1;		/* -1 means ok; otherwise number of
> -				    spaces needed.  */
> -
> -	  if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
> -	    {
> -	      /* There should 1 space between 'operator' and 'TYPENAME'.  */
> -	      if (opname[-1] != ' ' || opname[-2] == ' ')
> -		fix = 1;
> -	    }
> -	  else
> -	    {
> -	      /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
> -	      if (opname[-1] == ' ')
> -		fix = 0;
> -	    }
> -	  /* If wrong number of spaces, fix it.  */
> -	  if (fix >= 0)
> -	    {
> -	      symbol_name_regexp_holder
> -		= string_printf ("operator%.*s%s", fix, " ", opname);
> -	      symbol_name_regexp = symbol_name_regexp_holder.c_str ();
> -	    }
> -	}
> -
> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
> -				? REG_ICASE : 0);
> -      preg.emplace (symbol_name_regexp, cflags,
> -		    _("Invalid regexp"));
> -    }
> -
> -  if (m_symbol_type_regexp != NULL)
> -    {
> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
> -				? REG_ICASE : 0);
> -      treg.emplace (m_symbol_type_regexp, cflags,
> -		    _("Invalid regexp"));
> -    }
> +  std::optional<compiled_regex> name_regex = compile_name_regex ();
> +  std::optional<compiled_regex> type_regex = compile_type_regex ();
> 
>    bool found_func_msymbol_without_debug_info = false;
>    std::set<symbol_search> result_set;
> @@ -5004,13 +5025,15 @@ global_symbol_searcher::search () const
>      {
>        /* Expand symtabs within objfile that possibly contain matching
>  	 symbols.  */
> -      found_func_msymbol_without_debug_info |= expand_symtabs (&objfile,
> preg);
> +      found_func_msymbol_without_debug_info
> +	|= expand_symtabs (&objfile, name_regex);
> 
>        /* Find matching symbols within OBJFILE and add them in to the
>  	 RESULT_SET set.  Use a set here so that we can easily detect
>  	 duplicates as we go, and can therefore track how many unique
>  	 matches we have found so far.  */
> -      if (!add_matching_symbols (&objfile, preg, treg, &result_set))
> +      if (!add_matching_symbols (&objfile, name_regex, type_regex,
> +				 &result_set))
>  	break;
>      }
> 
> @@ -5025,12 +5048,12 @@ global_symbol_searcher::search () const
>    if ((found_func_msymbol_without_debug_info
>         || (m_filenames.empty () && m_kind == symbol_search_kind::VARIABLE))
>        && !m_exclude_minsyms
> -      && !treg.has_value ())
> +      && !type_regex.has_value ())
>      {
>        gdb_assert (m_kind == symbol_search_kind::VARIABLE
>  		  || m_kind == symbol_search_kind::FUNCTION);
>        for (objfile &objfile : current_program_space->objfiles ())
> -	if (!add_matching_msymbols (&objfile, preg, &result))
> +	if (!add_matching_msymbols (&objfile, name_regex, &result))
>  	  break;
>      }
> 
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index a35449fbeeda..1675b126b19d 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2669,31 +2669,38 @@ class global_symbol_searcher
>       of SIZE_MAX, there is no "unlimited".  */
>    size_t m_max_search_results = SIZE_MAX;
> 
> -  /* Expand symtabs in OBJFILE that match PREG, are of type M_KIND.  Return
> -     true if any msymbols were seen that we should later consider adding to
> -     the results list.  */
> -  bool expand_symtabs (objfile *objfile,
> -		       const std::optional<compiled_regex> &preg) const;
> +  /* Compile M_SYMBOL_NAME_REGEXP, if set.  */

The "compile_name_regex" method also corrects spaces for the parser.
I think we could mention this in the comment.

> +  std::optional<compiled_regex> compile_name_regex () const;
> 
> -  /* Add symbols from symtabs in OBJFILE that match PREG, and TREG, and
> are
> -     of type M_KIND, to the results set RESULTS_SET.  Return false if we
> -     stop adding results early due to having already found too many results
> -     (based on M_MAX_SEARCH_RESULTS limit), otherwise return true.
> +  /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
> +  std::optional<compiled_regex> compile_type_regex () const;
> +
> +  /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type
> M_KIND.
> +     Return true if any msymbols were seen that we should later consider
> +     adding to the results list.  */
> +  bool expand_symtabs (objfile *objfile,
> +		       const std::optional<compiled_regex> &name_regex) const;
> +
> +  /* Add symbols from symtabs in OBJFILE that match NAME_REGEX, and
> +     TYPE_REGEX, and are of type M_KIND, to the results set RESULTS_SET.
> +     Return false if we stop adding results early due to having already
> +     found too many results (based on M_MAX_SEARCH_RESULTS limit),
> +     otherwise return true.
>       Returning true does not indicate that any results were added, just
>       that we didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.
> */
>    bool add_matching_symbols (objfile *objfile,
> -			     const std::optional<compiled_regex> &preg,
> -			     const std::optional<compiled_regex> &treg,
> +			     const std::optional<compiled_regex> &name_regex,
> +			     const std::optional<compiled_regex> &type_regex,
>  			     std::set<symbol_search> *result_set) const;
> 
> -  /* Add msymbols from OBJFILE that match PREG and M_KIND, to the results
> -     vector RESULTS.  Return false if we stop adding results early due to
> -     having already found too many results (based on max search results
> -     limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning true
> -     does not indicate that any results were added, just that we didn't
> -     _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
> +  /* Add msymbols from OBJFILE that match NAME_REGEX and M_KIND, to
> the
> +     results vector RESULTS.  Return false if we stop adding results early
> +     due to having already found too many results (based on max search
> +     results limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning
> +     true does not indicate that any results were added, just that we
> +     didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
>    bool add_matching_msymbols (objfile *objfile,
> -			      const std::optional<compiled_regex> &preg,
> +			      const std::optional<compiled_regex>
> &name_regex,
>  			      std::vector<symbol_search> *results) const;
> 
>    /* Return true if MSYMBOL is of type KIND.  */
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 2/9] gdb: factor out regexp compilation from global_symbol_searcher::search

[Simon Marchi]

On 2026-09-30 08:10, Rohr, Stephan wrote:
>> @@ -4854,14 +4855,15 @@
>> global_symbol_searcher::add_matching_symbols
>>  	      if (!sym->matches (domain))
>>  		continue;
>>
>> -	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
>> -						   nullptr, 0) != 0)
>> +	      if (name_regex.has_value ()
>> +		  && name_regex->exec (sym->natural_name (), 0,
>> +				       nullptr, 0) != 0)
>>  		continue;
>>
>>  	      if (((sym->domain () == VAR_DOMAIN
>>  		    || sym->domain () == FUNCTION_DOMAIN)
>> -		   && treg.has_value ()
>> -		   && !treg_matches_sym_type_name (*treg, sym)))
>> +		   && type_regex.has_value ()
>> +		   && !treg_matches_sym_type_name (*type_regex, sym)))
> 
> Should we also rename " treg_matches_sym_type_name" as part of this
> refactoring?   Also, the debug prints from " treg_matches_sym_type_name".

You're right, I'll rename to regex_matches_sym_type_name.

We can remove the function name from the debug print, since the debug
print macro already includes the function name.

Since I found more preg/treg to rename in stack.c, I'll move the
renaming part of this change to its own patch.  This patch will focus on
factoring out the regex compilation code.

>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index a35449fbeeda..1675b126b19d 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2669,31 +2669,38 @@ class global_symbol_searcher
>>       of SIZE_MAX, there is no "unlimited".  */
>>    size_t m_max_search_results = SIZE_MAX;
>>
>> -  /* Expand symtabs in OBJFILE that match PREG, are of type M_KIND.  Return
>> -     true if any msymbols were seen that we should later consider adding to
>> -     the results list.  */
>> -  bool expand_symtabs (objfile *objfile,
>> -		       const std::optional<compiled_regex> &preg) const;
>> +  /* Compile M_SYMBOL_NAME_REGEXP, if set.  */
> 
> The "compile_name_regex" method also corrects spaces for the parser.
> I think we could mention this in the comment.

Ok, will use:

  /* Compile M_SYMBOL_NAME_REGEXP, if set.

     This function normalizes the spacing when it detects a C++ operator in the
     regexp.  */
  std::optional<compiled_regex> compile_name_regex () const;

Simon

[PATCH 3/9] gdb: simplify insertion in global_symbol_searcher::add_matching_symbols

[Simon Marchi]

std::set::insert does nothing if an equivalent element is already
in the set, so checking with find first is redundant.  Replace find+insert
with emplace.

Change-Id: I869284407cfefb9b2d7ad7305fe04fbdaee25f1f
---
 gdb/symtab.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index c4f76a494abd..4b2c4be8d117 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4880,13 +4880,9 @@ global_symbol_searcher::add_matching_symbols
 	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
 		continue;
 
+	      /* Match, insert if not already in the results.  */
 	      if (result_set->size () < m_max_search_results)
-		{
-		  /* Match, insert if not already in the results.  */
-		  symbol_search ss (block, sym);
-		  if (result_set->find (ss) == result_set->end ())
-		    result_set->insert (ss);
-		}
+		result_set->emplace (block, sym);
 	      else
 		return false;
 	    }
-- 
2.55.0

RE: [PATCH 3/9] gdb: simplify insertion in global_symbol_searcher::add_matching_symbols

[Rohr, Stephan]

Hi Simon,

one NIT, otherwise looks good to me.

Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>

Thanks
Stephan

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 3/9] gdb: simplify insertion in
> global_symbol_searcher::add_matching_symbols
> 
> std::set::insert does nothing if an equivalent element is already
> in the set, so checking with find first is redundant.  Replace find+insert
> with emplace.
> 
> Change-Id: I869284407cfefb9b2d7ad7305fe04fbdaee25f1f
> ---
>  gdb/symtab.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index c4f76a494abd..4b2c4be8d117 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4880,13 +4880,9 @@ global_symbol_searcher::add_matching_symbols
>  	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
>  		continue;
> 
> +	      /* Match, insert if not already in the results.  */

I think we can remove that comment, too.  It was targeting the find & insert
mechanism, which "emplace" takes care of.

>  	      if (result_set->size () < m_max_search_results)
> -		{
> -		  /* Match, insert if not already in the results.  */
> -		  symbol_search ss (block, sym);
> -		  if (result_set->find (ss) == result_set->end ())
> -		    result_set->insert (ss);
> -		}
> +		result_set->emplace (block, sym);
>  	      else
>  		return false;
>  	    }
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 3/9] gdb: simplify insertion in global_symbol_searcher::add_matching_symbols

[Simon Marchi]

On 2026-09-30 04:20, Rohr, Stephan wrote:
> Hi Simon,
> 
> one NIT, otherwise looks good to me.
> 
> Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>
> 
> Thanks
> Stephan
> 
>> -----Original Message-----
>> From: Simon Marchi <simon.marchi@efficios.com>
>> Sent: Tuesday, 29 September 2026 21:39
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 3/9] gdb: simplify insertion in
>> global_symbol_searcher::add_matching_symbols
>>
>> std::set::insert does nothing if an equivalent element is already
>> in the set, so checking with find first is redundant.  Replace find+insert
>> with emplace.
>>
>> Change-Id: I869284407cfefb9b2d7ad7305fe04fbdaee25f1f
>> ---
>>  gdb/symtab.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index c4f76a494abd..4b2c4be8d117 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4880,13 +4880,9 @@ global_symbol_searcher::add_matching_symbols
>>  	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
>>  		continue;
>>
>> +	      /* Match, insert if not already in the results.  */
> 
> I think we can remove that comment, too.  It was targeting the find & insert
> mechanism, which "emplace" takes care of.

Yeah... I left it because it might not be obvious that emplace does
that, so I wanted to keep the comment to describe the intent.  I think
it doesn't hurt to keep it.

Simon

[PATCH 4/9] gdb: use a switch on m_kind in add_matching_symbols

[Simon Marchi]

add_matching_symbols applies some filters that only make sense for some
kinds of searches: the type regexp for variables and functions, the
LOC_UNRESOLVED / enum constant filter for variables, and the line number
filter for modules.  Some of these check m_kind, others check the
symbol's domain.  Since the earlier sym->matches check only lets through
symbols whose domain corresponds to m_kind, the two are equivalent.

Group these filters in a single switch on m_kind.  Add a comment for
the module filter, which skips module declarations, because it wasn't
obvious to me at first sight.

Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
---
 gdb/symtab.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 4b2c4be8d117..2bb1cbf1b9ad 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4860,25 +4860,30 @@ global_symbol_searcher::add_matching_symbols
 				       nullptr, 0) != 0)
 		continue;
 
-	      if (((sym->domain () == VAR_DOMAIN
-		    || sym->domain () == FUNCTION_DOMAIN)
-		   && type_regex.has_value ()
-		   && !treg_matches_sym_type_name (*type_regex, sym)))
-		continue;
-
-	      if (m_kind == symbol_search_kind::VARIABLE)
+	      switch (m_kind)
 		{
+		case symbol_search_kind::VARIABLE:
+		  /* LOC_CONST can be used for more than just enums, e.g.,
+		     C++ static const members.  We only want to skip enums
+		     here.  */
 		  if (sym->loc_class () == LOC_UNRESOLVED
-		      /* LOC_CONST can be used for more than
-			 just enums, e.g., c++ static const
-			 members.  We only want to skip enums
-			 here.  */
 		      || (sym->loc_class () == LOC_CONST
-			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
+			  && sym->type ()->code () == TYPE_CODE_ENUM))
 		    continue;
+		  [[fallthrough]];
+
+		case symbol_search_kind::FUNCTION:
+		  if (type_regex.has_value ()
+		      && !treg_matches_sym_type_name (*type_regex, sym))
+		    continue;
+		  break;
+
+		case symbol_search_kind::MODULE:
+		  /* Skip module declarations, keep only definitions.  */
+		  if (sym->line () == 0)
+		    continue;
+		  break;
 		}
-	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
-		continue;
 
 	      /* Match, insert if not already in the results.  */
 	      if (result_set->size () < m_max_search_results)
-- 
2.55.0

Re: [PATCH 4/9] gdb: use a switch on m_kind in add_matching_symbols

[Simon Marchi]

Seems like you missed doing reply all, so re-adding gdb-patches.

On 2026-09-30 08:32, Rohr, Stephan wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Marchi <simon.marchi@efficios.com>
>> Sent: Tuesday, 29 September 2026 21:39
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 4/9] gdb: use a switch on m_kind in add_matching_symbols
>>
>> add_matching_symbols applies some filters that only make sense for some
>> kinds of searches: the type regexp for variables and functions, the
>> LOC_UNRESOLVED / enum constant filter for variables, and the line number
>> filter for modules.  Some of these check m_kind, others check the
>> symbol's domain.  Since the earlier sym->matches check only lets through
>> symbols whose domain corresponds to m_kind, the two are equivalent.
>>
>> Group these filters in a single switch on m_kind.  Add a comment for
>> the module filter, which skips module declarations, because it wasn't
>> obvious to me at first sight.
>>
>> Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
>> ---
>>  gdb/symtab.c | 33 +++++++++++++++++++--------------
>>  1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 4b2c4be8d117..2bb1cbf1b9ad 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4860,25 +4860,30 @@
>> global_symbol_searcher::add_matching_symbols
>>  				       nullptr, 0) != 0)
>>  		continue;
>>
>> -	      if (((sym->domain () == VAR_DOMAIN
>> -		    || sym->domain () == FUNCTION_DOMAIN)
>> -		   && type_regex.has_value ()
>> -		   && !treg_matches_sym_type_name (*type_regex, sym)))
>> -		continue;
>> -
>> -	      if (m_kind == symbol_search_kind::VARIABLE)
>> +	      switch (m_kind)
>>  		{
>> +		case symbol_search_kind::VARIABLE:
>> +		  /* LOC_CONST can be used for more than just enums, e.g.,
>> +		     C++ static const members.  We only want to skip enums
>> +		     here.  */
>>  		  if (sym->loc_class () == LOC_UNRESOLVED
>> -		      /* LOC_CONST can be used for more than
>> -			 just enums, e.g., c++ static const
>> -			 members.  We only want to skip enums
>> -			 here.  */
>>  		      || (sym->loc_class () == LOC_CONST
>> -			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
>> +			  && sym->type ()->code () == TYPE_CODE_ENUM))
>>  		    continue;
>> +		  [[fallthrough]];
>> +
>> +		case symbol_search_kind::FUNCTION:
>> +		  if (type_regex.has_value ()
>> +		      && !treg_matches_sym_type_name (*type_regex, sym))
>> +		    continue;
>> +		  break;
>> +
>> +		case symbol_search_kind::MODULE:
>> +		  /* Skip module declarations, keep only definitions.  */
>> +		  if (sym->line () == 0)
>> +		    continue;
>> +		  break;
> 
> For consistency, I think adding the fourth case would be good:
> 
> 		case symbol_search_kind::TYPE:
> 		  break;
> 
> If the -Wno-switch is ever to be removed, this won't produce a warning.

I don't agree with this one, we don't need it precisely because we don't
use -Wswitch.  If we ever want to enable -Wswitch, there are a ton of
sites we'll need to adjust, this one is no different.

Simon

[PATCH 5/9] gdb: make global_symbol_searcher::is_suitable_msymbol non-static

[Simon Marchi]

is_suitable_msymbol is a static method taking the search kind as a
parameter, but all callers pass m_kind.  Make it a const non-static
method that uses m_kind directly.

Change-Id: I803e7cddbb317a8af35a82ea9e528c64f6326f31
---
 gdb/symtab.c | 10 +++++-----
 gdb/symtab.h |  5 ++---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2bb1cbf1b9ad..9ce6911666c6 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4722,7 +4722,7 @@ to_search_flags (symbol_search_kind kind)
 
 bool
 global_symbol_searcher::is_suitable_msymbol
-	(symbol_search_kind kind, const minimal_symbol *msymbol)
+	(const minimal_symbol *msymbol) const
 {
   switch (msymbol->type ())
     {
@@ -4730,12 +4730,12 @@ global_symbol_searcher::is_suitable_msymbol
     case mst_bss:
     case mst_file_data:
     case mst_file_bss:
-      return kind == symbol_search_kind::VARIABLE;
+      return m_kind == symbol_search_kind::VARIABLE;
     case mst_text:
     case mst_file_text:
     case mst_solib_trampoline:
     case mst_text_gnu_ifunc:
-      return kind == symbol_search_kind::FUNCTION;
+      return m_kind == symbol_search_kind::FUNCTION;
     default:
       return false;
     }
@@ -4793,7 +4793,7 @@ global_symbol_searcher::expand_symtabs
 	  if (msymbol->created_by_gdb)
 	    continue;
 
-	  if (is_suitable_msymbol (m_kind, msymbol))
+	  if (is_suitable_msymbol (msymbol))
 	    {
 	      if (!name_regex.has_value ()
 		  || name_regex->exec (msymbol->natural_name (), 0,
@@ -4911,7 +4911,7 @@ global_symbol_searcher::add_matching_msymbols
       if (msymbol->created_by_gdb)
 	continue;
 
-      if (is_suitable_msymbol (m_kind, msymbol))
+      if (is_suitable_msymbol (msymbol))
 	{
 	  if (!name_regex.has_value ()
 	      || name_regex->exec (msymbol->natural_name (), 0,
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 1675b126b19d..50d7000681df 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2703,9 +2703,8 @@ class global_symbol_searcher
 			      const std::optional<compiled_regex> &name_regex,
 			      std::vector<symbol_search> *results) const;
 
-  /* Return true if MSYMBOL is of type KIND.  */
-  static bool is_suitable_msymbol (symbol_search_kind kind,
-				   const minimal_symbol *msymbol);
+  /* Return true if MSYMBOL is of type M_KIND.  */
+  bool is_suitable_msymbol (const minimal_symbol *msymbol) const;
 };
 
 /* When searching for Fortran symbols within modules (functions/variables)
-- 
2.55.0

RE: [PATCH 5/9] gdb: make global_symbol_searcher::is_suitable_msymbol non-static

[Rohr, Stephan]

This looks good to me.

Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 5/9] gdb: make
> global_symbol_searcher::is_suitable_msymbol non-static
> 
> is_suitable_msymbol is a static method taking the search kind as a
> parameter, but all callers pass m_kind.  Make it a const non-static
> method that uses m_kind directly.
> 
> Change-Id: I803e7cddbb317a8af35a82ea9e528c64f6326f31
> ---
>  gdb/symtab.c | 10 +++++-----
>  gdb/symtab.h |  5 ++---
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 2bb1cbf1b9ad..9ce6911666c6 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4722,7 +4722,7 @@ to_search_flags (symbol_search_kind kind)
> 
>  bool
>  global_symbol_searcher::is_suitable_msymbol
> -	(symbol_search_kind kind, const minimal_symbol *msymbol)
> +	(const minimal_symbol *msymbol) const
>  {
>    switch (msymbol->type ())
>      {
> @@ -4730,12 +4730,12 @@ global_symbol_searcher::is_suitable_msymbol
>      case mst_bss:
>      case mst_file_data:
>      case mst_file_bss:
> -      return kind == symbol_search_kind::VARIABLE;
> +      return m_kind == symbol_search_kind::VARIABLE;
>      case mst_text:
>      case mst_file_text:
>      case mst_solib_trampoline:
>      case mst_text_gnu_ifunc:
> -      return kind == symbol_search_kind::FUNCTION;
> +      return m_kind == symbol_search_kind::FUNCTION;
>      default:
>        return false;
>      }
> @@ -4793,7 +4793,7 @@ global_symbol_searcher::expand_symtabs
>  	  if (msymbol->created_by_gdb)
>  	    continue;
> 
> -	  if (is_suitable_msymbol (m_kind, msymbol))
> +	  if (is_suitable_msymbol (msymbol))
>  	    {
>  	      if (!name_regex.has_value ()
>  		  || name_regex->exec (msymbol->natural_name (), 0,
> @@ -4911,7 +4911,7 @@
> global_symbol_searcher::add_matching_msymbols
>        if (msymbol->created_by_gdb)
>  	continue;
> 
> -      if (is_suitable_msymbol (m_kind, msymbol))
> +      if (is_suitable_msymbol (msymbol))
>  	{
>  	  if (!name_regex.has_value ()
>  	      || name_regex->exec (msymbol->natural_name (), 0,
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 1675b126b19d..50d7000681df 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2703,9 +2703,8 @@ class global_symbol_searcher
>  			      const std::optional<compiled_regex>
> &name_regex,
>  			      std::vector<symbol_search> *results) const;
> 
> -  /* Return true if MSYMBOL is of type KIND.  */
> -  static bool is_suitable_msymbol (symbol_search_kind kind,
> -				   const minimal_symbol *msymbol);
> +  /* Return true if MSYMBOL is of type M_KIND.  */
> +  bool is_suitable_msymbol (const minimal_symbol *msymbol) const;
>  };
> 
>  /* When searching for Fortran symbols within modules (functions/variables)
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH 6/9] gdb: update comment of global_symbol_searcher::expand_symtabs

[Simon Marchi]

Update the comment of expand_symtabs to describe more accurately what
the return value means.  It is true when a matching function msymbol for
which no debug info exists is found.

Change-Id: I7c26c065b31575066ed2398563cdc1dac8869f8b
---
 gdb/symtab.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gdb/symtab.h b/gdb/symtab.h
index 50d7000681df..acbe41d131b7 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2676,8 +2676,9 @@ class global_symbol_searcher
   std::optional<compiled_regex> compile_type_regex () const;
 
   /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type M_KIND.
-     Return true if any msymbols were seen that we should later consider
-     adding to the results list.  */
+     Return true if a matching function msymbol without debug info was
+     seen, meaning that msymbols should later be considered for the results
+     list.  */
   bool expand_symtabs (objfile *objfile,
 		       const std::optional<compiled_regex> &name_regex) const;
 
-- 
2.55.0

RE: [PATCH 6/9] gdb: update comment of global_symbol_searcher::expand_symtabs

[Rohr, Stephan]

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 6/9] gdb: update comment of
> global_symbol_searcher::expand_symtabs
> 
> Update the comment of expand_symtabs to describe more accurately what
> the return value means.  It is true when a matching function msymbol for
> which no debug info exists is found.
> 
> Change-Id: I7c26c065b31575066ed2398563cdc1dac8869f8b
> ---
>  gdb/symtab.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 50d7000681df..acbe41d131b7 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2676,8 +2676,9 @@ class global_symbol_searcher
>    std::optional<compiled_regex> compile_type_regex () const;
> 
>    /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type
> M_KIND.
> -     Return true if any msymbols were seen that we should later consider
> -     adding to the results list.  */
> +     Return true if a matching function msymbol without debug info was
> +     seen, meaning that msymbols should later be considered for the results
> +     list.  */

If we update comment, what about updating the first sentence to

"Expand symtabs in OBJFILE that match NAME_REGEX and are of type M_KIND."

This reads better from my point of view.

I was also wondering if it's worth updating the comments inside the "expand_symtabs"
function:

  - [..] When no file names were given the caller unconditionally rescans [..]

     The "unconditionally" can be dropped.  It is gated in "search ()".

- The comment this contains "SEARCH_VAR_DOMAIN".  Should this be changed to
   "symbol_search_kind:: VARIABLE" instead?  Not sure if this is the right patch for this
   change.

>    bool expand_symtabs (objfile *objfile,
>  		       const std::optional<compiled_regex> &name_regex) const;
> 
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 6/9] gdb: update comment of global_symbol_searcher::expand_symtabs

[Simon Marchi]

On 2026-09-30 09:08, Rohr, Stephan wrote:
>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index 50d7000681df..acbe41d131b7 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2676,8 +2676,9 @@ class global_symbol_searcher
>>    std::optional<compiled_regex> compile_type_regex () const;
>>
>>    /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type
>> M_KIND.
>> -     Return true if any msymbols were seen that we should later consider
>> -     adding to the results list.  */
>> +     Return true if a matching function msymbol without debug info was
>> +     seen, meaning that msymbols should later be considered for the results
>> +     list.  */
> 
> If we update comment, what about updating the first sentence to
> 
> "Expand symtabs in OBJFILE that match NAME_REGEX and are of type M_KIND."
> 
> This reads better from my point of view.

Agreed, fixed.

> I was also wondering if it's worth updating the comments inside the "expand_symtabs"
> function:
> 
>   - [..] When no file names were given the caller unconditionally rescans [..]
> 
>      The "unconditionally" can be dropped.  It is gated in "search ()".

I'll do that.  Honestly, I'm not even sure what this part of the comment
brings.


> - The comment this contains "SEARCH_VAR_DOMAIN".  Should this be changed to
>    "symbol_search_kind:: VARIABLE" instead?  Not sure if this is the right patch for this
>    change.

I'll just say "for variables".

Simon

[PATCH 7/9] gdb: add matching helpers to global_symbol_searcher

[Simon Marchi]

The methods of global_symbol_searcher repeat the same checks in several
places.  Add some helpers to make the code easier to follow:

 - name_matches: check a name against the (optional) name regexp

 - symtab_matches_filenames: check a symtab against the searched file
   names

 - symbol_matches: all the filtering done on full symbols in
   add_matching_symbols

 - msymbol_matches: the filtering done on minimal symbols, shared by
   expand_symtabs and add_matching_msymbols

Switch expand_symtabs, add_matching_symbols and add_matching_msymbols to
use early continues to avoid deeply nested ifs.

Change-Id: I553f70fb8ea9c7e55b89bb1204a402938c116c3b
---
 gdb/symtab.c | 205 ++++++++++++++++++++++++++++-----------------------
 gdb/symtab.h |  13 ++++
 2 files changed, 127 insertions(+), 91 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 9ce6911666c6..79e43eafde18 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4741,6 +4741,93 @@ global_symbol_searcher::is_suitable_msymbol
     }
 }
 
+/* Return true if NAME matches NAME_REGEX, or if NAME_REGEX is empty.  */
+
+static bool
+name_matches (const std::optional<compiled_regex> &name_regex,
+	      const char *name)
+{
+  return (!name_regex.has_value ()
+	  || name_regex->exec (name, 0, nullptr, 0) == 0);
+}
+
+/* See symtab.h.  */
+
+bool
+global_symbol_searcher::symtab_matches_filenames (symtab *symtab) const
+{
+  /* Check first sole SYMTAB->FILENAME.  It does not need to be a substring
+     of symtab_to_fullname as it may contain "./" etc.  */
+  if (file_matches (symtab->filename (), m_filenames, false))
+    return true;
+
+  if (!basenames_may_differ
+      && !file_matches (lbasename (symtab->filename ()), m_filenames, true))
+    return false;
+
+  return file_matches (symtab_to_fullname (symtab), m_filenames, false);
+}
+
+/* See symtab.h.  */
+
+bool
+global_symbol_searcher::symbol_matches
+	(const symbol *sym,
+	 const std::optional<compiled_regex> &name_regex,
+	 const std::optional<compiled_regex> &type_regex) const
+{
+  if (!symtab_matches_filenames (sym->symtab ()))
+    return false;
+
+  if (!sym->matches (to_search_flags (m_kind)))
+    return false;
+
+  if (!name_matches (name_regex, sym->natural_name ()))
+    return false;
+
+  switch (m_kind)
+    {
+    case symbol_search_kind::VARIABLE:
+      /* LOC_CONST can be used for more than just enums, e.g., C++ static
+	 const members.  We only want to skip enums here.  */
+      if (sym->loc_class () == LOC_UNRESOLVED
+	  || (sym->loc_class () == LOC_CONST
+	      && sym->type ()->code () == TYPE_CODE_ENUM))
+	return false;
+      [[fallthrough]];
+
+    case symbol_search_kind::FUNCTION:
+      if (type_regex.has_value ()
+	  && !treg_matches_sym_type_name (*type_regex, sym))
+	return false;
+      break;
+
+    case symbol_search_kind::MODULE:
+      /* Skip module declarations, keep only definitions.  */
+      if (sym->line () == 0)
+	return false;
+      break;
+    }
+
+  return true;
+}
+
+/* See symtab.h.  */
+
+bool
+global_symbol_searcher::msymbol_matches
+	(const minimal_symbol *msymbol,
+	 const std::optional<compiled_regex> &name_regex) const
+{
+  if (msymbol->created_by_gdb)
+    return false;
+
+  if (!is_suitable_msymbol (msymbol))
+    return false;
+
+  return name_matches (name_regex, msymbol->natural_name ());
+}
+
 /* See symtab.h.  */
 
 bool
@@ -4763,8 +4850,7 @@ global_symbol_searcher::expand_symtabs
      &lookup_name_info::match_any (),
      [&] (const char *symname)
      {
-       return (!name_regex.has_value ()
-	       || name_regex->exec (symname, 0, NULL, 0) == 0);
+       return name_matches (name_regex, symname);
      },
      NULL,
      SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
@@ -4790,25 +4876,16 @@ global_symbol_searcher::expand_symtabs
 	{
 	  QUIT;
 
-	  if (msymbol->created_by_gdb)
+	  if (!msymbol_matches (msymbol, name_regex))
 	    continue;
 
-	  if (is_suitable_msymbol (msymbol))
-	    {
-	      if (!name_regex.has_value ()
-		  || name_regex->exec (msymbol->natural_name (), 0,
-				       NULL, 0) == 0)
-		{
-		  /* An important side-effect of this lookup function is
-		     to expand the symbol table if msymbol is found, later
-		     in the process we will add matching symbols or
-		     msymbols to the results list, and that requires that
-		     the symbols tables are expanded.  */
-		  if (find_compunit_symtab_for_pc
-			(msymbol->value_address (objfile)) == nullptr)
-		    found_func_msymbol_without_debug_info = true;
-		}
-	    }
+	  /* An important side-effect of this lookup function is to expand
+	     the symbol table if msymbol is found, later in the process we
+	     will add matching symbols or msymbols to the results list, and
+	     that requires that the symbols tables are expanded.  */
+	  if (find_compunit_symtab_for_pc
+		(msymbol->value_address (objfile)) == nullptr)
+	    found_func_msymbol_without_debug_info = true;
 	}
     }
 
@@ -4824,8 +4901,6 @@ global_symbol_searcher::add_matching_symbols
 	 const std::optional<compiled_regex> &type_regex,
 	 std::set<symbol_search> *result_set) const
 {
-  domain_search_flags domain = to_search_flags (m_kind);
-
   /* Add matching symbols (if not already present).  */
   for (compunit_symtab &cust : objfile->compunits ())
     {
@@ -4837,54 +4912,11 @@ global_symbol_searcher::add_matching_symbols
 
 	  for (struct symbol *sym : block_iterator_range (b))
 	    {
-	      struct symtab *real_symtab = sym->symtab ();
-
 	      QUIT;
 
-	      /* Check first sole REAL_SYMTAB->FILENAME.  It does
-		 not need to be a substring of symtab_to_fullname as
-		 it may contain "./" etc.  */
-	      if (!(file_matches (real_symtab->filename (), m_filenames, false)
-		    || ((basenames_may_differ
-			 || file_matches (lbasename (real_symtab->filename ()),
-					  m_filenames, true))
-			&& file_matches (symtab_to_fullname (real_symtab),
-					 m_filenames, false))))
+	      if (!symbol_matches (sym, name_regex, type_regex))
 		continue;
 
-	      if (!sym->matches (domain))
-		continue;
-
-	      if (name_regex.has_value ()
-		  && name_regex->exec (sym->natural_name (), 0,
-				       nullptr, 0) != 0)
-		continue;
-
-	      switch (m_kind)
-		{
-		case symbol_search_kind::VARIABLE:
-		  /* LOC_CONST can be used for more than just enums, e.g.,
-		     C++ static const members.  We only want to skip enums
-		     here.  */
-		  if (sym->loc_class () == LOC_UNRESOLVED
-		      || (sym->loc_class () == LOC_CONST
-			  && sym->type ()->code () == TYPE_CODE_ENUM))
-		    continue;
-		  [[fallthrough]];
-
-		case symbol_search_kind::FUNCTION:
-		  if (type_regex.has_value ()
-		      && !treg_matches_sym_type_name (*type_regex, sym))
-		    continue;
-		  break;
-
-		case symbol_search_kind::MODULE:
-		  /* Skip module declarations, keep only definitions.  */
-		  if (sym->line () == 0)
-		    continue;
-		  break;
-		}
-
 	      /* Match, insert if not already in the results.  */
 	      if (result_set->size () < m_max_search_results)
 		result_set->emplace (block, sym);
@@ -4908,34 +4940,25 @@ global_symbol_searcher::add_matching_msymbols
     {
       QUIT;
 
-      if (msymbol->created_by_gdb)
+      if (!msymbol_matches (msymbol, name_regex))
 	continue;
 
-      if (is_suitable_msymbol (msymbol))
-	{
-	  if (!name_regex.has_value ()
-	      || name_regex->exec (msymbol->natural_name (), 0,
-				   NULL, 0) == 0)
-	    {
-	      /* For functions we can do a quick check of whether the
-		 symbol might be found via find_pc_symtab.  */
-	      if (m_kind != symbol_search_kind::FUNCTION
-		  || (find_compunit_symtab_for_pc
-		      (msymbol->value_address (objfile)) == NULL))
-		{
-		  if (lookup_symbol_in_objfile_from_linkage_name
-		      (objfile, msymbol->linkage_name (),
-		       SEARCH_VFT).symbol == NULL)
-		    {
-		      /* Matching msymbol, add it to the results list.  */
-		      if (results->size () < m_max_search_results)
-			results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
-		      else
-			return false;
-		    }
-		}
-	    }
-	}
+      /* For functions we can do a quick check of whether the
+	 symbol might be found via find_pc_symtab.  */
+      if (m_kind == symbol_search_kind::FUNCTION
+	  && (find_compunit_symtab_for_pc
+	      (msymbol->value_address (objfile)) != NULL))
+	continue;
+
+      if (lookup_symbol_in_objfile_from_linkage_name
+	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
+	continue;
+
+      /* Matching msymbol, add it to the results list.  */
+      if (results->size () < m_max_search_results)
+	results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
+      else
+	return false;
     }
 
   return true;
diff --git a/gdb/symtab.h b/gdb/symtab.h
index acbe41d131b7..5dd0475e4491 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2675,6 +2675,19 @@ class global_symbol_searcher
   /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
   std::optional<compiled_regex> compile_type_regex () const;
 
+  /* Return true if SYMTAB matches one of M_FILENAMES.  */
+  bool symtab_matches_filenames (symtab *symtab) const;
+
+  /* Return true if SYM is of type M_KIND, is in a file matching
+     M_FILENAMES, 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 type M_KIND and matches NAME_REGEX.  */
+  bool msymbol_matches (const minimal_symbol *msymbol,
+			const std::optional<compiled_regex> &name_regex) const;
+
   /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type M_KIND.
      Return true if a matching function msymbol without debug info was
      seen, meaning that msymbols should later be considered for the results
-- 
2.55.0

RE: [PATCH 7/9] gdb: add matching helpers to global_symbol_searcher

[Rohr, Stephan]

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 7/9] gdb: add matching helpers to global_symbol_searcher
> 
> The methods of global_symbol_searcher repeat the same checks in several
> places.  Add some helpers to make the code easier to follow:
> 
>  - name_matches: check a name against the (optional) name regexp
> 
>  - symtab_matches_filenames: check a symtab against the searched file
>    names
> 
>  - symbol_matches: all the filtering done on full symbols in
>    add_matching_symbols
> 
>  - msymbol_matches: the filtering done on minimal symbols, shared by
>    expand_symtabs and add_matching_msymbols
> 
> Switch expand_symtabs, add_matching_symbols and
> add_matching_msymbols to
> use early continues to avoid deeply nested ifs.
> 
> Change-Id: I553f70fb8ea9c7e55b89bb1204a402938c116c3b
> ---
>  gdb/symtab.c | 205 ++++++++++++++++++++++++++++-----------------------
>  gdb/symtab.h |  13 ++++
>  2 files changed, 127 insertions(+), 91 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 9ce6911666c6..79e43eafde18 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4741,6 +4741,93 @@ global_symbol_searcher::is_suitable_msymbol
>      }
>  }
> 
> +/* Return true if NAME matches NAME_REGEX, or if NAME_REGEX is empty.
> */
> +
> +static bool
> +name_matches (const std::optional<compiled_regex> &name_regex,
> +	      const char *name)
> +{
> +  return (!name_regex.has_value ()
> +	  || name_regex->exec (name, 0, nullptr, 0) == 0);
> +}
> +
> +/* See symtab.h.  */
> +
> +bool
> +global_symbol_searcher::symtab_matches_filenames (symtab *symtab)
> const
> +{
> +  /* Check first sole SYMTAB->FILENAME.  It does not need to be a substring
> +     of symtab_to_fullname as it may contain "./" etc.  */
> +  if (file_matches (symtab->filename (), m_filenames, false))
> +    return true;
> +
> +  if (!basenames_may_differ
> +      && !file_matches (lbasename (symtab->filename ()), m_filenames, true))
> +    return false;
> +
> +  return file_matches (symtab_to_fullname (symtab), m_filenames, false);
> +}
> +
> +/* See symtab.h.  */
> +
> +bool
> +global_symbol_searcher::symbol_matches
> +	(const symbol *sym,
> +	 const std::optional<compiled_regex> &name_regex,
> +	 const std::optional<compiled_regex> &type_regex) const
> +{
> +  if (!symtab_matches_filenames (sym->symtab ()))
> +    return false;
> +
> +  if (!sym->matches (to_search_flags (m_kind)))
> +    return false;
> +
> +  if (!name_matches (name_regex, sym->natural_name ()))
> +    return false;
> +
> +  switch (m_kind)
> +    {
> +    case symbol_search_kind::VARIABLE:
> +      /* LOC_CONST can be used for more than just enums, e.g., C++ static
> +	 const members.  We only want to skip enums here.  */
> +      if (sym->loc_class () == LOC_UNRESOLVED
> +	  || (sym->loc_class () == LOC_CONST
> +	      && sym->type ()->code () == TYPE_CODE_ENUM))
> +	return false;
> +      [[fallthrough]];
> +
> +    case symbol_search_kind::FUNCTION:
> +      if (type_regex.has_value ()
> +	  && !treg_matches_sym_type_name (*type_regex, sym))
> +	return false;
> +      break;
> +
> +    case symbol_search_kind::MODULE:
> +      /* Skip module declarations, keep only definitions.  */
> +      if (sym->line () == 0)
> +	return false;
> +      break;
> +    }
> +
> +  return true;
> +}
> +
> +/* See symtab.h.  */
> +
> +bool
> +global_symbol_searcher::msymbol_matches
> +	(const minimal_symbol *msymbol,
> +	 const std::optional<compiled_regex> &name_regex) const
> +{
> +  if (msymbol->created_by_gdb)
> +    return false;
> +
> +  if (!is_suitable_msymbol (msymbol))
> +    return false;
> +
> +  return name_matches (name_regex, msymbol->natural_name ());
> +}
> +
>  /* See symtab.h.  */
> 
>  bool
> @@ -4763,8 +4850,7 @@ global_symbol_searcher::expand_symtabs
>       &lookup_name_info::match_any (),
>       [&] (const char *symname)
>       {
> -       return (!name_regex.has_value ()
> -	       || name_regex->exec (symname, 0, NULL, 0) == 0);
> +       return name_matches (name_regex, symname);
>       },
>       NULL,
>       SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
> @@ -4790,25 +4876,16 @@ global_symbol_searcher::expand_symtabs
>  	{
>  	  QUIT;
> 
> -	  if (msymbol->created_by_gdb)
> +	  if (!msymbol_matches (msymbol, name_regex))
>  	    continue;
> 
> -	  if (is_suitable_msymbol (msymbol))
> -	    {
> -	      if (!name_regex.has_value ()
> -		  || name_regex->exec (msymbol->natural_name (), 0,
> -				       NULL, 0) == 0)
> -		{
> -		  /* An important side-effect of this lookup function is
> -		     to expand the symbol table if msymbol is found, later
> -		     in the process we will add matching symbols or
> -		     msymbols to the results list, and that requires that
> -		     the symbols tables are expanded.  */
> -		  if (find_compunit_symtab_for_pc
> -			(msymbol->value_address (objfile)) == nullptr)
> -		    found_func_msymbol_without_debug_info = true;
> -		}
> -	    }
> +	  /* An important side-effect of this lookup function is to expand
> +	     the symbol table if msymbol is found, later in the process we
> +	     will add matching symbols or msymbols to the results list, and
> +	     that requires that the symbols tables are expanded.  */
> +	  if (find_compunit_symtab_for_pc
> +		(msymbol->value_address (objfile)) == nullptr)
> +	    found_func_msymbol_without_debug_info = true;
>  	}
>      }
> 
> @@ -4824,8 +4901,6 @@ global_symbol_searcher::add_matching_symbols
>  	 const std::optional<compiled_regex> &type_regex,
>  	 std::set<symbol_search> *result_set) const
>  {
> -  domain_search_flags domain = to_search_flags (m_kind);

IIUC this is moved to "symbol_matches"?  This implies "to_search_flags" is run
for every symbol.  This looks unnecessary.  We could add an additional argument to
"symbol_matches".

> -
>    /* Add matching symbols (if not already present).  */
>    for (compunit_symtab &cust : objfile->compunits ())
>      {
> @@ -4837,54 +4912,11 @@
> global_symbol_searcher::add_matching_symbols
> 
>  	  for (struct symbol *sym : block_iterator_range (b))
>  	    {
> -	      struct symtab *real_symtab = sym->symtab ();
> -
>  	      QUIT;
> 
> -	      /* Check first sole REAL_SYMTAB->FILENAME.  It does
> -		 not need to be a substring of symtab_to_fullname as
> -		 it may contain "./" etc.  */
> -	      if (!(file_matches (real_symtab->filename (), m_filenames, false)
> -		    || ((basenames_may_differ
> -			 || file_matches (lbasename (real_symtab->filename
> ()),
> -					  m_filenames, true))
> -			&& file_matches (symtab_to_fullname (real_symtab),
> -					 m_filenames, false))))
> +	      if (!symbol_matches (sym, name_regex, type_regex))
>  		continue;
> 
> -	      if (!sym->matches (domain))
> -		continue;
> -
> -	      if (name_regex.has_value ()
> -		  && name_regex->exec (sym->natural_name (), 0,
> -				       nullptr, 0) != 0)
> -		continue;
> -
> -	      switch (m_kind)
> -		{
> -		case symbol_search_kind::VARIABLE:
> -		  /* LOC_CONST can be used for more than just enums, e.g.,
> -		     C++ static const members.  We only want to skip enums
> -		     here.  */
> -		  if (sym->loc_class () == LOC_UNRESOLVED
> -		      || (sym->loc_class () == LOC_CONST
> -			  && sym->type ()->code () == TYPE_CODE_ENUM))
> -		    continue;
> -		  [[fallthrough]];
> -
> -		case symbol_search_kind::FUNCTION:
> -		  if (type_regex.has_value ()
> -		      && !treg_matches_sym_type_name (*type_regex, sym))
> -		    continue;
> -		  break;
> -
> -		case symbol_search_kind::MODULE:
> -		  /* Skip module declarations, keep only definitions.  */
> -		  if (sym->line () == 0)
> -		    continue;
> -		  break;
> -		}
> -
>  	      /* Match, insert if not already in the results.  */
>  	      if (result_set->size () < m_max_search_results)
>  		result_set->emplace (block, sym);
> @@ -4908,34 +4940,25 @@
> global_symbol_searcher::add_matching_msymbols
>      {
>        QUIT;
> 
> -      if (msymbol->created_by_gdb)
> +      if (!msymbol_matches (msymbol, name_regex))
>  	continue;
> 
> -      if (is_suitable_msymbol (msymbol))
> -	{
> -	  if (!name_regex.has_value ()
> -	      || name_regex->exec (msymbol->natural_name (), 0,
> -				   NULL, 0) == 0)
> -	    {
> -	      /* For functions we can do a quick check of whether the
> -		 symbol might be found via find_pc_symtab.  */
> -	      if (m_kind != symbol_search_kind::FUNCTION
> -		  || (find_compunit_symtab_for_pc
> -		      (msymbol->value_address (objfile)) == NULL))
> -		{
> -		  if (lookup_symbol_in_objfile_from_linkage_name
> -		      (objfile, msymbol->linkage_name (),
> -		       SEARCH_VFT).symbol == NULL)
> -		    {
> -		      /* Matching msymbol, add it to the results list.  */
> -		      if (results->size () < m_max_search_results)
> -			results->emplace_back (GLOBAL_BLOCK, msymbol,
> objfile);
> -		      else
> -			return false;
> -		    }
> -		}
> -	    }
> -	}
> +      /* For functions we can do a quick check of whether the
> +	 symbol might be found via find_pc_symtab.  */
> +      if (m_kind == symbol_search_kind::FUNCTION
> +	  && (find_compunit_symtab_for_pc
> +	      (msymbol->value_address (objfile)) != NULL))
> +	continue;
> +
> +      if (lookup_symbol_in_objfile_from_linkage_name
> +	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
> +	continue;
> +
> +      /* Matching msymbol, add it to the results list.  */
> +      if (results->size () < m_max_search_results)
> +	results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
> +      else
> +	return false;
>      }
> 
>    return true;
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index acbe41d131b7..5dd0475e4491 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2675,6 +2675,19 @@ class global_symbol_searcher
>    /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
>    std::optional<compiled_regex> compile_type_regex () const;
> 
> +  /* Return true if SYMTAB matches one of M_FILENAMES.  */
> +  bool symtab_matches_filenames (symtab *symtab) const;
> +
> +  /* Return true if SYM is of type M_KIND, is in a file matching

Rephrase to "is of kind M_KIND"? 

> +     M_FILENAMES, 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 type M_KIND and matches NAME_REGEX.
> */

Same here.

> +  bool msymbol_matches (const minimal_symbol *msymbol,
> +			const std::optional<compiled_regex> &name_regex)
> const;

The function also checks the "created_by_gdb" flag and "is_suitable_msymbol".
Should this be mentioned in the comment, too?

> +
>    /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type
> M_KIND.
>       Return true if a matching function msymbol without debug info was
>       seen, meaning that msymbols should later be considered for the results
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 7/9] gdb: add matching helpers to global_symbol_searcher

[Simon Marchi]

(I failed to reply-all the first time, adding gdb-patches this time)

On 2026-09-30 09:51, Rohr, Stephan wrote:
>> @@ -4824,8 +4901,6 @@ global_symbol_searcher::add_matching_symbols
>>  	 const std::optional<compiled_regex> &type_regex,
>>  	 std::set<symbol_search> *result_set) const
>>  {
>> -  domain_search_flags domain = to_search_flags (m_kind);
> 
> IIUC this is moved to "symbol_matches"?  This implies "to_search_flags" is run
> for every symbol.  This looks unnecessary.  We could add an additional argument to
> "symbol_matches".

I thought about that, but I am not sure it matters in practice, since
to_search_flags compiles down to so few instructions.

If you insist that we shouldn't do this call for every minsym, I would
add an m_domain field and pre-compute it.  But I don't think it's
needed.

>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index acbe41d131b7..5dd0475e4491 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2675,6 +2675,19 @@ class global_symbol_searcher
>>    /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
>>    std::optional<compiled_regex> compile_type_regex () const;
>>
>> +  /* Return true if SYMTAB matches one of M_FILENAMES.  */
>> +  bool symtab_matches_filenames (symtab *symtab) const;
>> +
>> +  /* Return true if SYM is of type M_KIND, is in a file matching
> 
> Rephrase to "is of kind M_KIND"? 

Done.

>> +     M_FILENAMES, 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 type M_KIND and matches NAME_REGEX.
>> */
> 
> Same here.

Done.

>> +  bool msymbol_matches (const minimal_symbol *msymbol,
>> +			const std::optional<compiled_regex> &name_regex)
>> const;
> 
> The function also checks the "created_by_gdb" flag and "is_suitable_msymbol".
> Should this be mentioned in the comment, too?

is_suitable_symbol basically checks m_kind, so it's already covered in
the comment.

As for created_by_gdb... It's subjective, but I don't think that a
function doc like this necessarily needs to talk about all the details
of the implementation.  But anyway, I added a mention:

  /* Return true if MSYMBOL is of kind M_KIND and matches NAME_REGEX.

     This function also filters out the minimal symbols artificially created by
     GDB.  */

Simon

[PATCH 8/9] gdb: pass result containers by reference in global_symbol_searcher

[Simon Marchi]

add_matching_symbols and add_matching_msymbols take the container to
add results to as a pointer, but it can never be nullptr.  Pass it by
reference instead.

Change-Id: I65d8e14f2f3152d83afbe3b348fc91a8817b87fa
---
 gdb/symtab.c | 17 ++++++++---------
 gdb/symtab.h |  4 ++--
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 79e43eafde18..10a794004d33 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4899,7 +4899,7 @@ global_symbol_searcher::add_matching_symbols
 	(objfile *objfile,
 	 const std::optional<compiled_regex> &name_regex,
 	 const std::optional<compiled_regex> &type_regex,
-	 std::set<symbol_search> *result_set) const
+	 std::set<symbol_search> &result_set) const
 {
   /* Add matching symbols (if not already present).  */
   for (compunit_symtab &cust : objfile->compunits ())
@@ -4918,8 +4918,8 @@ global_symbol_searcher::add_matching_symbols
 		continue;
 
 	      /* Match, insert if not already in the results.  */
-	      if (result_set->size () < m_max_search_results)
-		result_set->emplace (block, sym);
+	      if (result_set.size () < m_max_search_results)
+		result_set.emplace (block, sym);
 	      else
 		return false;
 	    }
@@ -4934,7 +4934,7 @@ global_symbol_searcher::add_matching_symbols
 bool
 global_symbol_searcher::add_matching_msymbols
 	(objfile *objfile, const std::optional<compiled_regex> &name_regex,
-	 std::vector<symbol_search> *results) const
+	 std::vector<symbol_search> &results) const
 {
   for (minimal_symbol *msymbol : objfile->msymbols ())
     {
@@ -4955,8 +4955,8 @@ global_symbol_searcher::add_matching_msymbols
 	continue;
 
       /* Matching msymbol, add it to the results list.  */
-      if (results->size () < m_max_search_results)
-	results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
+      if (results.size () < m_max_search_results)
+	results.emplace_back (GLOBAL_BLOCK, msymbol, objfile);
       else
 	return false;
     }
@@ -5056,8 +5056,7 @@ global_symbol_searcher::search () const
 	 RESULT_SET set.  Use a set here so that we can easily detect
 	 duplicates as we go, and can therefore track how many unique
 	 matches we have found so far.  */
-      if (!add_matching_symbols (&objfile, name_regex, type_regex,
-				 &result_set))
+      if (!add_matching_symbols (&objfile, name_regex, type_regex, result_set))
 	break;
     }
 
@@ -5077,7 +5076,7 @@ global_symbol_searcher::search () const
       gdb_assert (m_kind == symbol_search_kind::VARIABLE
 		  || m_kind == symbol_search_kind::FUNCTION);
       for (objfile &objfile : current_program_space->objfiles ())
-	if (!add_matching_msymbols (&objfile, name_regex, &result))
+	if (!add_matching_msymbols (&objfile, name_regex, result))
 	  break;
     }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 5dd0475e4491..e382b64b427c 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2705,7 +2705,7 @@ class global_symbol_searcher
   bool add_matching_symbols (objfile *objfile,
 			     const std::optional<compiled_regex> &name_regex,
 			     const std::optional<compiled_regex> &type_regex,
-			     std::set<symbol_search> *result_set) const;
+			     std::set<symbol_search> &result_set) const;
 
   /* Add msymbols from OBJFILE that match NAME_REGEX and M_KIND, to the
      results vector RESULTS.  Return false if we stop adding results early
@@ -2715,7 +2715,7 @@ class global_symbol_searcher
      didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
   bool add_matching_msymbols (objfile *objfile,
 			      const std::optional<compiled_regex> &name_regex,
-			      std::vector<symbol_search> *results) const;
+			      std::vector<symbol_search> &results) const;
 
   /* Return true if MSYMBOL is of type M_KIND.  */
   bool is_suitable_msymbol (const minimal_symbol *msymbol) const;
-- 
2.55.0

RE: [PATCH 8/9] gdb: pass result containers by reference in global_symbol_searcher

[Rohr, Stephan]

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:39
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 8/9] gdb: pass result containers by reference in
> global_symbol_searcher
> 
> add_matching_symbols and add_matching_msymbols take the container to
> add results to as a pointer, but it can never be nullptr.  Pass it by
> reference instead.
> 
> Change-Id: I65d8e14f2f3152d83afbe3b348fc91a8817b87fa
> ---
>  gdb/symtab.c | 17 ++++++++---------
>  gdb/symtab.h |  4 ++--
>  2 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 79e43eafde18..10a794004d33 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4899,7 +4899,7 @@ global_symbol_searcher::add_matching_symbols
>  	(objfile *objfile,
>  	 const std::optional<compiled_regex> &name_regex,
>  	 const std::optional<compiled_regex> &type_regex,
> -	 std::set<symbol_search> *result_set) const
> +	 std::set<symbol_search> &result_set) const

Just a general question: does it make sense to keep "objfile" as pointer
or could this also be converted to reference for consistency?

>  {
>    /* Add matching symbols (if not already present).  */
>    for (compunit_symtab &cust : objfile->compunits ())
> @@ -4918,8 +4918,8 @@ global_symbol_searcher::add_matching_symbols
>  		continue;
> 
>  	      /* Match, insert if not already in the results.  */
> -	      if (result_set->size () < m_max_search_results)
> -		result_set->emplace (block, sym);
> +	      if (result_set.size () < m_max_search_results)
> +		result_set.emplace (block, sym);
>  	      else
>  		return false;
>  	    }
> @@ -4934,7 +4934,7 @@ global_symbol_searcher::add_matching_symbols
>  bool
>  global_symbol_searcher::add_matching_msymbols
>  	(objfile *objfile, const std::optional<compiled_regex> &name_regex,
> -	 std::vector<symbol_search> *results) const
> +	 std::vector<symbol_search> &results) const
>  {
>    for (minimal_symbol *msymbol : objfile->msymbols ())
>      {
> @@ -4955,8 +4955,8 @@
> global_symbol_searcher::add_matching_msymbols
>  	continue;
> 
>        /* Matching msymbol, add it to the results list.  */
> -      if (results->size () < m_max_search_results)
> -	results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
> +      if (results.size () < m_max_search_results)
> +	results.emplace_back (GLOBAL_BLOCK, msymbol, objfile);
>        else
>  	return false;
>      }
> @@ -5056,8 +5056,7 @@ global_symbol_searcher::search () const
>  	 RESULT_SET set.  Use a set here so that we can easily detect
>  	 duplicates as we go, and can therefore track how many unique
>  	 matches we have found so far.  */
> -      if (!add_matching_symbols (&objfile, name_regex, type_regex,
> -				 &result_set))
> +      if (!add_matching_symbols (&objfile, name_regex, type_regex,
> result_set))
>  	break;
>      }
> 
> @@ -5077,7 +5076,7 @@ global_symbol_searcher::search () const
>        gdb_assert (m_kind == symbol_search_kind::VARIABLE
>  		  || m_kind == symbol_search_kind::FUNCTION);
>        for (objfile &objfile : current_program_space->objfiles ())
> -	if (!add_matching_msymbols (&objfile, name_regex, &result))
> +	if (!add_matching_msymbols (&objfile, name_regex, result))
>  	  break;
>      }
> 
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 5dd0475e4491..e382b64b427c 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2705,7 +2705,7 @@ class global_symbol_searcher
>    bool add_matching_symbols (objfile *objfile,
>  			     const std::optional<compiled_regex> &name_regex,
>  			     const std::optional<compiled_regex> &type_regex,
> -			     std::set<symbol_search> *result_set) const;
> +			     std::set<symbol_search> &result_set) const;
> 

Nit: The comment in symtab.h mentions "RESULTS_SET", but it should be
"RESULT_SET".  This is not the correct patch to address this, I just noticed now.

>    /* Add msymbols from OBJFILE that match NAME_REGEX and M_KIND, to
> the
>       results vector RESULTS.  Return false if we stop adding results early
> @@ -2715,7 +2715,7 @@ class global_symbol_searcher
>       didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
>    bool add_matching_msymbols (objfile *objfile,
>  			      const std::optional<compiled_regex>
> &name_regex,
> -			      std::vector<symbol_search> *results) const;
> +			      std::vector<symbol_search> &results) const;
> 
>    /* Return true if MSYMBOL is of type M_KIND.  */
>    bool is_suitable_msymbol (const minimal_symbol *msymbol) const;
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 8/9] gdb: pass result containers by reference in global_symbol_searcher

[Simon Marchi]

On 2026-09-30 09:58, Rohr, Stephan wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Marchi <simon.marchi@efficios.com>
>> Sent: Tuesday, 29 September 2026 21:39
>> To: gdb-patches@sourceware.org
>> Cc: Simon Marchi <simon.marchi@efficios.com>
>> Subject: [PATCH 8/9] gdb: pass result containers by reference in
>> global_symbol_searcher
>>
>> add_matching_symbols and add_matching_msymbols take the container to
>> add results to as a pointer, but it can never be nullptr.  Pass it by
>> reference instead.
>>
>> Change-Id: I65d8e14f2f3152d83afbe3b348fc91a8817b87fa
>> ---
>>  gdb/symtab.c | 17 ++++++++---------
>>  gdb/symtab.h |  4 ++--
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 79e43eafde18..10a794004d33 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4899,7 +4899,7 @@ global_symbol_searcher::add_matching_symbols
>>  	(objfile *objfile,
>>  	 const std::optional<compiled_regex> &name_regex,
>>  	 const std::optional<compiled_regex> &type_regex,
>> -	 std::set<symbol_search> *result_set) const
>> +	 std::set<symbol_search> &result_set) const
> 
> Just a general question: does it make sense to keep "objfile" as pointer
> or could this also be converted to reference for consistency?

It could be converted to a reference as well, I'll do it.

>> diff --git a/gdb/symtab.h b/gdb/symtab.h
>> index 5dd0475e4491..e382b64b427c 100644
>> --- a/gdb/symtab.h
>> +++ b/gdb/symtab.h
>> @@ -2705,7 +2705,7 @@ class global_symbol_searcher
>>    bool add_matching_symbols (objfile *objfile,
>>  			     const std::optional<compiled_regex> &name_regex,
>>  			     const std::optional<compiled_regex> &type_regex,
>> -			     std::set<symbol_search> *result_set) const;
>> +			     std::set<symbol_search> &result_set) const;
>>
> 
> Nit: The comment in symtab.h mentions "RESULTS_SET", but it should be
> "RESULT_SET".  This is not the correct patch to address this, I just noticed now.

Fixed that too.

Thanks,

Simon

[PATCH 9/9] gdb: fix some comments in global_symbol_searcher

[Simon Marchi]

Fix a few comments in and around global_symbol_searcher:

 - The comment of global_symbol_searcher::m_exclude_minsyms refers to
   M_SYMBOL_REGEXP, which does not (or no longer) exist.  It means
   M_SYMBOL_NAME_REGEXP.

 - The comment in global_symbol_searcher::add_matching_msymbols refers
   to find_pc_symtab, which does not exist anymore.  The code calls
   find_compunit_symtab_for_pc.

 - The comment of global_symbol_searcher calls it a structure, but it
   is a class.

Change-Id: I0ec18651688406bb525752a53b9558096a798aef
---
 gdb/symtab.c | 2 +-
 gdb/symtab.h | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 10a794004d33..611815a568d6 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4944,7 +4944,7 @@ global_symbol_searcher::add_matching_msymbols
 	continue;
 
       /* For functions we can do a quick check of whether the
-	 symbol might be found via find_pc_symtab.  */
+	 symbol might be found via find_compunit_symtab_for_pc.  */
       if (m_kind == symbol_search_kind::FUNCTION
 	  && (find_compunit_symtab_for_pc
 	      (msymbol->value_address (objfile)) != NULL))
diff --git a/gdb/symtab.h b/gdb/symtab.h
index e382b64b427c..1d92907fce2c 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2604,8 +2604,8 @@ enum class symbol_search_kind
 };
 
 /* In order to search for global symbols of a particular kind matching
-   particular regular expressions, create an instance of this structure and
-   call the SEARCH member function.  */
+   particular regular expressions, create an instance of this class and call
+   the SEARCH member function.  */
 class global_symbol_searcher
 {
 public:
@@ -2661,8 +2661,8 @@ class global_symbol_searcher
   /* Regular expression to match against the symbol type.  */
   const char *m_symbol_type_regexp = nullptr;
 
-  /* When this flag is false then minsyms that match M_SYMBOL_REGEXP will
-     be included in the results, otherwise they are excluded.  */
+  /* When this flag is false then minsyms that match M_SYMBOL_NAME_REGEXP
+     will be included in the results, otherwise they are excluded.  */
   bool m_exclude_minsyms = false;
 
   /* Maximum number of search results.  We currently impose a hard limit
-- 
2.55.0

RE: [PATCH 9/9] gdb: fix some comments in global_symbol_searcher

[Rohr, Stephan]

We could fix the comment regarding "RESULTS_SET" I mentioned in
patch #8 feedback in this patch.  In the same comment in symtab.h for methods
"add_matching_symbols" and "add_matching_msymbols" we also have
"MAX_SEARCH_RESULTS" vs. "M_MAX_SEARCH_RESULTS".  Which one is correct?
We could use this patch for proper alignment.

> -----Original Message-----
> From: Simon Marchi <simon.marchi@efficios.com>
> Sent: Tuesday, 29 September 2026 21:40
> To: gdb-patches@sourceware.org
> Cc: Simon Marchi <simon.marchi@efficios.com>
> Subject: [PATCH 9/9] gdb: fix some comments in global_symbol_searcher
> 
> Fix a few comments in and around global_symbol_searcher:
> 
>  - The comment of global_symbol_searcher::m_exclude_minsyms refers to
>    M_SYMBOL_REGEXP, which does not (or no longer) exist.  It means
>    M_SYMBOL_NAME_REGEXP.
> 
>  - The comment in global_symbol_searcher::add_matching_msymbols refers
>    to find_pc_symtab, which does not exist anymore.  The code calls
>    find_compunit_symtab_for_pc.
> 
>  - The comment of global_symbol_searcher calls it a structure, but it
>    is a class.
> 
> Change-Id: I0ec18651688406bb525752a53b9558096a798aef
> ---
>  gdb/symtab.c | 2 +-
>  gdb/symtab.h | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 10a794004d33..611815a568d6 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4944,7 +4944,7 @@
> global_symbol_searcher::add_matching_msymbols
>  	continue;
> 
>        /* For functions we can do a quick check of whether the
> -	 symbol might be found via find_pc_symtab.  */
> +	 symbol might be found via find_compunit_symtab_for_pc.  */
>        if (m_kind == symbol_search_kind::FUNCTION
>  	  && (find_compunit_symtab_for_pc
>  	      (msymbol->value_address (objfile)) != NULL))
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index e382b64b427c..1d92907fce2c 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -2604,8 +2604,8 @@ enum class symbol_search_kind
>  };
> 
>  /* In order to search for global symbols of a particular kind matching
> -   particular regular expressions, create an instance of this structure and
> -   call the SEARCH member function.  */
> +   particular regular expressions, create an instance of this class and call
> +   the SEARCH member function.  */
>  class global_symbol_searcher
>  {
>  public:
> @@ -2661,8 +2661,8 @@ class global_symbol_searcher
>    /* Regular expression to match against the symbol type.  */
>    const char *m_symbol_type_regexp = nullptr;
> 
> -  /* When this flag is false then minsyms that match M_SYMBOL_REGEXP will
> -     be included in the results, otherwise they are excluded.  */
> +  /* When this flag is false then minsyms that match
> M_SYMBOL_NAME_REGEXP
> +     will be included in the results, otherwise they are excluded.  */
>    bool m_exclude_minsyms = false;
> 
>    /* Maximum number of search results.  We currently impose a hard limit
> --
> 2.55.0

________________________________________
Intel Deutschland GmbH 

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany 

Tel: +49 (89) 99143-0 

www.intel.de 

Managing Directors: Candice Moore, Jeffrey Schneiderman, Ramachandran Sitaraman

Chairperson of the Supervisory Board: Sonja Pierer

Registered Seat: Munich Commercial Register B: Amtsgericht Munich HRB 186928

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH 9/9] gdb: fix some comments in global_symbol_searcher

[Simon Marchi]

On 2026-09-30 10:01, Rohr, Stephan wrote:
> We could fix the comment regarding "RESULTS_SET" I mentioned in
> patch #8 feedback in this patch.  In the same comment in symtab.h for methods
> "add_matching_symbols" and "add_matching_msymbols" we also have
> "MAX_SEARCH_RESULTS" vs. "M_MAX_SEARCH_RESULTS".  Which one is correct?
> We could use this patch for proper alignment.

Ok, I included the RESULT_SET fix in this patch.

It would be M_MAX_SEARCH_RESULTS nowadays.

Simon

[PATCH v2 00/11] Cleanups in global_symbol_searcher

[simon.marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

This is v2 of:

  https://inbox.sourceware.org/gdb-patches/20260929194119.155169-1-simon.marchi@efficios.com/T/#m02147ed61720d62b82a7514f72c29f3863ca046d

where I address most of Stephan's comments.

There are 2 new patches:

 - I split the preg/treg renaming in its own patch
 - I added one patch to make stack.c re-use a function from symtab.c to
   compile the symbol search regex

Simon Marchi (11):
  gdb: add symbol_search_kind enum for global_symbol_searcher
  gdb: rename some preg/treg variables
  gdb: factor out regexp compilation from global_symbol_searcher::search
  gdb: use compile_symbol_search_regex in stack.c
  gdb: simplify insertion in
    global_symbol_searcher::add_matching_symbols
  gdb: use a switch on m_kind in add_matching_symbols
  gdb: make global_symbol_searcher::is_suitable_msymbol non-static
  gdb: update comments of global_symbol_searcher::expand_symtabs
  gdb: add matching helpers to global_symbol_searcher
  gdb: change some pointers to references in global_symbol_searcher
  gdb: fix some comments in global_symbol_searcher

 gdb/mi/mi-symbol-cmds.c |  36 +--
 gdb/python/python.c     |   2 +-
 gdb/stack.c             |  63 ++----
 gdb/symtab.c            | 471 ++++++++++++++++++++++------------------
 gdb/symtab.h            | 137 ++++++++----
 5 files changed, 401 insertions(+), 308 deletions(-)


base-commit: 8eeaa4ee84d3b590321f7a3595a3c7ba8da31a11
-- 
2.55.0

[PATCH v2 01/11] gdb: add symbol_search_kind enum for global_symbol_searcher

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

While reading the global_symbol_searcher code, something that was not clear
to me was that despite accepting a domain_search_flags, searches are
mutually exclusive between types, variables and functions.

One snippet of code that really confused me was (now removed by previous
commit ca1c3c309d "gdb: Skip useless minsym scan in variable-only symbol
searches"):

		  if ((kind & SEARCH_FUNCTION_DOMAIN) != 0
		      ? (find_compunit_symtab_for_pc
			 (msymbol->value_address (objfile)) == NULL)
		      : (lookup_symbol_in_objfile_from_linkage_name
			 (objfile, msymbol->linkage_name (),
			  SEARCH_VFT)
			 .symbol == NULL))

I kept thinking: what if you search for both functions and variables,
this will take the "function" path, this can't work.  The answer is that
it's not supposed to happen.

This patch adds the symbol_search_kind enum and uses it in
global_symbol_searcher (and a few caller functions), making it clear
that you search just one kind at a time.

The new enum has a certain overlap with enum domain_search_flag, but I
think that having the more scoped enum makes global_symbol_searcher
easier to understand, by specifying clearly what it needs to handle.

Change-Id: I4a3f4752c7be51a4da5831f9780d21e6e7271b38
---
 gdb/mi/mi-symbol-cmds.c |  36 +++++++------
 gdb/python/python.c     |   2 +-
 gdb/symtab.c            | 112 ++++++++++++++++++++++------------------
 gdb/symtab.h            |  34 ++++++++----
 4 files changed, 106 insertions(+), 78 deletions(-)

diff --git a/gdb/mi/mi-symbol-cmds.c b/gdb/mi/mi-symbol-cmds.c
index 58f2feef5533..30b8ec495e8f 100644
--- a/gdb/mi/mi-symbol-cmds.c
+++ b/gdb/mi/mi-symbol-cmds.c
@@ -71,7 +71,7 @@ mi_cmd_symbol_list_lines (const char *command, const char *const *argv,
    results.  */
 
 static void
-output_debug_symbol (ui_out *uiout, domain_search_flags kind,
+output_debug_symbol (ui_out *uiout, symbol_search_kind kind,
 		     struct symbol *sym, int block)
 {
   ui_out_emit_tuple tuple_emitter (uiout);
@@ -80,7 +80,8 @@ output_debug_symbol (ui_out *uiout, domain_search_flags kind,
     uiout->field_unsigned ("line", sym->line ());
   uiout->field_string ("name", sym->print_name ());
 
-  if ((kind & (SEARCH_FUNCTION_DOMAIN | SEARCH_VAR_DOMAIN)) != 0)
+  if (kind == symbol_search_kind::FUNCTION
+      || kind == symbol_search_kind::VARIABLE)
     {
       string_file tmp_stream;
       type_print (sym->type (), "", &tmp_stream, -1);
@@ -111,7 +112,7 @@ output_nondebug_symbol (ui_out *uiout, const bound_minimal_symbol &msymbol)
    and then prints the matching [m]symbols in an MI structured format.  */
 
 static void
-mi_symbol_info (domain_search_flags kind, const char *name_regexp,
+mi_symbol_info (symbol_search_kind kind, const char *name_regexp,
 		const char *type_regexp, bool exclude_minsyms,
 		size_t max_results)
 {
@@ -188,7 +189,7 @@ parse_max_results_option (const char *arg)
    Processes command line options from ARGV and ARGC.  */
 
 static void
-mi_info_functions_or_variables (domain_search_flags kind,
+mi_info_functions_or_variables (symbol_search_kind kind,
 				const char *const *argv, int argc)
 {
   size_t max_results = SIZE_MAX;
@@ -215,7 +216,7 @@ mi_info_functions_or_variables (domain_search_flags kind,
   while (1)
     {
       const char *cmd_string
-	= ((kind == SEARCH_FUNCTION_DOMAIN)
+	= ((kind == symbol_search_kind::FUNCTION)
 	   ? "-symbol-info-functions" : "-symbol-info-variables");
       int opt = mi_getopt (cmd_string, argc, argv, opts, &oind, &oarg);
       if (opt < 0)
@@ -251,7 +252,7 @@ using module_symbol_search_iterator
 static module_symbol_search_iterator
 output_module_symbols_in_single_module_and_file
 	(struct ui_out *uiout, module_symbol_search_iterator iter,
-	 const module_symbol_search_iterator end, domain_search_flags kind)
+	 const module_symbol_search_iterator end, symbol_search_kind kind)
 {
   /* The symbol for the module in which the first result resides.  */
   const symbol *first_module_symbol = iter->first.symbol;
@@ -286,7 +287,7 @@ output_module_symbols_in_single_module_and_file
 static module_symbol_search_iterator
 output_module_symbols_in_single_module
 	(struct ui_out *uiout, module_symbol_search_iterator iter,
-	 const module_symbol_search_iterator end, domain_search_flags kind)
+	 const module_symbol_search_iterator end, symbol_search_kind kind)
 {
   gdb_assert (iter->first.symbol != nullptr);
   gdb_assert (iter->second.symbol != nullptr);
@@ -314,7 +315,7 @@ output_module_symbols_in_single_module
    command line options passed to the MI command.  */
 
 static void
-mi_info_module_functions_or_variables (domain_search_flags kind,
+mi_info_module_functions_or_variables (symbol_search_kind kind,
 				       const char *const *argv, int argc)
 {
   const char *module_regexp = nullptr;
@@ -341,7 +342,7 @@ mi_info_module_functions_or_variables (domain_search_flags kind,
   while (1)
     {
       const char *cmd_string
-	= ((kind == SEARCH_FUNCTION_DOMAIN)
+	= ((kind == symbol_search_kind::FUNCTION)
 	   ? "-symbol-info-module-functions"
 	   : "-symbol-info-module-variables");
       int opt = mi_getopt (cmd_string, argc, argv, opts, &oind, &oarg);
@@ -383,7 +384,7 @@ void
 mi_cmd_symbol_info_functions (const char *command, const char *const *argv,
 			      int argc)
 {
-  mi_info_functions_or_variables (SEARCH_FUNCTION_DOMAIN, argv, argc);
+  mi_info_functions_or_variables (symbol_search_kind::FUNCTION, argv, argc);
 }
 
 /* Implement -symbol-info-module-functions command.  */
@@ -392,7 +393,8 @@ void
 mi_cmd_symbol_info_module_functions (const char *command,
 				     const char *const *argv, int argc)
 {
-  mi_info_module_functions_or_variables (SEARCH_FUNCTION_DOMAIN, argv, argc);
+  mi_info_module_functions_or_variables (symbol_search_kind::FUNCTION,
+					 argv, argc);
 }
 
 /* Implement -symbol-info-module-variables command.  */
@@ -401,7 +403,8 @@ void
 mi_cmd_symbol_info_module_variables (const char *command,
 				     const char *const *argv, int argc)
 {
-  mi_info_module_functions_or_variables (SEARCH_VAR_DOMAIN, argv, argc);
+  mi_info_module_functions_or_variables (symbol_search_kind::VARIABLE,
+					 argv, argc);
 }
 
 /* Implement -symbol-inf-modules command.  */
@@ -444,7 +447,8 @@ mi_cmd_symbol_info_modules (const char *command, const char *const *argv,
 	}
     }
 
-  mi_symbol_info (SEARCH_MODULE_DOMAIN, regexp, nullptr, true, max_results);
+  mi_symbol_info (symbol_search_kind::MODULE, regexp, nullptr, true,
+		  max_results);
 }
 
 /* Implement -symbol-info-types command.  */
@@ -487,8 +491,8 @@ mi_cmd_symbol_info_types (const char *command, const char *const *argv,
 	}
     }
 
-  mi_symbol_info (SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN, regexp, nullptr,
-		  true, max_results);
+  mi_symbol_info (symbol_search_kind::TYPE, regexp, nullptr, true,
+		  max_results);
 }
 
 /* Implement -symbol-info-variables command.  */
@@ -497,5 +501,5 @@ void
 mi_cmd_symbol_info_variables (const char *command, const char *const *argv,
 			      int argc)
 {
-  mi_info_functions_or_variables (SEARCH_VAR_DOMAIN, argv, argc);
+  mi_info_functions_or_variables (symbol_search_kind::VARIABLE, argv, argc);
 }
diff --git a/gdb/python/python.c b/gdb/python/python.c
index d51cd83966be..467317b7996e 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -848,7 +848,7 @@ gdbpy_rbreak (PyObject *self, PyObject *args, PyObject *kw)
       minsyms_p = cmp;
     }
 
-  global_symbol_searcher spec (SEARCH_FUNCTION_DOMAIN, regex);
+  global_symbol_searcher spec (symbol_search_kind::FUNCTION, regex);
 
   /* The "symtabs" keyword is any Python iterable object that returns
      a gdb.Symtab on each iteration.  If specified, iterate through
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 27070f74f83e..f80e96b48285 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4698,11 +4698,31 @@ treg_matches_sym_type_name (const compiled_regex &treg,
   return treg.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
 }
 
+/* Return the domain search flags matching symbols of kind KIND.  */
+
+static domain_search_flags
+to_search_flags (symbol_search_kind kind)
+{
+  switch (kind)
+    {
+    case symbol_search_kind::VARIABLE:
+      return SEARCH_VAR_DOMAIN;
+    case symbol_search_kind::FUNCTION:
+      return SEARCH_FUNCTION_DOMAIN;
+    case symbol_search_kind::TYPE:
+      return SEARCH_TYPE_DOMAIN | SEARCH_STRUCT_DOMAIN;
+    case symbol_search_kind::MODULE:
+      return SEARCH_MODULE_DOMAIN;
+    }
+
+  gdb_assert_not_reached ("invalid symbol_search_kind");
+}
+
 /* See symtab.h.  */
 
 bool
 global_symbol_searcher::is_suitable_msymbol
-	(const domain_search_flags kind, const minimal_symbol *msymbol)
+	(symbol_search_kind kind, const minimal_symbol *msymbol)
 {
   switch (msymbol->type ())
     {
@@ -4710,12 +4730,12 @@ global_symbol_searcher::is_suitable_msymbol
     case mst_bss:
     case mst_file_data:
     case mst_file_bss:
-      return (kind & SEARCH_VAR_DOMAIN) != 0;
+      return kind == symbol_search_kind::VARIABLE;
     case mst_text:
     case mst_file_text:
     case mst_solib_trampoline:
     case mst_text_gnu_ifunc:
-      return (kind & SEARCH_FUNCTION_DOMAIN) != 0;
+      return kind == symbol_search_kind::FUNCTION;
     default:
       return false;
     }
@@ -4727,7 +4747,6 @@ bool
 global_symbol_searcher::expand_symtabs
 	(objfile *objfile, const std::optional<compiled_regex> &preg) const
 {
-  domain_search_flags kind = m_kind;
   bool found_func_msymbol_without_debug_info = false;
 
   auto do_file_match = [&] (const char *filename, bool basenames)
@@ -4748,7 +4767,7 @@ global_symbol_searcher::expand_symtabs
      },
      NULL,
      SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
-     kind);
+     to_search_flags (m_kind));
 
   /* Here, we search through the minimal symbol tables for functions that
      match, and force their symbols to be read.  The symbol will then be found
@@ -4764,7 +4783,7 @@ global_symbol_searcher::expand_symtabs
      symtab.  When no file names were given the caller unconditionally rescans
      the minimal symbols for SEARCH_VAR_DOMAIN.  */
   if (m_filenames.empty ()
-      && (kind & SEARCH_FUNCTION_DOMAIN) != 0)
+      && m_kind == symbol_search_kind::FUNCTION)
     {
       for (minimal_symbol *msymbol : objfile->msymbols ())
 	{
@@ -4773,7 +4792,7 @@ global_symbol_searcher::expand_symtabs
 	  if (msymbol->created_by_gdb)
 	    continue;
 
-	  if (is_suitable_msymbol (kind, msymbol))
+	  if (is_suitable_msymbol (m_kind, msymbol))
 	    {
 	      if (!preg.has_value ()
 		  || preg->exec (msymbol->natural_name (), 0,
@@ -4804,7 +4823,7 @@ global_symbol_searcher::add_matching_symbols
 	 const std::optional<compiled_regex> &treg,
 	 std::set<symbol_search> *result_set) const
 {
-  domain_search_flags kind = m_kind;
+  domain_search_flags domain = to_search_flags (m_kind);
 
   /* Add matching symbols (if not already present).  */
   for (compunit_symtab &cust : objfile->compunits ())
@@ -4832,7 +4851,7 @@ global_symbol_searcher::add_matching_symbols
 					 m_filenames, false))))
 		continue;
 
-	      if (!sym->matches (kind))
+	      if (!sym->matches (domain))
 		continue;
 
 	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
@@ -4845,7 +4864,7 @@ global_symbol_searcher::add_matching_symbols
 		   && !treg_matches_sym_type_name (*treg, sym)))
 		continue;
 
-	      if ((kind & SEARCH_VAR_DOMAIN) != 0)
+	      if (m_kind == symbol_search_kind::VARIABLE)
 		{
 		  if (sym->loc_class () == LOC_UNRESOLVED
 		      /* LOC_CONST can be used for more than
@@ -4882,8 +4901,6 @@ global_symbol_searcher::add_matching_msymbols
 	(objfile *objfile, const std::optional<compiled_regex> &preg,
 	 std::vector<symbol_search> *results) const
 {
-  domain_search_flags kind = m_kind;
-
   for (minimal_symbol *msymbol : objfile->msymbols ())
     {
       QUIT;
@@ -4891,7 +4908,7 @@ global_symbol_searcher::add_matching_msymbols
       if (msymbol->created_by_gdb)
 	continue;
 
-      if (is_suitable_msymbol (kind, msymbol))
+      if (is_suitable_msymbol (m_kind, msymbol))
 	{
 	  if (!preg.has_value ()
 	      || preg->exec (msymbol->natural_name (), 0,
@@ -4899,7 +4916,7 @@ global_symbol_searcher::add_matching_msymbols
 	    {
 	      /* For functions we can do a quick check of whether the
 		 symbol might be found via find_pc_symtab.  */
-	      if ((kind & SEARCH_FUNCTION_DOMAIN) == 0
+	      if (m_kind != symbol_search_kind::FUNCTION
 		  || (find_compunit_symtab_for_pc
 		      (msymbol->value_address (objfile)) == NULL))
 		{
@@ -5006,12 +5023,12 @@ global_symbol_searcher::search () const
      minimal symbol, as we assume that a minimal symbol does not have a
      type.  */
   if ((found_func_msymbol_without_debug_info
-       || (m_filenames.empty () && (m_kind & SEARCH_VAR_DOMAIN) != 0))
+       || (m_filenames.empty () && m_kind == symbol_search_kind::VARIABLE))
       && !m_exclude_minsyms
       && !treg.has_value ())
     {
-      gdb_assert ((m_kind & (SEARCH_VAR_DOMAIN | SEARCH_FUNCTION_DOMAIN))
-		  != 0);
+      gdb_assert (m_kind == symbol_search_kind::VARIABLE
+		  || m_kind == symbol_search_kind::FUNCTION);
       for (objfile &objfile : current_program_space->objfiles ())
 	if (!add_matching_msymbols (&objfile, preg, &result))
 	  break;
@@ -5143,7 +5160,7 @@ print_msymbol_info (bound_minimal_symbol msymbol)
 
 static void
 symtab_symbol_info (bool quiet, bool exclude_minsyms,
-		    const char *regexp, domain_enum kind,
+		    const char *regexp, symbol_search_kind kind,
 		    const char *t_regexp, int from_tty)
 {
   const char *last_filename = "";
@@ -5152,11 +5169,7 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
   if (regexp != nullptr && *regexp == '\0')
     regexp = nullptr;
 
-  domain_search_flags flags = to_search_flags (kind);
-  if (kind == TYPE_DOMAIN)
-    flags |= SEARCH_STRUCT_DOMAIN;
-
-  global_symbol_searcher spec (flags, regexp);
+  global_symbol_searcher spec (kind, regexp);
   spec.set_symbol_type_regexp (t_regexp);
   spec.set_exclude_minsyms (exclude_minsyms);
   std::vector<symbol_search> symbols = spec.search ();
@@ -5166,20 +5179,18 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
       const char *classname;
       switch (kind)
 	{
-	case VAR_DOMAIN:
+	case symbol_search_kind::VARIABLE:
 	  classname = "variable";
 	  break;
-	case FUNCTION_DOMAIN:
+	case symbol_search_kind::FUNCTION:
 	  classname = "function";
 	  break;
-	case TYPE_DOMAIN:
+	case symbol_search_kind::TYPE:
 	  classname = "type";
 	  break;
-	case MODULE_DOMAIN:
+	case symbol_search_kind::MODULE:
 	  classname = "module";
 	  break;
-	default:
-	  gdb_assert_not_reached ("invalid domain enum");
 	}
 
       if (regexp != NULL)
@@ -5304,7 +5315,7 @@ info_variables_command (const char *args, int from_tty)
     args = nullptr;
 
   symtab_symbol_info
-    (opts.quiet, opts.exclude_minsyms, args, VAR_DOMAIN,
+    (opts.quiet, opts.exclude_minsyms, args, symbol_search_kind::VARIABLE,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
      from_tty);
 }
@@ -5323,7 +5334,7 @@ info_functions_command (const char *args, int from_tty)
     args = nullptr;
 
   symtab_symbol_info
-    (opts.quiet, opts.exclude_minsyms, args, FUNCTION_DOMAIN,
+    (opts.quiet, opts.exclude_minsyms, args, symbol_search_kind::FUNCTION,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
      from_tty);
 }
@@ -5366,8 +5377,8 @@ info_types_command (const char *args, int from_tty)
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
   if (args != nullptr && *args == '\0')
     args = nullptr;
-  symtab_symbol_info (opts.quiet, false, args, TYPE_DOMAIN, nullptr,
-		      from_tty);
+  symtab_symbol_info (opts.quiet, false, args, symbol_search_kind::TYPE,
+		      nullptr, from_tty);
 }
 
 /* Command completer for 'info types' command.  */
@@ -5399,8 +5410,8 @@ info_modules_command (const char *args, int from_tty)
     (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
   if (args != nullptr && *args == '\0')
     args = nullptr;
-  symtab_symbol_info (opts.quiet, true, args, MODULE_DOMAIN, nullptr,
-		      from_tty);
+  symtab_symbol_info (opts.quiet, true, args, symbol_search_kind::MODULE,
+		      nullptr, from_tty);
 }
 
 /* Implement the 'info main' command.  */
@@ -5441,7 +5452,7 @@ rbreak_command (const char *regexp, int from_tty)
      because the std::move nullifies file_name.  */
   bool file_name_p = file_name != nullptr;
 
-  global_symbol_searcher spec (SEARCH_FUNCTION_DOMAIN, regexp);
+  global_symbol_searcher spec (symbol_search_kind::FUNCTION, regexp);
   if (file_name_p)
     spec.add_filename (std::move (file_name));
   std::vector<symbol_search> symbols = spec.search ();
@@ -6654,12 +6665,12 @@ static struct cmd_list_element *info_module_cmdlist = NULL;
 
 std::vector<module_symbol_search>
 search_module_symbols (const char *module_regexp, const char *regexp,
-		       const char *type_regexp, domain_search_flags kind)
+		       const char *type_regexp, symbol_search_kind kind)
 {
   std::vector<module_symbol_search> results;
 
   /* Search for all modules matching MODULE_REGEXP.  */
-  global_symbol_searcher spec1 (SEARCH_MODULE_DOMAIN, module_regexp);
+  global_symbol_searcher spec1 (symbol_search_kind::MODULE, module_regexp);
   spec1.set_exclude_minsyms (true);
   std::vector<symbol_search> modules = spec1.search ();
 
@@ -6705,9 +6716,10 @@ search_module_symbols (const char *module_regexp, const char *regexp,
 static void
 info_module_subcommand (bool quiet, const char *module_regexp,
 			const char *regexp, const char *type_regexp,
-			domain_search_flags kind)
+			symbol_search_kind kind)
 {
-  gdb_assert (kind == SEARCH_FUNCTION_DOMAIN || kind == SEARCH_VAR_DOMAIN);
+  gdb_assert (kind == symbol_search_kind::FUNCTION
+	      || kind == symbol_search_kind::VARIABLE);
 
   /* Print a header line.  Don't build the header line bit by bit as this
      prevents internationalisation.  */
@@ -6718,12 +6730,12 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	  if (type_regexp == nullptr)
 	    {
 	      if (regexp == nullptr)
-		gdb_printf ((kind == SEARCH_VAR_DOMAIN
+		gdb_printf ((kind == symbol_search_kind::VARIABLE
 			     ? _("All variables in all modules:")
 			     : _("All functions in all modules:")));
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression"
 			" \"%s\" in all modules:")
 		    : _("All functions matching regular expression"
@@ -6734,7 +6746,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	    {
 	      if (regexp == nullptr)
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables with type matching regular "
 			"expression \"%s\" in all modules:")
 		    : _("All functions with type matching regular "
@@ -6742,7 +6754,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 		   type_regexp);
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression "
 			"\"%s\",\n\twith type matching regular "
 			"expression \"%s\" in all modules:")
@@ -6758,7 +6770,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	    {
 	      if (regexp == nullptr)
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables in all modules matching regular "
 			"expression \"%s\":")
 		    : _("All functions in all modules matching regular "
@@ -6766,7 +6778,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 		   module_regexp);
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression "
 			"\"%s\",\n\tin all modules matching regular "
 			"expression \"%s\":")
@@ -6779,7 +6791,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 	    {
 	      if (regexp == nullptr)
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables with type matching regular "
 			"expression \"%s\"\n\tin all modules matching "
 			"regular expression \"%s\":")
@@ -6789,7 +6801,7 @@ info_module_subcommand (bool quiet, const char *module_regexp,
 		   type_regexp, module_regexp);
 	      else
 		gdb_printf
-		  ((kind == SEARCH_VAR_DOMAIN
+		  ((kind == symbol_search_kind::VARIABLE
 		    ? _("All variables matching regular expression "
 			"\"%s\",\n\twith type matching regular expression "
 			"\"%s\",\n\tin all modules matching regular "
@@ -6901,7 +6913,7 @@ info_module_functions_command (const char *args, int from_tty)
     (opts.quiet,
      opts.module_regexp.empty () ? nullptr : opts.module_regexp.c_str (), args,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
-     SEARCH_FUNCTION_DOMAIN);
+     symbol_search_kind::FUNCTION);
 }
 
 /* Implements the 'info module variables' command.  */
@@ -6920,7 +6932,7 @@ info_module_variables_command (const char *args, int from_tty)
     (opts.quiet,
      opts.module_regexp.empty () ? nullptr : opts.module_regexp.c_str (), args,
      opts.type_regexp.empty () ? nullptr : opts.type_regexp.c_str (),
-     SEARCH_VAR_DOMAIN);
+     symbol_search_kind::VARIABLE);
 }
 
 /* Command completer for 'info module ...' sub-commands.  */
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 6443216fd7f0..d21933759b6a 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2586,6 +2586,24 @@ struct symbol_search
 				  const symbol_search &sym_b);
 };
 
+/* The kind of symbols searched for by global_symbol_searcher.  */
+
+enum class symbol_search_kind
+{
+  /* Variables, excluding enum constants.  */
+  VARIABLE,
+
+  /* Functions.  */
+  FUNCTION,
+
+  /* Type names, including those with a struct/union/enum prefix (STRUCT
+     domain).  */
+  TYPE,
+
+  /* Fortran modules.  */
+  MODULE,
+};
+
 /* In order to search for global symbols of a particular kind matching
    particular regular expressions, create an instance of this structure and
    call the SEARCH member function.  */
@@ -2594,7 +2612,7 @@ class global_symbol_searcher
 public:
 
   /* Constructor.  */
-  global_symbol_searcher (domain_search_flags kind,
+  global_symbol_searcher (symbol_search_kind kind,
 			  const char *symbol_name_regexp)
     : m_kind (kind),
       m_symbol_name_regexp (symbol_name_regexp)
@@ -2635,14 +2653,8 @@ class global_symbol_searcher
   /* The set of source files to search in for matching symbols.  */
   std::vector<gdb::unique_xmalloc_ptr<char>> m_filenames;
 
-  /* The kind of symbols are we searching for.
-     VARIABLES_DOMAIN - Search all symbols, excluding functions, type
-			names, and constants (enums).
-     FUNCTIONS_DOMAIN - Search all functions..
-     TYPES_DOMAIN     - Search all type names.
-     MODULES_DOMAIN   - Search all Fortran modules.
-     ALL_DOMAIN       - Not valid for this function.  */
-  domain_search_flags m_kind;
+  /* The kind of symbols we are searching for.  */
+  symbol_search_kind m_kind;
 
   /* Regular expression to match against the symbol name.  */
   const char *m_symbol_name_regexp = nullptr;
@@ -2686,7 +2698,7 @@ class global_symbol_searcher
 			      std::vector<symbol_search> *results) const;
 
   /* Return true if MSYMBOL is of type KIND.  */
-  static bool is_suitable_msymbol (const domain_search_flags kind,
+  static bool is_suitable_msymbol (symbol_search_kind kind,
 				   const minimal_symbol *msymbol);
 };
 
@@ -2703,7 +2715,7 @@ using module_symbol_search = std::pair<symbol_search, symbol_search>;
    within the module.  */
 extern std::vector<module_symbol_search> search_module_symbols
 	(const char *module_regexp, const char *regexp,
-	 const char *type_regexp, domain_search_flags kind);
+	 const char *type_regexp, symbol_search_kind kind);
 
 /* Convert a global or static symbol SYM (based on BLOCK, which should be
    either GLOBAL_BLOCK or STATIC_BLOCK) into a string for use in 'info'
-- 
2.55.0

Re: [PATCH v2 01/11] gdb: add symbol_search_kind enum for global_symbol_searcher

[Andrew Burgess]

I think this is looking good.  Just one minor comment inline below:

simon.marchi@polymtl.ca writes:

> @@ -5166,20 +5179,18 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
>        const char *classname;
>        switch (kind)
>  	{
> -	case VAR_DOMAIN:
> +	case symbol_search_kind::VARIABLE:
>  	  classname = "variable";
>  	  break;
> -	case FUNCTION_DOMAIN:
> +	case symbol_search_kind::FUNCTION:
>  	  classname = "function";
>  	  break;
> -	case TYPE_DOMAIN:
> +	case symbol_search_kind::TYPE:
>  	  classname = "type";
>  	  break;
> -	case MODULE_DOMAIN:
> +	case symbol_search_kind::MODULE:
>  	  classname = "module";
>  	  break;
> -	default:
> -	  gdb_assert_not_reached ("invalid domain enum");
>  	}

I initially wrote: please don't drop the default case, but then I
checked the V1 review and saw this change was made specifically to
address the feedback there.

What I don't understand is you said:

> > I think we could drop the "default" case as part of this refactoring.  All
> > cases are handled; we lose compile time warnings for unhandled cases with
> > the "default" case.
> 
> Agreed that we can drop the default.  Note that we don't build with
> -Wswitch, but we specifically enable it locally here and there.

If the warning is mostly off, including here, then deleting the default
seems like the wrong choice.

I'm not too fussed, I think there's a middle group, that would keep my
happy:

 1. initialise classname to nullptr, and

 2. follow this switch statement with:
      gdb_assert (classname != nullptr);

Even with the warning on it doesn't protect us from random garbage being
cast to type symbol_search_kind and then passed to this function, which
the 'default:' case does protect against.  But the trailing assert is
probably better anyway as it ensures each switch case is doing what
needs to be done.

With that:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 01/11] gdb: add symbol_search_kind enum for global_symbol_searcher

[Simon Marchi]

On 10/1/26 6:30 AM, Andrew Burgess wrote:
> 
> I think this is looking good.  Just one minor comment inline below:
> 
> simon.marchi@polymtl.ca writes:
> 
>> @@ -5166,20 +5179,18 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
>>        const char *classname;
>>        switch (kind)
>>  	{
>> -	case VAR_DOMAIN:
>> +	case symbol_search_kind::VARIABLE:
>>  	  classname = "variable";
>>  	  break;
>> -	case FUNCTION_DOMAIN:
>> +	case symbol_search_kind::FUNCTION:
>>  	  classname = "function";
>>  	  break;
>> -	case TYPE_DOMAIN:
>> +	case symbol_search_kind::TYPE:
>>  	  classname = "type";
>>  	  break;
>> -	case MODULE_DOMAIN:
>> +	case symbol_search_kind::MODULE:
>>  	  classname = "module";
>>  	  break;
>> -	default:
>> -	  gdb_assert_not_reached ("invalid domain enum");
>>  	}
> 
> I initially wrote: please don't drop the default case, but then I
> checked the V1 review and saw this change was made specifically to
> address the feedback there.
> 
> What I don't understand is you said:
> 
>>> I think we could drop the "default" case as part of this refactoring.  All
>>> cases are handled; we lose compile time warnings for unhandled cases with
>>> the "default" case.
>>
>> Agreed that we can drop the default.  Note that we don't build with
>> -Wswitch, but we specifically enable it locally here and there.
> 
> If the warning is mostly off, including here, then deleting the default
> seems like the wrong choice.
> 
> I'm not too fussed, I think there's a middle group, that would keep my
> happy:
> 
>  1. initialise classname to nullptr, and
> 
>  2. follow this switch statement with:
>       gdb_assert (classname != nullptr);

You're right, I applied the suggestion without thinking too much about
it.  We definitely want an assert to fire instead of printing from an
uninitialized pointer.  If that same code was in a function of its own,
I would have done it like this:

static const char *
ssk_to_string (symbol_search_kind kind)
{
  switch (kind)
    {
    case symbol_search_kind::VARIABLE:
      return "variable";

    case symbol_search_kind::FUNCTION:
      return "function";

    case symbol_search_kind::TYPE:
      return "type";

    case symbol_search_kind::MODULE:
      return "module";
    }

  gdb_assert_not_reached ("invalid symbol_search_kind value");
}

There is even an example of that in the same patch (the to_search_flags
function).

I'll initialize the variable with nullptr and add a nullptr check after
the switch.  I prefer this over the default, because if we ever enable
-Wswitch (which will probably never happen, I tried it before and it was
painful), then we'll get a compile-time error if we are missing a case.

I don't think that this code is duplicated anywhere, so I don't think
it's worth moving it to a separate function.

> 
> Even with the warning on it doesn't protect us from random garbage being
> cast to type symbol_search_kind and then passed to this function, which
> the 'default:' case does protect against.  But the trailing assert is
> probably better anyway as it ensures each switch case is doing what
> needs to be done.
> 
> With that:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon

Re: [PATCH v2 01/11] gdb: add symbol_search_kind enum for global_symbol_searcher

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> You're right, I applied the suggestion without thinking too much about
Simon> it.  We definitely want an assert to fire instead of printing from an
Simon> uninitialized pointer.  If that same code was in a function of its own,
Simon> I would have done it like this:
[...]

Simon> I'll initialize the variable with nullptr and add a nullptr check after
Simon> the switch.  I prefer this over the default, because if we ever enable
Simon> -Wswitch (which will probably never happen, I tried it before and it was
Simon> painful), then we'll get a compile-time error if we are missing a case.

I tend to think that only some subset of switches really need this
treatment anyway.  A while ago I asked for an easier way to enable
-Wswitch-enum selectively:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108811

I guess we could accomplish this today with _Pragma if we didn't mind
wrapping such switches in begin/end pairs.

I think -Wswitch-enum is maybe better than -Wswitch and in the switches
that matter, gdb shouldn't have a 'default', just a total enumeration of
cases.

Tom

[PATCH v2 02/11] gdb: rename some preg/treg variables

[simon.marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

global_symbol_searcher and some code around that use "preg" and "treg" as
variable, parameter or field names.  Rename to the clearer "name_regex" and
"type_regex".

I guess that "preg" came from the parameter name of regcomp/regexec, and
treg was derived from that.

Change-Id: Ic74c6acde11976d0b26757614d2b2c3093b147cf
---
 gdb/stack.c  | 20 ++++++++--------
 gdb/symtab.c | 68 +++++++++++++++++++++++++++-------------------------
 gdb/symtab.h | 42 +++++++++++++++++---------------
 3 files changed, 68 insertions(+), 62 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index 7fe05bb52a81..79a67865affe 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2289,8 +2289,8 @@ iterate_over_block_local_vars_printing
 
 struct print_variable_and_value_data
 {
-  std::optional<compiled_regex> preg;
-  std::optional<compiled_regex> treg;
+  std::optional<compiled_regex> name_regex;
+  std::optional<compiled_regex> type_regex;
   struct frame_id frame_id;
   int num_tabs;
   struct ui_file *stream;
@@ -2312,11 +2312,11 @@ print_variable_and_value_data::operator() (const char *print_name,
 {
   frame_info_ptr frame;
 
-  if (preg.has_value ()
-      && preg->exec (sym->natural_name (), 0, NULL, 0) != 0)
+  if (name_regex.has_value ()
+      && name_regex->exec (sym->natural_name (), 0, NULL, 0) != 0)
     return;
-  if (treg.has_value ()
-      && !treg_matches_sym_type_name (*treg, sym))
+  if (type_regex.has_value ()
+      && !regex_matches_sym_type_name (*type_regex, sym))
     return;
   if (language_def (sym->language ())->symbol_printing_suppressed (sym))
     return;
@@ -2399,8 +2399,8 @@ print_frame_local_vars (const frame_info_ptr &frame,
       return;
     }
 
-  prepare_reg (regexp, &cb_data.preg);
-  prepare_reg (t_regexp, &cb_data.treg);
+  prepare_reg (regexp, &cb_data.name_regex);
+  prepare_reg (t_regexp, &cb_data.type_regex);
   cb_data.frame_id = get_frame_id (frame);
   cb_data.num_tabs = 4 * num_tabs;
   cb_data.stream = stream;
@@ -2592,8 +2592,8 @@ print_frame_arg_vars (const frame_info_ptr &frame,
       return;
     }
 
-  prepare_reg (regexp, &cb_data.preg);
-  prepare_reg (t_regexp, &cb_data.treg);
+  prepare_reg (regexp, &cb_data.name_regex);
+  prepare_reg (t_regexp, &cb_data.type_regex);
   cb_data.frame_id = get_frame_id (frame);
   cb_data.num_tabs = 0;
   cb_data.stream = stream;
diff --git a/gdb/symtab.c b/gdb/symtab.c
index f80e96b48285..aae82218b5cf 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4666,18 +4666,16 @@ symbol_search::compare_search_syms (const symbol_search &sym_a,
     return -1;
 }
 
-/* Returns true if the type_name of symbol_type of SYM matches TREG.
-   If SYM has no symbol_type or symbol_name, returns false.  */
+/* See symtab.h.  */
 
 bool
-treg_matches_sym_type_name (const compiled_regex &treg,
-			    const struct symbol *sym)
+regex_matches_sym_type_name (const compiled_regex &type_regex,
+			     const symbol *sym)
 {
   struct type *sym_type;
   std::string printed_sym_type_name;
 
-  symbol_lookup_debug_printf_v ("treg_matches_sym_type_name, sym %s",
-				sym->natural_name ());
+  symbol_lookup_debug_printf_v ("sym %s", sym->natural_name ());
 
   sym_type = sym->type ();
   if (sym_type == NULL)
@@ -4695,7 +4693,7 @@ treg_matches_sym_type_name (const compiled_regex &treg,
   if (printed_sym_type_name.empty ())
     return false;
 
-  return treg.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
+  return type_regex.exec (printed_sym_type_name.c_str (), 0, NULL, 0) == 0;
 }
 
 /* Return the domain search flags matching symbols of kind KIND.  */
@@ -4745,7 +4743,8 @@ global_symbol_searcher::is_suitable_msymbol
 
 bool
 global_symbol_searcher::expand_symtabs
-	(objfile *objfile, const std::optional<compiled_regex> &preg) const
+	(objfile *objfile,
+	 const std::optional<compiled_regex> &name_regex) const
 {
   bool found_func_msymbol_without_debug_info = false;
 
@@ -4762,8 +4761,8 @@ global_symbol_searcher::expand_symtabs
      &lookup_name_info::match_any (),
      [&] (const char *symname)
      {
-       return (!preg.has_value ()
-	       || preg->exec (symname, 0, NULL, 0) == 0);
+       return (!name_regex.has_value ()
+	       || name_regex->exec (symname, 0, NULL, 0) == 0);
      },
      NULL,
      SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
@@ -4794,9 +4793,9 @@ global_symbol_searcher::expand_symtabs
 
 	  if (is_suitable_msymbol (m_kind, msymbol))
 	    {
-	      if (!preg.has_value ()
-		  || preg->exec (msymbol->natural_name (), 0,
-				 NULL, 0) == 0)
+	      if (!name_regex.has_value ()
+		  || name_regex->exec (msymbol->natural_name (), 0,
+				       NULL, 0) == 0)
 		{
 		  /* An important side-effect of this lookup function is
 		     to expand the symbol table if msymbol is found, later
@@ -4819,8 +4818,8 @@ global_symbol_searcher::expand_symtabs
 bool
 global_symbol_searcher::add_matching_symbols
 	(objfile *objfile,
-	 const std::optional<compiled_regex> &preg,
-	 const std::optional<compiled_regex> &treg,
+	 const std::optional<compiled_regex> &name_regex,
+	 const std::optional<compiled_regex> &type_regex,
 	 std::set<symbol_search> *result_set) const
 {
   domain_search_flags domain = to_search_flags (m_kind);
@@ -4854,14 +4853,15 @@ global_symbol_searcher::add_matching_symbols
 	      if (!sym->matches (domain))
 		continue;
 
-	      if (preg.has_value () && preg->exec (sym->natural_name (), 0,
-						   nullptr, 0) != 0)
+	      if (name_regex.has_value ()
+		  && name_regex->exec (sym->natural_name (), 0,
+				       nullptr, 0) != 0)
 		continue;
 
 	      if (((sym->domain () == VAR_DOMAIN
 		    || sym->domain () == FUNCTION_DOMAIN)
-		   && treg.has_value ()
-		   && !treg_matches_sym_type_name (*treg, sym)))
+		   && type_regex.has_value ()
+		   && !regex_matches_sym_type_name (*type_regex, sym)))
 		continue;
 
 	      if (m_kind == symbol_search_kind::VARIABLE)
@@ -4898,7 +4898,7 @@ global_symbol_searcher::add_matching_symbols
 
 bool
 global_symbol_searcher::add_matching_msymbols
-	(objfile *objfile, const std::optional<compiled_regex> &preg,
+	(objfile *objfile, const std::optional<compiled_regex> &name_regex,
 	 std::vector<symbol_search> *results) const
 {
   for (minimal_symbol *msymbol : objfile->msymbols ())
@@ -4910,9 +4910,9 @@ global_symbol_searcher::add_matching_msymbols
 
       if (is_suitable_msymbol (m_kind, msymbol))
 	{
-	  if (!preg.has_value ()
-	      || preg->exec (msymbol->natural_name (), 0,
-			     NULL, 0) == 0)
+	  if (!name_regex.has_value ()
+	      || name_regex->exec (msymbol->natural_name (), 0,
+				   NULL, 0) == 0)
 	    {
 	      /* For functions we can do a quick check of whether the
 		 symbol might be found via find_pc_symtab.  */
@@ -4943,8 +4943,8 @@ global_symbol_searcher::add_matching_msymbols
 std::vector<symbol_search>
 global_symbol_searcher::search () const
 {
-  std::optional<compiled_regex> preg;
-  std::optional<compiled_regex> treg;
+  std::optional<compiled_regex> name_regex;
+  std::optional<compiled_regex> type_regex;
 
   if (m_symbol_name_regexp != NULL)
     {
@@ -4986,16 +4986,16 @@ global_symbol_searcher::search () const
 
       int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
 				? REG_ICASE : 0);
-      preg.emplace (symbol_name_regexp, cflags,
-		    _("Invalid regexp"));
+      name_regex.emplace (symbol_name_regexp, cflags,
+			  _("Invalid regexp"));
     }
 
   if (m_symbol_type_regexp != NULL)
     {
       int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
 				? REG_ICASE : 0);
-      treg.emplace (m_symbol_type_regexp, cflags,
-		    _("Invalid regexp"));
+      type_regex.emplace (m_symbol_type_regexp, cflags,
+			  _("Invalid regexp"));
     }
 
   bool found_func_msymbol_without_debug_info = false;
@@ -5004,13 +5004,15 @@ global_symbol_searcher::search () const
     {
       /* Expand symtabs within objfile that possibly contain matching
 	 symbols.  */
-      found_func_msymbol_without_debug_info |= expand_symtabs (&objfile, preg);
+      found_func_msymbol_without_debug_info
+	|= expand_symtabs (&objfile, name_regex);
 
       /* Find matching symbols within OBJFILE and add them in to the
 	 RESULT_SET set.  Use a set here so that we can easily detect
 	 duplicates as we go, and can therefore track how many unique
 	 matches we have found so far.  */
-      if (!add_matching_symbols (&objfile, preg, treg, &result_set))
+      if (!add_matching_symbols (&objfile, name_regex, type_regex,
+				 &result_set))
 	break;
     }
 
@@ -5025,12 +5027,12 @@ global_symbol_searcher::search () const
   if ((found_func_msymbol_without_debug_info
        || (m_filenames.empty () && m_kind == symbol_search_kind::VARIABLE))
       && !m_exclude_minsyms
-      && !treg.has_value ())
+      && !type_regex.has_value ())
     {
       gdb_assert (m_kind == symbol_search_kind::VARIABLE
 		  || m_kind == symbol_search_kind::FUNCTION);
       for (objfile &objfile : current_program_space->objfiles ())
-	if (!add_matching_msymbols (&objfile, preg, &result))
+	if (!add_matching_msymbols (&objfile, name_regex, &result))
 	  break;
     }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index d21933759b6a..e4cfa31e25e7 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2670,31 +2670,32 @@ class global_symbol_searcher
      of SIZE_MAX, there is no "unlimited".  */
   size_t m_max_search_results = SIZE_MAX;
 
-  /* Expand symtabs in OBJFILE that match PREG, are of type M_KIND.  Return
-     true if any msymbols were seen that we should later consider adding to
-     the results list.  */
+  /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type M_KIND.
+     Return true if any msymbols were seen that we should later consider
+     adding to the results list.  */
   bool expand_symtabs (objfile *objfile,
-		       const std::optional<compiled_regex> &preg) const;
+		       const std::optional<compiled_regex> &name_regex) const;
 
-  /* Add symbols from symtabs in OBJFILE that match PREG, and TREG, and are
-     of type M_KIND, to the results set RESULTS_SET.  Return false if we
-     stop adding results early due to having already found too many results
-     (based on M_MAX_SEARCH_RESULTS limit), otherwise return true.
+  /* Add symbols from symtabs in OBJFILE that match NAME_REGEX, and
+     TYPE_REGEX, and are of type M_KIND, to the results set RESULTS_SET.
+     Return false if we stop adding results early due to having already
+     found too many results (based on M_MAX_SEARCH_RESULTS limit),
+     otherwise return true.
      Returning true does not indicate that any results were added, just
      that we didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
   bool add_matching_symbols (objfile *objfile,
-			     const std::optional<compiled_regex> &preg,
-			     const std::optional<compiled_regex> &treg,
+			     const std::optional<compiled_regex> &name_regex,
+			     const std::optional<compiled_regex> &type_regex,
 			     std::set<symbol_search> *result_set) const;
 
-  /* Add msymbols from OBJFILE that match PREG and M_KIND, to the results
-     vector RESULTS.  Return false if we stop adding results early due to
-     having already found too many results (based on max search results
-     limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning true
-     does not indicate that any results were added, just that we didn't
-     _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
+  /* Add msymbols from OBJFILE that match NAME_REGEX and M_KIND, to the
+     results vector RESULTS.  Return false if we stop adding results early
+     due to having already found too many results (based on max search
+     results limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning
+     true does not indicate that any results were added, just that we
+     didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
   bool add_matching_msymbols (objfile *objfile,
-			      const std::optional<compiled_regex> &preg,
+			      const std::optional<compiled_regex> &name_regex,
 			      std::vector<symbol_search> *results) const;
 
   /* Return true if MSYMBOL is of type KIND.  */
@@ -2723,8 +2724,11 @@ extern std::vector<module_symbol_search> search_module_symbols
 
 extern std::string symbol_to_info_string (struct symbol *sym, int block);
 
-extern bool treg_matches_sym_type_name (const compiled_regex &treg,
-					const struct symbol *sym);
+/* Returns true if the SYM's type name matches TYPE_REGEX.
+   If SYM has no type name, returns false.  */
+
+extern bool regex_matches_sym_type_name (const compiled_regex &type_regex,
+					 const symbol *sym);
 
 /* The name of the ``main'' function.  */
 extern const char *main_name ();
-- 
2.55.0

Re: [PATCH v2 02/11] gdb: rename some preg/treg variables

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> global_symbol_searcher and some code around that use "preg" and "treg" as
> variable, parameter or field names.  Rename to the clearer "name_regex" and
> "type_regex".
>
> I guess that "preg" came from the parameter name of regcomp/regexec, and
> treg was derived from that.

There are 5 lines that you touch where NULL could be changed to nullptr.

I thought we generally made that update whenever we touched a line, but
I'll leave it up to you.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 02/11] gdb: rename some preg/treg variables

[Simon Marchi]

On 10/1/26 8:59 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> global_symbol_searcher and some code around that use "preg" and "treg" as
>> variable, parameter or field names.  Rename to the clearer "name_regex" and
>> "type_regex".
>>
>> I guess that "preg" came from the parameter name of regcomp/regexec, and
>> treg was derived from that.
> 
> There are 5 lines that you touch where NULL could be changed to nullptr.
> 
> I thought we generally made that update whenever we touched a line, but
> I'll leave it up to you.

Yes but I don't always remember to do it, done now.

> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon

[PATCH v2 03/11] gdb: factor out regexp compilation from global_symbol_searcher::search

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Factor out the scopes that compile the regexps, in order to make
global_symbol_searcher::search itself simpler to read.  Factor out a helper
to compile a symbol search regex as well.

Change-Id: Ie4485b2eb593967069ac635c933df817bc5304e6
---
 gdb/symtab.c | 118 ++++++++++++++++++++++++++++++---------------------
 gdb/symtab.h |   9 ++++
 2 files changed, 78 insertions(+), 49 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index aae82218b5cf..49e8719f4ec0 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4938,66 +4938,86 @@ global_symbol_searcher::add_matching_msymbols
   return true;
 }
 
-/* See symtab.h.  */
+/* Compile REGEXP, a symbol name or symbol type regexp.
 
-std::vector<symbol_search>
-global_symbol_searcher::search () const
+   If REGEXP is nullptr, return an empty optional.  */
+
+static std::optional<compiled_regex>
+compile_symbol_search_regex (const char *regexp)
 {
-  std::optional<compiled_regex> name_regex;
-  std::optional<compiled_regex> type_regex;
+  if (regexp == nullptr)
+    return {};
 
-  if (m_symbol_name_regexp != NULL)
-    {
-      const char *symbol_name_regexp = m_symbol_name_regexp;
-      std::string symbol_name_regexp_holder;
+  int cflags
+    = REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE : 0);
 
-      /* Make sure spacing is right for C++ operators.
-	 This is just a courtesy to make the matching less sensitive
-	 to how many spaces the user leaves between 'operator'
-	 and <TYPENAME> or <OPERATOR>.  */
-      const char *op_end;
-      const char *opname = operator_chars (symbol_name_regexp, &op_end);
+  return std::make_optional<compiled_regex> (regexp, cflags,
+					    _("Invalid regexp"));
+}
 
-      if (*opname)
-	{
-	  int fix = -1;		/* -1 means ok; otherwise number of
-				    spaces needed.  */
+/* See symtab.h.  */
 
-	  if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
-	    {
-	      /* There should 1 space between 'operator' and 'TYPENAME'.  */
-	      if (opname[-1] != ' ' || opname[-2] == ' ')
-		fix = 1;
-	    }
-	  else
-	    {
-	      /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
-	      if (opname[-1] == ' ')
-		fix = 0;
-	    }
-	  /* If wrong number of spaces, fix it.  */
-	  if (fix >= 0)
-	    {
-	      symbol_name_regexp_holder
-		= string_printf ("operator%.*s%s", fix, " ", opname);
-	      symbol_name_regexp = symbol_name_regexp_holder.c_str ();
-	    }
-	}
+std::optional<compiled_regex>
+global_symbol_searcher::compile_name_regex () const
+{
+  if (m_symbol_name_regexp == nullptr)
+    return {};
 
-      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
-				? REG_ICASE : 0);
-      name_regex.emplace (symbol_name_regexp, cflags,
-			  _("Invalid regexp"));
-    }
+  const char *symbol_name_regexp = m_symbol_name_regexp;
+  std::string symbol_name_regexp_holder;
+
+  /* Make sure spacing is right for C++ operators.
+     This is just a courtesy to make the matching less sensitive
+     to how many spaces the user leaves between 'operator'
+     and <TYPENAME> or <OPERATOR>.  */
+  const char *op_end;
+  const char *opname = operator_chars (symbol_name_regexp, &op_end);
 
-  if (m_symbol_type_regexp != NULL)
+  if (*opname)
     {
-      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
-				? REG_ICASE : 0);
-      type_regex.emplace (m_symbol_type_regexp, cflags,
-			  _("Invalid regexp"));
+      /* -1 means ok; otherwise number of spaces needed.  */
+      int fix = -1;
+
+      if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
+	{
+	  /* There should 1 space between 'operator' and 'TYPENAME'.  */
+	  if (opname[-1] != ' ' || opname[-2] == ' ')
+	    fix = 1;
+	}
+      else
+	{
+	  /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
+	  if (opname[-1] == ' ')
+	    fix = 0;
+	}
+      /* If wrong number of spaces, fix it.  */
+      if (fix >= 0)
+	{
+	  symbol_name_regexp_holder
+	    = string_printf ("operator%.*s%s", fix, " ", opname);
+	  symbol_name_regexp = symbol_name_regexp_holder.c_str ();
+	}
     }
 
+  return compile_symbol_search_regex (symbol_name_regexp);
+}
+
+/* See symtab.h.  */
+
+std::optional<compiled_regex>
+global_symbol_searcher::compile_type_regex () const
+{
+  return compile_symbol_search_regex (m_symbol_type_regexp);
+}
+
+/* See symtab.h.  */
+
+std::vector<symbol_search>
+global_symbol_searcher::search () const
+{
+  std::optional<compiled_regex> name_regex = compile_name_regex ();
+  std::optional<compiled_regex> type_regex = compile_type_regex ();
+
   bool found_func_msymbol_without_debug_info = false;
   std::set<symbol_search> result_set;
   for (objfile &objfile : current_program_space->objfiles ())
diff --git a/gdb/symtab.h b/gdb/symtab.h
index e4cfa31e25e7..e60b32230239 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2670,6 +2670,15 @@ class global_symbol_searcher
      of SIZE_MAX, there is no "unlimited".  */
   size_t m_max_search_results = SIZE_MAX;
 
+  /* Compile M_SYMBOL_NAME_REGEXP, if set.
+
+     This function normalizes the spacing when it detects a C++ operator in the
+     regexp.  */
+  std::optional<compiled_regex> compile_name_regex () const;
+
+  /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
+  std::optional<compiled_regex> compile_type_regex () const;
+
   /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type M_KIND.
      Return true if any msymbols were seen that we should later consider
      adding to the results list.  */
-- 
2.55.0

Re: [PATCH v2 03/11] gdb: factor out regexp compilation from global_symbol_searcher::search

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Factor out the scopes that compile the regexps, in order to make
> global_symbol_searcher::search itself simpler to read.  Factor out a helper
> to compile a symbol search regex as well.

Three super minor nits, two of which are pre-existing, see inline...

> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index aae82218b5cf..49e8719f4ec0 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4938,66 +4938,86 @@ global_symbol_searcher::add_matching_msymbols
>    return true;
>  }
>  
> -/* See symtab.h.  */
> +/* Compile REGEXP, a symbol name or symbol type regexp.
>  
> -std::vector<symbol_search>
> -global_symbol_searcher::search () const
> +   If REGEXP is nullptr, return an empty optional.  */
> +
> +static std::optional<compiled_regex>
> +compile_symbol_search_regex (const char *regexp)
>  {
> -  std::optional<compiled_regex> name_regex;
> -  std::optional<compiled_regex> type_regex;
> +  if (regexp == nullptr)
> +    return {};
>  
> -  if (m_symbol_name_regexp != NULL)
> -    {
> -      const char *symbol_name_regexp = m_symbol_name_regexp;
> -      std::string symbol_name_regexp_holder;
> +  int cflags
> +    = REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE : 0);
>  
> -      /* Make sure spacing is right for C++ operators.
> -	 This is just a courtesy to make the matching less sensitive
> -	 to how many spaces the user leaves between 'operator'
> -	 and <TYPENAME> or <OPERATOR>.  */
> -      const char *op_end;
> -      const char *opname = operator_chars (symbol_name_regexp, &op_end);
> +  return std::make_optional<compiled_regex> (regexp, cflags,
> +					    _("Invalid regexp"));

I think the indentation of the "Invalid regexp" line is off.

> +}
>  
> -      if (*opname)
> -	{
> -	  int fix = -1;		/* -1 means ok; otherwise number of
> -				    spaces needed.  */
> +/* See symtab.h.  */
>  
> -	  if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
> -	    {
> -	      /* There should 1 space between 'operator' and 'TYPENAME'.  */
> -	      if (opname[-1] != ' ' || opname[-2] == ' ')
> -		fix = 1;
> -	    }
> -	  else
> -	    {
> -	      /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
> -	      if (opname[-1] == ' ')
> -		fix = 0;
> -	    }
> -	  /* If wrong number of spaces, fix it.  */
> -	  if (fix >= 0)
> -	    {
> -	      symbol_name_regexp_holder
> -		= string_printf ("operator%.*s%s", fix, " ", opname);
> -	      symbol_name_regexp = symbol_name_regexp_holder.c_str ();
> -	    }
> -	}
> +std::optional<compiled_regex>
> +global_symbol_searcher::compile_name_regex () const
> +{
> +  if (m_symbol_name_regexp == nullptr)
> +    return {};
>  
> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
> -				? REG_ICASE : 0);
> -      name_regex.emplace (symbol_name_regexp, cflags,
> -			  _("Invalid regexp"));
> -    }
> +  const char *symbol_name_regexp = m_symbol_name_regexp;
> +  std::string symbol_name_regexp_holder;
> +
> +  /* Make sure spacing is right for C++ operators.
> +     This is just a courtesy to make the matching less sensitive
> +     to how many spaces the user leaves between 'operator'
> +     and <TYPENAME> or <OPERATOR>.  */
> +  const char *op_end;
> +  const char *opname = operator_chars (symbol_name_regexp, &op_end);
>  
> -  if (m_symbol_type_regexp != NULL)
> +  if (*opname)
>      {
> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
> -				? REG_ICASE : 0);
> -      type_regex.emplace (m_symbol_type_regexp, cflags,
> -			  _("Invalid regexp"));
> +      /* -1 means ok; otherwise number of spaces needed.  */
> +      int fix = -1;
> +
> +      if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
> +	{
> +	  /* There should 1 space between 'operator' and 'TYPENAME'.  */

This comment has a (pre-existing) missing 'be':

  /* There should BE 1 space between .... */

> +	  if (opname[-1] != ' ' || opname[-2] == ' ')
> +	    fix = 1;
> +	}
> +      else
> +	{
> +	  /* There should 0 spaces between 'operator' and 'OPERATOR'.  */

Same missing 'BE' here.

With those all fixed:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 03/11] gdb: factor out regexp compilation from global_symbol_searcher::search

[Simon Marchi]

>> +static std::optional<compiled_regex>
>> +compile_symbol_search_regex (const char *regexp)
>>  {
>> -  std::optional<compiled_regex> name_regex;
>> -  std::optional<compiled_regex> type_regex;
>> +  if (regexp == nullptr)
>> +    return {};
>>  
>> -  if (m_symbol_name_regexp != NULL)
>> -    {
>> -      const char *symbol_name_regexp = m_symbol_name_regexp;
>> -      std::string symbol_name_regexp_holder;
>> +  int cflags
>> +    = REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE : 0);
>>  
>> -      /* Make sure spacing is right for C++ operators.
>> -	 This is just a courtesy to make the matching less sensitive
>> -	 to how many spaces the user leaves between 'operator'
>> -	 and <TYPENAME> or <OPERATOR>.  */
>> -      const char *op_end;
>> -      const char *opname = operator_chars (symbol_name_regexp, &op_end);
>> +  return std::make_optional<compiled_regex> (regexp, cflags,
>> +					    _("Invalid regexp"));
> 
> I think the indentation of the "Invalid regexp" line is off.

Ah yeah it's because I switched to make_optional at the last minute (it
avoids using std::in_place.

>> -  if (m_symbol_type_regexp != NULL)
>> +  if (*opname)
>>      {
>> -      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
>> -				? REG_ICASE : 0);
>> -      type_regex.emplace (m_symbol_type_regexp, cflags,
>> -			  _("Invalid regexp"));
>> +      /* -1 means ok; otherwise number of spaces needed.  */
>> +      int fix = -1;
>> +
>> +      if (c_isalpha (*opname) || *opname == '_' || *opname == '$')
>> +	{
>> +	  /* There should 1 space between 'operator' and 'TYPENAME'.  */
> 
> This comment has a (pre-existing) missing 'be':
> 
>   /* There should BE 1 space between .... */
> 
>> +	  if (opname[-1] != ' ' || opname[-2] == ' ')
>> +	    fix = 1;
>> +	}
>> +      else
>> +	{
>> +	  /* There should 0 spaces between 'operator' and 'OPERATOR'.  */
> 
> Same missing 'BE' here.

Fixed those 2.

> 
> With those all fixed:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon

[PATCH v2 04/11] gdb: use compile_symbol_search_regex in stack.c

[simon.marchi]

From: Simon Marchi <simon.marchi@polymtl.ca>

prepare_reg in stack.c does the same thing as compile_symbol_search_regex
in symtab.c, but with an awkward output parameter.  Expose
compile_symbol_search_regex in symtab.h and use it instead of prepare_reg.

Since compiled_regex is not movable, the optionals in
print_variable_and_value_data must be initialized directly from the
return value of compile_symbol_search_regex.  Add a constructor to
print_variable_and_value_data to do so.  Make the constructor initialize
the other fields as well, instead of assigning them after construction.

Change-Id: I69939bba3488a888ef17b4d5ec71d5a9efe83dda
---
 gdb/stack.c  | 51 ++++++++++++++++++---------------------------------
 gdb/symtab.c |  6 ++----
 gdb/symtab.h |  8 ++++++++
 3 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index 79a67865affe..c6466e2cb799 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2289,13 +2289,24 @@ iterate_over_block_local_vars_printing
 
 struct print_variable_and_value_data
 {
+  print_variable_and_value_data (const char *regexp, const char *t_regexp,
+				 frame_id frame_id, int num_tabs,
+				 ui_file *stream, bool print_shadowed)
+    : name_regex (compile_symbol_search_regex (regexp)),
+      type_regex (compile_symbol_search_regex (t_regexp)),
+      frame_id (frame_id),
+      num_tabs (num_tabs),
+      stream (stream),
+      print_shadowed (print_shadowed)
+  {}
+
   std::optional<compiled_regex> name_regex;
   std::optional<compiled_regex> type_regex;
   struct frame_id frame_id;
   int num_tabs;
   struct ui_file *stream;
-  int values_printed;
-  bool print_shadowed = true;
+  int values_printed = 0;
+  bool print_shadowed;
   bool printed_shadowed_variables = false;
   bool omitted_shadowed_variables = false;
 
@@ -2344,22 +2355,6 @@ print_variable_and_value_data::operator() (const char *print_name,
     printed_shadowed_variables = true;
 }
 
-/* Prepares the regular expression REG from REGEXP.
-   If REGEXP is NULL, it results in an empty regular expression.  */
-
-static void
-prepare_reg (const char *regexp, std::optional<compiled_regex> *reg)
-{
-  if (regexp != NULL)
-    {
-      int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
-				? REG_ICASE : 0);
-      reg->emplace (regexp, cflags, _("Invalid regexp"));
-    }
-  else
-    reg->reset ();
-}
-
 /* Print all variables from the innermost up to the function block of FRAME.
    Print them with values to STREAM indented by NUM_TABS.
    PRINT_SHADOWED controls whether shadowed variables are printed.
@@ -2379,7 +2374,6 @@ print_frame_local_vars (const frame_info_ptr &frame,
 			const char *t_regexp, int num_tabs,
 			struct ui_file *stream)
 {
-  struct print_variable_and_value_data cb_data;
   const struct block *block;
   std::optional<CORE_ADDR> pc;
 
@@ -2399,13 +2393,9 @@ print_frame_local_vars (const frame_info_ptr &frame,
       return;
     }
 
-  prepare_reg (regexp, &cb_data.name_regex);
-  prepare_reg (t_regexp, &cb_data.type_regex);
-  cb_data.frame_id = get_frame_id (frame);
-  cb_data.num_tabs = 4 * num_tabs;
-  cb_data.stream = stream;
-  cb_data.values_printed = 0;
-  cb_data.print_shadowed = print_shadowed;
+  print_variable_and_value_data cb_data (regexp, t_regexp,
+					 get_frame_id (frame), 4 * num_tabs,
+					 stream, print_shadowed);
 
   /* Temporarily change the selected frame to the given FRAME.
      This allows routines that rely on the selected frame instead
@@ -2572,7 +2562,6 @@ print_frame_arg_vars (const frame_info_ptr &frame,
 		      const char *regexp, const char *t_regexp,
 		      struct ui_file *stream)
 {
-  struct print_variable_and_value_data cb_data;
   struct symbol *func;
   std::optional<CORE_ADDR> pc;
 
@@ -2592,12 +2581,8 @@ print_frame_arg_vars (const frame_info_ptr &frame,
       return;
     }
 
-  prepare_reg (regexp, &cb_data.name_regex);
-  prepare_reg (t_regexp, &cb_data.type_regex);
-  cb_data.frame_id = get_frame_id (frame);
-  cb_data.num_tabs = 0;
-  cb_data.stream = stream;
-  cb_data.values_printed = 0;
+  print_variable_and_value_data cb_data (regexp, t_regexp,
+					 get_frame_id (frame), 0, stream, true);
 
   iterate_over_block_arg_vars (func->value_block (),
     [&] (const char *print_name, symbol *sym)
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 49e8719f4ec0..2c63f9456bc3 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4938,11 +4938,9 @@ global_symbol_searcher::add_matching_msymbols
   return true;
 }
 
-/* Compile REGEXP, a symbol name or symbol type regexp.
-
-   If REGEXP is nullptr, return an empty optional.  */
+/* See symtab.h.  */
 
-static std::optional<compiled_regex>
+std::optional<compiled_regex>
 compile_symbol_search_regex (const char *regexp)
 {
   if (regexp == nullptr)
diff --git a/gdb/symtab.h b/gdb/symtab.h
index e60b32230239..de8cf40974e0 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2604,6 +2604,14 @@ enum class symbol_search_kind
   MODULE,
 };
 
+/* Compile REGEXP, a regexp used to match symbol names or types.  The regexp
+   is case-insensitive if case sensitivity is off.
+
+   If REGEXP is nullptr, return an empty optional.  */
+
+extern std::optional<compiled_regex> compile_symbol_search_regex
+  (const char *regexp);
+
 /* In order to search for global symbols of a particular kind matching
    particular regular expressions, create an instance of this structure and
    call the SEARCH member function.  */
-- 
2.55.0

Re: [PATCH v2 04/11] gdb: use compile_symbol_search_regex in stack.c

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@polymtl.ca>
>
> prepare_reg in stack.c does the same thing as compile_symbol_search_regex
> in symtab.c, but with an awkward output parameter.  Expose
> compile_symbol_search_regex in symtab.h and use it instead of prepare_reg.
>
> Since compiled_regex is not movable, the optionals in
> print_variable_and_value_data must be initialized directly from the
> return value of compile_symbol_search_regex.  Add a constructor to
> print_variable_and_value_data to do so.  Make the constructor initialize
> the other fields as well, instead of assigning them after construction.

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 04/11] gdb: use compile_symbol_search_regex in stack.c

[Tom Tromey]

>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:

Simon> Since compiled_regex is not movable

I have a patch to do that if it would be useful.

Tom

Re: [PATCH v2 04/11] gdb: use compile_symbol_search_regex in stack.c

[Simon Marchi]

On 10/1/26 11:36 AM, Tom Tromey wrote:
>>>>>> "Simon" == simon marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> Since compiled_regex is not movable
> 
> I have a patch to do that if it would be useful.
> 
> Tom

I think the resulting code with the constructor makes sense anyway, so I
would say it's not strictly needed for this series.

Thanks,

Simon

[PATCH v2 05/11] gdb: simplify insertion in global_symbol_searcher::add_matching_symbols

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

std::set::insert does nothing if an equivalent element is already
in the set, so checking with find first is redundant.  Replace find+insert
with emplace.

Change-Id: I869284407cfefb9b2d7ad7305fe04fbdaee25f1f
Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>
---
 gdb/symtab.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 2c63f9456bc3..35dd2981a53a 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4878,13 +4878,9 @@ global_symbol_searcher::add_matching_symbols
 	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
 		continue;
 
+	      /* Match, insert if not already in the results.  */
 	      if (result_set->size () < m_max_search_results)
-		{
-		  /* Match, insert if not already in the results.  */
-		  symbol_search ss (block, sym);
-		  if (result_set->find (ss) == result_set->end ())
-		    result_set->insert (ss);
-		}
+		result_set->emplace (block, sym);
 	      else
 		return false;
 	    }
-- 
2.55.0

Re: [PATCH v2 05/11] gdb: simplify insertion in global_symbol_searcher::add_matching_symbols

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> std::set::insert does nothing if an equivalent element is already
> in the set, so checking with find first is redundant.  Replace find+insert
> with emplace.

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

[PATCH v2 06/11] gdb: use a switch on m_kind in add_matching_symbols

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

add_matching_symbols applies some filters that only make sense for some
kinds of searches: the type regexp for variables and functions, the
LOC_UNRESOLVED / enum constant filter for variables, and the line number
filter for modules.  Some of these check m_kind, others check the
symbol's domain.  Since the earlier sym->matches check only lets through
symbols whose domain corresponds to m_kind, the two are equivalent.

Group these filters in a single switch on m_kind.  Add a comment for
the module filter, which skips module declarations, because it wasn't
obvious to me at first sight.

Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
---
 gdb/symtab.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 35dd2981a53a..6cc85a7eacc5 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4858,25 +4858,30 @@ global_symbol_searcher::add_matching_symbols
 				       nullptr, 0) != 0)
 		continue;
 
-	      if (((sym->domain () == VAR_DOMAIN
-		    || sym->domain () == FUNCTION_DOMAIN)
-		   && type_regex.has_value ()
-		   && !regex_matches_sym_type_name (*type_regex, sym)))
-		continue;
-
-	      if (m_kind == symbol_search_kind::VARIABLE)
+	      switch (m_kind)
 		{
+		case symbol_search_kind::VARIABLE:
+		  /* LOC_CONST can be used for more than just enums, e.g.,
+		     C++ static const members.  We only want to skip enums
+		     here.  */
 		  if (sym->loc_class () == LOC_UNRESOLVED
-		      /* LOC_CONST can be used for more than
-			 just enums, e.g., c++ static const
-			 members.  We only want to skip enums
-			 here.  */
 		      || (sym->loc_class () == LOC_CONST
-			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
+			  && sym->type ()->code () == TYPE_CODE_ENUM))
 		    continue;
+		  [[fallthrough]];
+
+		case symbol_search_kind::FUNCTION:
+		  if (type_regex.has_value ()
+		      && !regex_matches_sym_type_name (*type_regex, sym))
+		    continue;
+		  break;
+
+		case symbol_search_kind::MODULE:
+		  /* Skip module declarations, keep only definitions.  */
+		  if (sym->line () == 0)
+		    continue;
+		  break;
 		}
-	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
-		continue;
 
 	      /* Match, insert if not already in the results.  */
 	      if (result_set->size () < m_max_search_results)
-- 
2.55.0

Re: [PATCH v2 06/11] gdb: use a switch on m_kind in add_matching_symbols

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> add_matching_symbols applies some filters that only make sense for some
> kinds of searches: the type regexp for variables and functions, the
> LOC_UNRESOLVED / enum constant filter for variables, and the line number
> filter for modules.  Some of these check m_kind, others check the
> symbol's domain.  Since the earlier sym->matches check only lets through
> symbols whose domain corresponds to m_kind, the two are equivalent.
>
> Group these filters in a single switch on m_kind.  Add a comment for
> the module filter, which skips module declarations, because it wasn't
> obvious to me at first sight.
>
> Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
> ---
>  gdb/symtab.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 35dd2981a53a..6cc85a7eacc5 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -4858,25 +4858,30 @@ global_symbol_searcher::add_matching_symbols
>  				       nullptr, 0) != 0)
>  		continue;
>  
> -	      if (((sym->domain () == VAR_DOMAIN
> -		    || sym->domain () == FUNCTION_DOMAIN)
> -		   && type_regex.has_value ()
> -		   && !regex_matches_sym_type_name (*type_regex, sym)))
> -		continue;
> -
> -	      if (m_kind == symbol_search_kind::VARIABLE)
> +	      switch (m_kind)
>  		{
> +		case symbol_search_kind::VARIABLE:
> +		  /* LOC_CONST can be used for more than just enums, e.g.,
> +		     C++ static const members.  We only want to skip enums
> +		     here.  */
>  		  if (sym->loc_class () == LOC_UNRESOLVED
> -		      /* LOC_CONST can be used for more than
> -			 just enums, e.g., c++ static const
> -			 members.  We only want to skip enums
> -			 here.  */
>  		      || (sym->loc_class () == LOC_CONST
> -			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
> +			  && sym->type ()->code () == TYPE_CODE_ENUM))
>  		    continue;
> +		  [[fallthrough]];
> +
> +		case symbol_search_kind::FUNCTION:
> +		  if (type_regex.has_value ()
> +		      && !regex_matches_sym_type_name (*type_regex, sym))
> +		    continue;
> +		  break;
> +
> +		case symbol_search_kind::MODULE:
> +		  /* Skip module declarations, keep only definitions.  */
> +		  if (sym->line () == 0)
> +		    continue;
> +		  break;

I agree with Stephan's suggestion that you should add:

  case symbol_search_kind::TYPE:
    break;

I would not focus on GDB's use or non-use of -Wswitch.  Instead, my
argument is that by specifically mentioning the extra case we make it
clear that nothing needs to be done rather than leaving it for a future
reader of the code to try and figure out if TYPE is missing on purpose,
or by accident.

I don't find the reasoning that there are lots of other missing case
labels compelling, I think the argument for or against should stand on
its own technical merits -- unless you're suggesting that the GDB style
is to trim the case list as much as possible?

That said, I only care enough to send one email on the topic :)  If
you're still not convinced then go ahead:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>  		}
> -	      if (sym->domain () == MODULE_DOMAIN && sym->line () == 0)
> -		continue;
>  
>  	      /* Match, insert if not already in the results.  */
>  	      if (result_set->size () < m_max_search_results)
> -- 
> 2.55.0

Re: [PATCH v2 06/11] gdb: use a switch on m_kind in add_matching_symbols

[Simon Marchi]

On 10/1/26 11:01 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> add_matching_symbols applies some filters that only make sense for some
>> kinds of searches: the type regexp for variables and functions, the
>> LOC_UNRESOLVED / enum constant filter for variables, and the line number
>> filter for modules.  Some of these check m_kind, others check the
>> symbol's domain.  Since the earlier sym->matches check only lets through
>> symbols whose domain corresponds to m_kind, the two are equivalent.
>>
>> Group these filters in a single switch on m_kind.  Add a comment for
>> the module filter, which skips module declarations, because it wasn't
>> obvious to me at first sight.
>>
>> Change-Id: Idf1b366b4cfd16187848b64075a0067190c111fc
>> ---
>>  gdb/symtab.c | 33 +++++++++++++++++++--------------
>>  1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 35dd2981a53a..6cc85a7eacc5 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -4858,25 +4858,30 @@ global_symbol_searcher::add_matching_symbols
>>  				       nullptr, 0) != 0)
>>  		continue;
>>  
>> -	      if (((sym->domain () == VAR_DOMAIN
>> -		    || sym->domain () == FUNCTION_DOMAIN)
>> -		   && type_regex.has_value ()
>> -		   && !regex_matches_sym_type_name (*type_regex, sym)))
>> -		continue;
>> -
>> -	      if (m_kind == symbol_search_kind::VARIABLE)
>> +	      switch (m_kind)
>>  		{
>> +		case symbol_search_kind::VARIABLE:
>> +		  /* LOC_CONST can be used for more than just enums, e.g.,
>> +		     C++ static const members.  We only want to skip enums
>> +		     here.  */
>>  		  if (sym->loc_class () == LOC_UNRESOLVED
>> -		      /* LOC_CONST can be used for more than
>> -			 just enums, e.g., c++ static const
>> -			 members.  We only want to skip enums
>> -			 here.  */
>>  		      || (sym->loc_class () == LOC_CONST
>> -			  && (sym->type ()->code () == TYPE_CODE_ENUM)))
>> +			  && sym->type ()->code () == TYPE_CODE_ENUM))
>>  		    continue;
>> +		  [[fallthrough]];
>> +
>> +		case symbol_search_kind::FUNCTION:
>> +		  if (type_regex.has_value ()
>> +		      && !regex_matches_sym_type_name (*type_regex, sym))
>> +		    continue;
>> +		  break;
>> +
>> +		case symbol_search_kind::MODULE:
>> +		  /* Skip module declarations, keep only definitions.  */
>> +		  if (sym->line () == 0)
>> +		    continue;
>> +		  break;
> 
> I agree with Stephan's suggestion that you should add:
> 
>   case symbol_search_kind::TYPE:
>     break;
> 
> I would not focus on GDB's use or non-use of -Wswitch.  Instead, my
> argument is that by specifically mentioning the extra case we make it
> clear that nothing needs to be done rather than leaving it for a future
> reader of the code to try and figure out if TYPE is missing on purpose,
> or by accident.
> 
> I don't find the reasoning that there are lots of other missing case
> labels compelling, I think the argument for or against should stand on
> its own technical merits -- unless you're suggesting that the GDB style
> is to trim the case list as much as possible?

When seeing an empty case, my intuition would be to remove it, but I
really don't have strong feelings about that.  Since you both mentioned
it, I'll add it.

> 
> That said, I only care enough to send one email on the topic :)  If
> you're still not convinced then go ahead:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon

[PATCH v2 07/11] gdb: make global_symbol_searcher::is_suitable_msymbol non-static

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

is_suitable_msymbol is a static method taking the search kind as a
parameter, but all callers pass m_kind.  Make it a const non-static
method that uses m_kind directly.

Change-Id: I803e7cddbb317a8af35a82ea9e528c64f6326f31
Reviewed-By: Rohr, Stephan <stephan.rohr@intel.com>
---
 gdb/symtab.c | 10 +++++-----
 gdb/symtab.h |  5 ++---
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 6cc85a7eacc5..a436e9011626 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4720,7 +4720,7 @@ to_search_flags (symbol_search_kind kind)
 
 bool
 global_symbol_searcher::is_suitable_msymbol
-	(symbol_search_kind kind, const minimal_symbol *msymbol)
+	(const minimal_symbol *msymbol) const
 {
   switch (msymbol->type ())
     {
@@ -4728,12 +4728,12 @@ global_symbol_searcher::is_suitable_msymbol
     case mst_bss:
     case mst_file_data:
     case mst_file_bss:
-      return kind == symbol_search_kind::VARIABLE;
+      return m_kind == symbol_search_kind::VARIABLE;
     case mst_text:
     case mst_file_text:
     case mst_solib_trampoline:
     case mst_text_gnu_ifunc:
-      return kind == symbol_search_kind::FUNCTION;
+      return m_kind == symbol_search_kind::FUNCTION;
     default:
       return false;
     }
@@ -4791,7 +4791,7 @@ global_symbol_searcher::expand_symtabs
 	  if (msymbol->created_by_gdb)
 	    continue;
 
-	  if (is_suitable_msymbol (m_kind, msymbol))
+	  if (is_suitable_msymbol (msymbol))
 	    {
 	      if (!name_regex.has_value ()
 		  || name_regex->exec (msymbol->natural_name (), 0,
@@ -4909,7 +4909,7 @@ global_symbol_searcher::add_matching_msymbols
       if (msymbol->created_by_gdb)
 	continue;
 
-      if (is_suitable_msymbol (m_kind, msymbol))
+      if (is_suitable_msymbol (msymbol))
 	{
 	  if (!name_regex.has_value ()
 	      || name_regex->exec (msymbol->natural_name (), 0,
diff --git a/gdb/symtab.h b/gdb/symtab.h
index de8cf40974e0..7afefaf3a322 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2715,9 +2715,8 @@ class global_symbol_searcher
 			      const std::optional<compiled_regex> &name_regex,
 			      std::vector<symbol_search> *results) const;
 
-  /* Return true if MSYMBOL is of type KIND.  */
-  static bool is_suitable_msymbol (symbol_search_kind kind,
-				   const minimal_symbol *msymbol);
+  /* Return true if MSYMBOL is of type M_KIND.  */
+  bool is_suitable_msymbol (const minimal_symbol *msymbol) const;
 };
 
 /* When searching for Fortran symbols within modules (functions/variables)
-- 
2.55.0

Re: [PATCH v2 07/11] gdb: make global_symbol_searcher::is_suitable_msymbol non-static

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> is_suitable_msymbol is a static method taking the search kind as a
> parameter, but all callers pass m_kind.  Make it a const non-static
> method that uses m_kind directly.

LGTM

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

[PATCH v2 08/11] gdb: update comments of global_symbol_searcher::expand_symtabs

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Update the doc comment of expand_symtabs to describe more accurately what
the return value means.  It is true when a matching function msymbol for
which no debug info exists is found.

Update a comment inside expand_symtabs:

 - remove "unconditionally", because it's not quite true
 - use "for variables" instead of "for SEARCH_VAR_DOMAIN".

Change-Id: I7c26c065b31575066ed2398563cdc1dac8869f8b
---
 gdb/symtab.c | 4 ++--
 gdb/symtab.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index a436e9011626..fc0ff22448ee 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4779,8 +4779,8 @@ global_symbol_searcher::expand_symtabs
      objfiles.
 
      Variables are not handled here, and looking them up does not expand any
-     symtab.  When no file names were given the caller unconditionally rescans
-     the minimal symbols for SEARCH_VAR_DOMAIN.  */
+     symtab.  When no file names were given the caller rescans the minimal
+     symbols for variables.  */
   if (m_filenames.empty ()
       && m_kind == symbol_search_kind::FUNCTION)
     {
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 7afefaf3a322..28b9d7b4c78d 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2687,9 +2687,10 @@ class global_symbol_searcher
   /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
   std::optional<compiled_regex> compile_type_regex () const;
 
-  /* Expand symtabs in OBJFILE that match NAME_REGEX, are of type M_KIND.
-     Return true if any msymbols were seen that we should later consider
-     adding to the results list.  */
+  /* Expand symtabs in OBJFILE that match NAME_REGEX and are of type M_KIND.
+     Return true if a matching function msymbol without debug info was
+     seen, meaning that msymbols should later be considered for the results
+     list.  */
   bool expand_symtabs (objfile *objfile,
 		       const std::optional<compiled_regex> &name_regex) const;
 
-- 
2.55.0

Re: [PATCH v2 08/11] gdb: update comments of global_symbol_searcher::expand_symtabs

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Update the doc comment of expand_symtabs to describe more accurately what
> the return value means.  It is true when a matching function msymbol for
> which no debug info exists is found.
>
> Update a comment inside expand_symtabs:
>
>  - remove "unconditionally", because it's not quite true
>  - use "for variables" instead of "for SEARCH_VAR_DOMAIN".

LGTM.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

[PATCH v2 09/11] gdb: add matching helpers to global_symbol_searcher

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

The methods of global_symbol_searcher repeat the same checks in several
places.  Add some helpers to make the code easier to follow:

 - name_matches: check a name against the (optional) name regexp

 - symtab_matches_filenames: check a symtab against the searched file
   names

 - symbol_matches: all the filtering done on full symbols in
   add_matching_symbols

 - msymbol_matches: the filtering done on minimal symbols, shared by
   expand_symtabs and add_matching_msymbols

Switch expand_symtabs, add_matching_symbols and add_matching_msymbols to
use early continues to avoid deeply nested ifs.

Change-Id: I553f70fb8ea9c7e55b89bb1204a402938c116c3b
---
 gdb/symtab.c | 205 ++++++++++++++++++++++++++++-----------------------
 gdb/symtab.h |  16 ++++
 2 files changed, 130 insertions(+), 91 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index fc0ff22448ee..ef21f285abcd 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4739,6 +4739,93 @@ global_symbol_searcher::is_suitable_msymbol
     }
 }
 
+/* Return true if NAME matches NAME_REGEX, or if NAME_REGEX is empty.  */
+
+static bool
+name_matches (const std::optional<compiled_regex> &name_regex,
+	      const char *name)
+{
+  return (!name_regex.has_value ()
+	  || name_regex->exec (name, 0, nullptr, 0) == 0);
+}
+
+/* See symtab.h.  */
+
+bool
+global_symbol_searcher::symtab_matches_filenames (symtab *symtab) const
+{
+  /* Check first sole SYMTAB->FILENAME.  It does not need to be a substring
+     of symtab_to_fullname as it may contain "./" etc.  */
+  if (file_matches (symtab->filename (), m_filenames, false))
+    return true;
+
+  if (!basenames_may_differ
+      && !file_matches (lbasename (symtab->filename ()), m_filenames, true))
+    return false;
+
+  return file_matches (symtab_to_fullname (symtab), m_filenames, false);
+}
+
+/* See symtab.h.  */
+
+bool
+global_symbol_searcher::symbol_matches
+	(const symbol *sym,
+	 const std::optional<compiled_regex> &name_regex,
+	 const std::optional<compiled_regex> &type_regex) const
+{
+  if (!symtab_matches_filenames (sym->symtab ()))
+    return false;
+
+  if (!sym->matches (to_search_flags (m_kind)))
+    return false;
+
+  if (!name_matches (name_regex, sym->natural_name ()))
+    return false;
+
+  switch (m_kind)
+    {
+    case symbol_search_kind::VARIABLE:
+      /* LOC_CONST can be used for more than just enums, e.g., C++ static
+	 const members.  We only want to skip enums here.  */
+      if (sym->loc_class () == LOC_UNRESOLVED
+	  || (sym->loc_class () == LOC_CONST
+	      && sym->type ()->code () == TYPE_CODE_ENUM))
+	return false;
+      [[fallthrough]];
+
+    case symbol_search_kind::FUNCTION:
+      if (type_regex.has_value ()
+	  && !regex_matches_sym_type_name (*type_regex, sym))
+	return false;
+      break;
+
+    case symbol_search_kind::MODULE:
+      /* Skip module declarations, keep only definitions.  */
+      if (sym->line () == 0)
+	return false;
+      break;
+    }
+
+  return true;
+}
+
+/* See symtab.h.  */
+
+bool
+global_symbol_searcher::msymbol_matches
+	(const minimal_symbol *msymbol,
+	 const std::optional<compiled_regex> &name_regex) const
+{
+  if (msymbol->created_by_gdb)
+    return false;
+
+  if (!is_suitable_msymbol (msymbol))
+    return false;
+
+  return name_matches (name_regex, msymbol->natural_name ());
+}
+
 /* See symtab.h.  */
 
 bool
@@ -4761,8 +4848,7 @@ global_symbol_searcher::expand_symtabs
      &lookup_name_info::match_any (),
      [&] (const char *symname)
      {
-       return (!name_regex.has_value ()
-	       || name_regex->exec (symname, 0, NULL, 0) == 0);
+       return name_matches (name_regex, symname);
      },
      NULL,
      SEARCH_GLOBAL_BLOCK | SEARCH_STATIC_BLOCK,
@@ -4788,25 +4874,16 @@ global_symbol_searcher::expand_symtabs
 	{
 	  QUIT;
 
-	  if (msymbol->created_by_gdb)
+	  if (!msymbol_matches (msymbol, name_regex))
 	    continue;
 
-	  if (is_suitable_msymbol (msymbol))
-	    {
-	      if (!name_regex.has_value ()
-		  || name_regex->exec (msymbol->natural_name (), 0,
-				       NULL, 0) == 0)
-		{
-		  /* An important side-effect of this lookup function is
-		     to expand the symbol table if msymbol is found, later
-		     in the process we will add matching symbols or
-		     msymbols to the results list, and that requires that
-		     the symbols tables are expanded.  */
-		  if (find_compunit_symtab_for_pc
-			(msymbol->value_address (objfile)) == nullptr)
-		    found_func_msymbol_without_debug_info = true;
-		}
-	    }
+	  /* An important side-effect of this lookup function is to expand
+	     the symbol table if msymbol is found, later in the process we
+	     will add matching symbols or msymbols to the results list, and
+	     that requires that the symbols tables are expanded.  */
+	  if (find_compunit_symtab_for_pc
+		(msymbol->value_address (objfile)) == nullptr)
+	    found_func_msymbol_without_debug_info = true;
 	}
     }
 
@@ -4822,8 +4899,6 @@ global_symbol_searcher::add_matching_symbols
 	 const std::optional<compiled_regex> &type_regex,
 	 std::set<symbol_search> *result_set) const
 {
-  domain_search_flags domain = to_search_flags (m_kind);
-
   /* Add matching symbols (if not already present).  */
   for (compunit_symtab &cust : objfile->compunits ())
     {
@@ -4835,54 +4910,11 @@ global_symbol_searcher::add_matching_symbols
 
 	  for (struct symbol *sym : block_iterator_range (b))
 	    {
-	      struct symtab *real_symtab = sym->symtab ();
-
 	      QUIT;
 
-	      /* Check first sole REAL_SYMTAB->FILENAME.  It does
-		 not need to be a substring of symtab_to_fullname as
-		 it may contain "./" etc.  */
-	      if (!(file_matches (real_symtab->filename (), m_filenames, false)
-		    || ((basenames_may_differ
-			 || file_matches (lbasename (real_symtab->filename ()),
-					  m_filenames, true))
-			&& file_matches (symtab_to_fullname (real_symtab),
-					 m_filenames, false))))
-		continue;
-
-	      if (!sym->matches (domain))
+	      if (!symbol_matches (sym, name_regex, type_regex))
 		continue;
 
-	      if (name_regex.has_value ()
-		  && name_regex->exec (sym->natural_name (), 0,
-				       nullptr, 0) != 0)
-		continue;
-
-	      switch (m_kind)
-		{
-		case symbol_search_kind::VARIABLE:
-		  /* LOC_CONST can be used for more than just enums, e.g.,
-		     C++ static const members.  We only want to skip enums
-		     here.  */
-		  if (sym->loc_class () == LOC_UNRESOLVED
-		      || (sym->loc_class () == LOC_CONST
-			  && sym->type ()->code () == TYPE_CODE_ENUM))
-		    continue;
-		  [[fallthrough]];
-
-		case symbol_search_kind::FUNCTION:
-		  if (type_regex.has_value ()
-		      && !regex_matches_sym_type_name (*type_regex, sym))
-		    continue;
-		  break;
-
-		case symbol_search_kind::MODULE:
-		  /* Skip module declarations, keep only definitions.  */
-		  if (sym->line () == 0)
-		    continue;
-		  break;
-		}
-
 	      /* Match, insert if not already in the results.  */
 	      if (result_set->size () < m_max_search_results)
 		result_set->emplace (block, sym);
@@ -4906,34 +4938,25 @@ global_symbol_searcher::add_matching_msymbols
     {
       QUIT;
 
-      if (msymbol->created_by_gdb)
+      if (!msymbol_matches (msymbol, name_regex))
 	continue;
 
-      if (is_suitable_msymbol (msymbol))
-	{
-	  if (!name_regex.has_value ()
-	      || name_regex->exec (msymbol->natural_name (), 0,
-				   NULL, 0) == 0)
-	    {
-	      /* For functions we can do a quick check of whether the
-		 symbol might be found via find_pc_symtab.  */
-	      if (m_kind != symbol_search_kind::FUNCTION
-		  || (find_compunit_symtab_for_pc
-		      (msymbol->value_address (objfile)) == NULL))
-		{
-		  if (lookup_symbol_in_objfile_from_linkage_name
-		      (objfile, msymbol->linkage_name (),
-		       SEARCH_VFT).symbol == NULL)
-		    {
-		      /* Matching msymbol, add it to the results list.  */
-		      if (results->size () < m_max_search_results)
-			results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
-		      else
-			return false;
-		    }
-		}
-	    }
-	}
+      /* For functions we can do a quick check of whether the
+	 symbol might be found via find_pc_symtab.  */
+      if (m_kind == symbol_search_kind::FUNCTION
+	  && (find_compunit_symtab_for_pc
+	      (msymbol->value_address (objfile)) != NULL))
+	continue;
+
+      if (lookup_symbol_in_objfile_from_linkage_name
+	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
+	continue;
+
+      /* Matching msymbol, add it to the results list.  */
+      if (results->size () < m_max_search_results)
+	results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
+      else
+	return false;
     }
 
   return true;
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 28b9d7b4c78d..cf629387a17c 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2687,6 +2687,22 @@ class global_symbol_searcher
   /* Compile M_SYMBOL_TYPE_REGEXP, if set.  */
   std::optional<compiled_regex> compile_type_regex () const;
 
+  /* Return true if SYMTAB matches one of M_FILENAMES.  */
+  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.  */
+  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.
+
+     This function also filters out the minimal symbols artificially created by
+     GDB.  */
+  bool msymbol_matches (const minimal_symbol *msymbol,
+			const std::optional<compiled_regex> &name_regex) const;
+
   /* Expand symtabs in OBJFILE that match NAME_REGEX and are of type M_KIND.
      Return true if a matching function msymbol without debug info was
      seen, meaning that msymbols should later be considered for the results
-- 
2.55.0

Re: [PATCH v2 09/11] gdb: add matching helpers to global_symbol_searcher

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> The methods of global_symbol_searcher repeat the same checks in several
> places.  Add some helpers to make the code easier to follow:
>
>  - name_matches: check a name against the (optional) name regexp
>
>  - symtab_matches_filenames: check a symtab against the searched file
>    names
>
>  - symbol_matches: all the filtering done on full symbols in
>    add_matching_symbols
>
>  - msymbol_matches: the filtering done on minimal symbols, shared by
>    expand_symtabs and add_matching_msymbols
>
> Switch expand_symtabs, add_matching_symbols and add_matching_msymbols to
> use early continues to avoid deeply nested ifs.
>
> Change-Id: I553f70fb8ea9c7e55b89bb1204a402938c116c3b
> ---
>  gdb/symtab.c | 205 ++++++++++++++++++++++++++++-----------------------
>  gdb/symtab.h |  16 ++++
>  2 files changed, 130 insertions(+), 91 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index fc0ff22448ee..ef21f285abcd 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c


> @@ -4906,34 +4938,25 @@ global_symbol_searcher::add_matching_msymbols
>      {
>        QUIT;
>  
> -      if (msymbol->created_by_gdb)
> +      if (!msymbol_matches (msymbol, name_regex))
>  	continue;
>  
> -      if (is_suitable_msymbol (msymbol))
> -	{
> -	  if (!name_regex.has_value ()
> -	      || name_regex->exec (msymbol->natural_name (), 0,
> -				   NULL, 0) == 0)
> -	    {
> -	      /* For functions we can do a quick check of whether the
> -		 symbol might be found via find_pc_symtab.  */
> -	      if (m_kind != symbol_search_kind::FUNCTION
> -		  || (find_compunit_symtab_for_pc
> -		      (msymbol->value_address (objfile)) == NULL))
> -		{
> -		  if (lookup_symbol_in_objfile_from_linkage_name
> -		      (objfile, msymbol->linkage_name (),
> -		       SEARCH_VFT).symbol == NULL)
> -		    {
> -		      /* Matching msymbol, add it to the results list.  */
> -		      if (results->size () < m_max_search_results)
> -			results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
> -		      else
> -			return false;
> -		    }
> -		}
> -	    }
> -	}
> +      /* For functions we can do a quick check of whether the
> +	 symbol might be found via find_pc_symtab.  */
> +      if (m_kind == symbol_search_kind::FUNCTION
> +	  && (find_compunit_symtab_for_pc
> +	      (msymbol->value_address (objfile)) != NULL))
> +	continue;
> +
> +      if (lookup_symbol_in_objfile_from_linkage_name
> +	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)

Two lines here where NULL can be updated to nullptr.

With that fix:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 09/11] gdb: add matching helpers to global_symbol_searcher

[Simon Marchi]

On 10/1/26 11:39 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> The methods of global_symbol_searcher repeat the same checks in several
>> places.  Add some helpers to make the code easier to follow:
>>
>>  - name_matches: check a name against the (optional) name regexp
>>
>>  - symtab_matches_filenames: check a symtab against the searched file
>>    names
>>
>>  - symbol_matches: all the filtering done on full symbols in
>>    add_matching_symbols
>>
>>  - msymbol_matches: the filtering done on minimal symbols, shared by
>>    expand_symtabs and add_matching_msymbols
>>
>> Switch expand_symtabs, add_matching_symbols and add_matching_msymbols to
>> use early continues to avoid deeply nested ifs.
>>
>> Change-Id: I553f70fb8ea9c7e55b89bb1204a402938c116c3b
>> ---
>>  gdb/symtab.c | 205 ++++++++++++++++++++++++++++-----------------------
>>  gdb/symtab.h |  16 ++++
>>  2 files changed, 130 insertions(+), 91 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index fc0ff22448ee..ef21f285abcd 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
> 
> 
>> @@ -4906,34 +4938,25 @@ global_symbol_searcher::add_matching_msymbols
>>      {
>>        QUIT;
>>  
>> -      if (msymbol->created_by_gdb)
>> +      if (!msymbol_matches (msymbol, name_regex))
>>  	continue;
>>  
>> -      if (is_suitable_msymbol (msymbol))
>> -	{
>> -	  if (!name_regex.has_value ()
>> -	      || name_regex->exec (msymbol->natural_name (), 0,
>> -				   NULL, 0) == 0)
>> -	    {
>> -	      /* For functions we can do a quick check of whether the
>> -		 symbol might be found via find_pc_symtab.  */
>> -	      if (m_kind != symbol_search_kind::FUNCTION
>> -		  || (find_compunit_symtab_for_pc
>> -		      (msymbol->value_address (objfile)) == NULL))
>> -		{
>> -		  if (lookup_symbol_in_objfile_from_linkage_name
>> -		      (objfile, msymbol->linkage_name (),
>> -		       SEARCH_VFT).symbol == NULL)
>> -		    {
>> -		      /* Matching msymbol, add it to the results list.  */
>> -		      if (results->size () < m_max_search_results)
>> -			results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
>> -		      else
>> -			return false;
>> -		    }
>> -		}
>> -	    }
>> -	}
>> +      /* For functions we can do a quick check of whether the
>> +	 symbol might be found via find_pc_symtab.  */
>> +      if (m_kind == symbol_search_kind::FUNCTION
>> +	  && (find_compunit_symtab_for_pc
>> +	      (msymbol->value_address (objfile)) != NULL))
>> +	continue;
>> +
>> +      if (lookup_symbol_in_objfile_from_linkage_name
>> +	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
> 
> Two lines here where NULL can be updated to nullptr.

Yes, I had already fixed them following your previous review.

> With that fix:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon

[PATCH v2 10/11] gdb: change some pointers to references in global_symbol_searcher

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

add_matching_symbols and add_matching_msymbols take the container to
add results to as a pointer, but it can never be nullptr.  Pass it by
reference instead.

The objfiles are also passed by pointer to the various helper methods,
switch that to references.

Change-Id: I65d8e14f2f3152d83afbe3b348fc91a8817b87fa
---
 gdb/symtab.c | 39 +++++++++++++++++++--------------------
 gdb/symtab.h | 10 +++++-----
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index ef21f285abcd..af748190ddd5 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4830,7 +4830,7 @@ global_symbol_searcher::msymbol_matches
 
 bool
 global_symbol_searcher::expand_symtabs
-	(objfile *objfile,
+	(objfile &objfile,
 	 const std::optional<compiled_regex> &name_regex) const
 {
   bool found_func_msymbol_without_debug_info = false;
@@ -4843,7 +4843,7 @@ global_symbol_searcher::expand_symtabs
   if (!m_filenames.empty ())
     file_matcher = do_file_match;
 
-  objfile->search
+  objfile.search
     (file_matcher,
      &lookup_name_info::match_any (),
      [&] (const char *symname)
@@ -4870,7 +4870,7 @@ global_symbol_searcher::expand_symtabs
   if (m_filenames.empty ()
       && m_kind == symbol_search_kind::FUNCTION)
     {
-      for (minimal_symbol *msymbol : objfile->msymbols ())
+      for (minimal_symbol *msymbol : objfile.msymbols ())
 	{
 	  QUIT;
 
@@ -4882,7 +4882,7 @@ global_symbol_searcher::expand_symtabs
 	     will add matching symbols or msymbols to the results list, and
 	     that requires that the symbols tables are expanded.  */
 	  if (find_compunit_symtab_for_pc
-		(msymbol->value_address (objfile)) == nullptr)
+		(msymbol->value_address (&objfile)) == nullptr)
 	    found_func_msymbol_without_debug_info = true;
 	}
     }
@@ -4894,13 +4894,13 @@ global_symbol_searcher::expand_symtabs
 
 bool
 global_symbol_searcher::add_matching_symbols
-	(objfile *objfile,
+	(objfile &objfile,
 	 const std::optional<compiled_regex> &name_regex,
 	 const std::optional<compiled_regex> &type_regex,
-	 std::set<symbol_search> *result_set) const
+	 std::set<symbol_search> &result_set) const
 {
   /* Add matching symbols (if not already present).  */
-  for (compunit_symtab &cust : objfile->compunits ())
+  for (compunit_symtab &cust : objfile.compunits ())
     {
       const struct blockvector *bv  = cust.blockvector ();
 
@@ -4916,8 +4916,8 @@ global_symbol_searcher::add_matching_symbols
 		continue;
 
 	      /* Match, insert if not already in the results.  */
-	      if (result_set->size () < m_max_search_results)
-		result_set->emplace (block, sym);
+	      if (result_set.size () < m_max_search_results)
+		result_set.emplace (block, sym);
 	      else
 		return false;
 	    }
@@ -4931,10 +4931,10 @@ global_symbol_searcher::add_matching_symbols
 
 bool
 global_symbol_searcher::add_matching_msymbols
-	(objfile *objfile, const std::optional<compiled_regex> &name_regex,
-	 std::vector<symbol_search> *results) const
+	(objfile &objfile, const std::optional<compiled_regex> &name_regex,
+	 std::vector<symbol_search> &results) const
 {
-  for (minimal_symbol *msymbol : objfile->msymbols ())
+  for (minimal_symbol *msymbol : objfile.msymbols ())
     {
       QUIT;
 
@@ -4945,16 +4945,16 @@ global_symbol_searcher::add_matching_msymbols
 	 symbol might be found via find_pc_symtab.  */
       if (m_kind == symbol_search_kind::FUNCTION
 	  && (find_compunit_symtab_for_pc
-	      (msymbol->value_address (objfile)) != NULL))
+	      (msymbol->value_address (&objfile)) != NULL))
 	continue;
 
       if (lookup_symbol_in_objfile_from_linkage_name
-	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
+	    (&objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
 	continue;
 
       /* Matching msymbol, add it to the results list.  */
-      if (results->size () < m_max_search_results)
-	results->emplace_back (GLOBAL_BLOCK, msymbol, objfile);
+      if (results.size () < m_max_search_results)
+	results.emplace_back (GLOBAL_BLOCK, msymbol, &objfile);
       else
 	return false;
     }
@@ -5047,14 +5047,13 @@ global_symbol_searcher::search () const
       /* Expand symtabs within objfile that possibly contain matching
 	 symbols.  */
       found_func_msymbol_without_debug_info
-	|= expand_symtabs (&objfile, name_regex);
+	|= expand_symtabs (objfile, name_regex);
 
       /* Find matching symbols within OBJFILE and add them in to the
 	 RESULT_SET set.  Use a set here so that we can easily detect
 	 duplicates as we go, and can therefore track how many unique
 	 matches we have found so far.  */
-      if (!add_matching_symbols (&objfile, name_regex, type_regex,
-				 &result_set))
+      if (!add_matching_symbols (objfile, name_regex, type_regex, result_set))
 	break;
     }
 
@@ -5074,7 +5073,7 @@ global_symbol_searcher::search () const
       gdb_assert (m_kind == symbol_search_kind::VARIABLE
 		  || m_kind == symbol_search_kind::FUNCTION);
       for (objfile &objfile : current_program_space->objfiles ())
-	if (!add_matching_msymbols (&objfile, name_regex, &result))
+	if (!add_matching_msymbols (objfile, name_regex, result))
 	  break;
     }
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index cf629387a17c..117d3fb0134f 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2707,7 +2707,7 @@ class global_symbol_searcher
      Return true if a matching function msymbol without debug info was
      seen, meaning that msymbols should later be considered for the results
      list.  */
-  bool expand_symtabs (objfile *objfile,
+  bool expand_symtabs (objfile &objfile,
 		       const std::optional<compiled_regex> &name_regex) const;
 
   /* Add symbols from symtabs in OBJFILE that match NAME_REGEX, and
@@ -2717,10 +2717,10 @@ class global_symbol_searcher
      otherwise return true.
      Returning true does not indicate that any results were added, just
      that we didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
-  bool add_matching_symbols (objfile *objfile,
+  bool add_matching_symbols (objfile &objfile,
 			     const std::optional<compiled_regex> &name_regex,
 			     const std::optional<compiled_regex> &type_regex,
-			     std::set<symbol_search> *result_set) const;
+			     std::set<symbol_search> &result_set) const;
 
   /* Add msymbols from OBJFILE that match NAME_REGEX and M_KIND, to the
      results vector RESULTS.  Return false if we stop adding results early
@@ -2728,9 +2728,9 @@ class global_symbol_searcher
      results limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning
      true does not indicate that any results were added, just that we
      didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
-  bool add_matching_msymbols (objfile *objfile,
+  bool add_matching_msymbols (objfile &objfile,
 			      const std::optional<compiled_regex> &name_regex,
-			      std::vector<symbol_search> *results) const;
+			      std::vector<symbol_search> &results) const;
 
   /* Return true if MSYMBOL is of type M_KIND.  */
   bool is_suitable_msymbol (const minimal_symbol *msymbol) const;
-- 
2.55.0

Re: [PATCH v2 10/11] gdb: change some pointers to references in global_symbol_searcher

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> add_matching_symbols and add_matching_msymbols take the container to
> add results to as a pointer, but it can never be nullptr.  Pass it by
> reference instead.
>
> The objfiles are also passed by pointer to the various helper methods,
> switch that to references.
>
> Change-Id: I65d8e14f2f3152d83afbe3b348fc91a8817b87fa
> ---
>  gdb/symtab.c | 39 +++++++++++++++++++--------------------
>  gdb/symtab.h | 10 +++++-----
>  2 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index ef21f285abcd..af748190ddd5 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c

> @@ -4945,16 +4945,16 @@ global_symbol_searcher::add_matching_msymbols
>  	 symbol might be found via find_pc_symtab.  */
>        if (m_kind == symbol_search_kind::FUNCTION
>  	  && (find_compunit_symtab_for_pc
> -	      (msymbol->value_address (objfile)) != NULL))
> +	      (msymbol->value_address (&objfile)) != NULL))
>  	continue;
>  
>        if (lookup_symbol_in_objfile_from_linkage_name
> -	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
> +	    (&objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
>  	continue;

Two lines above here can replace NULL with nullptr.

With that done:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 10/11] gdb: change some pointers to references in global_symbol_searcher

[Simon Marchi]

On 10/1/26 11:53 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> add_matching_symbols and add_matching_msymbols take the container to
>> add results to as a pointer, but it can never be nullptr.  Pass it by
>> reference instead.
>>
>> The objfiles are also passed by pointer to the various helper methods,
>> switch that to references.
>>
>> Change-Id: I65d8e14f2f3152d83afbe3b348fc91a8817b87fa
>> ---
>>  gdb/symtab.c | 39 +++++++++++++++++++--------------------
>>  gdb/symtab.h | 10 +++++-----
>>  2 files changed, 24 insertions(+), 25 deletions(-)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index ef21f285abcd..af748190ddd5 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
> 
>> @@ -4945,16 +4945,16 @@ global_symbol_searcher::add_matching_msymbols
>>  	 symbol might be found via find_pc_symtab.  */
>>        if (m_kind == symbol_search_kind::FUNCTION
>>  	  && (find_compunit_symtab_for_pc
>> -	      (msymbol->value_address (objfile)) != NULL))
>> +	      (msymbol->value_address (&objfile)) != NULL))
>>  	continue;
>>  
>>        if (lookup_symbol_in_objfile_from_linkage_name
>> -	    (objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
>> +	    (&objfile, msymbol->linkage_name (), SEARCH_VFT).symbol != NULL)
>>  	continue;
> 
> Two lines above here can replace NULL with nullptr.

Done.

> With that done:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon

[PATCH v2 11/11] gdb: fix some comments in global_symbol_searcher

[simon.marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Fix a few comments in and around global_symbol_searcher:

 - The comment of global_symbol_searcher::m_exclude_minsyms refers to
   M_SYMBOL_REGEXP, which does not (or no longer) exist.  It means
   M_SYMBOL_NAME_REGEXP.

 - The comment in global_symbol_searcher::add_matching_msymbols refers
   to find_pc_symtab, which does not exist anymore.  The code calls
   find_compunit_symtab_for_pc.

 - The comment of global_symbol_searcher calls it a structure, but it
   is a class.

 - RESULTS_SET -> RESULT_SET

 - MAX_SEARCH_RESULTS -> M_MAX_SEARCH_RESULTS

Change-Id: I0ec18651688406bb525752a53b9558096a798aef
---
 gdb/symtab.c |  2 +-
 gdb/symtab.h | 14 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index af748190ddd5..eb76cb83bdb4 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -4942,7 +4942,7 @@ global_symbol_searcher::add_matching_msymbols
 	continue;
 
       /* For functions we can do a quick check of whether the
-	 symbol might be found via find_pc_symtab.  */
+	 symbol might be found via find_compunit_symtab_for_pc.  */
       if (m_kind == symbol_search_kind::FUNCTION
 	  && (find_compunit_symtab_for_pc
 	      (msymbol->value_address (&objfile)) != NULL))
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 117d3fb0134f..bd719af66697 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2613,8 +2613,8 @@ extern std::optional<compiled_regex> compile_symbol_search_regex
   (const char *regexp);
 
 /* In order to search for global symbols of a particular kind matching
-   particular regular expressions, create an instance of this structure and
-   call the SEARCH member function.  */
+   particular regular expressions, create an instance of this class and call
+   the SEARCH member function.  */
 class global_symbol_searcher
 {
 public:
@@ -2670,8 +2670,8 @@ class global_symbol_searcher
   /* Regular expression to match against the symbol type.  */
   const char *m_symbol_type_regexp = nullptr;
 
-  /* When this flag is false then minsyms that match M_SYMBOL_REGEXP will
-     be included in the results, otherwise they are excluded.  */
+  /* When this flag is false then minsyms that match M_SYMBOL_NAME_REGEXP
+     will be included in the results, otherwise they are excluded.  */
   bool m_exclude_minsyms = false;
 
   /* Maximum number of search results.  We currently impose a hard limit
@@ -2711,12 +2711,12 @@ class global_symbol_searcher
 		       const std::optional<compiled_regex> &name_regex) const;
 
   /* Add symbols from symtabs in OBJFILE that match NAME_REGEX, and
-     TYPE_REGEX, and are of type M_KIND, to the results set RESULTS_SET.
+     TYPE_REGEX, and are of type M_KIND, to the results set RESULT_SET.
      Return false if we stop adding results early due to having already
      found too many results (based on M_MAX_SEARCH_RESULTS limit),
      otherwise return true.
      Returning true does not indicate that any results were added, just
-     that we didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
+     that we didn't _not_ add a result due to reaching M_MAX_SEARCH_RESULTS.  */
   bool add_matching_symbols (objfile &objfile,
 			     const std::optional<compiled_regex> &name_regex,
 			     const std::optional<compiled_regex> &type_regex,
@@ -2727,7 +2727,7 @@ class global_symbol_searcher
      due to having already found too many results (based on max search
      results limit M_MAX_SEARCH_RESULTS), otherwise return true.  Returning
      true does not indicate that any results were added, just that we
-     didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
+     didn't _not_ add a result due to reaching M_MAX_SEARCH_RESULTS.  */
   bool add_matching_msymbols (objfile &objfile,
 			      const std::optional<compiled_regex> &name_regex,
 			      std::vector<symbol_search> &results) const;
-- 
2.55.0

Re: [PATCH v2 11/11] gdb: fix some comments in global_symbol_searcher

[Andrew Burgess]

simon.marchi@polymtl.ca writes:

> From: Simon Marchi <simon.marchi@efficios.com>
>
> Fix a few comments in and around global_symbol_searcher:
>
>  - The comment of global_symbol_searcher::m_exclude_minsyms refers to
>    M_SYMBOL_REGEXP, which does not (or no longer) exist.  It means
>    M_SYMBOL_NAME_REGEXP.
>
>  - The comment in global_symbol_searcher::add_matching_msymbols refers
>    to find_pc_symtab, which does not exist anymore.  The code calls
>    find_compunit_symtab_for_pc.
>
>  - The comment of global_symbol_searcher calls it a structure, but it
>    is a class.
>
>  - RESULTS_SET -> RESULT_SET
>
>  - MAX_SEARCH_RESULTS -> M_MAX_SEARCH_RESULTS

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

Re: [PATCH v2 11/11] gdb: fix some comments in global_symbol_searcher

[Simon Marchi]

On 10/1/26 11:56 AM, Andrew Burgess wrote:
> simon.marchi@polymtl.ca writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> Fix a few comments in and around global_symbol_searcher:
>>
>>  - The comment of global_symbol_searcher::m_exclude_minsyms refers to
>>    M_SYMBOL_REGEXP, which does not (or no longer) exist.  It means
>>    M_SYMBOL_NAME_REGEXP.
>>
>>  - The comment in global_symbol_searcher::add_matching_msymbols refers
>>    to find_pc_symtab, which does not exist anymore.  The code calls
>>    find_compunit_symtab_for_pc.
>>
>>  - The comment of global_symbol_searcher calls it a structure, but it
>>    is a class.
>>
>>  - RESULTS_SET -> RESULT_SET
>>
>>  - MAX_SEARCH_RESULTS -> M_MAX_SEARCH_RESULTS
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew

Thanks for the reviews, I addressed your comments and pushed the series.

Simon