public inbox for archer@sourceware.org
 help / color / mirror / Atom feed
* gdb side of improved linker-debugger interface
@ 2011-06-07  9:48 Gary Benson
  2011-06-07 16:29 ` Tom Tromey
  2011-06-07 17:20 ` Jan Kratochvil
  0 siblings, 2 replies; 10+ messages in thread
From: Gary Benson @ 2011-06-07  9:48 UTC (permalink / raw)
  To: archer

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

Hi all,

A week or so ago I mailed this list a description of a new linker-
debugger interface based on SystemTap probes, along with a patch
containing the glibc side:

  http://www.cygwin.com/ml/archer/2011-q2/msg00000.html

In the meantime I've been working on the gdb side of this, which is
available in the archer-gbenson-stap-rtld branch.  I've also attached
a patch (which applies to archer-sergiodj-stap-patch-split) in case
you just want to have a look.

The deal is basically that gdb would normally set one solib event
breakpoint, on _dl_debug_state.  This patch makes it look for the two
SystemTap probes described in my previous mail, and if it finds them,
set a pair of breakpoints on those instead.  When stop-on-solib-events
is set, gdb will stop on both breakpoints, mimicing the old behaviour.
When stop-on-solib-events is not set, gdb will only stop on the post-
modification breakpoint.  This allows solib mapping and unmapping to
be tracked as before while almost halving the time taken (I did some
basic tests, eg loading 1000 simple libraries took 14s as opposed to
26s).  This helps https://bugzilla.redhat.com/show_bug.cgi?id=698001.
The post-modification breakpoint is called later than previously, to
match where Solaris libc calls _dl_debug_state, which addresses
https://bugzilla.redhat.com/show_bug.cgi?id=658851.

I've not implemented anything to do with dlmopen, but the information
required for gdb to discover the libraries' namespaces can be obtained
from the SystemTap probes.  I've also not done anything special about
STT_GNU_IFUNC functions, since if one of these is causing problems you
could simply set gdb to break on its relocator without needing any
special support in stop-on-solib-events.

I've seen mention that the RT_CONSISTENT called too early bug was the
cause of another issue where libthread_db would not be loaded if
libpthread was loaded with dlopen rather than being linked in, but I
haven't been able to discover if this really was ever the case.  If
anybody knows (or has a testcase) could you fill me in on it please.

I'm still waiting for a reply to my mail about in-process debuggers,
so I haven't changed anything on that regard yet.

Cheers,
Gary

-- 
http://gbenson.net/

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

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6a130b6..2490967 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,18 @@
+2011-06-06  Gary Benson  <gbenson@redhat.com>
+
+	* infrun.c (set_stop_on_solib_events): New function.
+	(_initialize_infrun): Call the above when stop_on_solib_events is set.
+	* solib.h (update_solib_breakpoints): New method declaration.
+	* solib.c (update_solib_breakpoints): New method.
+	* solist.h (target_so_ops): New field update_breakpoints.
+	* solib-svr4.c (svr4_info): New fields pre_mod_probe and post_mod_probe.
+	(svr4_update_solib_event_breakpoint): New function.
+	(svr4_update_solib_event_breakpoints): Likewise.
+	(svr4_create_solib_event_breakpoints): Likewise.
+	(enable_break): Use svr4_create_solib_event_breakpoints instead of
+	create_solib_event_breakpoint to create solib event breakpoints.
+	(_initialize_svr4_solib): Initialize svr4_so_ops.update_breakpoints.
+
 2011-05-09  Doug Evans  <dje@google.com>
 
 	* NEWS: Mention --with-iconv-bin.
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 4f1fbd9..425a7b0 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -324,6 +324,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)
@@ -7122,7 +7129,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 bcb94e7..41b3344 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -48,6 +48,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);
@@ -335,6 +337,10 @@ struct svr4_info
   CORE_ADDR interp_text_sect_high;
   CORE_ADDR interp_plt_sect_low;
   CORE_ADDR interp_plt_sect_high;
+
+  /* SystemTap probes.  */
+  const struct stap_probe *pre_mod_probe;
+  const struct stap_probe *post_mod_probe;
 };
 
 /* Per-program-space data key.  */
@@ -1310,6 +1316,91 @@ 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;
+
+  for (loc = b->loc; loc; loc = loc->next)
+    {
+      if (loc->pspace == current_program_space
+	  && loc->address == info->pre_mod_probe->address)
+	{
+	  b->enable_state = stop_on_solib_events;
+	  return 1;
+	}
+    }
+
+  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->pre_mod_probe)
+    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 obj_section *os;
+
+  os = find_pc_section (address);
+  if (os != NULL)
+    {
+      struct svr4_info *info = get_svr4_info ();
+
+      info->pre_mod_probe = find_probe_in_objfile (os->objfile, "rtld",
+						   "r_debug_mod_starting");
+
+      info->post_mod_probe = find_probe_in_objfile (os->objfile, "rtld",
+						    "r_debug_mod_complete");
+
+      if (info->pre_mod_probe && info->post_mod_probe)
+	{
+	  /* The pre-modification probe is triggered before a link map
+	     is changed.  This probe isn't used internally, so we only
+	     break on it when stop-on-solib-events is on.  */
+	  create_solib_event_breakpoint (gdbarch,
+					 info->pre_mod_probe->address);
+
+	  /* The post-modification probe is triggered whenever a link
+	     map has been changed.  We always need to break on this
+	     one so we can track libraries being loaded and
+	     unloaded.  */
+	  create_solib_event_breakpoint (gdbarch,
+					 info->post_mod_probe->address);
+	  
+	  
+	  svr4_update_solib_event_breakpoints ();
+	  return;
+	}
+    }
+
+  create_solib_event_breakpoint (gdbarch, address);
+}
+
 /*
 
    LOCAL FUNCTION
@@ -1365,6 +1456,8 @@ enable_break (struct svr4_info *info, int from_tty)
   info->interp_text_sect_low = info->interp_text_sect_high = 0;
   info->interp_plt_sect_low = info->interp_plt_sect_high = 0;
 
+  info->pre_mod_probe = info->post_mod_probe = NULL;
+
   /* 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
@@ -1396,7 +1489,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
@@ -1434,7 +1527,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;
 	}
     }
@@ -1588,7 +1681,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;
 	}
@@ -1614,7 +1708,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;
 	}
     }
@@ -1630,7 +1724,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;
 	    }
 	}
@@ -2454,4 +2548,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 a9f46e0..129724d 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1309,6 +1309,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 c473d85..7b3881c 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -78,4 +78,8 @@ extern void set_solib_ops (struct gdbarch *gdbarch,
 
 extern int libpthread_name_p (const char *name);
 
+/* 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 dad11be..14ede10 100644
--- a/gdb/solist.h
+++ b/gdb/solist.h
@@ -137,6 +137,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 *).  */

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-07  9:48 gdb side of improved linker-debugger interface Gary Benson
@ 2011-06-07 16:29 ` Tom Tromey
  2011-06-09 14:26   ` Gary Benson
  2011-06-07 17:20 ` Jan Kratochvil
  1 sibling, 1 reply; 10+ messages in thread
From: Tom Tromey @ 2011-06-07 16:29 UTC (permalink / raw)
  To: archer

>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> A week or so ago I mailed this list a description of a new linker-
Gary> debugger interface based on SystemTap probes, along with a patch
Gary> containing the glibc side:
Gary>   http://www.cygwin.com/ml/archer/2011-q2/msg00000.html

If this is ready then the next step is to try to get it into glibc.

I don't think it can go into glibc mainline, but perhaps we can get it
onto Roland's branch.

One thing that would be useful is a write-up explaining the issues and
why this approach is correct.  What I recall from some bug (I don't
recall which one) on the topic is that Ulrich was of the opinion that
the debug hook was in the right spot and that GDB was wrong.  It would
be good to have a rebuttal for this position in particular.

I'm not sure what the protocol is here -- email to glibc-alpha, or just
to Roland.  glibc-alpha will likely get you flamed, don your protective
gear.

Gary> I've seen mention that the RT_CONSISTENT called too early bug was the
Gary> cause of another issue where libthread_db would not be loaded if
Gary> libpthread was loaded with dlopen rather than being linked in, but I
Gary> haven't been able to discover if this really was ever the case.  If
Gary> anybody knows (or has a testcase) could you fill me in on it please.

I failed in my attempt to make a simple reproducer.
I know David Malcolm had this reliably fail with his Python plugin for
GCC; so you might try that, though it seems rather large for a test case.

Gary> +  /* SystemTap probes.  */
Gary> +  const struct stap_probe *pre_mod_probe;
Gary> +  const struct stap_probe *post_mod_probe;

You are going to have to merge or rebase and update this a bit.
I changed find_probe_in_objfile on the branch, to account for probes
with multiple locations.

Once the patch is ready I think it should go on archer-sergiodj-stap and
then on origin/archer-sergiodj-stap-patch-split as the last patch in the
series.

Gary> +static int
Gary> +svr4_update_solib_event_breakpoint (struct breakpoint *b, void *arg)
Gary> +{
Gary> +  struct svr4_info *info = get_svr4_info ();
Gary> +  struct bp_location *loc;
Gary> +
Gary> +  for (loc = b->loc; loc; loc = loc->next)
Gary> +    {
Gary> +      if (loc->pspace == current_program_space
Gary> +	  && loc->address == info->pre_mod_probe->address)
Gary> +	{
Gary> +	  b->enable_state = stop_on_solib_events;

This should have a constant from `enum enable_state' on the RHS.

Gary> -	  create_solib_event_breakpoint (target_gdbarch, sym_addr);
Gary> +	  svr4_create_solib_event_breakpoints (target_gdbarch, sym_addr);

I don't know this code very well, but it seems to me that it is looking
at different ways to set the solib breakpoints.  Your current code runs
at the very end -- GDB still tries the existing ways to find the
breakpoint locations, and then, if one is found, instead inserts at the
probe point if that is found.

Assuming this reading is correct, it seems like it would be better to
just check for SystemTap probe points early in enable_break, and then
prefer those to the other methods.

Tom

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-07  9:48 gdb side of improved linker-debugger interface Gary Benson
  2011-06-07 16:29 ` Tom Tromey
@ 2011-06-07 17:20 ` Jan Kratochvil
  2011-06-08 11:01   ` Gary Benson
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2011-06-07 17:20 UTC (permalink / raw)
  To: archer

On Tue, 07 Jun 2011 11:48:08 +0200, Gary Benson wrote:
> I've seen mention that the RT_CONSISTENT called too early bug was the
> cause of another issue where libthread_db would not be loaded if
> libpthread was loaded with dlopen rather than being linked in, but I
> haven't been able to discover if this really was ever the case.  If
> anybody knows (or has a testcase) could you fill me in on it please.

the testcase from:
https://bugzilla.redhat.com/show_bug.cgi?id=179072#c13

works for me - that is it does not work.  I haven't tried it with your
glibc+gdb, I guess it works now?


Thanks,
Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-07 17:20 ` Jan Kratochvil
@ 2011-06-08 11:01   ` Gary Benson
  2011-06-08 11:48     ` Jan Kratochvil
  0 siblings, 1 reply; 10+ messages in thread
From: Gary Benson @ 2011-06-08 11:01 UTC (permalink / raw)
  To: archer

Jan Kratochvil wrote:
> On Tue, 07 Jun 2011 11:48:08 +0200, Gary Benson wrote:
> > I've seen mention that the RT_CONSISTENT called too early bug was
> > the cause of another issue where libthread_db would not be loaded
> > if libpthread was loaded with dlopen rather than being linked in,
> > but I haven't been able to discover if this really was ever the
> > case.  If anybody knows (or has a testcase) could you fill me in
> > on it please.
> 
> the testcase from:
> https://bugzilla.redhat.com/show_bug.cgi?id=179072#c13
> 
> works for me - that is it does not work.  I haven't tried it with
> your glibc+gdb, I guess it works now?

It actually doesn't fail on standard F14 gdb:

  $ gdb ./testload
  GNU gdb (GDB) Fedora (7.2-51.fc14)
  Copyright (C) 2010 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "x86_64-redhat-linux-gnu".
  For bug reporting instructions, please see:
  <http://www.gnu.org/software/gdb/bugs/>...
  Reading symbols from /home/gary/work/archer/rh179072-tests/testload...done.
  (gdb) r
  Starting program: /home/gary/work/archer/rh179072-tests/testload 
  [Thread debugging using libthread_db enabled]
  [New Thread 0x7ffff7dd5700 (LWP 5584)]
  [Thread 0x7ffff7dd5700 (LWP 5584) exited]
  
  Program exited normally.

Not sure what's happening there...

Cheers,
Gary

-- 
http://gbenson.net/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-08 11:01   ` Gary Benson
@ 2011-06-08 11:48     ` Jan Kratochvil
  2011-06-08 12:29       ` Gary Benson
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Kratochvil @ 2011-06-08 11:48 UTC (permalink / raw)
  To: archer

On Wed, 08 Jun 2011 13:00:44 +0200, Gary Benson wrote:
> > the testcase from:
> > https://bugzilla.redhat.com/show_bug.cgi?id=179072#c13
[...]
> It actually doesn't fail on standard F14 gdb:

Please do:
# prelink -u /lib64/libpthread-2.13.so

$ gdb ./testload
GNU gdb (GDB) Fedora (7.2-51.fc14)
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/jkratoch/t/rh179072-tests/testload...done.
(gdb) r
Starting program: /home/jkratoch/t/rh179072-tests/testload 
[Thread debugging using libthread_db enabled]
Cannot find new threads: generic error
(gdb) _


Thanks,
Jan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-08 11:48     ` Jan Kratochvil
@ 2011-06-08 12:29       ` Gary Benson
  0 siblings, 0 replies; 10+ messages in thread
From: Gary Benson @ 2011-06-08 12:29 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: archer

Jan Kratochvil wrote:
> On Wed, 08 Jun 2011 13:00:44 +0200, Gary Benson wrote:
> > > the testcase from:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=179072#c13
> [...]
> > It actually doesn't fail on standard F14 gdb:
> 
> Please do:
> # prelink -u /lib64/libpthread-2.13.so
> 
> $ gdb ./testload
> GNU gdb (GDB) Fedora (7.2-51.fc14)
> Copyright (C) 2010 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
> and "show warranty" for details.
> This GDB was configured as "x86_64-redhat-linux-gnu".
> For bug reporting instructions, please see:
> <http://www.gnu.org/software/gdb/bugs/>...
> Reading symbols from /home/jkratoch/t/rh179072-tests/testload...done.
> (gdb) r
> Starting program: /home/jkratoch/t/rh179072-tests/testload 
> [Thread debugging using libthread_db enabled]
> Cannot find new threads: generic error
> (gdb) _

Ah, after unprelinking I get that failure with F14 gdb.  But, it works
nicely with the new stuff:

  $ ../build/gdb/gdb -args ../../glibc/install/lib/ld-2.13.90.so ./testload
  GNU gdb (GDB) 7.3.50.20110510-cvs
  Copyright (C) 2011 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "x86_64-unknown-linux-gnu".
  For bug reporting instructions, please see:
  <http://www.gnu.org/software/gdb/bugs/>...
  Reading symbols from /home/gary/work/glibc/install/lib/ld-2.13.90.so...done.
  (gdb) r
  Starting program: /home/gary/work/glibc/install/lib/ld-2.13.90.so ./testload
  [Thread debugging using libthread_db enabled]
  [New Thread 0x7ffff762a700 (LWP 6969)]
  [Thread 0x7ffff762a700 (LWP 6969) exited]
  [Inferior 1 (process 6966) exited normally]
  (gdb) _

I'm glad I have this reproduced now, it's IMO a much more compelling
argument for the fix than the somewhat arbitrary testcase the bug was
originally filed with.

Thanks!

Cheers,
Gary

-- 
http://gbenson.net/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-09 14:26   ` Gary Benson
@ 2011-06-08 16:27     ` Tom Tromey
  2011-06-09 14:09       ` Gary Benson
  0 siblings, 1 reply; 10+ messages in thread
From: Tom Tromey @ 2011-06-08 16:27 UTC (permalink / raw)
  To: archer

Gary> I did it that way to ensure we're looking for the probes in the
Gary> correct object.  It uses the original code to locate _dl_debug_state,
Gary> then assumes that the object containing _dl_debug_state is the linker
Gary> and looks for the probes in that.  I don't know if this is the right
Gary> solution though.

Ok, I see.  I think it would be fine the other way, but this is ok with
me too.

Tom

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-08 16:27     ` Tom Tromey
@ 2011-06-09 14:09       ` Gary Benson
  2011-06-09 14:43         ` Tom Tromey
  0 siblings, 1 reply; 10+ messages in thread
From: Gary Benson @ 2011-06-09 14:09 UTC (permalink / raw)
  To: archer

Tom Tromey wrote:
> Gary> I did it that way to ensure we're looking for the probes in
> Gary> the correct object.  It uses the original code to locate
> Gary> _dl_debug_state, then assumes that the object containing
> Gary> _dl_debug_state is the linker and looks for the probes in
> Gary> that.  I don't know if this is the right solution though.
> 
> Ok, I see.  I think it would be fine the other way, but this is
> ok with me too.

What would the other way be?  I'm aware the way I've done it is
convoluted, and obviously it would be neater and simpler to just
check at the top of enable_break, but I don't know how you would
locate the objfile (unless there another way of locating probes
that doesn't require that?)  I guess my thinking was that there
is all this code in enable_break for dealing with all kinds of
different platforms---and that obviously works---and I wanted to
take advantage of that.

Cheers,
Gary

-- 
http://gbenson.net/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-07 16:29 ` Tom Tromey
@ 2011-06-09 14:26   ` Gary Benson
  2011-06-08 16:27     ` Tom Tromey
  0 siblings, 1 reply; 10+ messages in thread
From: Gary Benson @ 2011-06-09 14:26 UTC (permalink / raw)
  To: Tom Tromey; +Cc: archer

Tom Tromey wrote:
> >>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:
> 
> Gary> A week or so ago I mailed this list a description of a new
> Gary> linker- debugger interface based on SystemTap probes, along
> Gary> with a patch containing the glibc side:
> Gary>   http://www.cygwin.com/ml/archer/2011-q2/msg00000.html
> 
> If this is ready then the next step is to try to get it into glibc.
> 
> I don't think it can go into glibc mainline, but perhaps we can get
> it onto Roland's branch.
> 
> One thing that would be useful is a write-up explaining the issues
> and why this approach is correct.  What I recall from some bug (I
> don't recall which one) on the topic is that Ulrich was of the
> opinion that the debug hook was in the right spot and that GDB was
> wrong.  It would be good to have a rebuttal for this position in
> particular.

I was thinking about this last night.  What I'm considering is adding
at least one more probe point, so there's one at the original location
(where Uli thinks is right) and one where we actually want to break.
I also want to look into the thing on
https://bugzilla.redhat.com/show_bug.cgi?id=698001#c1 where they
mention calling their function (_dl_debug_fast_state) *only* when the
link map has actually changed as I didn't realise there were
situations where it wouldn't.

> I'm not sure what the protocol is here -- email to glibc-alpha, or
> just to Roland.  glibc-alpha will likely get you flamed, don your
> protective gear.

I emailed Andreas privately a while back and he said to discuss it on
glibc-alpha, so I guess that's the place for this.

> Gary> I've seen mention that the RT_CONSISTENT called too early bug
> Gary> was the cause of another issue where libthread_db would not be
> Gary> loaded if libpthread was loaded with dlopen rather than being
> Gary> linked in, but I haven't been able to discover if this really
> Gary> was ever the case.  If anybody knows (or has a testcase) could
> Gary> you fill me in on it please.
> 
> I failed in my attempt to make a simple reproducer.
> I know David Malcolm had this reliably fail with his Python plugin
> for GCC; so you might try that, though it seems rather large for a
> test case.

I have this now: http://sourceware.org/ml/archer/2011-q2/msg00022.html

> Gary> +  /* SystemTap probes.  */
> Gary> +  const struct stap_probe *pre_mod_probe;
> Gary> +  const struct stap_probe *post_mod_probe;
> 
> You are going to have to merge or rebase and update this a bit.  I
> changed find_probe_in_objfile on the branch, to account for probes
> with multiple locations.

Ok, thanks for the heads up.

> Gary> +static int
> Gary> +svr4_update_solib_event_breakpoint (struct breakpoint *b, void *arg)
> Gary> +{
> Gary> +  struct svr4_info *info = get_svr4_info ();
> Gary> +  struct bp_location *loc;
> Gary> +
> Gary> +  for (loc = b->loc; loc; loc = loc->next)
> Gary> +    {
> Gary> +      if (loc->pspace == current_program_space
> Gary> +	  && loc->address == info->pre_mod_probe->address)
> Gary> +	{
> Gary> +	  b->enable_state = stop_on_solib_events;
> 
> This should have a constant from `enum enable_state' on the RHS.

Ah, ok, I'll change it.  

> Gary> -	  create_solib_event_breakpoint (target_gdbarch, sym_addr);
> Gary> +	  svr4_create_solib_event_breakpoints (target_gdbarch, sym_addr);
> 
> I don't know this code very well, but it seems to me that it is
> looking at different ways to set the solib breakpoints.  Your
> current code runs at the very end -- GDB still tries the existing
> ways to find the breakpoint locations, and then, if one is found,
> instead inserts at the probe point if that is found.
> 
> Assuming this reading is correct, it seems like it would be better
> to just check for SystemTap probe points early in enable_break, and
> then prefer those to the other methods.

I did it that way to ensure we're looking for the probes in the
correct object.  It uses the original code to locate _dl_debug_state,
then assumes that the object containing _dl_debug_state is the linker
and looks for the probes in that.  I don't know if this is the right
solution though.

Cheers,
Gary

-- 
http://gbenson.net/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gdb side of improved linker-debugger interface
  2011-06-09 14:09       ` Gary Benson
@ 2011-06-09 14:43         ` Tom Tromey
  0 siblings, 0 replies; 10+ messages in thread
From: Tom Tromey @ 2011-06-09 14:43 UTC (permalink / raw)
  To: archer

>> Ok, I see.  I think it would be fine the other way, but this is
>> ok with me too.

Gary> What would the other way be?

Oh, nothing, since I was not thinking clearly about what this code is
doing.

Tom

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-06-09 14:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-07  9:48 gdb side of improved linker-debugger interface Gary Benson
2011-06-07 16:29 ` Tom Tromey
2011-06-09 14:26   ` Gary Benson
2011-06-08 16:27     ` Tom Tromey
2011-06-09 14:09       ` Gary Benson
2011-06-09 14:43         ` Tom Tromey
2011-06-07 17:20 ` Jan Kratochvil
2011-06-08 11:01   ` Gary Benson
2011-06-08 11:48     ` Jan Kratochvil
2011-06-08 12:29       ` Gary Benson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).