From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20966 invoked by alias); 11 Jun 2004 22:00:31 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 20949 invoked from network); 11 Jun 2004 22:00:29 -0000 Received: from unknown (HELO walton.kettenis.dyndns.org) (213.93.77.109) by sourceware.org with SMTP; 11 Jun 2004 22:00:29 -0000 Received: from elgar.kettenis.dyndns.org (elgar.kettenis.dyndns.org [192.168.0.2]) by walton.kettenis.dyndns.org (8.12.6p3/8.12.6) with ESMTP id i5BM0KrG000257; Sat, 12 Jun 2004 00:00:20 +0200 (CEST) (envelope-from kettenis@elgar.kettenis.dyndns.org) Received: from elgar.kettenis.dyndns.org (localhost [127.0.0.1]) by elgar.kettenis.dyndns.org (8.12.6p3/8.12.6) with ESMTP id i5BM0Koh000388; Sat, 12 Jun 2004 00:00:20 +0200 (CEST) (envelope-from kettenis@elgar.kettenis.dyndns.org) Received: (from kettenis@localhost) by elgar.kettenis.dyndns.org (8.12.6p3/8.12.6/Submit) id i5BM0JBA000385; Sat, 12 Jun 2004 00:00:19 +0200 (CEST) Date: Fri, 11 Jun 2004 22:00:00 -0000 Message-Id: <200406112200.i5BM0JBA000385@elgar.kettenis.dyndns.org> From: Mark Kettenis To: jjohnstn@redhat.com CC: gdb-patches@sources.redhat.com In-reply-to: <40CA20B0.4060106@redhat.com> (message from Jeff Johnston on Fri, 11 Jun 2004 17:14:24 -0400) Subject: Re: [RFC]: x86 threaded watchpoint support [1/3] References: <40CA20B0.4060106@redhat.com> X-SW-Source: 2004-06/txt/msg00281.txt.bz2 Date: Fri, 11 Jun 2004 17:14:24 -0400 From: Jeff Johnston 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; + } } }