[RFA] Improved linker-debugger interface

[Gary Benson <gbenson@redhat.com>]

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