* [RFC]: x86 threaded watchpoint support [1/3]
@ 2004-06-11 21:14 Jeff Johnston
2004-06-11 22:00 ` Mark Kettenis
2004-06-12 9:35 ` Eli Zaretskii
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Johnston @ 2004-06-11 21:14 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]
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.
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.
With this change plus a change to breakpoint.c, threaded watchpoints work on the
x86. Part 2 will be the breakpoint.c change which is not x86-specific. Part 3
will be a test case when I figure out how to code it properly. If you want to
test it with the two patches applied, just debug gdb.threads/schedlock and issue:
b main
run
watch args[0]
watch args[1]
continue
continue
continue
etc...
Ok?
-- Jeff J.
2004-06-11 Jeff Johnston <jjohnstn@redhat.com>
* i386-linux-nat.c (i386_linux_dr_get): Use target_get_lwp
function to get lwp of current ptid to use in the PTRACE
call.
* i386-nat.c: Add comments regarding the DR status register.
(i386_stopped_data_address): Switch to the trap_ptid before
getting the dr_status register.
(i386_stopped_by_hwbp): Ditto.
* lin-lwp.c (child_wait): Set the trap_ptid when appropriate.
* target.h (struct target ops): Add new function to_get_lwp.
(target_get_lwp): New macro.
* target.c (INHERIT): Inherit to_get_lwp.
(update_current_target): Default to_get_lwp to ptid_get_lwp.
(init_dummy_target): Ditto.
* thread-db.c (thread_db_get_lwp): New function.
(thread_db_init): Expose thread_db_get_lwp in target vector.
[-- Attachment #2: x86-watchpoint.patch1 --]
[-- Type: text/plain, Size: 8214 bytes --]
Index: i386-linux-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-linux-nat.c,v
retrieving revision 1.55
diff -u -p -r1.55 i386-linux-nat.c
--- i386-linux-nat.c 9 Apr 2004 16:31:01 -0000 1.55
+++ i386-linux-nat.c 11 Jun 2004 20:55:28 -0000
@@ -649,10 +649,11 @@ i386_linux_dr_get (int regnum)
int tid;
unsigned long value;
- /* FIXME: kettenis/2001-01-29: It's not clear what we should do with
- multi-threaded processes here. For now, pretend there is just
- one thread. */
- tid = PIDGET (inferior_ptid);
+ /* The debug status register is thread-specific and so we should
+ use the current lwp when fetching the debug registers. */
+ tid = target_get_lwp (inferior_ptid);
+ if (tid == 0)
+ tid = PIDGET (inferior_ptid); /* Not a threaded program. */
/* FIXME: kettenis/2001-03-27: Calling perror_with_name if the
ptrace call fails breaks debugging remote targets. The correct
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;
+
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 ();
+ 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)
{
- 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: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.75
diff -u -p -r1.75 target.c
--- target.c 9 Jun 2004 20:09:42 -0000 1.75
+++ target.c 11 Jun 2004 20:55:28 -0000
@@ -422,6 +422,7 @@ update_current_target (void)
INHERIT (to_notice_signals, t);
INHERIT (to_thread_alive, t);
INHERIT (to_find_new_threads, t);
+ INHERIT (to_get_lwp, t);
INHERIT (to_pid_to_str, t);
INHERIT (to_extra_thread_info, t);
INHERIT (to_stop, t);
@@ -601,6 +602,9 @@ update_current_target (void)
de_fault (to_find_new_threads,
(void (*) (void))
target_ignore);
+ de_fault (to_get_lwp,
+ (long (*) (ptid_t))
+ ptid_get_lwp);
de_fault (to_extra_thread_info,
(char *(*) (struct thread_info *))
return_zero);
@@ -1616,6 +1620,7 @@ init_dummy_target (void)
dummy_target.to_find_memory_regions = dummy_find_memory_regions;
dummy_target.to_make_corefile_notes = dummy_make_corefile_notes;
dummy_target.to_xfer_partial = default_xfer_partial;
+ dummy_target.to_get_lwp = ptid_get_lwp;
dummy_target.to_magic = OPS_MAGIC;
}
\f
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.60
diff -u -p -r1.60 target.h
--- target.h 7 Jun 2004 17:58:32 -0000 1.60
+++ target.h 11 Jun 2004 20:55:29 -0000
@@ -370,6 +370,7 @@ struct target_ops
void (*to_notice_signals) (ptid_t ptid);
int (*to_thread_alive) (ptid_t ptid);
void (*to_find_new_threads) (void);
+ long (*to_get_lwp) (ptid_t ptid);
char *(*to_pid_to_str) (ptid_t);
char *(*to_extra_thread_info) (struct thread_info *);
void (*to_stop) (void);
@@ -815,6 +816,10 @@ extern void target_load (char *arg, int
#define target_find_new_threads() \
(*current_target.to_find_new_threads) (); \
+/* Get the lwp for a thread. */
+#define target_get_lwp(ptid) \
+ (*current_target.to_get_lwp) (ptid); \
+
/* Make target stop in a continuable fashion. (For instance, under
Unix, this should act like SIGSTOP). This function is normally
used by GUIs to implement a stop button. */
Index: thread-db.c
===================================================================
RCS file: /cvs/src/src/gdb/thread-db.c,v
retrieving revision 1.42
diff -u -p -r1.42 thread-db.c
--- thread-db.c 7 Jun 2004 22:35:55 -0000 1.42
+++ thread-db.c 11 Jun 2004 20:55:29 -0000
@@ -408,6 +408,22 @@ lwp_from_thread (ptid_t ptid)
}
\f
+static long
+thread_db_get_lwp (ptid_t ptid)
+{
+ if (!is_thread (ptid))
+ return ptid.pid;
+ else
+ {
+ struct thread_info *thread_info = find_thread_pid (ptid);
+ if (!thread_info)
+ return 0;
+
+ thread_db_get_info (thread_info);
+ return thread_info->private->ti.ti_lid;
+ }
+}
+
void
thread_db_init (struct target_ops *target)
{
@@ -1382,6 +1398,7 @@ init_thread_db_ops (void)
thread_db_ops.to_mourn_inferior = thread_db_mourn_inferior;
thread_db_ops.to_thread_alive = thread_db_thread_alive;
thread_db_ops.to_find_new_threads = thread_db_find_new_threads;
+ thread_db_ops.to_get_lwp = thread_db_get_lwp;
thread_db_ops.to_pid_to_str = thread_db_pid_to_str;
thread_db_ops.to_stratum = thread_stratum;
thread_db_ops.to_has_thread_control = tc_schedlock;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]: x86 threaded watchpoint support [1/3]
2004-06-11 21:14 [RFC]: x86 threaded watchpoint support [1/3] Jeff Johnston
@ 2004-06-11 22:00 ` Mark Kettenis
2004-06-11 22:46 ` Daniel Jacobowitz
2004-06-12 9:35 ` Eli Zaretskii
1 sibling, 1 reply; 6+ messages in thread
From: Mark Kettenis @ 2004-06-11 22:00 UTC (permalink / raw)
To: jjohnstn; +Cc: gdb-patches
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;
+ }
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]: x86 threaded watchpoint support [1/3]
2004-06-11 22:00 ` Mark Kettenis
@ 2004-06-11 22:46 ` Daniel Jacobowitz
2004-06-12 17:12 ` Mark Kettenis
2004-06-14 18:28 ` Jeff Johnston
0 siblings, 2 replies; 6+ messages in thread
From: Daniel Jacobowitz @ 2004-06-11 22:46 UTC (permalink / raw)
To: Mark Kettenis; +Cc: jjohnstn, gdb-patches
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]: x86 threaded watchpoint support [1/3]
2004-06-11 21:14 [RFC]: x86 threaded watchpoint support [1/3] Jeff Johnston
2004-06-11 22:00 ` Mark Kettenis
@ 2004-06-12 9:35 ` Eli Zaretskii
1 sibling, 0 replies; 6+ messages in thread
From: Eli Zaretskii @ 2004-06-12 9:35 UTC (permalink / raw)
To: Jeff Johnston; +Cc: gdb-patches
> Date: Fri, 11 Jun 2004 17:14:24 -0400
> From: Jeff Johnston <jjohnstn@redhat.com>
>
> /* 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];
If the DR_STATUS register is thread specific, I think the mirroring
should be redesigned to support that out of the box. The solution you
suggest sounds like working around the existing code, but there's no
reason to do that, IMHO.
> +extern ptid_t trap_ptid;
> +extern ptid_t inferior_ptid;
As Mark points out, this will break any x86 target that doesn't
define these in its source files. So this solution is not a good
one. A better solution would be to define target-specific accessor
functions that would return these values.
> + 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;
Yikes! Why not redesign I386_DR_LOW_GET_STATUS so that it could
accept the required thread id directly?
Bottom line: I think we need to redesign the low-level watchpoint
support for x86 so that it supports per-thread hardware watchpoints.
(I actually raised this issue back when the current code was designed
and was told to disregard it. Now it came back to haunt us.)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]: x86 threaded watchpoint support [1/3]
2004-06-11 22:46 ` Daniel Jacobowitz
@ 2004-06-12 17:12 ` Mark Kettenis
2004-06-14 18:28 ` Jeff Johnston
1 sibling, 0 replies; 6+ messages in thread
From: Mark Kettenis @ 2004-06-12 17:12 UTC (permalink / raw)
To: drow; +Cc: jjohnstn, gdb-patches
Date: Fri, 11 Jun 2004 18:45:59 -0400
From: 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.
Well, at the point that I wrote the GNU/Linux debug register support
functions, it wasn't clear to me whether the debug registers were
per-vm-space or per-process. If they're per-process, I can go along
with your reasoning. If Jeffs code really works (and I have no reason
to believe that it doesn't), it appears that the debug registers are
indeed per-process right now. But has that always been the case?
> 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.
So we have a layering violation. Is it possible to fix that? TIDGET
does work in the fetch_registers case. Instead of adding a function
to the target vector that converts a thread ID into an LWP, we should
probably model the hardware breakpoint code more along the lines of
fetch_register. It might be time to simply add the debug registers to
GDB's register cache.
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.
Well at least this threaded watchpoint support seems to asume the
1:1-threading model that's dominant on GNU/Linux.
> 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.
Ah sorry. The kernel/glibc headers don't define a proper constant or
macro for this extended waitstatus?
I must say that I'm not thrilled by having trap_ptid leak out into yet
more files. It's an extremely underspecified interface.
Worse it's not a real interface at all, and GNU/Linux-specific. Using
it is simply unacceptable.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC]: x86 threaded watchpoint support [1/3]
2004-06-11 22:46 ` Daniel Jacobowitz
2004-06-12 17:12 ` Mark Kettenis
@ 2004-06-14 18:28 ` Jeff Johnston
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Johnston @ 2004-06-14 18:28 UTC (permalink / raw)
To: Daniel Jacobowitz; +Cc: Mark Kettenis, gdb-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-06-14 18:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2004-06-11 21:14 [RFC]: x86 threaded watchpoint support [1/3] Jeff Johnston
2004-06-11 22:00 ` Mark Kettenis
2004-06-11 22:46 ` Daniel Jacobowitz
2004-06-12 17:12 ` Mark Kettenis
2004-06-14 18:28 ` Jeff Johnston
2004-06-12 9:35 ` Eli Zaretskii
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox