[RFA] Improved linker-debugger interface

[RFA] Improved linker-debugger interface

[Gary Benson]

[-- Attachment #1: Type: text/plain, Size: 3247 bytes --]

Hi all,

Back in June or so last year I spent some time working on an improved
debug interface with the runtime linker to help address the following
bugs:

  https://bugzilla.redhat.com/show_bug.cgi?id=658851
  aka http://sources.redhat.com/bugzilla/show_bug.cgi?id=2328
  "_dl_debug_state() RT_CONSISTENT called too early"

  https://bugzilla.redhat.com/show_bug.cgi?id=698001
  "improve GDB performance on an application performing
  a lot of object loading."

  http://sourceware.org/bugzilla/show_bug.cgi?id=11839
  "gdb does not detect calls to dlmopen"

The current linker-debugger interface has a structure (r_debug)
containing a list of loaded libraries, and an empty function
(_dl_debug_state) for debuggers to set breakpoints on and which
the linker calls both before and after modifying this list.
The problems with the current interface are as follows:

  - There is one place where glibc calls _dl_debug_state earlier than
    Solaris libc.  This is #658851.  It is unlikely that glibc will
    ever be changed to make it compatible with Solaris libc, which
    means GDB reports libraries as loaded and ready before they
    really are.

  - This interface was presumably invented before dlmopen() was, so
    there's only provision in it for one namespace.  In glibc each
    namespace has it's own r_debug structure, but there is no way for
    the linker to communicate the addresses of the others to the
    debugger.  This is PR 11839.

  - In normal use GDB only needs to stop _after_ the list is modified.
    Because _dl_debug_state is called both before and after, GDB stops
    twice as often as it needs to.  This is #698001, the gist of it at
    any rate.  
    
  - When stop-on-solib-events is set, however, it is useful to stop
    both before and after library loads.

My proposed solution is to insert a number of SystemTap probes into
glibc.  My current setup has a probe everywhere _dl_debug_state is
called, and an extra pair to surround relocation events, but new
probes could be added as and when necessary.

This approach solves the various problems like so:

  - Debuggers can pick and choose which probes to set breakpoints
    on.  By using the "relocation completed" probe instead of the
    one mirroring _dl_debug_state debuggers can stop after relocations
    have occurred, matching the behaviour of Solaris libc.

  - All probes have namespace id and r_debug address arguments,
    allowing debuggers to see changes in namespaces other than
    the default.

  - When stop-on-solib-events is unset, GDB does not have to stop
    before changes are made, only after.  By disabling the "before"
    breakpoints the number of stops made can be halved.

The attached patch modifies GDB to search for the SystemTap probes
in the runtime linker, and to use them instead of _dl_debug_state
if found.  If the probes are not found then GDB will fall back to
its previous behaviour.  When probes are used, GDB stops after
relocation, fixing #658851, and stops before changes are made are
inhibited.  I've not done anything on the GDB side to deal with the
dlmopen() issue, but it's now possible to fix it using the data
supplied by the new interface.

Does this look ok?

Cheers,
Gary

-- 
http://gbenson.net/

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 15219 bytes --]

gdb/
2012-05-04  Gary Benson  <gbenson@redhat.com>

	* infrun.c (set_stop_on_solib_events): New function.
	(_initialize_infrun): Use the above for "set stop-on-solib-events".
	* solib-svr4.c (probe_info): New struct.
	(probe_info): New static variable.
	(NUM_PROBES): New definition.
	(svr4_info): New fields "probes" and "using_probes".
	(svr4_update_solib_event_breakpoint): New function.
	(svr4_update_solib_event_breakpoints): Likewise.
	(svr4_create_solib_event_breakpoints): Likewise.
	(enable_break): Free probes before creating breakpoints.
	Use svr4_create_solib_event_breakpoints to create breakpoints.
	(svr4_pspace_data_cleanup): Free probes.
	(_initialize_svr4_solib): Initialise svr4_so_ops.update_breakpoints.
	* solib.h (update_solib_breakpoints): New function definition.
	* solib.c (update_solib_breakpoints): New function.
	* solist.h (target_so_ops): New field "update_breakpoints".

gdb/testsuite
2012-05-04  Gary Benson  <gbenson@redhat.com>

	* gdb.base/break-interp.exp (solib_bp): New constant.
	(reach_1): Use the above instead of "_dl_debug_state".
	(test_attach): Likewise.
	(test_ld): Likewise.

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 ();
+} 
+
 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"
+
 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,
+   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},
+  {"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]))
+
 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;
 };
 
 /* 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;
+		      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
+   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]))
+	    {
+	      int j;
+
+	      for (j = i - 1; j >= 0; j--)
+		{
+		  VEC_free (probe_p, info->probes[j]);
+		  info->probes[j] = NULL;
+		}
+
+	      all_probes_found = 0;
+	      break;
+	    }
+	}
+
+      if (all_probes_found)
+	{
+	  info->using_probes = 1;
+
+	  for (i = 0; i < NUM_PROBES; i++)
+	    {
+	      struct probe *probe;
+	      int ix;
+
+	      for (ix = 0;
+		   VEC_iterate (probe_p, info->probes[i], ix, probe);
+		   ++ix)
+		create_solib_event_breakpoint (gdbarch, probe->address);
+	    }
+
+	  svr4_update_solib_event_breakpoints ();
+	  return;
+	}
+    }
+
+  create_solib_event_breakpoint (gdbarch, address);
+}
+
 /* Helper function for gdb_bfd_lookup_symbol.  */
 
 static int
@@ -1440,10 +1601,18 @@ enable_break (struct svr4_info *info, int from_tty)
   asection *interp_sect;
   gdb_byte *interp_name;
   CORE_ADDR sym_addr;
+  int i;
 
   info->interp_text_sect_low = info->interp_text_sect_high = 0;
   info->interp_plt_sect_low = info->interp_plt_sect_high = 0;
 
+  for (i = 0; i < NUM_PROBES; i++)
+    {
+      VEC_free (probe_p, info->probes[i]);
+      info->probes[i] = NULL;
+    }
+  info->using_probes = 0;
+
   /* If we already have a shared library list in the target, and
      r_debug contains r_brk, set the breakpoint there - this should
      mean r_brk has already been relocated.  Assume the dynamic linker
@@ -1475,7 +1644,7 @@ enable_break (struct svr4_info *info, int from_tty)
 	 That knowledge is encoded in the address, if it's Thumb the low bit
 	 is 1.  However, we've stripped that info above and it's not clear
 	 what all the consequences are of passing a non-addr_bits_remove'd
-	 address to create_solib_event_breakpoint.  The call to
+	 address to svr4_create_solib_event_breakpoints.  The call to
 	 find_pc_section verifies we know about the address and have some
 	 hope of computing the right kind of breakpoint to use (via
 	 symbol info).  It does mean that GDB needs to be pointed at a
@@ -1513,7 +1682,7 @@ enable_break (struct svr4_info *info, int from_tty)
 		+ bfd_section_size (tmp_bfd, interp_sect);
 	    }
 
-	  create_solib_event_breakpoint (target_gdbarch, sym_addr);
+	  svr4_create_solib_event_breakpoints (target_gdbarch, sym_addr);
 	  return 1;
 	}
     }
@@ -1668,7 +1837,8 @@ enable_break (struct svr4_info *info, int from_tty)
 
       if (sym_addr != 0)
 	{
-	  create_solib_event_breakpoint (target_gdbarch, load_addr + sym_addr);
+	  svr4_create_solib_event_breakpoints (target_gdbarch,
+					       load_addr + sym_addr);
 	  xfree (interp_name);
 	  return 1;
 	}
@@ -1694,7 +1864,7 @@ enable_break (struct svr4_info *info, int from_tty)
 	  sym_addr = gdbarch_convert_from_func_ptr_addr (target_gdbarch,
 							 sym_addr,
 							 &current_target);
-	  create_solib_event_breakpoint (target_gdbarch, sym_addr);
+	  svr4_create_solib_event_breakpoints (target_gdbarch, sym_addr);
 	  return 1;
 	}
     }
@@ -1710,7 +1880,7 @@ enable_break (struct svr4_info *info, int from_tty)
 	      sym_addr = gdbarch_convert_from_func_ptr_addr (target_gdbarch,
 							     sym_addr,
 							     &current_target);
-	      create_solib_event_breakpoint (target_gdbarch, sym_addr);
+	      svr4_create_solib_event_breakpoints (target_gdbarch, sym_addr);
 	      return 1;
 	    }
 	}
@@ -2486,4 +2656,5 @@ _initialize_svr4_solib (void)
   svr4_so_ops.lookup_lib_global_symbol = elf_lookup_lib_symbol;
   svr4_so_ops.same = svr4_same;
   svr4_so_ops.keep_data_in_core = svr4_keep_data_in_core;
+  svr4_so_ops.update_breakpoints = svr4_update_solib_event_breakpoints;
 }
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);
+
 #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)}
+
 # Implementation of reach.
 
 proc reach_1 {func command displacement} {
-    global gdb_prompt expect_out
+    global gdb_prompt expect_out solib_bp
 
-    if {$func == "_dl_debug_state"} {
+    if {$func == $solib_bp} {
 	# Breakpoint on _dl_debug_state can have problems due to its overlap
 	# with the existing internal breakpoint from GDB.
+	# With also _dl_debug_notify we would need even two breakpoints.
 	gdb_test_no_output "set stop-on-solib-events 1"
     } elseif {! [gdb_breakpoint $func allow-pending]} {
 	return
@@ -142,21 +149,21 @@ proc reach_1 {func command displacement} {
 	    exp_continue
 	}
 	-re "Breakpoint \[0-9\]+, \\.?(__GI_)?$func \\(.*\\) at .*:\[0-9\]+\r\n.*$gdb_prompt $" {
-	    if {$func == "_dl_debug_state"} {
+	    if {$func == $solib_bp} {
 		fail $test
 	    } else {
 		pass $test
 	    }
 	}
 	-re "Breakpoint \[0-9\]+, \[0-9xa-f\]+ in \\.?(__GI_)?$func \\(\\).*\r\n$gdb_prompt $" {
-	    if {$func == "_dl_debug_state"} {
+	    if {$func == $solib_bp} {
 		fail $test
 	    } else {
 		pass $test
 	    }
 	}
 	-re "Stopped due to (spurious )?shared library event.*\r\n$gdb_prompt $" {
-	    if {$func == "_dl_debug_state"} {
+	    if {$func == $solib_bp} {
 		if {$debug_state_count == 0} {
 		    # First stop does not yet relocate the _start function
 		    # descriptor on ppc64.
@@ -175,7 +182,7 @@ proc reach_1 {func command displacement} {
 	fail $test_displacement
     }
 
-    if {$func == "_dl_debug_state"} {
+    if {$func == $solib_bp} {
 	gdb_test_no_output "set stop-on-solib-events 0"
     }
 }
@@ -357,7 +364,7 @@ proc test_attach {file displacement {relink_args ""}} {
 }
 
 proc test_ld {file ifmain trynosym displacement} {
-    global srcdir subdir gdb_prompt expect_out inferior_exited_re
+    global srcdir subdir gdb_prompt expect_out inferior_exited_re solib_bp
 
     # First test normal `file'-command loaded $FILE with symbols.
 
@@ -385,9 +392,9 @@ proc test_ld {file ifmain trynosym displacement} {
 	gdb_test_no_output "set args ${objdir}/${subdir}/$binfile_test" "set args OBJDIR/${subdir}/$binfile_test"
     }
 
-    reach "_dl_debug_state" "run" $displacement
+    reach $solib_bp "run" $displacement
 
-    gdb_test "bt" "#0 +\[^\r\n\]*\\m(__GI_)?_dl_debug_state\\M.*" "dl bt"
+    gdb_test "bt" "#0 +\[^\r\n\]*\\m(__GI_)?$solib_bp\\M.*" "dl bt"
 
     if $ifmain {
 	reach "main" continue "NONE"
@@ -399,7 +406,7 @@ proc test_ld {file ifmain trynosym displacement} {
 
     # Try re-run if the new PIE displacement takes effect.
     gdb_test "kill" "" "kill" {Kill the program being debugged\? \(y or n\) } "y"
-    reach "_dl_debug_state" "run" $displacement
+    reach $solib_bp "run" $displacement
 
     if $ifmain {
 	test_core $file $displacement
@@ -431,7 +438,7 @@ proc test_ld {file ifmain trynosym displacement} {
 	gdb_test "exec-file $file" "exec-file $escapedfile" "load"
 
 	if $ifmain {
-	    reach "_dl_debug_state" run $displacement
+	    reach $solib_bp run $displacement
 
 	    # Use two separate gdb_test_multiple statements to avoid timeouts due
 	    # to slow processing of wildcard capturing long output

Re: [RFA] Improved linker-debugger interface

[Sergio Durigan Junior]

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

Re: [RFA] Improved linker-debugger interface

[Jan Kratochvil]

On Sat, 05 May 2012 06:38:57 +0200, Sergio Durigan Junior wrote:
> On Friday, May 04 2012, Gary Benson wrote:
> > +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;

In such case 'unsigned mandatory_p : 1' as otherwise its true value is -1.


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

'_p' as a predicate usually flags validity of some other field (such as
a hypothetical field 'mandatory' in this case).  I do not see the 'predicate'
need to be valid here, this is normal flag.


> > +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},
> > +};
[...]
> The brackets should be indented like this:
> 
> struct foo bar[] =
>   {
>     { "bla", 0 },
>     ...
>   };

No - see GNU Coding Standards, there is an example for it.  Current GDB
codebase is not always correct in this regard.


> Also, I've been thinking about creating some predicate that would
> confirm if some probe is of certain type of not.

There is that
	gdb_assert (probe_generic->pops == &stap_probe_ops);

for this purpose.


(This is not a Gary's patch review yet.)


Thanks,
Jan

Re: [RFA] Improved linker-debugger interface

[Sergio Durigan Junior]

On Saturday, May 05 2012, Jan Kratochvil wrote:

>> Also, I've been thinking about creating some predicate that would
>> confirm if some probe is of certain type of not.
>
> There is that
> 	gdb_assert (probe_generic->pops == &stap_probe_ops);
>
> for this purpose.

Which can only be used inside stap-probe.c.  I was talking about
something that would validate a SystemTap probe outside stap-probe.c.

-- 
Sergio

Re: [RFA] Improved linker-debugger interface

[Jan Kratochvil]

On Sat, 05 May 2012 08:10:55 +0200, Sergio Durigan Junior wrote:
> On Saturday, May 05 2012, Jan Kratochvil wrote:
> > There is that
> > 	gdb_assert (probe_generic->pops == &stap_probe_ops);
> >
> > for this purpose.
> 
> Which can only be used inside stap-probe.c.

Why?  stap_probe_ops can be made extern-public in stap-probe.h if there is now
a need for it.

But in general there should never be such backend-specific conditional, the
virtualization makes no sense in such case at all.

If the current probe.h interface is insufficient then it should be extended.

I have to look at the Gary's patch more.


Thanks,
Jan

Re: [RFA] Improved linker-debugger interface

[Jan Kratochvil]

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

Re: [RFA] Improved linker-debugger interface

[Sergio Durigan Junior]

On Monday, May 07 2012, Jan Kratochvil wrote:

> On Fri, 04 May 2012 17:21:29 +0200, Gary Benson wrote:

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

FYI, this is because of:

    http://sourceware.org/ml/gdb-cvs/2012-05/msg00035.html

Which I have committed just after reviewing Gary's patch.

-- 
Sergio

Re: [RFA] Improved linker-debugger interface

[Tom Tromey]

Gary> My current setup has a probe everywhere _dl_debug_state is called,

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

I don't see that as such a big deal.  We have patches in gdb for all
kinds of system- and compiler-specific behavior.  At least in this case
the patches in question are public and free software.

I do agree that we should make another attempt to get the probes
upstream; I just don't think success at that should be a precondition
for this patch.

FWIW, we already have support for the glibc longjmp probes in the tree
now.

Tom

Re: [RFA] Improved linker-debugger interface

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Mon, 07 May 2012 14:27:27 -0600
> 
> Gary> My current setup has a probe everywhere _dl_debug_state is called,
> 
> Jan> I believe you should submit the STAP (SystemTap) probes patch to
> Jan> GNU libc along.  I do not see too great to patch GNU gdb for a libc
> Jan> feature not in GNU libc.
> 
> I don't see that as such a big deal.  We have patches in gdb for all
> kinds of system- and compiler-specific behavior.  At least in this case
> the patches in question are public and free software.

I think this is a big deal.  We can't just add support for every
feature that looks neat into GDB.  In the long run, that will lead to
an unmaintainable codebase.  

I'm pretty annoyed by the whole SystemTap thing.  You presented this
as being something pretty generic.  But it turns out this is not only
Linux-specfic, but pretty much a completely RedHat-specific thing it
seems.  And I think I've figured out why: SystemTap relies on utrace,
which is not present in the official Linux kernel source tree.  And as
far as I can see it will remain that way in the near future.  So
unless you are running RedHat Linux, you'll not only need to build a
patched glibc, but you also need to build a patched kernel to be able
to use these new SystemTap probes.  Not many people will do that!

Having the basic SystemTap support in GDB is fine.  But depending on
it to fix issues with core GDB functionality like the ability to debug
shared libraries is a different matter.  It means that people using
RedHat Linux, almost certainly including any RedHat engineers
contributing here will no longer test the codepaths that don't rely on
SystemTap.  And people on other Linux variants will never test the
codepaths that rely on SystemTap.  That'll inevitably lead to more
breakage.

> I do agree that we should make another attempt to get the probes
> upstream; I just don't think success at that should be a precondition
> for this patch.

If you ask me, having utrace in the official Linux kernel should be a
precondition for this patch as well.

> FWIW, we already have support for the glibc longjmp probes in the tree
> now.

In the glibc tree?  Or in the GDB tree?  If these probes are not
present in the official glibc tree, the support for those particular
probes should not be in the official GDB tree either.

Re: [RFA] Improved linker-debugger interface

[Jan Kratochvil]

Hi Mark,

I will make only several technical fix ups.


On Mon, 07 May 2012 23:31:45 +0200, Mark Kettenis wrote:
> SystemTap relies on utrace,

No longer, there is development version of utrace-less SystemTap.


> So unless you are running RedHat Linux, you'll not only need to build
> a patched glibc, but you also need to build a patched kernel to be able to
> use these new SystemTap probes.

This is unrelated.  While you can use glibc SystemTap probes for additional
functionality from kernel you do not have to.  You can use arbitrary kernel
(probably not even Linux one as long as you build glibc there), GDB always
fetches the probe points from glibc on its own in userland.


> It means that people using RedHat Linux, almost certainly including any

FYI the brand is "Red Hat", not "RedHat".

> RedHat engineers contributing here will no longer test the codepaths that
> don't rely on SystemTap.  And people on other Linux variants will never test
> the codepaths that rely on SystemTap.  That'll inevitably lead to more
> breakage.

It is only about FSF glibc, neither related to SystemTap nor related to Red
Hat.


Regards,
Jan

Re: [RFA] Improved linker-debugger interface

[Sergio Durigan Junior]

Hi Mark,

On Monday, May 07 2012, Mark Kettenis wrote:

> I'm pretty annoyed by the whole SystemTap thing.  You presented this
> as being something pretty generic.  But it turns out this is not only
> Linux-specfic, but pretty much a completely RedHat-specific thing it
> seems.  And I think I've figured out why: SystemTap relies on utrace,
> which is not present in the official Linux kernel source tree.  And as
> far as I can see it will remain that way in the near future.  So
> unless you are running RedHat Linux, you'll not only need to build a
> patched glibc, but you also need to build a patched kernel to be able
> to use these new SystemTap probes.  Not many people will do that!

SystemTap is not a Red Hat-specific thing, and the support that I have
added on GDB does not rely on utrace at all.  It seems we are talking
about two different things here (userspace probes vs. kernel probes).

GDB currently supports static probes in userspace.  Those happen to be
implemented by SystemTap using <sys/sdt.h>, and (again) does not depend
on utrace, but rather on GCC + ELF support (that's why Jan asked me to
make the compilation of `stap-probe.c' conditional to the ELF support on
the target).

FWIW, Debian also ships a version of <sys/sdt.h>:

  http://packages.debian.org/squeeze/all/systemtap-sdt-dev/filelist

Some distros do not ship it, but they can perfectly do so if their
community wants.

And the upstream GCC already have static probes in it, along with many
more projects.

So in my view, this is not Red Hat specific, nor utrace-dependent.

> Having the basic SystemTap support in GDB is fine.  But depending on
> it to fix issues with core GDB functionality like the ability to debug
> shared libraries is a different matter.  It means that people using
> RedHat Linux, almost certainly including any RedHat engineers
> contributing here will no longer test the codepaths that don't rely on
> SystemTap.  And people on other Linux variants will never test the
> codepaths that rely on SystemTap.  That'll inevitably lead to more
> breakage.
>
>> I do agree that we should make another attempt to get the probes
>> upstream; I just don't think success at that should be a precondition
>> for this patch.
>
> If you ask me, having utrace in the official Linux kernel should be a
> precondition for this patch as well.

I hope I made it clear that, as a non-utrace-dependent feature, I think
this precondition is not valid anymore.

>> FWIW, we already have support for the glibc longjmp probes in the tree
>> now.
>
> In the glibc tree?  Or in the GDB tree?  If these probes are not
> present in the official glibc tree, the support for those particular
> probes should not be in the official GDB tree either.

The support is present in the GDB tree.

-- 
Sergio

Re: [RFA] Improved linker-debugger interface

[Tom Tromey]

>>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:

Mark> I'm pretty annoyed by the whole SystemTap thing.  You presented this
Mark> as being something pretty generic.  But it turns out this is not only
Mark> Linux-specfic, but pretty much a completely RedHat-specific thing it
Mark> seems.  And I think I've figured out why: SystemTap relies on utrace,
Mark> which is not present in the official Linux kernel source tree.  And as
Mark> far as I can see it will remain that way in the near future.  So
Mark> unless you are running RedHat Linux, you'll not only need to build a
Mark> patched glibc, but you also need to build a patched kernel to be able
Mark> to use these new SystemTap probes.  Not many people will do that!

None of this is the accurate.

I'm also annoyed now, because I explained all this at length, twice, and
also sent references to the documentation, explaining how the probe
feature is a relatively generic ELF feature, not dependent on the main
body of System Tap at all.  Apparently you didn't read any of this.


To sum up again, the code in gdb relates to static probe points.

From the user's point of view, a probe is a spot in the code with a name
and arguments.

From the implementation point of view, a probe is an entry in an ELF
section, designed in a way to minimize overhead, plus a no-op in the
code.

GDB reads this new section and uses it to find the probe points, and to
decode the probe arguments.  There is zero dependency on System Tap, or
the kernel, or utrace, or anything else.  It is purely reading an ELF
section.

The probe implementation is a header file.  Currently this is maintained
in System Tap.  However, even this is relatively independent; it depends
on a GCC extension, but this is part of upstream GCC.  I would not be
surprised if this code worked on any GCC/ELF system.

Mark> Having the basic SystemTap support in GDB is fine.  But depending on
Mark> it to fix issues with core GDB functionality like the ability to debug
Mark> shared libraries is a different matter.  It means that people using
Mark> RedHat Linux, almost certainly including any RedHat engineers
Mark> contributing here will no longer test the codepaths that don't rely on
Mark> SystemTap.  And people on other Linux variants will never test the
Mark> codepaths that rely on SystemTap.  That'll inevitably lead to more
Mark> breakage.

This is already the situation for compilers and kernels, which in
practice touch much more code in gdb.

Tom

Re: [RFA] Improved linker-debugger interface

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Tue, 08 May 2012 07:37:12 -0600
> 
> >>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:
> 
> Mark> I'm pretty annoyed by the whole SystemTap thing.  You presented this
> Mark> as being something pretty generic.  But it turns out this is not only
> Mark> Linux-specfic, but pretty much a completely RedHat-specific thing it
> Mark> seems.  And I think I've figured out why: SystemTap relies on utrace,
> Mark> which is not present in the official Linux kernel source tree.  And as
> Mark> far as I can see it will remain that way in the near future.  So
> Mark> unless you are running RedHat Linux, you'll not only need to build a
> Mark> patched glibc, but you also need to build a patched kernel to be able
> Mark> to use these new SystemTap probes.  Not many people will do that!
> 
> None of this is the accurate.
> 
> I'm also annoyed now, because I explained all this at length, twice, and
> also sent references to the documentation, explaining how the probe
> feature is a relatively generic ELF feature, not dependent on the main
> body of System Tap at all.  Apparently you didn't read any of this.

Sorry, but I looked back in my private mail archive and the mailing
list archives, and I couldn't find a mail from you with references to
documentation.  I did some digging around myself, and landed at:

  <http://sourceware.org/systemtap/SystemTap_Beginners_Guide/userspace-probing.html>

which mentions the dependency on utrace quite prominently.

Combined with your reply to one of my questions:

> Mark> So you are saying that, at least in principle, it should be possible
> Mark> to use the SystemTap toolchain on any ELF-based system to do
> Mark> user-space tracing without needing any kernel support?  That'd be cool.

> Nope, just the static markers coming from <sdt.h>.  The rest of
> SystemTap is Linux-specific, dynamically creating and loading kernel
> modules.

This confused me enough to think that kernel support was necessary for
GDB's usage of the probes as well.

> To sum up again, the code in gdb relates to static probe points.
> 
> From the user's point of view, a probe is a spot in the code with a name
> and arguments.
> 
> From the implementation point of view, a probe is an entry in an ELF
> section, designed in a way to minimize overhead, plus a no-op in the
> code.
> 
> GDB reads this new section and uses it to find the probe points, and to
> decode the probe arguments.  There is zero dependency on System Tap, or
> the kernel, or utrace, or anything else.  It is purely reading an ELF
> section.
> 
> The probe implementation is a header file.  Currently this is maintained
> in System Tap.  However, even this is relatively independent; it depends
> on a GCC extension, but this is part of upstream GCC.  I would not be
> surprised if this code worked on any GCC/ELF system.

Thanks for explaining this again.  This makes it crystal clear.

> Mark> Having the basic SystemTap support in GDB is fine.  But depending on
> Mark> it to fix issues with core GDB functionality like the ability to debug
> Mark> shared libraries is a different matter.  It means that people using
> Mark> RedHat Linux, almost certainly including any RedHat engineers
> Mark> contributing here will no longer test the codepaths that don't rely on
> Mark> SystemTap.  And people on other Linux variants will never test the
> Mark> codepaths that rely on SystemTap.  That'll inevitably lead to more
> Mark> breakage.
> 
> This is already the situation for compilers and kernels, which in
> practice touch much more code in gdb.

Yes, but reduction of the number of code paths is still a worthy goal.

Anyway, with my misunderstandings about SystemTap cleared up, my
reservations about using probes to handle runt-time linker events are
gone.  I still do think that having those probes in the official glibc
tree should be a prerequisite for ading this code to glibc.  These
probes effectively become part of the libc ABI[1].  We need some sort
of commitment from the glibc developers to not break that ABI.  And in
my view the only way to achieve this is to have those probes upstream.

[1] This contradicts what Roland McGrath wrote on the libc-lpha mailing list
    http://sourceware.org/ml/libc-alpha/2011-03/msg00001.html

Re: [RFA] Improved linker-debugger interface

[Tom Tromey]

>>>>> "Mark" == Mark Kettenis <mark.kettenis@xs4all.nl> writes:

Mark>   <http://sourceware.org/systemtap/SystemTap_Beginners_Guide/userspace-probing.html>

Mark> which mentions the dependency on utrace quite prominently.

You want http://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation

Mark> Anyway, with my misunderstandings about SystemTap cleared up, my
Mark> reservations about using probes to handle runt-time linker events are
Mark> gone.  I still do think that having those probes in the official glibc
Mark> tree should be a prerequisite for ading this code to glibc.  These
Mark> probes effectively become part of the libc ABI[1].  We need some sort
Mark> of commitment from the glibc developers to not break that ABI.  And in
Mark> my view the only way to achieve this is to have those probes upstream.

FWIW for the longjmp case, where the code is currently in-tree, there
really aren't alternative code paths to consider.  Due to PC mangling,
the existing longjmp code simply doesn't work with any glibc newer than
2006 or so.

I've asked the local glibc folks to try again to push the patches.

If you take Roland's lack of ABI promise seriously, and combine it with
your reasoning above, then we must never use these probes in gdb.  But,
that is an absurd result, since the probes basically exist for use by
gdb.

So, something has to give.

I think that if gdb were using the probes, we can use this to argue that
glibc should only make ABI-compatible changes.  The ABI rules for probes
are much more relaxed than the rules for normal C constructs, so this is
not a problem.

There are two choices for ABI compatible changes here; one is to keep
existing probe names and arguments, adding any needed new arguments to
the end of the list; or simply renaming the probes.  Deleting probes is
also ok (contrary to the usual glibc ABI rules), because gdb and other
users will always be adaptive.

In sum, I don't consider this a problem in practice.  I'm confident that
glibc maintainers will listen to reason.

Tom