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

Mark Kettenis
2004-06-12T17:12:08Z

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.