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

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

[Andrew Cagney]

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

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

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

From: Daniel Jacobowitz <drow@mvista.com>
To: Andrew Cagney <ac131313@redhat.com>
Cc: gdb@sources.redhat.com
Subject: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]
Date: Thu, 21 Nov 2002 19:14:47 -0500
Message-ID: <20021122001447.GA7884@nevyn.them.org>

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: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]]

[Andrew Cagney]

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

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

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#11953735 --]
[-- Type: message/rfc822, Size: 14090 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 368 bytes --]

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

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

From: Daniel Jacobowitz <drow@mvista.com>
To: Andrew Cagney <ac131313@redhat.com>
Cc: gdb@sources.redhat.com
Subject: Re: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]
Date: Thu, 21 Nov 2002 19:14:47 -0500
Message-ID: <20021122001447.GA7884@nevyn.them.org>

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: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]]

[Mark Kettenis]

   Date: Wed, 27 Nov 2002 15:55:32 -0500
   From: Andrew Cagney <ac131313@redhat.com>

   This is a multi-part message in MIME format.
   --------------090809040308030407020108
   Content-Type: text/plain; charset=us-ascii; format=flowed
   Content-Transfer-Encoding: 7bit

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

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

Apparently not :-(.  Anyway, I've looked at it now and it appears that
GDB as it stands now isn't ready to cope with the abstraction of the
lin-lwp layer as I intended it.  So I guess that until we overhaul the
way GDB deals with threads, Daniels patch (sans the #ifdef) is the way
to go.

Mark

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

[Andrew Cagney]

>   Is there any chance that the attached could be reviewed over the next 
>    few hours?
> 
> Apparently not :-(.  Anyway, I've looked at it now and it appears that
> GDB as it stands now isn't ready to cope with the abstraction of the
> lin-lwp layer as I intended it.  So I guess that until we overhaul the
> way GDB deals with threads, Daniels patch (sans the #ifdef) is the way
> to go.

(The US has holidays round this time?)

Thanks!  I'll merge it in, test it and then start the 5.2.91 process.

Andrew

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

[Andrew Cagney]

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

>   Is there any chance that the attached could be reviewed over the next    few hours?
> 
> Apparently not :-(.  Anyway, I've looked at it now and it appears that
> GDB as it stands now isn't ready to cope with the abstraction of the
> lin-lwp layer as I intended it.  So I guess that until we overhaul the
> way GDB deals with threads, Daniels patch (sans the #ifdef) is the way
> to go.
> 
> (The US has holidays round this time?)
> 
> Thanks!  I'll merge it in, test it and then start the 5.2.91 process.

Er, no I wont :-(

The attached is the refind patch.  I added the comment:

+  /* NOTE: cagney/2002-12-02: This assumes that the target code can
+     directly transfer the register values into the register cache.
+     This works fine when there is a 1:1 mapping between light weight
+     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
+     an N:1 (user-land threads), or N:M (combination of user-land and
+     LWP threading), this does not work.  An LWP can be sitting in the
+     thread context switch code and hence, the LWP's registers belong
+     to no thread.  */

however, with the patch applied, I see (and consistently, well 2 out of 
2, which is pretty amasing for the thread testsuite) the new failure:


gdb.threads/killed.exp: GDB exits after multi-threaded program exits messily

looking at the log file:

(gdb) run
Starting program: /home/cagney/gdb/native/gdb/testsuite/gdb.threads/killed
[New Thread 1024 (LWP 6831)]
[New Thread 2049 (LWP 6832)]
[New Thread 1026 (LWP 6833)]
Cannot find user-level thread for LWP 6833: generic error
(gdb) PASS: gdb.threads/killed.exp: run program to completion
quit
The program is running.  Exit anyway? (y or n) y
Cannot find thread 2049: generic error
(gdb) FAIL: gdb.threads/killed.exp: GDB exits after multi-threaded 
program exits
  messily (gdb/568)

Which doesn't occure when the patch isn't applied.

The test system was:
$ uname -a
Linux torrens 2.4.9-13 #1 Tue Oct 30 20:11:04 EST 2001 i686 unknown

I'm instead, for the moment, going to document this as a known problem. 
  (It's a maintainer command so normal users won't use it).

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5885 bytes --]

2002-12-03  Andrew Cagney  <ac131313@redhat.com>

	* sparc-nat.c (fetch_inferior_registers)
	(store_inferior_registers): Add comment on problem of LWP vs
	threads.
	
	From 2002-11-21 Daniel Jacobowitz <drow@mvista.com>
	* 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.
	Fix PR gdb/725

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.35.2.1
diff -u -r1.35.2.1 lin-lwp.c
--- lin-lwp.c	26 Nov 2002 01:32:21 -0000	1.35.2.1
+++ lin-lwp.c	3 Dec 2002 16:39:30 -0000
@@ -1346,32 +1346,6 @@
   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,
@@ -1431,8 +1405,10 @@
   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.13
diff -u -r1.13 sparc-nat.c
--- sparc-nat.c	21 Apr 2002 05:34:06 -0000	1.13
+++ sparc-nat.c	3 Dec 2002 16:39:30 -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,19 @@
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int i;
+  int fetch_pid;
+
+  /* NOTE: cagney/2002-12-02: This assumes that the target code can
+     directly transfer the register values into the register cache.
+     This works fine when there is a 1:1 mapping between light weight
+     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
+     an N:1 (user-land threads), or N:M (combination of user-land and
+     LWP threading), this does not work.  An LWP can be sitting in the
+     thread context switch code and hence, the LWP's registers belong
+     to no thread.  */
+  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 +89,7 @@
       || regno >= Y_REGNUM
       || (!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");
 
@@ -105,7 +119,7 @@
       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");
@@ -151,6 +165,19 @@
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
+  int store_pid;
+
+  /* NOTE: cagney/2002-12-02: This assumes that the target code can
+     directly transfer the register values into the register cache.
+     This works fine when there is a 1:1 mapping between light weight
+     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
+     an N:1 (user-land threads), or N:M (combination of user-land and
+     LWP threading), this does not work.  An LWP can be sitting in the
+     thread context switch code and hence, the LWP's registers belong
+     to no thread.  */
+  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.  */
@@ -233,7 +260,7 @@
       inferior_registers.r_y =
 	*(int *) &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");
     }
@@ -247,7 +274,7 @@
       memcpy (&inferior_fp_registers.Fpu_fsr,
 	      &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: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]]

[Daniel Jacobowitz]

On Tue, Dec 03, 2002 at 12:10:34PM -0500, Andrew Cagney wrote:
> >  Is there any chance that the attached could be reviewed over the next    
> >  few hours?
> >
> >Apparently not :-(.  Anyway, I've looked at it now and it appears that
> >GDB as it stands now isn't ready to cope with the abstraction of the
> >lin-lwp layer as I intended it.  So I guess that until we overhaul the
> >way GDB deals with threads, Daniels patch (sans the #ifdef) is the way
> >to go.
> >
> >(The US has holidays round this time?)
> >
> >Thanks!  I'll merge it in, test it and then start the 5.2.91 process.
> 
> Er, no I wont :-(
> 
> The attached is the refind patch.  I added the comment:
> 
> +  /* NOTE: cagney/2002-12-02: This assumes that the target code can
> +     directly transfer the register values into the register cache.
> +     This works fine when there is a 1:1 mapping between light weight
> +     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
> +     an N:1 (user-land threads), or N:M (combination of user-land and
> +     LWP threading), this does not work.  An LWP can be sitting in the
> +     thread context switch code and hence, the LWP's registers belong
> +     to no thread.  */

First of all, this comment is wrong.  I think we're miscommunicating
on what the patch does.  At this point the fetch_inferior_registers
code has an inferior_ptid which looks like this:
  PID = pid, LWPID = 0, TID = 0
or
  PID = pid, LWPID = otherpid, TID = 0

Don't get confused by the use of TIDGET.  Look at the definition of
TIDGET; it gets the _LWP_ id.  This's a search and destroy candidate if
I ever saw one.

Some upper layer has already taken the TID, mapped it to an LWP id, and
is asking for that LWP's registers by the time we get here.  So the LWP
is known to belong to the thread we are querying.


> 
> however, with the patch applied, I see (and consistently, well 2 out of 
> 2, which is pretty amasing for the thread testsuite) the new failure:
> 
> 
> gdb.threads/killed.exp: GDB exits after multi-threaded program exits messily
> 
> looking at the log file:
> 
> (gdb) run
> Starting program: /home/cagney/gdb/native/gdb/testsuite/gdb.threads/killed
> [New Thread 1024 (LWP 6831)]
> [New Thread 2049 (LWP 6832)]
> [New Thread 1026 (LWP 6833)]
> Cannot find user-level thread for LWP 6833: generic error
> (gdb) PASS: gdb.threads/killed.exp: run program to completion
> quit
> The program is running.  Exit anyway? (y or n) y
> Cannot find thread 2049: generic error
> (gdb) FAIL: gdb.threads/killed.exp: GDB exits after multi-threaded 
> program exits
>  messily (gdb/568)
> 
> Which doesn't occure when the patch isn't applied.

Are you sure about this last bit?  I see this failure even without the
patch, on an i386 SMP system.  I just checked it moments ago.


> 
> The test system was:
> $ uname -a
> Linux torrens 2.4.9-13 #1 Tue Oct 30 20:11:04 EST 2001 i686 unknown
> 
> I'm instead, for the moment, going to document this as a known problem. 
>  (It's a maintainer command so normal users won't use it).
> 
> Andrew
> 

> 2002-12-03  Andrew Cagney  <ac131313@redhat.com>
> 
> 	* sparc-nat.c (fetch_inferior_registers)
> 	(store_inferior_registers): Add comment on problem of LWP vs
> 	threads.
> 	
> 	From 2002-11-21 Daniel Jacobowitz <drow@mvista.com>
> 	* 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.
> 	Fix PR gdb/725
> 
> Index: lin-lwp.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/lin-lwp.c,v
> retrieving revision 1.35.2.1
> diff -u -r1.35.2.1 lin-lwp.c
> --- lin-lwp.c	26 Nov 2002 01:32:21 -0000	1.35.2.1
> +++ lin-lwp.c	3 Dec 2002 16:39:30 -0000
> @@ -1346,32 +1346,6 @@
>    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,
> @@ -1431,8 +1405,10 @@
>    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.13
> diff -u -r1.13 sparc-nat.c
> --- sparc-nat.c	21 Apr 2002 05:34:06 -0000	1.13
> +++ sparc-nat.c	3 Dec 2002 16:39:30 -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,19 @@
>    struct regs inferior_registers;
>    struct fp_status inferior_fp_registers;
>    int i;
> +  int fetch_pid;
> +
> +  /* NOTE: cagney/2002-12-02: This assumes that the target code can
> +     directly transfer the register values into the register cache.
> +     This works fine when there is a 1:1 mapping between light weight
> +     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
> +     an N:1 (user-land threads), or N:M (combination of user-land and
> +     LWP threading), this does not work.  An LWP can be sitting in the
> +     thread context switch code and hence, the LWP's registers belong
> +     to no thread.  */
> +  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 +89,7 @@
>        || regno >= Y_REGNUM
>        || (!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");
>  
> @@ -105,7 +119,7 @@
>        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");
> @@ -151,6 +165,19 @@
>    struct regs inferior_registers;
>    struct fp_status inferior_fp_registers;
>    int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
> +  int store_pid;
> +
> +  /* NOTE: cagney/2002-12-02: This assumes that the target code can
> +     directly transfer the register values into the register cache.
> +     This works fine when there is a 1:1 mapping between light weight
> +     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
> +     an N:1 (user-land threads), or N:M (combination of user-land and
> +     LWP threading), this does not work.  An LWP can be sitting in the
> +     thread context switch code and hence, the LWP's registers belong
> +     to no thread.  */
> +  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.  */
> @@ -233,7 +260,7 @@
>        inferior_registers.r_y =
>  	*(int *) &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");
>      }
> @@ -247,7 +274,7 @@
>        memcpy (&inferior_fp_registers.Fpu_fsr,
>  	      &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: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]]

[Andrew Cagney]

>> Er, no I wont :-(
>> 
>> The attached is the refind patch.  I added the comment:
>> 
>> +  /* NOTE: cagney/2002-12-02: This assumes that the target code can
>> +     directly transfer the register values into the register cache.
>> +     This works fine when there is a 1:1 mapping between light weight
>> +     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
>> +     an N:1 (user-land threads), or N:M (combination of user-land and
>> +     LWP threading), this does not work.  An LWP can be sitting in the
>> +     thread context switch code and hence, the LWP's registers belong
>> +     to no thread.  */
> 
> 
> First of all, this comment is wrong.

Why?

The code is assuming that the LWP registers belong to the currently 
selected thread's regcache.  That is a pretty scary assumption.

[I'll use that wording]

>  I think we're miscommunicating
> on what the patch does.  At this point the fetch_inferior_registers
> code has an inferior_ptid which looks like this:
>   PID = pid, LWPID = 0, TID = 0
> or
>   PID = pid, LWPID = otherpid, TID = 0

> Don't get confused by the use of TIDGET.  Look at the definition of
> TIDGET; it gets the _LWP_ id.  This's a search and destroy candidate if
> I ever saw one.

I'll add that.

> Some upper layer has already taken the TID, mapped it to an LWP id, and
> is asking for that LWP's registers by the time we get here.  So the LWP
> is known to belong to the thread we are querying.


>> however, with the patch applied, I see (and consistently, well 2 out of 
>> 2, which is pretty amasing for the thread testsuite) the new failure:
>> 
>> 
>> gdb.threads/killed.exp: GDB exits after multi-threaded program exits messily
>> 
>> looking at the log file:
>> 
>> (gdb) run
>> Starting program: /home/cagney/gdb/native/gdb/testsuite/gdb.threads/killed
>> [New Thread 1024 (LWP 6831)]
>> [New Thread 2049 (LWP 6832)]
>> [New Thread 1026 (LWP 6833)]
>> Cannot find user-level thread for LWP 6833: generic error
>> (gdb) PASS: gdb.threads/killed.exp: run program to completion
>> quit
>> The program is running.  Exit anyway? (y or n) y
>> Cannot find thread 2049: generic error
>> (gdb) FAIL: gdb.threads/killed.exp: GDB exits after multi-threaded 
>> program exits
>>  messily (gdb/568)
>> 
>> Which doesn't occure when the patch isn't applied.
> 
> 
> Are you sure about this last bit?  I see this failure even without the
> patch, on an i386 SMP system.  I just checked it moments ago.

Yes.  Not on an SMP machine though.

Andrew

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

[Daniel Jacobowitz]

On Tue, Dec 03, 2002 at 02:20:17PM -0500, Andrew Cagney wrote:
> 
> >>Er, no I wont :-(
> >>
> >>The attached is the refind patch.  I added the comment:
> >>
> >>+  /* NOTE: cagney/2002-12-02: This assumes that the target code can
> >>+     directly transfer the register values into the register cache.
> >>+     This works fine when there is a 1:1 mapping between light weight
> >>+     process (LWP) (a.k.a. process on GNU/Linux) and the thread.  On
> >>+     an N:1 (user-land threads), or N:M (combination of user-land and
> >>+     LWP threading), this does not work.  An LWP can be sitting in the
> >>+     thread context switch code and hence, the LWP's registers belong
> >>+     to no thread.  */
> >
> >
> >First of all, this comment is wrong.
> 
> Why?
> 
> The code is assuming that the LWP registers belong to the currently 
> selected thread's regcache.  That is a pretty scary assumption.
> 
> [I'll use that wording]

It's not an assumption at this point.  proc-service.c:230 to
thread_db_fetch_registers is the only path into lin_lwp_fetch_registers.
And that does:
  inferior_ptid = BUILD_LWP (lwpid, ph->pid);

So at this point we _know_ that the thread we're querying has its
registers in the LWP.  That's the whole point.

> 
> > I think we're miscommunicating
> >on what the patch does.  At this point the fetch_inferior_registers
> >code has an inferior_ptid which looks like this:
> >  PID = pid, LWPID = 0, TID = 0
> >or
> >  PID = pid, LWPID = otherpid, TID = 0
> 
> >Don't get confused by the use of TIDGET.  Look at the definition of
> >TIDGET; it gets the _LWP_ id.  This's a search and destroy candidate if
> >I ever saw one.
> 
> I'll add that.
> 
> >Some upper layer has already taken the TID, mapped it to an LWP id, and
> >is asking for that LWP's registers by the time we get here.  So the LWP
> >is known to belong to the thread we are querying.
> 
> 
> >>however, with the patch applied, I see (and consistently, well 2 out of 
> >>2, which is pretty amasing for the thread testsuite) the new failure:
> >>
> >>
> >>gdb.threads/killed.exp: GDB exits after multi-threaded program exits 
> >>messily
> >>
> >>looking at the log file:
> >>
> >>(gdb) run
> >>Starting program: /home/cagney/gdb/native/gdb/testsuite/gdb.threads/killed
> >>[New Thread 1024 (LWP 6831)]
> >>[New Thread 2049 (LWP 6832)]
> >>[New Thread 1026 (LWP 6833)]
> >>Cannot find user-level thread for LWP 6833: generic error
> >>(gdb) PASS: gdb.threads/killed.exp: run program to completion
> >>quit
> >>The program is running.  Exit anyway? (y or n) y
> >>Cannot find thread 2049: generic error
> >>(gdb) FAIL: gdb.threads/killed.exp: GDB exits after multi-threaded 
> >>program exits
> >> messily (gdb/568)
> >>
> >>Which doesn't occure when the patch isn't applied.
> >
> >
> >Are you sure about this last bit?  I see this failure even without the
> >patch, on an i386 SMP system.  I just checked it moments ago.
> 
> Yes.  Not on an SMP machine though.

According to Michael it already shows up in all of his configurations
in current CVS... I see the same thing here.  It's a little timing
sensitive, I don't know why it didn't show up beforehand for you.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

[Andrew Cagney]

> 
> It's not an assumption at this point.  proc-service.c:230 to
> thread_db_fetch_registers is the only path into lin_lwp_fetch_registers.
> And that does:
>   inferior_ptid = BUILD_LWP (lwpid, ph->pid);
> 
> So at this point we _know_ that the thread we're querying has its
> registers in the LWP.  That's the whole point.

To just to clarify something, I'm being critical of the overall design 
here, not your patch.  The comment is for sparc-nat.c (which also gets 
used on sun-4 and free bsd systems).

The target/*-nat interface shouldn't be be making assumptions such as 
this (global state, only ever one LWP / thread active, ...).  Doing so 
is poor design, it leads to bugs just like the one being discussed here.

If, instead, the code had been correctly implemented (what 5 years ago?) 
this bug, and these on-going problems, wouldn't have occured.

Andrew

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

[Daniel Jacobowitz]

On Tue, Dec 03, 2002 at 04:40:49PM -0500, Andrew Cagney wrote:
> >
> >It's not an assumption at this point.  proc-service.c:230 to
> >thread_db_fetch_registers is the only path into lin_lwp_fetch_registers.
> >And that does:
> >  inferior_ptid = BUILD_LWP (lwpid, ph->pid);
> >
> >So at this point we _know_ that the thread we're querying has its
> >registers in the LWP.  That's the whole point.
> 
> To just to clarify something, I'm being critical of the overall design 
> here, not your patch.  The comment is for sparc-nat.c (which also gets 
> used on sun-4 and free bsd systems).

Oh :)  Sparc-nat is, um, a mess.  I was just cleaning it up for
completeness here.

> The target/*-nat interface shouldn't be be making assumptions such as 
> this (global state, only ever one LWP / thread active, ...).  Doing so 
> is poor design, it leads to bugs just like the one being discussed here.
> 
> If, instead, the code had been correctly implemented (what 5 years ago?) 
> this bug, and these on-going problems, wouldn't have occured.

If the comment's for sparc-nat then I've got no objection to your
revision - I'd still really like to see this patch in 5.3.  I'm kind of
curious why it causes killed.exp to fail for you, since I've never once
seen killed.exp _pass_.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

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

[Andrew Cagney]

>   I'm kind of
> curious why it causes killed.exp to fail for you, since I've never once
> seen killed.exp _pass_.

Ok, I've run some more tests and yep, pass, fail, pass, fail, ...

I'll commit the patch.

Andrew

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

[Andrew Cagney]

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

>   I'm kind of
> curious why it causes killed.exp to fail for you, since I've never once
> seen killed.exp _pass_.
> 
> Ok, I've run some more tests and yep, pass, fail, pass, fail, ...
> 
> I'll commit the patch.

The attached is what's in.

Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 5731 bytes --]

2002-12-03  Andrew Cagney  <ac131313@redhat.com>

	* sparc-nat.c (fetch_inferior_registers)
	(store_inferior_registers): Add comment on problem of LWP vs
	threads.
	
	From 2002-11-21 Daniel Jacobowitz <drow@mvista.com>
	* 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.
	Fix PR gdb/725.

Index: lin-lwp.c
===================================================================
RCS file: /cvs/src/src/gdb/lin-lwp.c,v
retrieving revision 1.35.2.1
diff -u -r1.35.2.1 lin-lwp.c
--- lin-lwp.c	26 Nov 2002 01:32:21 -0000	1.35.2.1
+++ lin-lwp.c	3 Dec 2002 22:29:11 -0000
@@ -1346,32 +1346,6 @@
   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,
@@ -1431,8 +1405,10 @@
   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.13
diff -u -r1.13 sparc-nat.c
--- sparc-nat.c	21 Apr 2002 05:34:06 -0000	1.13
+++ sparc-nat.c	3 Dec 2002 22:29:11 -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,25 @@
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int i;
+  int fetch_pid;
+
+  /* NOTE: cagney/2002-12-03: This code assumes that the currently
+     selected light weight processes' registers can be written
+     directly into the selected thread's register cache.  This works
+     fine when given an 1:1 LWP:thread model (such as found on
+     GNU/Linux) but will, likely, have problems when used on an N:1
+     (userland threads) or N:M (userland multiple LWP) model.  In the
+     case of the latter two, the LWP's registers do not necessarily
+     belong to the selected thread (the LWP could be in the middle of
+     executing the thread switch code).
+
+     These functions should instead be paramaterized with an explicit
+     object (struct regcache, struct thread_info?) into which the LWPs
+     registers can be written.  */
+
+  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 +95,7 @@
       || regno >= Y_REGNUM
       || (!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");
 
@@ -105,7 +125,7 @@
       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");
@@ -151,6 +171,13 @@
   struct regs inferior_registers;
   struct fp_status inferior_fp_registers;
   int wanna_store = INT_REGS + STACK_REGS + FP_REGS;
+  int store_pid;
+
+  /* NOTE: cagney/2002-12-02: See comment in fetch_inferior_registers
+     about threaded assumptions.  */
+  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.  */
@@ -233,7 +260,7 @@
       inferior_registers.r_y =
 	*(int *) &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");
     }
@@ -247,7 +274,7 @@
       memcpy (&inferior_fp_registers.Fpu_fsr,
 	      &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: [Fwd: Re: gdb/725: Crash using debug target and regcaches (in 5.3 branch?)]]

[Michael Elizabeth Chastain]

FAIL: gdb.threads/killed.exp: GDB exits after multi-threaded program exits messily (gdb/568)

I see lots of these too.  In fact I see it in *every* configuration
in both gdb HEAD and gdb branch (132 configurations total).

  target = native, host = i686-pc-linux-gnu%rh-8, glibc = 2.2.93-5-rh
  intel celeron single-processor system

  % uname -a
  Linux berman.michael-chastain.com 2.4.18-14 #1 Wed Sep 4 13:35:50 EDT 2002 i686 i686 i386 GNU/Linux

Maybe it's because my gdb's are a week old though (2002-11-25).

My data point,

Michael C