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

Mark Kettenis
2004-06-11T22:00:31Z

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;
+ }
}
}