[review v3] gdb/mi: Add new commands -symbol-info-{functions,variables,types}

["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/+/266
......................................................................


Patch Set 3:

(7 comments)

Since you were looking for suggestions for how to make the output code less ugly: I think the best way would be to make the result from the symbol search code more structured from the start, not just return a flat list of symbols.  That might not be a realistic goal for the short term though.

But what you can do now is iterate on the vector as if it was structured like that:

 if there are debug symbols:
   prepare the emitter for debug symbols
   for each symtab:
     prepare the emitter for the symtab
     for each symbol:
       output the symbol

 if there are non-debug symbols:
   prepare the emitter for non-debug symbols
   for each non-debug symbols:
     output the symbol

This way you don't need any optionals, the code is linear and straighforward to follow, the order of emitter destruction is automatically good because each emitter is in the right scope.  I have uploaded the users/simark/mi-symbols-output branch on Sourceware with a commit that does that, feel free to steal from it.  But the main part is:

 static void
 mi_symbol_info (enum search_domain kind, const char *regexp,
 		const char *t_regexp, bool exclude_minsyms)
 {
   /* Must make sure that if we're interrupted, symbols gets freed.  */
   global_symbol_searcher sym_search (kind, regexp, t_regexp, exclude_minsyms);
   std::vector<symbol_search> symbols = sym_search.search ();
   ui_out *uiout = current_uiout;
   int i = 0;
 
   ui_out_emit_tuple outer_symbols_emitter (uiout, "symbols");
 
   /* Debug symbols are placed first. */
   if (i < symbols.size () && symbols[i].msymbol.minsym == nullptr)
     {
       ui_out_emit_list debug_symbols_list_emitter (uiout, "debug");
 
       /* As long as we have debug symbols...  */
       while (i < symbols.size () && symbols[i].msymbol.minsym == nullptr)
 	{
 	  symtab *symtab = symbol_symtab (symbols[i].symbol);
 	  ui_out_emit_tuple symtab_tuple_emitter (uiout, nullptr);
 
 	  uiout->field_string ("filename", symtab_to_filename_for_display (symtab));
 	  uiout->field_string ("fullname", symtab_to_fullname (symtab));
 
 	  ui_out_emit_list symbols_list_emitter (uiout, "symbols");
 
 	  /* As long as we have debug symbols from this symtab...  */
 	  while (i < symbols.size ()
 		 && symbols[i].msymbol.minsym == nullptr
 		 && symbol_symtab (symbols[i].symbol) == symtab)
 	    {
 	      symbol_search &s = symbols[i];
 
 	      output_debug_symbol(uiout, kind, s.symbol, s.block);
 
 	      i++;
 	    }
 	}
     }
 
   /* Non-debug symbols are placed after.  */
   if (i < symbols.size ())
     {
       ui_out_emit_list nondebug_symbols_list_emitter (uiout, "nondebug");
 
       /* As long as we have nondebug symbols...  */
       while (i < symbols.size ())
 	{
 	  gdb_assert (symbols[i].msymbol.minsym != nullptr);
 
 	  output_nondebug_symbol(uiout, symbols[i].msymbol);
 
 	  i++;
 	}
     }
 }

| --- gdb/mi/mi-symbol-cmds.c
| +++ gdb/mi/mi-symbol-cmds.c
| @@ -62,0 +190,38 @@ class mi_symbol_info_emitter
| +
| +	/* Record the current symtab.  */
| +	m_last_symtab = s;
| +      }
| +  }
| +
| +public:
| +  /* Constructor.  */
| +  mi_symbol_info_emitter (struct ui_out *uiout)
| +    : m_last_symtab (nullptr),

PS3, Line 199:

I don't think we have a rule about which style to use, but this one
could be initialized directly where the field is declared, I find it
nice:

 const symtab *m_last_symtab = nullptr;

| +      m_uiout (uiout),
| +      m_outer_symbols (uiout, "symbols")
| +  { /* Nothing.  */ }
| +
| +  /* Output P a symbol found by searching for symbols of type KIND.  */
| +  void output (const symbol_search &p, enum search_domain kind)
| +  {
| +    if (p.msymbol.minsym != NULL)
| +      {
| +	/* If this is the first nondebug symbol, and we have previous

PS3, Line 209:

previously

| +	   outputted a debug symbol then we need to close down all of the
| +	   emitters related to printing debug symbols.  */
| +	maybe_finish_debug_output ();
| +
| +	/* If this is the first nondebug symbol then we need to create the
| +	   emitters related to printing nondebug symbols.  */
| +	maybe_start_nondebug_symbol_output ();
| +
| +	/* We are no safe to emit the nondebug symbol.  */

PS3, Line 218:

now

| +	output_nondebug_symbol (p.msymbol);
| +      }
| +    else
| +      {
| +	/* All debug symbols should appear in the list before all
| +	   non-debug symbols.  */
| +	gdb_assert (!have_started_nondebug_symbol_output ());
| +
| +	/* If this is the first debug symbol then we need to create the

 ...

| @@ -62,0 +234,48 @@ public:
| +
| +	/* Emit information for this debug symbol.  */
| +	mi_info_one_symbol_details (kind, p.symbol, p.block);
| +      }
| +  }
| +};
| +
| +/* This is the guts of the commands '-symbol-info-functions',
| +   '-symbol-info-variables', and '-symbol-info-types'.  It calls
| +   search_symbols to find all matches and then prints the matching

PS3, Line 243:

This should be changed.

| +   [m]symbols in an MI structured format.  This is similar to
| +   symtab_symbol_info in symtab.c.  All the arguments are used to
| +   initialise a SEARCH_SYMBOLS_SPEC, see symtab.h for a description of

PS3, Line 246:

This too.

| +   their meaning.  */
| +
| +static void
| +mi_symbol_info (enum search_domain kind, const char *regexp,
| +		const char *t_regexp, bool exclude_minsyms)
| +{
| +  /* Must make sure that if we're interrupted, symbols gets freed.  */
| +  global_symbol_searcher sym_search (kind, regexp, t_regexp, exclude_minsyms);
| +  std::vector<symbol_search> symbols = sym_search.search ();
| +
| +  mi_symbol_info_emitter emitter (current_uiout);
| +  for (const symbol_search &p : symbols)
| +    {
| +      QUIT;
| +      emitter.output (p, kind);

PS3, Line 261:

Since `kind` never changes, couldn't it be passed just once to the
constructor?

| +    }
| +}
| +
| +/* Helper for mi_cmd_symbol_info_{functions,variables} - depending on KIND.
| +   Processes command line options from ARGV and ARGC.  */
| +
| +static void
| +mi_info_functions_or_variables (enum search_domain kind, char **argv, int argc)
| +{
| +  const char *regexp = nullptr;
| +  const char *t_regexp = nullptr;

PS3, Line 272:

I know these variable names are used elsewhere for the same purpose,
but I think that "name_regexp" and "type_regexp" would make it them
much clearer.

| +  bool exclude_minsyms = true;
| +
| +  enum opt
| +    {
| +     INCLUDE_NONDEBUG_OPT, TYPE_REGEXP_OPT, NAME_REGEXP_OPT
| +    };
| +  static const struct mi_opt opts[] =
| +  {
| +    {"-include-nondebug" , INCLUDE_NONDEBUG_OPT, 0},

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic2fc6a6750bbce91cdde2344791014e5ef45642d
Gerrit-Change-Number: 266
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Joel Brobecker <brobecker@adacore.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Thu, 21 Nov 2019 05:39:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment