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

[Jeff Johnston <jjohnstn@redhat.com>]

Daniel Jacobowitz wrote:
> 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.
> 
>

I'm glad to see everyone responding.

I did not verify that DR6 is thread-specific for *all* versions of GNU/Linux.  I 
  simply made the observation and confirmed it by asking a Red Hat kernel 
developer.  I can get the developer to verify that his response was meant for 
all GNU/Linux.  However, a better question might be: does anyone know of any 
version of GNU/Linux where the current x86 code works for threads?

My observation is that the rest of the debug registers are global.  My example 
sets the watch addresses and DR7 from the main thread and this works across the 
new threads.  I also am able to set a new watchpoint from a thread that breaks 
in the main thread, etc...  Thus, I only had to modify the DR6 code rather than 
all the debug registers.

>>   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.
> 

Could you provide any details?  Is there anything I can do interim?

> 
>>   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.
>

Is the right way to first ensure that gdb switches to the trap thread so that we 
simply get the DR6 for the then-current thread?

-- Jeff J.