This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Michael (I suspect Mark is in bed by now),

Is there any chance that the attached could be reviewed over the next few hours?

Daniel (who is off this week -> us long weekend) noted that the #ifdef vis:

+#ifdef __linux__
+ fetch_pid = TIDGET (inferior_ptid);
+ if (fetch_pid == 0)
+ fetch_pid = PIDGET (inferior_ptid);
+#else
+ fetch_pid = PIDGET (inferior_ptid);
+#endif

isn't needed. The code should be unconditional. I can fix/commit that provided the patch is ok.

It is effectively the last remaining fix for 5.3.

Andrew
--- Begin Message --- For the thread maintainers (this was unfortunatly hidden in a gdb@ discussion).

####### ##### #####
# # # # #
# # #
###### ##### ###
# ### # #
# # ### # #
##### ### ##### #

DanielJ's off line for a week so someone will also need to commit.

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

--- End Message ---
--- End Message ---

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]