Re: [RFC]: x86 threaded watchpoint support [1/3]

[Daniel Jacobowitz <drow@false.org>]

On Sat, Jun 12, 2004 at 12:00:19AM +0200, Mark Kettenis wrote:
>    Date: Fri, 11 Jun 2004 17:14:24 -0400
>    From: Jeff Johnston <jjohnstn@redhat.com>
> 
>    The following patch gets threaded watchpoint support working for
>    the x86.  On x86 linux, the dr_status register is thread-specific.
>    This means that the current method which uses the PID to call
>    PTRACE is wrong.  I have changed this to use the current lwp for
>    the inferior_ptid.  Corresponding to this, the
>    i386_stopped_data_address function switches the inferior_ptid to
>    the trap_ptid.  Thus, we can see if we really stopped for a
>    watchpoint or hardware breakpoint.
> 
> I'm not surprised that the current stuff is wrong.  However, have you
> verified that the dr_status register is thread-specific for *all*
> versions of GNU/Linux that we support and not just the RedHat kernel
> that you're working with?

I'm pretty sure - GNU/Linux has never had a concept of "process
registers", since threads have evolved from processes rather than the
other way around.

>    Because the thread-db.c code changes the trap_ptid into a
>    high-level ptid (pid + tid), I had to add a new target vector
>    interface which gives back the lwp for a given ptid.  This is used
>    by the low level dr get routine.
> 
> Really?  Doesn't TIDGET work for you?

TIDGET at this point is the thread ID, i.e. internal state from NPTL or
LinuxThreads.  It's not the LWP id that we need in order to all ptrace.

It sounds like this new vector is really the death blow to thread-db.c.
Maybe I should see how much of it we can throw away.  Copious free time
and all that.

>    Index: lin-lwp.c
>    ===================================================================
>    RCS file: /cvs/src/src/gdb/lin-lwp.c,v
>    retrieving revision 1.55
>    diff -u -p -r1.55 lin-lwp.c
>    --- lin-lwp.c	25 May 2004 14:58:28 -0000	1.55
>    +++ lin-lwp.c	11 Jun 2004 20:55:28 -0000
>    @@ -1200,21 +1200,29 @@ child_wait (ptid_t ptid, struct target_w
> 	   }
> 
> 	  /* Handle GNU/Linux's extended waitstatus for trace events.  */
>    -      if (pid != -1 && WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP
>    -	  && status >> 16 != 0)
>    +      if (pid != -1 && WIFSTOPPED (status) && WSTOPSIG (status) == SIGTRAP)
> 	   {
>    -	  linux_handle_extended_wait (pid, status, ourstatus);
>    +	  /* Set trap_ptid like lin_lwp_wait does.  This is needed
>    +	     for watchpoint support.  For example, the x86 linux
>    +	     watchpoints need to know what thread an event occurred
>    +	     on so as to read the correct thread-specific status.  */
>    +          trap_ptid = pid_to_ptid (pid);
> 
>    -	  /* If we see a clone event, detach the child, and don't
>    -	     report the event.  It would be nice to offer some way to
>    -	     switch into a non-thread-db based threaded mode at this
>    -	     point.  */
>    -	  if (ourstatus->kind == TARGET_WAITKIND_SPURIOUS)
>    +	  if (status >> 16 != 0)
> 
> What's this shift supposed to do?

This one's my fault so I'll answer for him: it was already there, up a
couple lines in the diff.  It's for the extended waitstatus support.

I must say that I'm not thrilled by having trap_ptid leak out into yet
more files.  It's an extremely underspecified interface.

-- 
Daniel Jacobowitz