This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Improved linker-debugger interface
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: gdb-patches at sourceware dot org
- Cc: Gary Benson <gbenson at redhat dot com>
- Date: Mon, 7 May 2012 18:56:48 +0200
- Subject: Re: [RFA] Improved linker-debugger interface
- References: <20120504152129.GA7418@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