Re: [rfc, gdbserver] Support hardware watchpoints on ARM

Pedro Alves
2011-09-21T14:20:15Z

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