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

[Pedro Alves <pedro@codesourcery.com>]

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