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: [RFC]: x86 threaded watchpoint support [1/3]


   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?

   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?

I've got a few comments on the code too. Please read on.

   Index: i386-nat.c
   ===================================================================
   RCS file: /cvs/src/src/gdb/i386-nat.c,v
   retrieving revision 1.8
   diff -u -p -r1.8 i386-nat.c
   --- i386-nat.c	5 Mar 2004 20:58:00 -0000	1.8
   +++ i386-nat.c	11 Jun 2004 20:55:28 -0000
   @@ -166,7 +166,10 @@
    #define ALL_DEBUG_REGISTERS(i)	for (i = 0; i < DR_NADDR; i++)

    /* Mirror the inferior's DRi registers.  We keep the status and
   -   control registers separated because they don't hold addresses.  */
   +   control registers separated because they don't hold addresses.  
   + 
   +   Note that the DR_STATUS register is thread-specific and the
   +   mirror value should be refreshed as necessary.  */
    static CORE_ADDR dr_mirror[DR_NADDR];
    static unsigned dr_status_mirror, dr_control_mirror;

   @@ -567,13 +570,22 @@ i386_region_ok_for_watchpoint (CORE_ADDR
    /* If the inferior has some watchpoint that triggered, return the
       address associated with that watchpoint.  Otherwise, return zero.  */

   +extern ptid_t trap_ptid;
   +extern ptid_t inferior_ptid;
   +

Any use of "extern" in .c files is wrong.  Worse, this change will
break other i386 targets that use i386-nat.c.

    CORE_ADDR
    i386_stopped_data_address (void)
    {
      CORE_ADDR addr = 0;
      int i;
   +  ptid_t saved_ptid = inferior_ptid;

   +  /* Debug status register is thread-specific.  A watchpoint will
   +     cause a trap to occur.  Switch to trap ptid to get relevant 
   +     status for that thread.  */
   +  inferior_ptid = trap_ptid;
      dr_status_mirror = I386_DR_LOW_GET_STATUS ();
   +  inferior_ptid = saved_ptid;

      ALL_DEBUG_REGISTERS(i)
	{
   @@ -603,8 +615,16 @@ int
    i386_stopped_by_hwbp (void)
    {
      int i;
   +  ptid_t saved_ptid = inferior_ptid;
   +
   +  /* Debug status register is thread-specific.  A hardware breakpoint
   +     will cause a trap to occur.  Switch to trap ptid to get relevant 
   +     status for that thread.  */

   +  inferior_ptid = trap_ptid;
      dr_status_mirror = I386_DR_LOW_GET_STATUS ();

Did you verify that I386_DR_LOW_GET_STATUS() doesn't throw any errors?
Otherwise you'll need to use cleanup handlers here.

   +  inferior_ptid = saved_ptid;
   +
      if (maint_show_dr)
	i386_show_dr ("stopped_by_hwbp", 0, 0, hw_execute);

   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?

	       {
   -	      ptrace (PTRACE_DETACH, ourstatus->value.related_pid, 0, 0);
   -	      ourstatus->kind = TARGET_WAITKIND_IGNORE;
   -	      pid = -1;
   -	      save_errno = EINTR;
   +	      linux_handle_extended_wait (pid, status, ourstatus);
   +
   +	      /* 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)
   +		{ 
   +		  ptrace (PTRACE_DETACH, ourstatus->value.related_pid, 0, 0);
   +		  ourstatus->kind = TARGET_WAITKIND_IGNORE;
   +		  pid = -1;
   +		  save_errno = EINTR;
   +		}
	       }
	   }


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