Re: [RFA] Improved linker-debugger interface

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, May 04 2012, Gary Benson wrote:

> Hi all,

Hi Gary,

Thanks for the patch.  Nice to see the SystemTap patch is useful :-).

I glanced over the code, but saw a few minor nits.

> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index ab51806..0f099cf 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -361,6 +361,13 @@ static struct symbol *step_start_function;
>  /* Nonzero if we want to give control to the user when we're notified
>     of shared library events by the dynamic linker.  */
>  int stop_on_solib_events;
> +
> +static void
> +set_stop_on_solib_events (char *args, int from_tty, struct cmd_list_element *c)
> +{
> +  update_solib_breakpoints ();
> +} 
> +

This function needs a comment.

> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 69d3cb5..4897d51 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -47,6 +47,8 @@
>  #include "auxv.h"
>  #include "exceptions.h"
>  
> +#include "stap-probe.h"

You don't need to include `stap-probe.h' here, just `probe.h'.
`stap-probe.h' contains only a definition of a structure which is useful
for per-arch parsing of probes' arguments.

> @@ -92,6 +94,32 @@ static const char * const solib_break_names[] =
>    NULL
>  };
>  
> +/* A list of SystemTap probes which, if present in the dynamic linker,
> +   allow more fine-grained breakpoints to be placed on shared library
> +   events.  */
> +
> +struct probe_info
> +  {
> +    /* The name of the probe.  */
> +    const char *name;
> +
> +    /* Nonzero if this probe must be stopped at even when
> +       stop-on-solib-events is off.  */
> +    int mandatory;

I don't know what others think about it, but the `mandatory' flag can be
a bitfield, like this:

  int mandatory_p : 1;

Also, since this is a predicate to indicate whether or not something
happens, it's better to put the `_p' suffix.

> +  };
> +
> +static const struct probe_info probe_info[] =
> +{
> +  {"rtld_init_start", 0},
> +  {"rtld_init_complete", 1},
> +  {"rtld_map_start", 0},
> +  {"rtld_reloc_complete", 1},
> +  {"rtld_unmap_start", 0},
> +  {"rtld_unmap_complete", 1},
> +};

This should also have a comment.

The brackets should be indented like this:

struct foo bar[] =
  {
    { "bla", 0 },
    ...
  };

Also, I think it's more legible when you put a space between the open
bracket and the "bla", like I did above.


> +
>  static const char * const bkpt_names[] =
>  {
>    "_start",
> @@ -313,6 +341,12 @@ struct svr4_info
>    CORE_ADDR interp_text_sect_high;
>    CORE_ADDR interp_plt_sect_low;
>    CORE_ADDR interp_plt_sect_high;
> +
> +  /* SystemTap probes.  */
> +  VEC (probe_p) *probes[NUM_PROBES];
> +
> +  /* Nonzero if we are using the SystemTap interface.  */
> +  int using_probes;

Same comment regarding using bitfield, but I'd like to hear what others
think about it.

This should also contain the `_p' suffix.

Also, I've been thinking about creating some predicate that would
confirm if some probe is of certain type of not.  The way it is today,
when you call `find_probes_in_objfile', there's no way to confirm that
the probes are SystemTap probes (obviously, today SystemTap probes are
the only allowed/implemented types of probes that use this backend, but
still...).

> diff --git a/gdb/solib.c b/gdb/solib.c
> index 656e8df..57f6c1a 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1211,6 +1211,18 @@ no_shared_libraries (char *ignored, int from_tty)
>    objfile_purge_solibs ();
>  }
>  
> +/* Enable or disable optional solib event breakpoints as appropriate.  */
> +
> +void
> +update_solib_breakpoints (void)
> +{
> +  struct target_so_ops *ops = solib_ops (target_gdbarch);
> +
> +  if (ops->update_breakpoints != NULL)
> +    ops->update_breakpoints ();
> +}
> +  
> +
>  /* Reload shared libraries, but avoid reloading the same symbol file
>     we already have loaded.  */
>  
> diff --git a/gdb/solib.h b/gdb/solib.h
> index 7a2ff84..65e3857 100644
> --- a/gdb/solib.h
> +++ b/gdb/solib.h
> @@ -91,4 +91,8 @@ extern CORE_ADDR gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
>  								      void *),
>  						    void *data);
>  
> +/* Enable or disable optional solib event breakpoints as appropriate.  */
> +
> +extern void update_solib_breakpoints (void);

This comment should be either on the header file, or on the definition
of the function, but not on both, I think.  I prefer comments on the
header file.

Again, thanks for the patch :-).

-- 
Sergio