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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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!

> 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 ...

(In this particular routine, we actually don't really have to
query the exact hardware count.  We might just as well do the
loop all the way through MAX_BPTS ...)

> On Monday 12 September 2011 18:23:00, Ulrich Weigand wrote:
> > +       if (arm_hwbp_control_is_enabled (proc_info->bpts[i].control))
> > +         if (ptrace (PTRACE_SETHBPREGS, pid, ((i << 1) + 1),
> > +             &proc_info->bpts[i].address) < 0)
> > +           error (_("Unexpected error setting breakpoint address"));
> > +
> > +       if (arm_hwbp_control_is_initialized (proc_info->bpts[i].control))
> > +         if (ptrace (PTRACE_SETHBPREGS, pid, ((i << 1) + 2),
> > +             &proc_info->bpts[i].control) < 0)
> > +           error (_("Unexpected error setting breakpoint"));
> 
> I think perror_with_name would be better, so we can see on the logs
> the errno ptrace set on error.

Agreed, I'll change this.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com