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

[Daniel Jacobowitz <drow at mvista dot com>]

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