Re: [PATCH 3/3] Add -file-list-shared-libraries MI command

[Simon Marchi <simon.marchi@polymtl.ca>]

The patch looks good to me in general.  I wrote a few minor comments 
inline.

On 2016-09-12 16:27, Marc-Andre Laperle wrote:
> @@ -108,3 +111,67 @@ mi_cmd_file_list_exec_source_files (char
> *command, char **argv, int argc)
> 
>    ui_out_end (uiout, ui_out_type_list);
>  }
> +
> +void
> +mi_cmd_file_list_shared_libraries (char *command, char **argv, int 
> argc)
> +{
> +  struct ui_out *uiout = current_uiout;
> +  const char *pattern;
> +  struct so_list *so = NULL;
> +  struct gdbarch *gdbarch = target_gdbarch ();
> +
> +  switch (argc)
> +    {
> +    case 0:
> +      pattern = NULL;
> +      break;
> +    case 1:
> +      pattern = argv[0];
> +      break;
> +    default:
> +      error (_("Usage: -file-list-shared-libraries [REGEXP]"));
> +      break;
> +    }
> +
> +  if (pattern != NULL)
> +    {
> +      char *re_err = re_comp (pattern);

const ?

> +
> +      if (re_err != NULL)
> +	error (_("Invalid regexp: %s"), re_err);
> +    }
> +
> +  update_solib_list (1);
> +
> +  /* Print the table header.  */
> +  ui_out_begin (uiout, ui_out_type_list, "shared-libraries");

It's not a big deal, but you could use 
make_cleanup_ui_out_list_begin_end, which avoids accidentally forgetting 
the corresponding ui_out_end.

> +
> +  ALL_SO_LIBS (so)
> +    {
> +      if (so->so_name[0] == '\0')
> +	continue;
> +      if (pattern != NULL && !re_exec (so->so_name))
> +	continue;
> +
> +      ui_out_begin (uiout, ui_out_type_tuple, NULL);

Same here, but with make_cleanup_ui_out_tuple_begin_end.

> +      if (so->addr_high != 0)
> +	{
> +	  ui_out_field_core_addr (uiout, "from", gdbarch, so->addr_low);
> +	  ui_out_field_core_addr (uiout, "to", gdbarch, so->addr_high);
> +	}
> +      else
> +	{
> +	  ui_out_field_skip (uiout, "from");
> +	  ui_out_field_skip (uiout, "to");
> +	}

You might be able to just remove the else.  I think ui_out_field_skip is 
only relevant for tables (which was used in the code that you took 
inspiration from), since you need to explicitly tell if you want to skip 
a table cell.  For tuples, if you want to omit a field, you just don't 
emit it.

> diff --git a/gdb/solib.h b/gdb/solib.h
> index 75490b6..ee621ce 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -73,6 +73,24 @@ extern void no_shared_libraries (char *ignored, int
> from_tty);
>  extern void set_solib_ops (struct gdbarch *gdbarch,
>  			   const struct target_so_ops *new_ops);
> 
> +/* Synchronize GDB's shared object list with inferior's.
> +
> +   Extract the list of currently loaded shared objects from the
> +   inferior, and compare it with the list of shared objects currently
> +   in GDB's so_list_head list.  Edit so_list_head to bring it in sync
> +   with the inferior's new list.
> +
> +   If we notice that the inferior has unloaded some shared objects,
> +   free any symbolic info GDB had read about those shared objects.
> +
> +   Don't load symbolic info for any new shared objects; just add them
> +   to the list, and leave their symbols_loaded flag clear.
> +
> +   If FROM_TTY is non-null, feel free to print messages about what
> +   we're doing.  */

I noticed that this comment, which you moved from the .c to the .h, 
refers so_list_head.  so_list_head is a define present in the .c, so the 
comment makes less sense now.  What about bringing the define to the .h, 
and at the same time you could use it in the definition of ALL_SO_LIBS?

> +    mi_gdb_test "222-file-list-shared-libraries" \
> +	"222\\^done,shared-libraries=\\\[.*\{from=\".*\",to=\".*\",syms-read=\"1\",name=\".*${libname}.so\"\}.*]"
> \
> +	"Getting a list of shared libraries."

For consistency with all tests, I would suggest changing the test name 
to "get the list of shared libraries" (no period at the end, 
non-capitalized, neutral verb tense which I don't the name).

>  }
> 
> -mi_expect_stop solib-event .* .* .* .* .* "check for solib event"
> +test_stop_on_solib_events
> +test_file_list_shared_libraries
> +mi_gdb_exit

I often see some tests ending with gdb_exit or mi_gdb_exit, but I don't 
think it's really required.  The code in lib/gdb.exp (e.g. gdb_exit) 
should take care of that, but I could be mistaken.