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

[Daniel Jacobowitz <drow at mvista dot com>]

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");
     }