[review v7] gdb/mi: Add -max-results parameter to some -symbol-info-* commands

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

Simon Marchi has posted comments on this change.

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


Patch Set 7:

(8 comments)

I think the way you refactored it is very nice, it breaks the thing up in smaller operations, easier to understand.  But it's difficult to figure out what's old stuff and what's new features.  Do you think you could split this in separate patches, one that does the refactoring (split in multiple helper methods), that would have no user visible changes, and then the one that adds --max-results?

On the general design of the patch:

If I understand correctly, the --max-results value doesn't bound the expansion of symtabs without one objfile.  When we have started to expand the matching symtabs of an objfile, there's no way to stop it unless it has gone through the whole objfile, right?  So let's say you do:

 -symbol-info-functions --max-results 2 a

we'll still proceed to expand all symtabs of the objfile that have a symbol containing `a`, which may take a very long time.  And once that's done, we'll pick the first 2 of all these symbols.

I think it's a reasonable behavior for this patch, it's better to get something working first and incrementally improve.

If we think about how to optimize later: the expand_symtabs_matching method of quick_symbol_functions already takes an "expansion_notify" callback, that is called after each individual symtab expansion.  I could see that callback returning a bool, to indicate whether to continue or stop expanding symtabs.  We would add the matching symbols from the symtab to our result set in that callback, so we could stop the expansions once we have reached our limit. WDYT?

| --- gdb/symtab.c
| +++ gdb/symtab.c
| @@ -4438,0 +4436,26 @@ global_symbol_searcher::is_suitable_msymbol
| +    = {mst_data, mst_text};
| +  static const enum minimal_symbol_type types2[]
| +    = {mst_bss, mst_file_text};
| +  static const enum minimal_symbol_type types3[]
| +    = {mst_file_data, mst_solib_trampoline};
| +  static const enum minimal_symbol_type types4[]
| +    = {mst_file_bss, mst_text_gnu_ifunc};
| +
| +  gdb_assert (kind == VARIABLES_DOMAIN || kind == FUNCTIONS_DOMAIN);
| +  gdb_assert (kind <= sizeof (types));

PS7, Line 4445:

Shouldn't that be `<`?

| +
| +  enum minimal_symbol_type ourtype = types[kind];
| +  enum minimal_symbol_type ourtype2 = types2[kind];
| +  enum minimal_symbol_type ourtype3 = types3[kind];
| +  enum minimal_symbol_type ourtype4 = types4[kind];
| +
| +  return (MSYMBOL_TYPE (msymbol) == ourtype

PS7, Line 4452:

I know this was pre-existing code, but that seems overly
complicated/obfuscated.  There's surely a clearer way to do this, like
(not tested, and I never remember how exactly we format switch-cases)

 switch (MSYMBOL_TYPE (msymbol))
   {
     case mst_data:
     case mst_bss:
     case mst_file_data:
     case mst_file_bss:
       return kind == VARIABLES_DOMAIN;

     case mst_text:
     case mst_file_text:
     case mst_solib_trampoline:
     case mst_text_gnu_ifunc:
       return kind == FUNCTIONS_DOMAIN;

     default:
       return false;
   }

| +	  || MSYMBOL_TYPE (msymbol) == ourtype2
| +	  || MSYMBOL_TYPE (msymbol) == ourtype3
| +	  || MSYMBOL_TYPE (msymbol) == ourtype4);
| +}
| +
| +/* See symtab.h.  */
| +
| +bool
| +global_symbol_searcher::expand_symtabs

 ...

| @@ -4438,0 +4504,54 @@ global_symbol_searcher::expand_symtabs
| +	  if (msymbol->created_by_gdb)
| +	    continue;
| +
| +	  if (is_suitable_msymbol (kind, msymbol))
| +	    {
| +	      if (!preg.has_value ()
| +		  || preg->exec (msymbol->natural_name (), 0,
| +				 NULL, 0) == 0)
| +		{
| +		  /* An important side-effect of these lookup functions is
| +		     to expand the symbol table if msymbol is found, for
| +		     the benefit of the next loop on compunits.  */

PS7, Line 4515:

I guess this comment isn't really meaningful anymore the way it is
stated, since the loop it refers to is not there anymore.  It's
perhaps still important, but just needs to be reformulated.

| +		  if (kind == FUNCTIONS_DOMAIN
| +		      ? (find_pc_compunit_symtab
| +			 (MSYMBOL_VALUE_ADDRESS (objfile, msymbol))
| +			 == NULL)
| +		      : (lookup_symbol_in_objfile_from_linkage_name
| +			 (objfile, msymbol->linkage_name (),
| +			  VAR_DOMAIN)
| +			 .symbol == NULL))
| +		    found_misc = true;
| +		}
| +	    }
| +	}
| +    }
| +
| +  return found_misc;
| +}
| +
| +/* See symtab.h.  */
| +bool

PS7, Line 4534:

Missing newline before this.

| +global_symbol_searcher::add_matching_symbols
| +	(objfile *objfile,
| +	 const gdb::optional<compiled_regex> &preg,
| +	 const gdb::optional<compiled_regex> &treg,
| +	 std::set<symbol_search> *result_set) const
| +{
| +  enum search_domain kind = m_kind;
| +
| +  /* Add matching symbols (if not already present).  */
| +  for (compunit_symtab *cust : objfile->compunits ())
| +    {
| +      const struct blockvector *bv  = COMPUNIT_BLOCKVECTOR (cust);
| +
| +      for (int i = GLOBAL_BLOCK; i <= STATIC_BLOCK; i++)

PS7, Line 4548:

Not for this patch, but I think we could eventually write this as

 for (block_enum block : { GLOBAL_BLOCK, STATIC_BLOCK })

I find it more expressive of what we're trying to do.

| +	{
| +	  struct block_iterator iter;
| +	  struct symbol *sym;
| +	  const struct block *b = BLOCKVECTOR_BLOCK (bv, i);
| +
| +	  ALL_BLOCK_SYMBOLS (b, iter, sym)
| +	    {
| +	      struct symtab *real_symtab = symbol_symtab (sym);
| +

 ...

| @@ -4590,17 +4725,4 @@ global_symbol_searcher::search () const
| -			     (objfile, msymbol->linkage_name (), VAR_DOMAIN)
| -			     .symbol == NULL))
| -			found_misc = 1;
| -		    }
| -		}
| -	    }
| -	}
| -    }
| -
| +  bool found_misc = false;

PS7, Line 4725:

Do you know why this is called `found_misc`?  Wouldn't it make more
sense to call it found_minsym?

| +  std::set<symbol_search> result_set;
|    for (objfile *objfile : current_program_space->objfiles ())
|      {
| -      for (compunit_symtab *cust : objfile->compunits ())
| -	{
| -	  bv = COMPUNIT_BLOCKVECTOR (cust);
| -	  for (i = GLOBAL_BLOCK; i <= STATIC_BLOCK; i++)
| -	    {
| -	      b = BLOCKVECTOR_BLOCK (bv, i);
| --- gdb/symtab.h
| +++ gdb/symtab.h
| @@ -2106,6 +2114,19 @@ private:
|    /* 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.  */
|    bool m_exclude_minsyms = false;
| +
| +  /* Maximum number of search results, set to -1 for unlimited, otherwise
| +     set to a positive value to limit the number of results returned.  */
| +  int m_max_search_results = -1;

PS7, Line 2123:

It might simplfy the code (avoid having to check for -1) to make this
field a size_t and using SIZE_MAX for "no limit".

| +
| +  /* 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 gdb::optional<compiled_regex> &preg) 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

 ...

| @@ -2112,0 +2136,19 @@ private:
| +     didn't _not_ add a result due to reaching MAX_SEARCH_RESULTS.  */
| +  bool add_matching_symbols (objfile *objfile,
| +			     const gdb::optional<compiled_regex> &preg,
| +			     const gdb::optional<compiled_regex> &treg,
| +			     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 in SEARCH_SPEC), otherwise return true.  Returning true

PS7, Line 2145:

That looks stale?

| +     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 gdb::optional<compiled_regex> &preg,
| +			      std::vector<symbol_search> *results) const;
| +
| +  /* Return true if MSYMBOL is of type KIND.  */
| +  static bool is_suitable_msymbol (const enum search_domain kind,
| +				   const minimal_symbol *msymbol);

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I90a28feb55b388fb46461a096c5db08b6b0bd427
Gerrit-Change-Number: 269
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Wed, 27 Nov 2019 05:31:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment