[Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Andrew Cagney]

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

FYI,

Too many memory reads/writes was one reason for a ptrace'd threaded 
shlib program running slow, I suspect this is the other.

Andrew

[-- Attachment #2: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?) --]
[-- Type: message/rfc822, Size: 5967 bytes --]

From: Andrew Cagney <ac131313@redhat.com>
To: nobody@sources.redhat.com
Cc: gdb-prs@sources.redhat.com,
Subject: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)
Date: 21 Nov 2002 21:18:01 -0000
Message-ID: <20021121211801.25549.qmail@sources.redhat.com>

The following reply was made to PR gdb/725; it has been noted by GNATS.

From: Andrew Cagney <ac131313@redhat.com>
To: gdb-gnats@sources.redhat.com
Cc:  
Subject: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)
Date: Thu, 21 Nov 2002 16:16:02 -0500

 "regcache.c" is innocent!
 
 (And the little I just learnt about GDB's thread implementation scares 
 me! :-)
 
 Briefly, the GNU/Linux thread code is giving regcache.c conflicting 
 stories over which inferior ptid should be in the register cache.  As a 
 consequence, every single register fetch leads to a regcache flush and 
 re-fetch.  Outch!
 
 
 Briefly,  core GDB tries to fetch a register.  This eventually leads to 
 the call:
 
 regcache_raw_read(REGNUM)
 
 	registers_tpid != inferior_tpid
 (gdb) print registers_ptid
 $6 = {pid = 31263, lwp = 0, tid = 0}
 (gdb) print inferior_ptid
 $7 = {pid = 31263, lwp = 31263, tid = 0}
 		-> flush regcache
 		-> registers_tpid = inferior_tpid
 	-- at this point regnum is invalid
 	target_fetch_registers (regnum)
 
 Since the inferior doesn't match the target, the cache is flushed, 
 inferior_ptid is updated, and the register is fetched.  The fetch flows 
 on down into the depths of the target and the call:
 
 Seen the problem yet?
 
 lin_lwp_fetch_registers(REGNUM):
 
 1347    lin_lwp_fetch_registers (int regno)
 1348    {
 1349      struct cleanup *old_chain = save_inferior_ptid ();
 1350
 1351      if (is_lwp (inferior_ptid))
 1352        inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
 1353
 (gdb)
 1354      fetch_inferior_registers (regno);
 1355
 1356      do_cleanups (old_chain);
 1357    }
 
 Seen the problem yet?
 
 Not sure why, but the code takes the path:
 	inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
 changing:
 (gdb) print inferior_ptid
 $10 = {pid = 31263, lwp = 31263, tid = 0}
 (gdb) n
 1354      fetch_inferior_registers (regno);
 (gdb) print inferior_ptid
 $11 = {pid = 31263, lwp = 0, tid = 0}
 (gdb)
 
 Seen the problem yet?
 
 And then proceeds to extract the registers using ptrace.  The extracted 
 registers are fed to the register cache using:
 
 supply_register(REGNUM, VAL):
 
 1209    supply_register (int regnum, const void *val)
 1210    {
 1211    #if 1
 1212      if (! ptid_equal (registers_ptid, inferior_ptid))
 1213        {
 1214          registers_changed ();
 1215          registers_ptid = inferior_ptid;
 1216        }
 (gdb)
 1217    #endif
 1218
 1219      set_register_cached (regnum, 1);
 
 and the assignment:
 
 1215          registers_ptid = inferior_ptid;
 (gdb) n
 1219      set_register_cached (regnum, 1);
 (gdb) print registers_ptid
 $14 = {pid = 31263, lwp = 0, tid = 0}
 (gdb)
 
 So, registers_ptid is set back to {pid = 31263, lwp = 0, tid = 0} ready 
 to start the entire cycle, again, on that very next register fetch!
 
 
 Two problems:
 
 
 - this means that the current register cache is being rendered useless 
 as it will never hang onto any register value.  Looks very like reason#2 
 for GDB being slow when debugging a threaded application.
 
 
 - in the case of `set debug target 1', the bug causes an infinite 
 recursion (the cause of the crash) because the code:
 1709    static void
 1710    debug_to_fetch_registers (int regno)
 1711    {
 1712      debug_target.to_fetch_registers (regno);
 1713      debug_print_register ("target_fetch_registers", regno);
 1714    }
 is assuming that the debug_target.to_fetch_registers (regno) call 
 results in the the register's value being entered into the cache, and 
 hence a further deprecated_read_register_gen() won't lead to a call to 
 target_fetch_registers()
 
 
 Fix?
 
 The long term fix is to have per-thread register caches, that is 
 progressing.
 
 I don't know about a short term fix though.
 
 Andrew
 

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Daniel Jacobowitz]

On Thu, Nov 21, 2002 at 05:42:24PM -0500, Andrew Cagney wrote:
> FYI,
> 
> Too many memory reads/writes was one reason for a ptrace'd threaded 
> shlib program running slow, I suspect this is the other.

Maybe, maybe not... definitely needs to go though!  Thanks for such a
thorough investigation, it gave me a good idea.

>  Briefly, the GNU/Linux thread code is giving regcache.c conflicting 
>  stories over which inferior ptid should be in the register cache.  As a 
>  consequence, every single register fetch leads to a regcache flush and 
>  re-fetch.  Outch!
>  
>  
>  Briefly,  core GDB tries to fetch a register.  This eventually leads to 
>  the call:
>  
>  regcache_raw_read(REGNUM)
>  
>  	registers_tpid != inferior_tpid
>  (gdb) print registers_ptid
>  $6 = {pid = 31263, lwp = 0, tid = 0}
>  (gdb) print inferior_ptid
>  $7 = {pid = 31263, lwp = 31263, tid = 0}
>  		-> flush regcache
>  		-> registers_tpid = inferior_tpid
>  	-- at this point regnum is invalid
>  	target_fetch_registers (regnum)
>  
>  Since the inferior doesn't match the target, the cache is flushed, 
>  inferior_ptid is updated, and the register is fetched.  The fetch flows 
>  on down into the depths of the target and the call:
>  
>  Seen the problem yet?

Yup.  Saw something else very interesting, too.

>  The long term fix is to have per-thread register caches, that is 
>  progressing.
>  
>  I don't know about a short term fix though.

I was working on a short-term fix and discovered it was almost entirely
in place already.  Look at a couple of random fetch_inferior_registers
implementations; every one that a GNU/Linux platform uses already will
fetch the LWP's registers if the LWP is non-zero.  So why not give that
to 'em?  Leave the inferior_ptid as it is, and make
fetch_inferior_registers honor the LWP id.

I fixed sparc-nat.c (the fix is a little gross but holds to the style
of the file); I fixed up lin-lwp; I timed a couple of tests.  Look at
what I got.

Currently, on my desktop, to run linux-dp.exp and schedlock.exp:
runtest linux-dp.exp schedlock.exp  101.06s user 50.80s system 130% cpu 1:56.27 total
With change:
runtest linux-dp.exp schedlock.exp  92.13s user 48.46s system 131% cpu 1:47.30 total

Wait, I'm being foolish.  Schedlock has some CPU-intensive loops in it
so when you're trying to get CPU information for GDB it's not a good
choice.  Trying again with linux-dp.exp and print-threads.exp,
currently:
runtest linux-dp.exp print-threads.exp  17.21s user 48.22s system 82% cpu 1:19.56 total
With change:
runtest linux-dp.exp print-threads.exp  16.67s user 45.35s system 82% cpu 1:15.27 total

Linux-dp also has some spinning and sending of SIGINTs, so again the
work done isn't entirely deterministic - but that's still an
interesting difference to me.  And we're definitely finding the correct
registers for each thread, at least on i386-linux; I'm pretty confident
about other targets, too.

And: it appears to fix the PR.  We do so terrifyingly much reading from
the inferior that it's kind of hard to tell; but normally it would have
crashed by now.  And I can "info threads" and print out $eip without it
looping again.

So, thoughts on the attached patch?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2002-11-21  Daniel Jacobowitz  <drow@mvista.com>

	Fix PR gdb/725
	* lin-lwp.c (lin_lwp_fetch_registers): Remove.
	(lin_lwp_store_registers): Remove.
	(init_lin_lwp_ops): Use fetch_inferior_registers
	and store_inferior_registers directly.
	* sparc-nat.c (fetch_inferior_registers): Honor LWP ID.
	(store_inferior_registers): Likewise.

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.36
diff -u -p -r1.36 lin-lwp.c
--- lin-lwp.c	31 Oct 2002 21:00:08 -0000	1.36
+++ lin-lwp.c	22 Nov 2002 00:12:14 -0000
@@ -1343,32 +1343,6 @@ lin_lwp_mourn_inferior (void)
   child_ops.to_mourn_inferior ();
 }
 
-static void
-lin_lwp_fetch_registers (int regno)
-{
-  struct cleanup *old_chain = save_inferior_ptid ();
-
-  if (is_lwp (inferior_ptid))
-    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
-
-  fetch_inferior_registers (regno);
-
-  do_cleanups (old_chain);
-}
-
-static void
-lin_lwp_store_registers (int regno)
-{
-  struct cleanup *old_chain = save_inferior_ptid ();
-
-  if (is_lwp (inferior_ptid))
-    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
-
-  store_inferior_registers (regno);
-
-  do_cleanups (old_chain);
-}
-
 static int
 lin_lwp_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
 		     struct mem_attrib *attrib,
@@ -1426,8 +1400,10 @@ init_lin_lwp_ops (void)
   lin_lwp_ops.to_detach = lin_lwp_detach;
   lin_lwp_ops.to_resume = lin_lwp_resume;
   lin_lwp_ops.to_wait = lin_lwp_wait;
-  lin_lwp_ops.to_fetch_registers = lin_lwp_fetch_registers;
-  lin_lwp_ops.to_store_registers = lin_lwp_store_registers;
+  /* fetch_inferior_registers and store_inferior_registers will
+     honor the LWP id, so we can use them directly.  */
+  lin_lwp_ops.to_fetch_registers = fetch_inferior_registers;
+  lin_lwp_ops.to_store_registers = store_inferior_registers;
   lin_lwp_ops.to_xfer_memory = lin_lwp_xfer_memory;
   lin_lwp_ops.to_kill = lin_lwp_kill;
   lin_lwp_ops.to_create_inferior = lin_lwp_create_inferior;
Index: sparc-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-nat.c,v
retrieving revision 1.15
diff -u -p -r1.15 sparc-nat.c
--- sparc-nat.c	14 Nov 2002 20:37:29 -0000	1.15
+++ sparc-nat.c	22 Nov 2002 00:12:14 -0000
@@ -1,5 +1,6 @@
 /* Functions specific to running gdb native on a SPARC running SunOS4.
-   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001
+   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001,
+   2002
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -58,6 +59,15 @@ fetch_inferior_registers (int regno)
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int i;
+  int fetch_pid;
+
+#ifdef __linux__
+  fetch_pid = TIDGET (inferior_ptid);
+  if (fetch_pid == 0)
+    fetch_pid = PIDGET (inferior_ptid);
+#else
+  fetch_pid = PIDGET (inferior_ptid);
+#endif
 
   /* We should never be called with deferred stores, because a prerequisite
      for writing regs is to have fetched them all (PREPARE_TO_STORE), sigh.  */
@@ -75,7 +85,7 @@ fetch_inferior_registers (int regno)
       || regno >= Y_REGNUM
       || (!deprecated_register_valid[SP_REGNUM] && regno < I7_REGNUM))
     {
-      if (0 != ptrace (PTRACE_GETREGS, PIDGET (inferior_ptid),
+      if (0 != ptrace (PTRACE_GETREGS, fetch_pid,
 		       (PTRACE_ARG3_TYPE) & inferior_registers, 0))
 	perror ("ptrace_getregs");
 
@@ -108,7 +118,7 @@ fetch_inferior_registers (int regno)
       regno == FPS_REGNUM ||
       (regno >= FP0_REGNUM && regno <= FP0_REGNUM + 31))
     {
-      if (0 != ptrace (PTRACE_GETFPREGS, PIDGET (inferior_ptid),
+      if (0 != ptrace (PTRACE_GETFPREGS, fetch_pid,
 		       (PTRACE_ARG3_TYPE) & inferior_fp_registers,
 		       0))
 	perror ("ptrace_getfpregs");
@@ -153,6 +163,15 @@ store_inferior_registers (int regno)
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
+  int store_pid;
+
+#ifdef __linux__
+  store_pid = TIDGET (inferior_ptid);
+  if (store_pid == 0)
+    store_pid = PIDGET (inferior_ptid);
+#else
+  store_pid = PIDGET (inferior_ptid);
+#endif
 
   /* First decide which pieces of machine-state we need to modify.  
      Default for regno == -1 case is all pieces.  */
@@ -236,7 +255,7 @@ store_inferior_registers (int regno)
       inferior_registers.r_y =
 	*(int *) &deprecated_registers[REGISTER_BYTE (Y_REGNUM)];
 
-      if (0 != ptrace (PTRACE_SETREGS, PIDGET (inferior_ptid),
+      if (0 != ptrace (PTRACE_SETREGS, store_pid,
 		       (PTRACE_ARG3_TYPE) & inferior_registers, 0))
 	perror ("ptrace_setregs");
     }
@@ -252,7 +271,7 @@ store_inferior_registers (int regno)
 	      &deprecated_registers[REGISTER_BYTE (FPS_REGNUM)],
 	      sizeof (FPU_FSR_TYPE));
       if (0 !=
-	  ptrace (PTRACE_SETFPREGS, PIDGET (inferior_ptid),
+	  ptrace (PTRACE_SETFPREGS, store_pid,
 		  (PTRACE_ARG3_TYPE) & inferior_fp_registers, 0))
 	perror ("ptrace_setfpregs");
     }

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Andrew Cagney]

> On Thu, Nov 21, 2002 at 05:42:24PM -0500, Andrew Cagney wrote:
> 
>> FYI,
>> 
>> Too many memory reads/writes was one reason for a ptrace'd threaded 
>> shlib program running slow, I suspect this is the other.
> 
> 
> Maybe, maybe not... definitely needs to go though!  Thanks for such a
> thorough investigation, it gave me a good idea.

[snip]

>> currently:
>> runtest linux-dp.exp print-threads.exp  17.21s user 48.22s system 82% cpu 1:19.56 total
>> With change:
>> runtest linux-dp.exp print-threads.exp  16.67s user 45.35s system 82% cpu 1:15.27 total

Given that the numbers are being overwhelmed by all those memory read 
ptrace calls, a ~5% improvement is significant.

Try something simpler like running gdb under strace (tweak 
testsuite/lib/gdb.exp to run 'strace $GDB' instead of $GDB) and then 
count how many ptrace calls of each type occure.

>>  Briefly, the GNU/Linux thread code is giving regcache.c conflicting 
>>  stories over which inferior ptid should be in the register cache.  As a 
>>  consequence, every single register fetch leads to a regcache flush and 
>>  re-fetch.  Outch!
>>  
>>  
>>  Briefly,  core GDB tries to fetch a register.  This eventually leads to 
>>  the call:
>>  
>>  regcache_raw_read(REGNUM)
>>  
>>  	registers_tpid != inferior_tpid
>>  (gdb) print registers_ptid
>>  $6 = {pid = 31263, lwp = 0, tid = 0}
>>  (gdb) print inferior_ptid
>>  $7 = {pid = 31263, lwp = 31263, tid = 0}
>>  		-> flush regcache
>>  		-> registers_tpid = inferior_tpid
>>  	-- at this point regnum is invalid
>>  	target_fetch_registers (regnum)
>>  
>>  Since the inferior doesn't match the target, the cache is flushed, 
>>  inferior_ptid is updated, and the register is fetched.  The fetch flows 
>>  on down into the depths of the target and the call:
>>  
>>  Seen the problem yet?
> 
> 
> Yup.  Saw something else very interesting, too.
> 
> 
>>  The long term fix is to have per-thread register caches, that is 
>>  progressing.
>>  
>>  I don't know about a short term fix though.
> 
> 
> I was working on a short-term fix and discovered it was almost entirely
> in place already.  Look at a couple of random fetch_inferior_registers
> implementations; every one that a GNU/Linux platform uses already will
> fetch the LWP's registers if the LWP is non-zero.  So why not give that
> to 'em?  Leave the inferior_ptid as it is, and make
> fetch_inferior_registers honor the LWP id.

It feels right.  I'm hopeing that, eventually, the code will supply the 
registers direct to a (one of many) `struct thread_info' object.

> So, thoughts on the attached patch?

Thread maintainer question (not so sure about the #ifdef linux though :-).

Andrew

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Andrew Cagney]

In fact:

cagney@torrens$ grep ptrace gdb.strace | wc -l
5236211

cagney@torrens$ grep PTRACE_PEEKUSER gdb.strace | wc -l
     162
cagney@torrens$ grep PTRACE_CONT gdb.strace | wc -l
     610
cagney@torrens$ grep SIGSTOP gdb.strace | grep kill | wc -l
     515

But, get this:

cagney@torrens$ grep PTRACE_PEEKTEXT gdb.strace | wc -l
5231130

so, until the PEEKTEXT is eliminated, it won't make any difference.

--

Plucking a ``random'' memory location out of thin air:

cagney@torrens$ grep 0x40040ea0 gdb.strace | grep ptrace | wc -l
    7038
cagney@torrens$ expr 7038 \* 250
1759500

(I think its a buffer that thread db needs - the buffer is read 7000 
times when the target only does ~160 continues (if that)).

Andrew

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Daniel Jacobowitz]

On Thu, Nov 21, 2002 at 07:42:54PM -0500, Andrew Cagney wrote:
> >On Thu, Nov 21, 2002 at 05:42:24PM -0500, Andrew Cagney wrote:
> >
> >>FYI,
> >>
> >>Too many memory reads/writes was one reason for a ptrace'd threaded 
> >>shlib program running slow, I suspect this is the other.
> >
> >
> >Maybe, maybe not... definitely needs to go though!  Thanks for such a
> >thorough investigation, it gave me a good idea.
> 
> [snip]
> 
> >>currently:
> >>runtest linux-dp.exp print-threads.exp  17.21s user 48.22s system 82% cpu 
> >>1:19.56 total
> >>With change:
> >>runtest linux-dp.exp print-threads.exp  16.67s user 45.35s system 82% cpu 
> >>1:15.27 total
> 
> Given that the numbers are being overwhelmed by all those memory read 
> ptrace calls, a ~5% improvement is significant.
> 
> Try something simpler like running gdb under strace (tweak 
> testsuite/lib/gdb.exp to run 'strace $GDB' instead of $GDB) and then 
> count how many ptrace calls of each type occure.

This was an excercise in patience; I had to mess with the timeouts,
too.  An initial strace log (of JUST ptrace calls!) was 221 MB.  Once I
bumped the timeouts high enough to run all of print-threads.exp without
DejaGNU getting bored and failing, it was even bigger: 918 MB.  As you
noticed in your next message, most of these are PEEKTEXT's.  This patch
does still make a tiny bit of difference, though.

Without the patch, the test takes about 16 minutes, has a 918MB log,
shows the one failure that I expected to see in print-threads.exp
(still haven't figured that one out), and:
   2113 PTRACE_???
     18 PTRACE_ATTACH
    233 PTRACE_CONT
18033347        PTRACE_PEEKTEXT
    121 PTRACE_PEEKUSER
    694 PTRACE_POKEDATA
      8 PTRACE_POKETEXT
     56 PTRACE_SINGLESTEP

With the patch the log was 480K smaller (i.e. the same size :), the run
took roughly the same time (not surprising, most of it is writing out
the log), same one failure, and:
   2107 PTRACE_???
     18 PTRACE_ATTACH
    241 PTRACE_CONT
18031743      PTRACE_PEEKTEXT
    121 PTRACE_PEEKUSER
    694 PTRACE_POKEDATA
      8 PTRACE_POKETEXT
     56 PTRACE_SINGLESTEP

The patch saved 6 PTRACE_??? calls and 1604 PTRACE_PEEKTEXT calls.  OK,
I'm not overwhelmed here.  The ??? are PTRACE_GETREGS/PTRACE_SETREGS,
of course.

> 
> >> Briefly, the GNU/Linux thread code is giving regcache.c conflicting 
> >> stories over which inferior ptid should be in the register cache.  As a 
> >> consequence, every single register fetch leads to a regcache flush and 
> >> re-fetch.  Outch!
> >> 
> >> 
> >> Briefly,  core GDB tries to fetch a register.  This eventually leads to 
> >> the call:
> >> 
> >> regcache_raw_read(REGNUM)
> >> 
> >> 	registers_tpid != inferior_tpid
> >> (gdb) print registers_ptid
> >> $6 = {pid = 31263, lwp = 0, tid = 0}
> >> (gdb) print inferior_ptid
> >> $7 = {pid = 31263, lwp = 31263, tid = 0}
> >> 		-> flush regcache
> >> 		-> registers_tpid = inferior_tpid
> >> 	-- at this point regnum is invalid
> >> 	target_fetch_registers (regnum)
> >> 
> >> Since the inferior doesn't match the target, the cache is flushed, 
> >> inferior_ptid is updated, and the register is fetched.  The fetch flows 
> >> on down into the depths of the target and the call:
> >> 
> >> Seen the problem yet?
> >
> >
> >Yup.  Saw something else very interesting, too.
> >
> >
> >> The long term fix is to have per-thread register caches, that is 
> >> progressing.
> >> 
> >> I don't know about a short term fix though.
> >
> >
> >I was working on a short-term fix and discovered it was almost entirely
> >in place already.  Look at a couple of random fetch_inferior_registers
> >implementations; every one that a GNU/Linux platform uses already will
> >fetch the LWP's registers if the LWP is non-zero.  So why not give that
> >to 'em?  Leave the inferior_ptid as it is, and make
> >fetch_inferior_registers honor the LWP id.
> 
> It feels right.  I'm hopeing that, eventually, the code will supply the 
> registers direct to a (one of many) `struct thread_info' object.
> 
> >So, thoughts on the attached patch?
> 
> Thread maintainer question (not so sure about the #ifdef linux though :-).

The #ifdef can just go actually.  The only operating system which uses
that file and threads is Linux, and when the BSDs get thread debugging
support I'd like to see them behave the same way, if they have multiple
LWPs.  Don't know anything about BSD threads.

So, thread maintainers, any opinion?  Here's the updated patch.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2002-11-21  Daniel Jacobowitz  <drow@mvista.com>

	Fix PR gdb/725
	* lin-lwp.c (lin_lwp_fetch_registers): Remove.
	(lin_lwp_store_registers): Remove.
	(init_lin_lwp_ops): Use fetch_inferior_registers
	and store_inferior_registers directly.
	* sparc-nat.c (fetch_inferior_registers): Honor LWP ID.
	(store_inferior_registers): Likewise.

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.36
diff -u -p -r1.36 lin-lwp.c
--- lin-lwp.c	31 Oct 2002 21:00:08 -0000	1.36
+++ lin-lwp.c	22 Nov 2002 02:35:31 -0000
@@ -1343,32 +1343,6 @@ lin_lwp_mourn_inferior (void)
   child_ops.to_mourn_inferior ();
 }
 
-static void
-lin_lwp_fetch_registers (int regno)
-{
-  struct cleanup *old_chain = save_inferior_ptid ();
-
-  if (is_lwp (inferior_ptid))
-    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
-
-  fetch_inferior_registers (regno);
-
-  do_cleanups (old_chain);
-}
-
-static void
-lin_lwp_store_registers (int regno)
-{
-  struct cleanup *old_chain = save_inferior_ptid ();
-
-  if (is_lwp (inferior_ptid))
-    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
-
-  store_inferior_registers (regno);
-
-  do_cleanups (old_chain);
-}
-
 static int
 lin_lwp_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
 		     struct mem_attrib *attrib,
@@ -1426,8 +1400,10 @@ init_lin_lwp_ops (void)
   lin_lwp_ops.to_detach = lin_lwp_detach;
   lin_lwp_ops.to_resume = lin_lwp_resume;
   lin_lwp_ops.to_wait = lin_lwp_wait;
-  lin_lwp_ops.to_fetch_registers = lin_lwp_fetch_registers;
-  lin_lwp_ops.to_store_registers = lin_lwp_store_registers;
+  /* fetch_inferior_registers and store_inferior_registers will
+     honor the LWP id, so we can use them directly.  */
+  lin_lwp_ops.to_fetch_registers = fetch_inferior_registers;
+  lin_lwp_ops.to_store_registers = store_inferior_registers;
   lin_lwp_ops.to_xfer_memory = lin_lwp_xfer_memory;
   lin_lwp_ops.to_kill = lin_lwp_kill;
   lin_lwp_ops.to_create_inferior = lin_lwp_create_inferior;
Index: sparc-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-nat.c,v
retrieving revision 1.15
diff -u -p -r1.15 sparc-nat.c
--- sparc-nat.c	14 Nov 2002 20:37:29 -0000	1.15
+++ sparc-nat.c	22 Nov 2002 02:35:31 -0000
@@ -1,5 +1,6 @@
 /* Functions specific to running gdb native on a SPARC running SunOS4.
-   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001
+   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001,
+   2002
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -58,6 +59,11 @@ fetch_inferior_registers (int regno)
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int i;
+  int fetch_pid;
+
+  fetch_pid = TIDGET (inferior_ptid);
+  if (fetch_pid == 0)
+    fetch_pid = PIDGET (inferior_ptid);
 
   /* We should never be called with deferred stores, because a prerequisite
      for writing regs is to have fetched them all (PREPARE_TO_STORE), sigh.  */
@@ -75,7 +81,7 @@ fetch_inferior_registers (int regno)
       || regno >= Y_REGNUM
       || (!deprecated_register_valid[SP_REGNUM] && regno < I7_REGNUM))
     {
-      if (0 != ptrace (PTRACE_GETREGS, PIDGET (inferior_ptid),
+      if (0 != ptrace (PTRACE_GETREGS, fetch_pid,
 		       (PTRACE_ARG3_TYPE) & inferior_registers, 0))
 	perror ("ptrace_getregs");
 
@@ -108,7 +114,7 @@ fetch_inferior_registers (int regno)
       regno == FPS_REGNUM ||
       (regno >= FP0_REGNUM && regno <= FP0_REGNUM + 31))
     {
-      if (0 != ptrace (PTRACE_GETFPREGS, PIDGET (inferior_ptid),
+      if (0 != ptrace (PTRACE_GETFPREGS, fetch_pid,
 		       (PTRACE_ARG3_TYPE) & inferior_fp_registers,
 		       0))
 	perror ("ptrace_getfpregs");
@@ -153,6 +159,11 @@ store_inferior_registers (int regno)
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
+  int store_pid;
+
+  store_pid = TIDGET (inferior_ptid);
+  if (store_pid == 0)
+    store_pid = PIDGET (inferior_ptid);
 
   /* First decide which pieces of machine-state we need to modify.  
      Default for regno == -1 case is all pieces.  */
@@ -236,7 +247,7 @@ store_inferior_registers (int regno)
       inferior_registers.r_y =
 	*(int *) &deprecated_registers[REGISTER_BYTE (Y_REGNUM)];
 
-      if (0 != ptrace (PTRACE_SETREGS, PIDGET (inferior_ptid),
+      if (0 != ptrace (PTRACE_SETREGS, store_pid,
 		       (PTRACE_ARG3_TYPE) & inferior_registers, 0))
 	perror ("ptrace_setregs");
     }
@@ -252,7 +263,7 @@ store_inferior_registers (int regno)
 	      &deprecated_registers[REGISTER_BYTE (FPS_REGNUM)],
 	      sizeof (FPU_FSR_TYPE));
       if (0 !=
-	  ptrace (PTRACE_SETFPREGS, PIDGET (inferior_ptid),
+	  ptrace (PTRACE_SETFPREGS, store_pid,
 		  (PTRACE_ARG3_TYPE) & inferior_fp_registers, 0))
 	perror ("ptrace_setfpregs");
     }

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Daniel Jacobowitz]

On Thu, Nov 21, 2002 at 09:37:24PM -0500, Daniel Jacobowitz wrote:
> 2002-11-21  Daniel Jacobowitz  <drow@mvista.com>
> 
> 	Fix PR gdb/725
> 	* lin-lwp.c (lin_lwp_fetch_registers): Remove.
> 	(lin_lwp_store_registers): Remove.
> 	(init_lin_lwp_ops): Use fetch_inferior_registers
> 	and store_inferior_registers directly.
> 	* sparc-nat.c (fetch_inferior_registers): Honor LWP ID.
> 	(store_inferior_registers): Likewise.

Any comments on this patch?  If you like it, feel free to commit it for
5.3 - I'll be out of town this week.

> 
> Index: lin-lwp.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/lin-lwp.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 lin-lwp.c
> --- lin-lwp.c	31 Oct 2002 21:00:08 -0000	1.36
> +++ lin-lwp.c	22 Nov 2002 02:35:31 -0000
> @@ -1343,32 +1343,6 @@ lin_lwp_mourn_inferior (void)
>    child_ops.to_mourn_inferior ();
>  }
>  
> -static void
> -lin_lwp_fetch_registers (int regno)
> -{
> -  struct cleanup *old_chain = save_inferior_ptid ();
> -
> -  if (is_lwp (inferior_ptid))
> -    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
> -
> -  fetch_inferior_registers (regno);
> -
> -  do_cleanups (old_chain);
> -}
> -
> -static void
> -lin_lwp_store_registers (int regno)
> -{
> -  struct cleanup *old_chain = save_inferior_ptid ();
> -
> -  if (is_lwp (inferior_ptid))
> -    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
> -
> -  store_inferior_registers (regno);
> -
> -  do_cleanups (old_chain);
> -}
> -
>  static int
>  lin_lwp_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
>  		     struct mem_attrib *attrib,
> @@ -1426,8 +1400,10 @@ init_lin_lwp_ops (void)
>    lin_lwp_ops.to_detach = lin_lwp_detach;
>    lin_lwp_ops.to_resume = lin_lwp_resume;
>    lin_lwp_ops.to_wait = lin_lwp_wait;
> -  lin_lwp_ops.to_fetch_registers = lin_lwp_fetch_registers;
> -  lin_lwp_ops.to_store_registers = lin_lwp_store_registers;
> +  /* fetch_inferior_registers and store_inferior_registers will
> +     honor the LWP id, so we can use them directly.  */
> +  lin_lwp_ops.to_fetch_registers = fetch_inferior_registers;
> +  lin_lwp_ops.to_store_registers = store_inferior_registers;
>    lin_lwp_ops.to_xfer_memory = lin_lwp_xfer_memory;
>    lin_lwp_ops.to_kill = lin_lwp_kill;
>    lin_lwp_ops.to_create_inferior = lin_lwp_create_inferior;
> Index: sparc-nat.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/sparc-nat.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 sparc-nat.c
> --- sparc-nat.c	14 Nov 2002 20:37:29 -0000	1.15
> +++ sparc-nat.c	22 Nov 2002 02:35:31 -0000
> @@ -1,5 +1,6 @@
>  /* Functions specific to running gdb native on a SPARC running SunOS4.
> -   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001
> +   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001,
> +   2002
>     Free Software Foundation, Inc.
>  
>     This file is part of GDB.
> @@ -58,6 +59,11 @@ fetch_inferior_registers (int regno)
>    struct regs inferior_registers;
>    struct fp_status inferior_fp_registers;
>    int i;
> +  int fetch_pid;
> +
> +  fetch_pid = TIDGET (inferior_ptid);
> +  if (fetch_pid == 0)
> +    fetch_pid = PIDGET (inferior_ptid);
>  
>    /* We should never be called with deferred stores, because a prerequisite
>       for writing regs is to have fetched them all (PREPARE_TO_STORE), sigh.  */
> @@ -75,7 +81,7 @@ fetch_inferior_registers (int regno)
>        || regno >= Y_REGNUM
>        || (!deprecated_register_valid[SP_REGNUM] && regno < I7_REGNUM))
>      {
> -      if (0 != ptrace (PTRACE_GETREGS, PIDGET (inferior_ptid),
> +      if (0 != ptrace (PTRACE_GETREGS, fetch_pid,
>  		       (PTRACE_ARG3_TYPE) & inferior_registers, 0))
>  	perror ("ptrace_getregs");
>  
> @@ -108,7 +114,7 @@ fetch_inferior_registers (int regno)
>        regno == FPS_REGNUM ||
>        (regno >= FP0_REGNUM && regno <= FP0_REGNUM + 31))
>      {
> -      if (0 != ptrace (PTRACE_GETFPREGS, PIDGET (inferior_ptid),
> +      if (0 != ptrace (PTRACE_GETFPREGS, fetch_pid,
>  		       (PTRACE_ARG3_TYPE) & inferior_fp_registers,
>  		       0))
>  	perror ("ptrace_getfpregs");
> @@ -153,6 +159,11 @@ store_inferior_registers (int regno)
>    struct regs inferior_registers;
>    struct fp_status inferior_fp_registers;
>    int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
> +  int store_pid;
> +
> +  store_pid = TIDGET (inferior_ptid);
> +  if (store_pid == 0)
> +    store_pid = PIDGET (inferior_ptid);
>  
>    /* First decide which pieces of machine-state we need to modify.  
>       Default for regno == -1 case is all pieces.  */
> @@ -236,7 +247,7 @@ store_inferior_registers (int regno)
>        inferior_registers.r_y =
>  	*(int *) &deprecated_registers[REGISTER_BYTE (Y_REGNUM)];
>  
> -      if (0 != ptrace (PTRACE_SETREGS, PIDGET (inferior_ptid),
> +      if (0 != ptrace (PTRACE_SETREGS, store_pid,
>  		       (PTRACE_ARG3_TYPE) & inferior_registers, 0))
>  	perror ("ptrace_setregs");
>      }
> @@ -252,7 +263,7 @@ store_inferior_registers (int regno)
>  	      &deprecated_registers[REGISTER_BYTE (FPS_REGNUM)],
>  	      sizeof (FPU_FSR_TYPE));
>        if (0 !=
> -	  ptrace (PTRACE_SETFPREGS, PIDGET (inferior_ptid),
> +	  ptrace (PTRACE_SETFPREGS, store_pid,
>  		  (PTRACE_ARG3_TYPE) & inferior_fp_registers, 0))
>  	perror ("ptrace_setfpregs");
>      }
> 

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Michael Snyder]

Daniel Jacobowitz wrote:
> 
> On Thu, Nov 21, 2002 at 09:37:24PM -0500, Daniel Jacobowitz wrote:
> > 2002-11-21  Daniel Jacobowitz  <drow@mvista.com>
> >
> >       Fix PR gdb/725
> >       * lin-lwp.c (lin_lwp_fetch_registers): Remove.
> >       (lin_lwp_store_registers): Remove.
> >       (init_lin_lwp_ops): Use fetch_inferior_registers
> >       and store_inferior_registers directly.
> >       * sparc-nat.c (fetch_inferior_registers): Honor LWP ID.
> >       (store_inferior_registers): Likewise.
> 
> Any comments on this patch?  If you like it, feel free to commit it for
> 5.3 - I'll be out of town this week.

Seems like the spirit is in the right place.
Mark is probably in a better position than I am to comment on
the implementation.

> 
> >
> > Index: lin-lwp.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/lin-lwp.c,v
> > retrieving revision 1.36
> > diff -u -p -r1.36 lin-lwp.c
> > --- lin-lwp.c 31 Oct 2002 21:00:08 -0000      1.36
> > +++ lin-lwp.c 22 Nov 2002 02:35:31 -0000
> > @@ -1343,32 +1343,6 @@ lin_lwp_mourn_inferior (void)
> >    child_ops.to_mourn_inferior ();
> >  }
> >
> > -static void
> > -lin_lwp_fetch_registers (int regno)
> > -{
> > -  struct cleanup *old_chain = save_inferior_ptid ();
> > -
> > -  if (is_lwp (inferior_ptid))
> > -    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
> > -
> > -  fetch_inferior_registers (regno);
> > -
> > -  do_cleanups (old_chain);
> > -}
> > -
> > -static void
> > -lin_lwp_store_registers (int regno)
> > -{
> > -  struct cleanup *old_chain = save_inferior_ptid ();
> > -
> > -  if (is_lwp (inferior_ptid))
> > -    inferior_ptid = pid_to_ptid (GET_LWP (inferior_ptid));
> > -
> > -  store_inferior_registers (regno);
> > -
> > -  do_cleanups (old_chain);
> > -}
> > -
> >  static int
> >  lin_lwp_xfer_memory (CORE_ADDR memaddr, char *myaddr, int len, int write,
> >                    struct mem_attrib *attrib,
> > @@ -1426,8 +1400,10 @@ init_lin_lwp_ops (void)
> >    lin_lwp_ops.to_detach = lin_lwp_detach;
> >    lin_lwp_ops.to_resume = lin_lwp_resume;
> >    lin_lwp_ops.to_wait = lin_lwp_wait;
> > -  lin_lwp_ops.to_fetch_registers = lin_lwp_fetch_registers;
> > -  lin_lwp_ops.to_store_registers = lin_lwp_store_registers;
> > +  /* fetch_inferior_registers and store_inferior_registers will
> > +     honor the LWP id, so we can use them directly.  */
> > +  lin_lwp_ops.to_fetch_registers = fetch_inferior_registers;
> > +  lin_lwp_ops.to_store_registers = store_inferior_registers;
> >    lin_lwp_ops.to_xfer_memory = lin_lwp_xfer_memory;
> >    lin_lwp_ops.to_kill = lin_lwp_kill;
> >    lin_lwp_ops.to_create_inferior = lin_lwp_create_inferior;
> > Index: sparc-nat.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/sparc-nat.c,v
> > retrieving revision 1.15
> > diff -u -p -r1.15 sparc-nat.c
> > --- sparc-nat.c       14 Nov 2002 20:37:29 -0000      1.15
> > +++ sparc-nat.c       22 Nov 2002 02:35:31 -0000
> > @@ -1,5 +1,6 @@
> >  /* Functions specific to running gdb native on a SPARC running SunOS4.
> > -   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001
> > +   Copyright 1989, 1992, 1993, 1994, 1996, 1997, 1998, 1999, 2000, 2001,
> > +   2002
> >     Free Software Foundation, Inc.
> >
> >     This file is part of GDB.
> > @@ -58,6 +59,11 @@ fetch_inferior_registers (int regno)
> >    struct regs inferior_registers;
> >    struct fp_status inferior_fp_registers;
> >    int i;
> > +  int fetch_pid;
> > +
> > +  fetch_pid = TIDGET (inferior_ptid);
> > +  if (fetch_pid == 0)
> > +    fetch_pid = PIDGET (inferior_ptid);
> >
> >    /* We should never be called with deferred stores, because a prerequisite
> >       for writing regs is to have fetched them all (PREPARE_TO_STORE), sigh.  */
> > @@ -75,7 +81,7 @@ fetch_inferior_registers (int regno)
> >        || regno >= Y_REGNUM
> >        || (!deprecated_register_valid[SP_REGNUM] && regno < I7_REGNUM))
> >      {
> > -      if (0 != ptrace (PTRACE_GETREGS, PIDGET (inferior_ptid),
> > +      if (0 != ptrace (PTRACE_GETREGS, fetch_pid,
> >                      (PTRACE_ARG3_TYPE) & inferior_registers, 0))
> >       perror ("ptrace_getregs");
> >
> > @@ -108,7 +114,7 @@ fetch_inferior_registers (int regno)
> >        regno == FPS_REGNUM ||
> >        (regno >= FP0_REGNUM && regno <= FP0_REGNUM + 31))
> >      {
> > -      if (0 != ptrace (PTRACE_GETFPREGS, PIDGET (inferior_ptid),
> > +      if (0 != ptrace (PTRACE_GETFPREGS, fetch_pid,
> >                      (PTRACE_ARG3_TYPE) & inferior_fp_registers,
> >                      0))
> >       perror ("ptrace_getfpregs");
> > @@ -153,6 +159,11 @@ store_inferior_registers (int regno)
> >    struct regs inferior_registers;
> >    struct fp_status inferior_fp_registers;
> >    int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
> > +  int store_pid;
> > +
> > +  store_pid = TIDGET (inferior_ptid);
> > +  if (store_pid == 0)
> > +    store_pid = PIDGET (inferior_ptid);
> >
> >    /* First decide which pieces of machine-state we need to modify.
> >       Default for regno == -1 case is all pieces.  */
> > @@ -236,7 +247,7 @@ store_inferior_registers (int regno)
> >        inferior_registers.r_y =
> >       *(int *) &deprecated_registers[REGISTER_BYTE (Y_REGNUM)];
> >
> > -      if (0 != ptrace (PTRACE_SETREGS, PIDGET (inferior_ptid),
> > +      if (0 != ptrace (PTRACE_SETREGS, store_pid,
> >                      (PTRACE_ARG3_TYPE) & inferior_registers, 0))
> >       perror ("ptrace_setregs");
> >      }
> > @@ -252,7 +263,7 @@ store_inferior_registers (int regno)
> >             &deprecated_registers[REGISTER_BYTE (FPS_REGNUM)],
> >             sizeof (FPU_FSR_TYPE));
> >        if (0 !=
> > -       ptrace (PTRACE_SETFPREGS, PIDGET (inferior_ptid),
> > +       ptrace (PTRACE_SETFPREGS, store_pid,
> >                 (PTRACE_ARG3_TYPE) & inferior_fp_registers, 0))
> >       perror ("ptrace_setfpregs");
> >      }
> >
> 
> --
> Daniel Jacobowitz
> MontaVista Software                         Debian GNU/Linux Developer

Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]

[Andrew Cagney]

> FYI,
> 
> Too many memory reads/writes was one reason for a ptrace'd threaded shlib program running slow, I suspect this is the other.

To correct this.

This should be a reason why GDB runs slow on GNU/Linux, but, isn't!  Per 
other thread, this inefficiency (a few 1 000 stray ptrace calls() is 
trivialized by the millions (yes >1 000 000) memory read ptrace calls. 
GDB was probably executing more memory ptrace() calls than the target 
program executes instructions .... (yea, right :-).

Andrew