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

[Pedro Alves <pedro@codesourcery.com>]

Hi Ulrich,

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.

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?

> +    }


> +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?

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.

-- 
Pedro Alves