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

[Pedro Alves <palves@redhat.com>]

Hi Marc-Andre,

Could you include a gdb/NEWS change as well, please?

On 02/03/2017 05:15 PM, Marc-Andre Laperle wrote:

>  @item =library-loaded,...
>  Reports that a new library file was loaded by the program.  This
> -notification has 4 fields---@var{id}, @var{target-name},
> -@var{host-name}, and @var{symbols-loaded}.  The @var{id} field is an
> +notification has 5 fields---@var{id}, @var{target-name},
> +@var{host-name}, @var{symbols-loaded} and @var{ranges}.  The @var{id} field is an
>  opaque identifier of the library.  For remote debugging case,
>  @var{target-name} and @var{host-name} fields give the name of the
>  library file on the target, and on the host respectively.  For native
> @@ -26556,7 +26556,8 @@ and should not be relied on to convey any useful information.  The
>  @var{thread-group} field, if present, specifies the id of the thread
>  group in whose context the library was loaded.  If the field is
>  absent, it means the library was loaded in the context of all present
> -thread groups.
> +thread groups. The @var{ranges} field specifies the ranges of addresses belonging
> +to this library.

Could you add some description of the subfields of the ranges
field?  Something like "each range has the following fields", or some
such.  Particularly important to mention I think is
whether "to" is inclusive or exclusive. 

E.g., from the "Memory Region Attributes" section of the manual:

 @item Lo Address
 The address defining the inclusive lower bound of the memory region.

 @item Hi Address
 The address defining the exclusive upper bound of the memory region.

Another from range stepping vCont action description:

 @item r @var{start},@var{end}
 Step once, and then keep stepping as long as the thread stops at
 addresses between @var{start} (inclusive) and @var{end} (exclusive).

It should be inclusive, in order to handle a range that covers all
the way to the end of the address space correctly (otherwise
one-past-the-end wraps around).   However, seems like the current
code puts an exclusive end range in high_addr, since
"target_section::endaddr" is one-past-the-end.  :-(

This comment in struct so_list would be nice to update somehow too:

    /* Record the range of addresses belonging to this shared library.
       There may not be just one (e.g. if two segments are relocated
       differently); but this is only used for "info sharedlibrary".  */
    CORE_ADDR addr_low, addr_high;

The "only used for" part is now inaccurate.

I'd suggest adding some small comment about the multiple-segments
case to the MI code that prints the ranges, so other folks running
into that code understand that there's something to be improved there.

> --- a/gdb/mi/mi-cmd-file.c
> +++ b/gdb/mi/mi-cmd-file.c
> @@ -20,11 +20,15 @@
>  #include "defs.h"
>  #include "mi-cmds.h"
>  #include "mi-getopt.h"
> +#include "mi-interp.h"
>  #include "ui-out.h"
>  #include "symtab.h"
>  #include "source.h"
>  #include "objfiles.h"
>  #include "psymtab.h"
> +#include "solib.h"
> +#include "solist.h"
> +#include "xregex.h"

Don't include "xregex.h", include gdb_regex.h:

 gdb/contrib/ari/gdb_ari.sh:318:BEGIN { doc["xregex.h"] = "\
 gdb/contrib/ari/gdb_ari.sh:319:Do not include xregex.h, instead include gdb_regex.h"
 gdb/contrib/ari/gdb_ari.sh:320:    category["xregex.h"] = ari_regression


> +
> +void
> +mi_cmd_file_list_shared_libraries (char *command, char **argv, int argc)

Add a

/* See foo.h.  */

comment.

> +{
> +  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]"));
> +    }
> +
> +  if (pattern != NULL)
> +    {
> +      const char *re_err = re_comp (pattern);
> +
> +      if (re_err != NULL)
> +	error (_("Invalid regexp: %s"), re_err);
> +    }
> +
> +  update_solib_list (1);
> +
> +  /* Print the table header.  */
> +  struct cleanup* cleanup = make_cleanup_ui_out_list_begin_end (uiout, "shared-libraries");

Formatting:

 - Space before '*', not after.
 - Too long line.

Write:

  struct cleanup *cleanup
    = make_cleanup_ui_out_list_begin_end (uiout, "shared-libraries");

> +
> +  ALL_SO_LIBS (so)
> +    {
> +      if (so->so_name[0] == '\0')
> +	continue;
> +      if (pattern != NULL && !re_exec (so->so_name))
> +	continue;
> +
> +      struct cleanup * tuple_clean_up

No space after *.

> +        = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +      mi_output_solib_attribs (uiout, so);
> +
> +      do_cleanups (tuple_clean_up);
> +    }
> +
> +  do_cleanups (cleanup);
> +}
+

> +void
> +mi_output_solib_attribs (ui_out *uiout, struct so_list *solib)

/* See bar.h.  */

> +{
> +  struct gdbarch *gdbarch = target_gdbarch ();
> +
> +  uiout->field_string ("id", solib->so_original_name);
> +  uiout->field_string ("target-name", solib->so_original_name);
> +  uiout->field_string ("host-name", solib->so_name);
> +  uiout->field_int ("symbols-loaded", solib->symbols_loaded);
> +  if (!gdbarch_has_global_solist (target_gdbarch ()))
> +    {
> +      uiout->field_fmt ("thread-group", "i%d", current_inferior ()->num);
> +    }

While at it, please remove the unnecessary {}.  (I know you're just
moving that code.)

Thanks,
Pedro Alves