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

["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 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.