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

Pedro Alves
2017-02-06T12:40:22Z

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