From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20983 invoked by alias); 7 May 2012 16:57:18 -0000 Received: (qmail 20969 invoked by uid 22791); 7 May 2012 16:57:15 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 07 May 2012 16:56:56 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q47GuutV012259 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 7 May 2012 12:56:56 -0400 Received: from host2.jankratochvil.net (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q47GunP3031927 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 7 May 2012 12:56:52 -0400 Date: Mon, 07 May 2012 16:57:00 -0000 From: Jan Kratochvil To: gdb-patches@sourceware.org Cc: Gary Benson Subject: Re: [RFA] Improved linker-debugger interface Message-ID: <20120507165648.GA22472@host2.jankratochvil.net> References: <20120504152129.GA7418@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-2022-jp Content-Disposition: inline In-Reply-To: <20120504152129.GA7418@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-05/txt/msg00182.txt.bz2 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