[review v5] gdb: Introduce global_symbol_searcher

["Simon Marchi (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/264
......................................................................


Patch Set 5: Code-Review+2

(6 comments)

LGTM, I just noted a few small comments.

| --- gdb/symtab.c
| +++ gdb/symtab.c
| @@ -4457,16 +4437,18 @@ /* See symtab.h.  */
|  
|  std::vector<symbol_search>
| -search_symbols (const char *regexp, enum search_domain kind,
| -		const char *t_regexp,
| -		int nfiles, const char *files[],
| -		bool exclude_minsyms)
| -{
| +global_symbol_searcher::search () const
| +{
| +  const char *regexp = m_symbol_regexp;
| +  const char *t_regexp = m_type_regexp;
| +  enum search_domain kind = m_kind;
| +  bool exclude_minsyms = m_exclude_minsyms;
| +  int nfiles = filenames.size ();

PS3, Line 4445:

Done

|    const struct blockvector *bv;
|    const struct block *b;
|    int i = 0;
|    struct block_iterator iter;
|    struct symbol *sym;
|    int found_misc = 0;
|    static const enum minimal_symbol_type types[]
|      = {mst_data, mst_text, mst_unknown};
|    static const enum minimal_symbol_type types2[]

 ...

| @@ -4539,16 +4521,19 @@ global_symbol_searcher::search () const
|      }
|  
|    /* Search through the partial symtabs *first* for all symbols
|       matching the regexp.  That way we don't have to reproduce all of
|       the machinery below.  */
|    expand_symtabs_matching ([&] (const char *filename, bool basenames)
|  			   {
| -			     return file_matches (filename, files, nfiles,
| -						  basenames);
| +			     /* EXPAND_SYMTABS_MATCHING expects a callback
| +				that returns an integer, not a boolean as
| +				FILE_MATCHES does.  */

PS3, Line 4530:

Done

| +			     return file_matches (filename, filenames,
| +						  basenames) ? 1 : 0;
|  			   },
|  			   lookup_name_info::match_any (),
|  			   [&] (const char *symname)
|  			   {
|  			     return (!preg.has_value ()
|  				     || preg->exec (symname,
|  						    0, NULL, 0) == 0);
| --- gdb/symtab.c
| +++ gdb/symtab.c
| @@ -4527,24 +4506,26 @@ global_symbol_searcher::search () const
|  	}
|  
|        int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
|  				? REG_ICASE : 0);
| -      preg.emplace (regexp, cflags, _("Invalid regexp"));
| -    }
| -
| -  if (t_regexp != NULL)
| +      preg.emplace (symbol_name_regexp, cflags,
| +		    _("Invalid m_symbol_name_regexp"));

PS5, Line 4511:

These are user visible regexp, I think they should be user friendly
messages like

  "Invalid symbol name regexp"

| +    }
| +
| +  if (m_symbol_type_regexp != NULL)
|      {
|        int cflags = REG_NOSUB | (case_sensitivity == case_sensitive_off
|  				? REG_ICASE : 0);
| -      treg.emplace (t_regexp, cflags, _("Invalid regexp"));
| +      treg.emplace (m_symbol_type_regexp, cflags,
| +		    _("Invalid m_symbol_namregexp"));

PS5, Line 4519:

Same.

|      }
|  
|    /* Search through the partial symtabs *first* for all symbols
| -     matching the regexp.  That way we don't have to reproduce all of
| +     matching the m_symbol_namregexp.  That way we don't have to reproduce all of

PS5, Line 4523:

m_symbol_name_regexp

|       the machinery below.  */
|    expand_symtabs_matching ([&] (const char *filename, bool basenames)
|  			   {
| -			     return file_matches (filename, files, nfiles,
| +			     return file_matches (filename, filenames,
|  						  basenames);
|  			   },
|  			   lookup_name_info::match_any (),
|  			   [&] (const char *symname)

 ...

| @@ -4670,16 +4652,16 @@ global_symbol_searcher::search () const
|  	}
|      }
|  
|    if (!result.empty ())
|      sort_search_symbols_remove_dups (&result);
|  
|    /* If there are no eyes, avoid all contact.  I mean, if there are
|       no debug symbols, then add matching minsyms.  But if the user wants
| -     to see symbols matching a type regexp, then never give a minimal symbol,
| +     to see symbols matching a type m_symbol_namregexp, then never give a minimal symbol,

PS5, Line 4660:

That sounds not correct.

|       as we assume that a minimal symbol does not have a type.  */
|  
| -  if ((found_misc || (nfiles == 0 && kind != FUNCTIONS_DOMAIN))
| -      && !exclude_minsyms
| +  if ((found_misc || (filenames.empty () && m_kind != FUNCTIONS_DOMAIN))
| +      && !m_exclude_minsyms
|        && !treg.has_value ())
|      {
|        for (objfile *objfile : current_program_space->objfiles ())

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I488ab292a892d9e9e84775c632c5f198b6ad3710
Gerrit-Change-Number: 264
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-Comment-Date: Sat, 23 Nov 2019 13:17:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: comment