From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18705 invoked by alias); 21 Sep 2011 14:20:15 -0000 Received: (qmail 18683 invoked by uid 22791); 21 Sep 2011 14:20:13 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,TW_HW X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 21 Sep 2011 14:19:55 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1R6NeU-0007Pn-Jj from pedro_alves@mentor.com ; Wed, 21 Sep 2011 07:19:54 -0700 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Wed, 21 Sep 2011 15:19:49 +0100 From: Pedro Alves To: "Ulrich Weigand" Subject: Re: [rfc, gdbserver] Support hardware watchpoints on ARM Date: Wed, 21 Sep 2011 14:26:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.0; x86_64; ; ) Cc: gdb-patches@sourceware.org, patches@linaro.org References: <201109212057.p8LKvAjb019685@d06av02.portsmouth.uk.ibm.com> In-Reply-To: <201109212057.p8LKvAjb019685@d06av02.portsmouth.uk.ibm.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201109211519.47648.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2011-09/txt/msg00384.txt.bz2 On Wednesday 21 September 2011 14:57:15, Ulrich Weigand wrote: > Pedro Alves wrote: > > > I was just looking over the patch before lunch, and > > meanwhile you've committed it. :-) It looks fine to me in any > > case. :-) I just had a couple minor remarks. > > Oops, sorry. Thanks for looking over it! NP! > > > On Monday 12 September 2011 18:23:00, Ulrich Weigand wrote: > > > + if (hwbp_type == arm_hwbp_break) > > > + { > > > + /* For breakpoints, the length field encodes the mode. */ > > > + switch (len) > > > + { > > > + case 2: /* 16-bit Thumb mode breakpoint */ > > > + case 3: /* 32-bit Thumb mode breakpoint */ > > > + mask = 0x3 << (addr & 2); > > > + break; > > > + case 4: /* 32-bit ARM mode breakpoint */ > > > + mask = 0xf; > > > + break; > > > + default: > > > + /* Unsupported. */ > > > + return -1; > > > + } > > > + > > > + addr &= ~3; > > > > Is this ~3 correct for 16-bit Thumb? > > Yes, it is. The address value must always have its two low bits > clear. For Thumb, the selection of which of the two halfwords the > breakpoint is to apply to is done via control bits (that's what > the "mask" value is about). > > > > +static void > > > +arm_prepare_to_resume (struct lwp_info *lwp) > > > +{ > > > + int pid = lwpid_of (lwp); > > > + struct process_info *proc = find_process_pid (pid_of (lwp)); > > > + struct arch_process_info *proc_info = proc->private->arch_private; > > > + struct arch_lwp_info *lwp_info = lwp->arch_private; > > > + int i; > > > + > > > + for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++) > > > > It's a bit unfortunate that arm_linux_get_hw_breakpoint_count > > relies on the current_inferior global having been set to LWP by > > the callers. We try to avoid that when we have an LWP handy. > > Can we make arm_linux_get_hw_breakpoint_count take an LWP argument? > > Well, since this is global system property that is actually only > queried once and then returned from a cache, adding a LWP argument > would appear to be somewhat misleading ... We can always just document what the argument means :-) In this case, it'd serve as currently stopped LWP to run ptrace on in case the cache is not set yet. You'd pass that down to arm_linux_get_hwbp_cap similarly: static const struct arm_linux_hwbp_cap * arm_linux_get_hwbp_cap (struct lwp_info *lwp) > +/* Get hold of the Hardware Breakpoint information for the target we are > + attached to. Returns NULL if the kernel doesn't support Hardware > + breakpoints at all, or a pointer to the information structure. */ > +static const struct arm_linux_hwbp_cap * > +arm_linux_get_hwbp_cap (void) > +{ > + /* The info structure we return. */ > + static struct arm_linux_hwbp_cap info; > + > + /* Is INFO in a good state? -1 means that no attempt has been made to > + initialize INFO; 0 means an attempt has been made, but it failed; 1 > + means INFO is in an initialized state. */ > + static int available = -1; > + > + if (available == -1) > + { > + int pid = lwpid_of (get_thread_lwp (current_inferior)); > + unsigned int val; > + > + if (ptrace (PTRACE_GETHBPREGS, pid, 0, &val) < 0) > + available = 0; ... because otherwise, if the callers change, this current_inferior here can end up pointing to a running LWP (non-stop mode, for example), or worse, to NULL. So whenever we have a function that takes an LWP as argument that calls into other functions that assume the current_inferior is already set as we want, we either make the first function (the one with the LWP arg) make sure to save/restore current_inferior itself, or change the callees to take an LWP as argument as well (and don't rely on global state). Anyway, no biggie. Just saying that in case I ever break these functions' assumptions. :-) -- Pedro Alves