From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12907 invoked by alias); 7 Feb 2014 15:32:56 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 12892 invoked by uid 89); 7 Feb 2014 15:32:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Feb 2014 15:32:54 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s17FWpsL019028 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 7 Feb 2014 10:32:51 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s17FWoFu003065; Fri, 7 Feb 2014 10:32:50 -0500 Message-ID: <52F4FCA1.7000308@redhat.com> Date: Fri, 07 Feb 2014 15:32:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Omair Javaid CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Support for HWbreak/watchpoint accross fork/vfork on arm-native References: <1391119790-6580-1-git-send-email-omair.javaid@linaro.org> <52EAD0DC.506@linaro.org> In-Reply-To: <52EAD0DC.506@linaro.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-02/txt/msg00204.txt.bz2 On 01/30/2014 10:23 PM, Omair Javaid wrote: > Accidentally missed the patch description and changelog: > > This patch updates arm native support for hardware breakpoints and watchpoints > to accommodate watchpoint/breakpoints across fork/vfork. Previously for arm > native a record of breakpoints at thread level was being kept which has been > changed to a process level record to come in sync with gdb-linux calls to > threads and process creation/destruction hooks. Thanks. This also changes the hwbreak/watchpoint insertion mechanism to the modern way of marking debug registers as needing update, but only really updating on resume, which is necessary for supporting watchpoints in non-stop mode (the current code tries to poke at running threads with ptrace, which fails). Please update the commit log to mention this. The patch looks overall mostly good, but I have a few comments below. > --- > gdb/arm-linux-nat.c | 417 ++++++++++++++++++++++++++++++++++------------------ > 1 file changed, 273 insertions(+), 144 deletions(-) > > diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c > index 6f56634..9a756c1 100644 > --- a/gdb/arm-linux-nat.c > +++ b/gdb/arm-linux-nat.c > @@ -787,70 +787,120 @@ struct arm_linux_hw_breakpoint > arm_hwbp_control_t control; > }; > > -/* Structure containing arrays of the break and watch points which are have > - active in each thread. > - > - The Linux ptrace interface to hardware break-/watch-points presents the > - values in a vector centred around 0 (which is used fo generic information). > - Positive indicies refer to breakpoint addresses/control registers, negative > - indices to watchpoint addresses/control registers. > - > - The Linux vector is indexed as follows: > - -((i << 1) + 2): Control register for watchpoint i. > - -((i << 1) + 1): Address register for watchpoint i. > - 0: Information register. > - ((i << 1) + 1): Address register for breakpoint i. > - ((i << 1) + 2): Control register for breakpoint i. Why remove this ptrace comment? It looks quite useful to me. > - > - This structure is used as a per-thread cache of the state stored by the > - kernel, so that we don't need to keep calling into the kernel to find a > - free breakpoint. > - > - We treat break-/watch-points with their enable bit clear as being deleted. > - */ > -typedef struct arm_linux_thread_points > +/* Since we cannot dynamically allocate subfields of arm_linux_process_info, > + assume a maximum number of supported break-/watchpoints. */ > +#define MAX_BPTS 16 > +#define MAX_WPTS 16 I saw no assertion making sure these maximums aren't overflowed. see aarch64_linux_get_debug_reg_capacity. > + /* Linked list. */ > + struct arm_linux_process_info *next; > + /* The process identifier. */ > + pid_t pid; > + /* Hardware breakpoints for this process. */ > + struct arm_linux_hw_breakpoint bpts[MAX_BPTS]; > + /* Hardware watchpoints for this process. */ > + struct arm_linux_hw_breakpoint wpts[MAX_WPTS]; It'd be a little cleaner if these two array were moved to a separate structure, like e.g., in the aarch64 port. > +/* Get hardware breakpoint state for process PID. */ > > - return t; > +static struct arm_linux_hw_breakpoint * > +arm_linux_get_hwbreak_state (pid_t pid) > +{ > + return arm_linux_process_info_get (pid)->bpts; > +} > + > +/* Get watchpoint state for process PID. */ > + > +static struct arm_linux_hw_breakpoint * > +arm_linux_get_watch_state (pid_t pid) > +{ > + return arm_linux_process_info_get (pid)->wpts; > } Then these two could be a single: static struct arm_linux_hw_breakpoint * arm_linux_get_point_state (pid_t pid) { return &arm_linux_process_info_get (pid)->state; } Users could then do: arm_linux_get_point_state (pid)->wpts; arm_linux_get_point_state (pid)->bpts; > +static int > +update_registers_callback (struct lwp_info *lwp, void *arg) > +{ > + struct update_registers_data *data = (struct update_registers_data *) arg; > + > + /* Force iterate_over_lwps to return matched lwp_info*. */ > + if (arg == NULL) > + return 1; I don't understand this. It seems nothing passes a NULL arg. > + > + if (lwp->arch_private == NULL) > + lwp->arch_private = XCNEW (struct arch_lwp_info); > + > + /* The actual update is done later just before resuming the lwp, > + we just mark that the registers need updating. */ > + if (data->watch) > + lwp->arch_private->wpts_changed[data->index] = 1; > + else > + lwp->arch_private->bpts_changed[data->index] = 1; > + > + /* If the lwp isn't stopped, force it to momentarily pause, so > + we can update its breakpoint registers. */ > + if (!lwp->stopped) > + linux_stop_lwp (lwp); > + > + return 0; > +} > if (watchpoint) > { > count = arm_linux_get_hw_watchpoint_count (); > - bpts = t->wpts; > - dir = -1; > + bpts = arm_linux_get_watch_state(pid); Space before parens. > } > else > { > count = arm_linux_get_hw_breakpoint_count (); > - bpts = t->bpts; > - dir = 1; > + bpts = arm_linux_get_hwbreak_state(pid); Ditto. > } > if (watchpoint) > { > count = arm_linux_get_hw_watchpoint_count (); > - bpts = t->wpts; > - dir = -1; > + bpts = arm_linux_get_watch_state(pid); Ditto. > } > else > { > count = arm_linux_get_hw_breakpoint_count (); > - bpts = t->bpts; > - dir = 1; > + bpts = arm_linux_get_hwbreak_state(pid); Ditto. > } > > +arm_linux_prepare_to_resume (struct lwp_info *lwp) > { ... > + for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++) > + if (arm_lwp_info->bpts_changed[i]) > + { > + errno = 0; > + pid = ptid_get_lwp (lwp->ptid); > + bpts = arm_linux_get_hwbreak_state (ptid_get_pid (lwp->ptid)); This should probably be moved out of the loop. As is it's doing a linear lookup per iteration. > - } > + if (arm_hwbp_control_is_enabled (bpts[i].control)) > + if (ptrace (PTRACE_SETHBPREGS, pid, > + (PTRACE_TYPE_ARG3) ((i << 1) + 1), &bpts[i].address) < 0) > + perror_with_name ("Unexpected error setting breakpoint address"); Missing _() for i18n. The old code had it. > > - if (t == NULL) > - return; > + if (bpts[i].control != 0) > + if (ptrace (PTRACE_SETHBPREGS, pid, > + (PTRACE_TYPE_ARG3) ((i << 1) + 2), &bpts[i].control) < 0) > + perror_with_name ("Unexpected error setting breakpoint"); Ditto. > > - VEC_unordered_remove (arm_linux_thread_points_p, arm_threads, i); > + arm_lwp_info->bpts_changed[i] = 0; > + } > > - xfree (t->bpts); > - xfree (t->wpts); > - xfree (t); > - } > + for (i = 0; i < arm_linux_get_hw_watchpoint_count (); i++) > + if (arm_lwp_info->wpts_changed[i]) > + { > + errno = 0; > + pid = ptid_get_lwp (lwp->ptid); > + wpts = arm_linux_get_watch_state (ptid_get_pid (lwp->ptid)); Likewise as comment above. > + > + if (arm_hwbp_control_is_enabled (wpts[i].control)) > + if (ptrace (PTRACE_SETHBPREGS, pid, > + (PTRACE_TYPE_ARG3) -((i << 1) + 1), &wpts[i].address) < 0) > + perror_with_name ("Unexpected error setting watchpoint address"); Ditto, etc. > +static void > +arm_linux_new_fork (struct lwp_info *parent, pid_t child_pid) > +{ > + parent_pid = ptid_get_pid (parent->ptid); > + parent_point_state = arm_linux_get_hwbreak_state (parent_pid); > + child_point_state = arm_linux_get_hwbreak_state (child_pid); > + memcpy (child_point_state, parent_point_state, > + sizeof (struct arm_linux_hw_breakpoint) * MAX_BPTS); > + > + parent_point_state = arm_linux_get_watch_state (parent_pid); > + child_point_state = arm_linux_get_watch_state (child_pid); > + memcpy (child_point_state, parent_point_state, > + sizeof (struct arm_linux_hw_breakpoint) * MAX_BPTS); The latter should be MAX_WPTS. But with the earlier proposed change, these two would simply turn into a single: parent_pid = ptid_get_pid (parent->ptid); parent_point_state = arm_linux_get_point_state (parent_pid); child_point_state = arm_linux_get_point_state (child_pid); *child_point_state = *parent_point_state;