Re: [RFA] Improved linker-debugger interface

[Jan Kratochvil <jan.kratochvil@redhat.com>]

Hi Gary,

On Fri, 04 May 2012 17:21:29 +0200, Gary Benson wrote:
> My current setup has a probe everywhere _dl_debug_state is called,

I believe you should submit the STAP (SystemTap) probes patch to GNU libc
along.  I do not see too great to patch GNU gdb for a libc feature not in GNU
libc.

(STAP probes are in Fedora 17+ glibc and RHEL-6.2+ glibc.)


> --- 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)

Missing function comment.


> +{
> +  update_solib_breakpoints ();
> +} 
> +
>  static void
>  show_stop_on_solib_events (struct ui_file *file, int from_tty,
>  			   struct cmd_list_element *c, const char *value)
> @@ -7243,7 +7250,7 @@ Show stopping for shared library events."), _("\
>  If nonzero, gdb will give control to the user when the dynamic linker\n\
>  notifies gdb of shared library events.  The most common event of interest\n\
>  to the user would be loading/unloading of a new library."),
> -			    NULL,
> +			    set_stop_on_solib_events,
>  			    show_stop_on_solib_events,
>  			    &setlist, &showlist);
>  
> 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"

Generic GDB code should only include "probe.h", not "stap-probe.h", so that
the probes can be of arbitrary kind.

It also no longer built with FSF GDB HEAD:
solib-svr4.c: In function ‘svr4_update_solib_event_breakpoint’:
solib-svr4.c:1463:33: error: dereferencing pointer to incomplete type

but it gets fixed just by:

-#include "stap-probe.h"
+#include "probe.h"


> +
>  static struct link_map_offsets *svr4_fetch_link_map_offsets (void);
>  static int svr4_have_link_map_offsets (void);
>  static void svr4_relocate_main_executable (void);
> @@ -92,6 +94,32 @@ static const char * const solib_break_names[] =
>    NULL
>  };
>  
> +/* A list of SystemTap probes which, if present in the dynamic linker,

No longer SystemTap.


> +   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;
> +  };
> +
> +static const struct probe_info probe_info[] =
> +{
> +  {"rtld_init_start", 0},

Formatting (as pointed out by Sergio):
     { "rtld_init_start", 0 },

> +  {"rtld_init_complete", 1},
> +  {"rtld_map_start", 0},
> +  {"rtld_reloc_complete", 1},
> +  {"rtld_unmap_start", 0},
> +  {"rtld_unmap_complete", 1},
> +};
> +
> +#define NUM_PROBES (sizeof(probe_info) / sizeof(probe_info[0]))

It would be:
   #define NUM_PROBES (sizeof (probe_info) / sizeof (probe_info[0]))

But libiberty/ already provides ARRAY_SIZE.


> +
>  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.  */

Not 'SystamTap', 'struct probe' is no longer SystemTap specific.


> +  VEC (probe_p) *probes[NUM_PROBES];
> +
> +  /* Nonzero if we are using the SystemTap interface.  */

Again.


> +  int using_probes;

Maybe it could be called 'probes_p' as for 'predicate' when discussed with
Sergio but I do not mind.


>  };
>  
>  /* Per-program-space data key.  */
> @@ -322,8 +356,15 @@ static void
>  svr4_pspace_data_cleanup (struct program_space *pspace, void *arg)
>  {
>    struct svr4_info *info;
> +  int i;
>  
>    info = program_space_data (pspace, solib_svr4_pspace_data);
> +  if (info == NULL)
> +    return;
> +
> +  for (i = 0; i < NUM_PROBES; i++)
> +    VEC_free (probe_p, info->probes[i]);
> +
>    xfree (info);
>  }
>  
> @@ -1392,6 +1433,126 @@ exec_entry_point (struct bfd *abfd, struct target_ops *targ)
>  					     targ);
>  }
>  
> +/* Helper function for svr4_update_solib_event_breakpoints.  */
> +
> +static int
> +svr4_update_solib_event_breakpoint (struct breakpoint *b, void *arg)
> +{
> +  struct svr4_info *info = get_svr4_info ();
> +  struct bp_location *loc;
> +
> +  if (b->type != bp_shlib_event)
> +    return 0;
> +
> +  for (loc = b->loc; loc; loc = loc->next)
> +    {
> +      int i;
> +
> +      for (i = 0; i < NUM_PROBES; i++)
> +	{
> +	  if (!probe_info[i].mandatory)
> +	    {
> +	      struct probe *probe;
> +	      int ix;
> +
> +	      for (ix = 0;
> +		   VEC_iterate (probe_p, info->probes[i], ix, probe);
> +		   ++ix)
> +		{
> +		  if (loc->pspace == current_program_space
> +		      && loc->address == probe->address)
> +		    {
> +		      b->enable_state =
> +			stop_on_solib_events ? bp_enabled : bp_disabled;

This unfinished '=' should be AFAIK only as a last resort, more normal
formatting would be:
		      b->enable_state = (stop_on_solib_events
					 ? bp_enabled : bp_disabled);

> +		      return 0;
> +		    }
> +		}
> +	    }
> +	}
> +    }
> +
> +  return 0;
> +}
> +
> +/* Enable or disable optional solib event breakpoints as appropriate.
> +   Called whenever stop_on_solib_events is changed.  */
> +
> +static void
> +svr4_update_solib_event_breakpoints (void)
> +{
> +  struct svr4_info *info = get_svr4_info ();
> +
> +  if (info->using_probes)
> +    iterate_over_breakpoints (svr4_update_solib_event_breakpoint, NULL);
> +}
> +
> +/* Both the SunOS and the SVR4 dynamic linkers call a marker function
> +   before and after mapping and unmapping shared libraries.  The sole
> +   purpose of this method is to allow debuggers to set a breakpoint so
> +   they can track these changes.
> +
> +   Some versions of the glibc dynamic linker contain SystemTap probes

No longer SystemTap.


> +   to allow more fine grained stopping.  Given the address of the
> +   original marker function, this function attempts to find these
> +   probes, and if found, sets breakpoints on those instead.  If the
> +   probes aren't found, a single breakpoint is set on the original
> +   SVR4 marker function.  */
> +
> +static void
> +svr4_create_solib_event_breakpoints (struct gdbarch *gdbarch, CORE_ADDR address)
> +{
> +  struct svr4_info *info = get_svr4_info ();
> +  struct obj_section *os;
> +
> +  os = find_pc_section (address);
> +  if (os != NULL)
> +    {
> +      int all_probes_found = 1;
> +      int i;
> +
> +      for (i = 0; i < NUM_PROBES; i++)
> +	{
> +	  info->probes[i] = find_probes_in_objfile (os->objfile, "rtld",
> +						    probe_info[i].name);
> +
> +	  if (!VEC_length(probe_p, info->probes[i]))

Formatting:
   	  if (!VEC_length (probe_p, info->probes[i]))


> +	    {
> +	      int j;
> +
> +	      for (j = i - 1; j >= 0; j--)
> +		{
> +		  VEC_free (probe_p, info->probes[j]);
> +		  info->probes[j] = NULL;
> +		}

This code is at multiple places (running VEC_free for all probes[x]), it can
be made a subroutine.  You can VEC_free every element here, not just 0..i-1,
others will be NULL.

0..i-1 seems needlessly magic to me.


> +
> +	      all_probes_found = 0;
> +	      break;
> +	    }
> +	}
[...]
> 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.  */

As pointed out by Sergio, duplicate function comment.


> +
> +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);
> +
>  #endif /* SOLIB_H */
> diff --git a/gdb/solist.h b/gdb/solist.h
> index 7413e3b..0d9046d 100644
> --- a/gdb/solist.h
> +++ b/gdb/solist.h
> @@ -149,6 +149,13 @@ struct target_so_ops
>         core file (in particular, for readonly sections).  */
>      int (*keep_data_in_core) (CORE_ADDR vaddr,
>  			      unsigned long size);
> +
> +    /* Enable or disable optional solib event breakpoints as
> +       appropriate.  This should be called whenever
> +       stop_on_solib_events is changed.  This pointer can be
> +       NULL, in which case no enabling or disabling is necessary
> +       for this target.  */
> +    void (*update_breakpoints) (void);
>    };
>  
>  /* Free the memory associated with a (so_list *).  */
> diff --git a/gdb/testsuite/gdb.base/break-interp.exp b/gdb/testsuite/gdb.base/break-interp.exp
> index 1e47b34..18d46d3 100644
> --- a/gdb/testsuite/gdb.base/break-interp.exp
> +++ b/gdb/testsuite/gdb.base/break-interp.exp
> @@ -109,14 +109,21 @@ proc strip_debug {dest} {
>      }
>  }
>  
> +# Former symbol for solib changes notifications was _dl_debug_state, newer one
> +# is dl_main (in fact _dl_debug_notify but it is inlined without any extra
> +# debug info), the right one one traps by `set stop-on-solib-events 1'.
> +
> +set solib_bp {(_dl_debug_state|dl_main)}

If the functionality stops working GDB will just fall back to non-STAP
_dl_debug_state and nothing will be seen on the testsuite results.

If STAP probes are not found then some testfile should report XFAIL + UNTESTED
(or some variant of it).

Modification of break-interp.exp is OK for keeping it compatible but it is not
a real test for the new functionality.  (I do not ask for performance test,
that is IMO more pain than good.)


> +
>  # Implementation of reach.
>  
[...]

Otherwise OK from me for check-in.


Thanks,
Jan