Re: [PATCH] aarch64-linux-nat respin...

[Pedro Alves <palves at redhat dot com>]

Hey Marcus,

On 02/06/2013 06:42 PM, Marcus Shawcroft wrote:
> +
> +/* Print the values of the cached breakpoint/watchpoint registers.
> +   This is called when debug_hw_points is larger than 1.  To set
> +   that up, type "maint set debug-hw-points 2" at GDB's prompt.  */

"maint set show-debug-regs"?

> +
> +static void
> +aarch64_show_debug_reg_state (struct aarch64_debug_reg_state *state,
> +			      const char *func, CORE_ADDR addr,
> +			      int len, int type)
> +{


> +static void
> +aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
> +{
> +  struct arch_lwp_info *info = lwp->arch_private;
> +
> +  /* NULL means this is the main thread still going through the shell,
> +     or, no watchpoint has been set yet.  In that case, there's
> +     nothing to do.  */
> +  if (info == NULL)
> +    return;
> +
> +  if (DR_HAS_CHANGED (info->dr_changed_bp)
> +      || DR_HAS_CHANGED (info->dr_changed_wp))
> +    {
> +      int tid = GET_LWP (lwp->ptid);
> +      struct aarch64_debug_reg_state *state = aarch64_get_debug_reg_state ();

Hmm.  This is always fetching the debug_reg_state of
the current inferior, but may not be the inferior of lwp.
I see the same bug on x86.  Sorry about that.  I'll fix it.

> +  /* Get hardware watchpoint register info.  */
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_WATCH, &iov) == 0
> +      && AARCH64_DEBUG_ARCH (dreg_state.dbg_info) == AARCH64_DEBUG_ARCH_V8)
> +    {
> +      aarch64_num_wp_regs = AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info);
> +      if (aarch64_num_wp_regs > AARCH64_HBP_MAX_NUM)
> +	warning ("Unexpected number of hardware watchpoint registers reported"
> +		 " by ptrace, got %d, expected %d.",
> +		 aarch64_num_wp_regs, AARCH64_HBP_MAX_NUM);

I think this should set aarch64_num_wp_regs to AARCH64_HBP_MAX_NUM or
0 in the warning case.

> +{
> +  const struct target_desc *result = NULL;
> +
> +  initialize_tdesc_aarch64 ();
> +  result = tdesc_aarch64;
> +  return result;
> +}

Just:

  return tdesc_aarch64;

> +	  /* no break; continue hunting for an exising one.  */

Typo: existing.

> +/* Implement the "to_can_use_hw_breakpoint" target_ops method.  */
> +
> +static int
> +aarch64_linux_can_use_hw_breakpoint (int type, int cnt, int ot)
> +{
> +  if (type == bp_hardware_watchpoint || type == bp_read_watchpoint
> +      || type == bp_access_watchpoint || type == bp_watchpoint)
> +    {
> +      if (cnt + ot > aarch64_num_wp_regs)
> +	return -1;
> +    }
> +  else if (type == bp_hardware_breakpoint)
> +    {
> +      if (cnt > aarch64_num_bp_regs)
> +	return -1;

This conflicts with the debug register refcounting, because
there's no way to tell from those arguments whether the corresponding
watchpoints/breakpoints actually share the same debug registers.
See i386_can_use_hw_breakpoint.  (The resource accounting in core
gdb is weak, and I've advocated to remove it completely).

> +    }
> +  else
> +    {
> +      gdb_assert_not_reached ("unrecognized breakpoint/watchpoint type");
> +    }
> +
> +  return 1;
> +}
> +



> +  /* This must be a hardware breakpoint.  */
> +  if (siginfo.si_signo != SIGTRAP
> +      || (siginfo.si_code & 0xffff) != 0x0004 /* TRAP_HWBKPT */ )

That's not what I meant when I asked if this is still needed.
If there are released kernels that support AArch64, but don't have
the TRAP_HWBKPT define, then do define it at the top.
If not, then use it unconditionally.  Hardcoding the number is the
worse solution.  It's frequent that while doing target bring up
these compat defines are necessary, but in the end they can be removed,
hence the original question.

> +++ b/gdb/config/aarch64/linux.mh
> @@ -0,0 +1,9 @@
> +# Host: AArch64 based machine running GNU/Linux
> +

We've just recently asked for a copyright header be added
to a new .mh file (powerpc/fbsd.mh).  Can you add one here
too, please?

Otherwise this is looking very good now.

-- 
Pedro Alves