[review v3] 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 3: Code-Review-1

(6 comments)

| --- gdb/symtab.c
| +++ gdb/symtab.c
| @@ -4355,8 +4342,11 @@ file_matches (const char *file, const char *files[], int nfiles, int basenames)
| -						   ? lbasename (files[i])
| -						   : files[i])))
| -	    return 1;
| -	}
| -    }
| -  else if (nfiles == 0)
| -    return 1;
| -  return 0;
| +/* Compare FILE against all the entries of FILENAMES.  If BASENAMES is
| +   non-zero compare only lbasename of FILENAMES.  */

PS3, Line 4343:

true

| +
| +static bool
| +file_matches (const char *file, const std::vector<const char *> &filenames,
| +	      bool basenames)
| +{
| +  if (filenames.empty ())
| +    return true;
| +
| +  for (const char *name : filenames)

 ...

| @@ -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:

I think it would be clearer to just use the fields directly in this
function.

|    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:

Does it?  Maybe this comes from an old patch, because the callback
type returns a bool, since:

 commit 14bc53a81471e0b550de1c24d4d5266f676aacc3
 Author:     Pedro Alves <palves@redhat.com>
 AuthorDate: Wed Feb 22 14:43:35 2017 +0000
 Commit:     Pedro Alves <palves@redhat.com>
 CommitDate: Thu Feb 23 16:16:06 2017 +0000

    Use gdb::function_view in iterate_over_symtabs & co

| +			     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);

 ...

| @@ -4837,17 +4822,16 @@ symtab_symbol_info (bool quiet, bool exclude_minsyms,
|      {"variable", "function", "type", "module"};
|    const char *last_filename = "";
|    int first = 1;
|  
|    gdb_assert (kind != ALL_DOMAIN);
|  
|    if (regexp != nullptr && *regexp == '\0')
|      regexp = nullptr;
|  
|    /* Must make sure that if we're interrupted, symbols gets freed.  */

PS3, Line 4831:

This comment seems really outdated, maybe remove it?

| -  std::vector<symbol_search> symbols = search_symbols (regexp, kind,
| -						       t_regexp, 0, NULL,
| -						       exclude_minsyms);
| +  global_symbol_searcher spec (kind, regexp, t_regexp, exclude_minsyms);
| +  std::vector<symbol_search> symbols = spec.search ();
|  
|    if (!quiet)
|      {
|        if (regexp != NULL)
| --- gdb/symtab.h
| +++ gdb/symtab.h
| @@ -2088,0 +2080,32 @@ extern std::vector<symbol_search> search_symbols (const char *,
| +/* 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.  */
| +class global_symbol_searcher
| +{
| +public:
| +
| +  /* Constructor.  */
| +  global_symbol_searcher (enum search_domain kind,
| +			  const char *symbol_regexp = nullptr,
| +			  const char *type_regexp = nullptr,
| +			  bool exclude_minsyms = false,
| +			  std::vector<const char *> filename = {})

PS3, Line 2092:

I don't know how big of a change it would be, but I think these
optional knobs would be better as methods on the class, rather than
parameters to the constructors.  The caller can just set those it
needs.  For example:

 global_symbol_searcher searcher (VARIABLES_DOMAIN);
 searcher.set_exclude_minsyms (true);
 searcher.set_name_regexp ("hello.*");

I think that's more readable than this, since the method names say
what they do:

 global_symbol_searcher searcher (VARIABLES_DOMAIN, "hello.*", NULL, false);

Also, as shown above, maybe "name_regexp", instead of "symbol_regexp",
since we're talking about the symbol name?

| +    : m_kind (kind),
| +      m_symbol_regexp (symbol_regexp),
| +      m_type_regexp (type_regexp),
| +      m_exclude_minsyms (exclude_minsyms)
| +  {
| +    /* The symbol searching is designed to only find one kind of thing.  */
| +    gdb_assert (m_kind != ALL_DOMAIN);
| +  }
| +
| +  /* Search the symbol table for matches as defined by SEARCH_SPEC.

PS3, Line 2102:

I know that comes from the old comment, but I think it would be a good
time to improve it.

When I read "the symbol table", I wondered: which symbol table?  I
don't see any symbol table being passed.  So maybe it should say
"Search symbols of the objfiles in the current program space" or
something like that, it would be more accurate.

| +
| +     Within each file the results are sorted locally; each symtab's global
| +     and static blocks are separately alphabetized.  Duplicate entries are
| +     removed.  */
| +  std::vector<symbol_search> search () const;
| +
| +  /* The set of source files to search in for matching symbols.  This is
| +     currently public so that it can be populated after this object has
| +     been constructed.  */

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I488ab292a892d9e9e84775c632c5f198b6ad3710
Gerrit-Change-Number: 264
Gerrit-PatchSet: 3
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: Thu, 21 Nov 2019 04:01:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment