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

[Pedro Alves <palves at redhat dot com>]

On 01/22/2013 06:11 PM, Marcus Shawcroft wrote:

> +#include "gregset.h"
> +
> +#include "features/aarch64.c"
> +
> +#define TRAP_HWBKPT 0x0004

Same comment as GDBserver.

> +static void
> +fetch_gregs_from_thread (struct regcache *regcache)
> +{
> +  int ret, regno, tid;
> +  elf_gregset_t regs;
> +  struct iovec iovec;
> +
> +  tid = get_thread_id (inferior_ptid);
> +
> +  iovec.iov_base = &regs;
> +  iovec.iov_len = sizeof (regs);
> +
> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, &iovec);
> +  if (ret < 0)
> +    {
> +      perror_with_name (_("Unable to fetch general registers."));
> +      return;

return after perror -> dead code.

> +static void
> +store_gregs_to_thread (const struct regcache *regcache)
> +{
> +  int ret, regno, tid;
> +  elf_gregset_t regs;
> +  struct iovec iovec;
> +
> +  tid = get_thread_id (inferior_ptid);
> +
> +  iovec.iov_base = &regs;
> +  iovec.iov_len = sizeof (regs);
> +
> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_PRSTATUS, &iovec);
> +  if (ret < 0)
> +    {
> +      perror_with_name (_("Unable to fetch general registers."));
> +      return;
> +    }
> +
> +  for (regno = AARCH64_X0_REGNUM; regno <= AARCH64_CPSR_REGNUM; regno++)
> +    if (REG_VALID == regcache_register_status (regcache, regno))
> +      regcache_raw_collect (regcache, regno,
> +			    (char *) &regs[regno - AARCH64_X0_REGNUM]);
> +
> +  ret = ptrace (PTRACE_SETREGSET, tid, NT_PRSTATUS, &iovec);
> +  if (ret < 0)
> +    {
> +      perror_with_name (_("Unable to store general registers."));
> +      return;

Ditto.  There are more instances, but I won't call them out
explicitly.

> +/* Implement the "to_read_description" target_ops method.  */
> +
> +static const struct target_desc *
> +aarch64_linux_read_description (struct target_ops *ops)
> +{
> +  CORE_ADDR hwcap = 0;
> +  const struct target_desc *result = NULL;
> +
> +  if (target_auxv_search (ops, AT_HWCAP, &hwcap) != 1)
> +    return NULL;

Is this just a placeholder, or did you mean to use hwcap?
Doesn't seem to be much point in this if not going to be
used.

> +
> +  initialize_tdesc_aarch64 ();
> +  result = tdesc_aarch64;
> +  return result;
> +}
> +


> +      if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_BREAK, (void *)&iov) == 0

Space after cast missing.

> +	  && AARCH64_DEBUG_ARCH (regs.dbg_info) == AARCH64_DEBUG_ARCH_V8)
> +	info.bp_count = AARCH64_DEBUG_NUM_SLOTS (regs.dbg_info);
> +      else
> +	return NULL;
> +
> +      if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_WATCH, (void *)&iov) == 0

Ditto.

> +	  && AARCH64_DEBUG_ARCH (regs.dbg_info) == AARCH64_DEBUG_ARCH_V8)
> +	info.wp_count = AARCH64_DEBUG_NUM_SLOTS (regs.dbg_info);
> +      else
> +	return NULL;
> +

Hmm, surprised to see this code is not equivalent to gdbserver's.

> +      info.max_wp_length = AARCH64_HWP_MAX_LEN_PER_REG;
> +      available = 1;
> +    }
> +
> +  return available == 1 ? &info : NULL;
> +}
> +
> +/* How many hardware breakpoints are available?  */
> +
> +static int
> +aarch64_linux_get_hw_breakpoint_count (void)
> +{
> +  const struct aarch64_linux_hwbp_cap *cap = aarch64_linux_get_hwbp_cap ();
> +
> +  return cap != NULL ? cap->bp_count : 0;
> +}
> +
> +/* How many hardware watchpoints are available?  */
> +
> +static int
> +aarch64_linux_get_hw_watchpoint_count (void)
> +{
> +  const struct aarch64_linux_hwbp_cap *cap = aarch64_linux_get_hwbp_cap ();
> +
> +  return cap != NULL ? cap->wp_count : 0;
> +}
> +
> +/* 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_linux_get_hw_watchpoint_count ())
> +	return -1;
> +    }
> +  else if (type == bp_hardware_breakpoint)
> +    {
> +      if (cnt > aarch64_linux_get_hw_breakpoint_count ())
> +	return -1;
> +    }
> +  else
> +    {
> +      gdb_assert_not_reached ("unrecognized breakpoint/watchpoint type");
> +      return -1;

return after gdb_assert is dead code.

Did you consider refcounting the debug registers usage, like x86,
so you can save debug registers with watchpoints that have
overlapping regions?

> +static inline unsigned int
> +aarch64_watchpoint_length (unsigned int ctrl)
> +{
> +  switch ((ctrl >> 5) & 0xff)
> +    {
> +    case 0x01: return 1; break;
> +    case 0x03: return 2; break;
> +    case 0x0f: return 4; break;
> +    case 0xff: return 8; break;
> +    default: return 0;

Same comment as in gdbserver.

> +static void
> +aarch64_linux_remove_hw_breakpoint1
> +  (const struct aarch64_linux_hw_breakpoint *bpt, int tid, int watchpoint)
> +{
> +  struct aarch64_linux_thread_points *t =
> +    aarch64_linux_find_breakpoints_by_tid (tid, 0);

= goes at beginning of new line.

> +/* Remove a hardware breakpoint.  */
> +
> +static int
> +aarch64_linux_remove_hw_breakpoint (struct gdbarch *gdbarch,
> +				    struct bp_target_info *bp_tgt)
> +{
> +  struct lwp_info *lp;
> +  struct aarch64_linux_hw_breakpoint p;
> +
> +  if (aarch64_linux_get_hw_breakpoint_count () == 0)
> +    return -1;
> +
> +  aarch64_linux_hw_breakpoint_initialize (gdbarch, bp_tgt, &p);
> +  ALL_LWPS (lp)
> +    aarch64_linux_remove_hw_breakpoint1 (&p, TIDGET (lp->ptid), 0);

This (and other similar loops in the file) ignore multi-inferior/process
completely.

It's also not as modern looking as the gdbserver patch.  :-(  :-/

I think it'd be good to update this patch to delay updating the
debug registers until the thread is resumed, like in the
gdbserver side.  It doesn't make much sense to introduce a new port
that does things the old way.  It saves ptrace calls, and, we introduced
that mechanism for non-stop mode, so you'll want it sooner or later.
The code ends up looking a lot like gdbserver's, so it shouldn't
be a difficult change.  Also see the i386 GNU/Linux port
(e.g., start at i386-linux-nat.c:update_debug_registers_callback)
for a GDB side example.

If you end up swearing at gdb's and gdbserver's duplication, by all
means, share the watchpoints code between them (put it in common/).
That's the direction we're heading, though no port is doing that
for watchpoints yet -- this is not a requirement for that reason.

-- 
Pedro Alves