[PATCH 4/5] AArch64 GDB and GDBSERVER Port V2

[Joel Brobecker]

> 2012-11-21  Jim MacArthur  <jim.macarthur@arm.com>
>             Marcus Shawcroft  <marcus.shawcroft@arm.com>
>             Nigel Stephens  <nigel.stephens@arm.com>
>             Yufeng Zhang  <yufeng.zhang@arm.com>
> 
>         * aarch64-linux-nat.c: New file.
>         * config/aarch64/linux.mh: New file.

As discussed in patch #1, the configure.host patch needs to be moved
here.

> +static void
> +aarch64_align_watchpoint (CORE_ADDR addr, int len, CORE_ADDR *aligned_addr_p,
> +			  int *aligned_len_p, CORE_ADDR *next_addr_p,
> +			  int *next_len_p)

This function needs documentation.

> +  return;
> +}

Unnecessary call to return at end of function.

> +struct aarch64_linux_hw_breakpoint
> +{
> +  /* Address to break on, or being watched.  */
> +  CORE_ADDR address;
> +  /* Control register for break-/watch- point.  */
> +  aarch64_hwbp_control_t control;
> +};
> +
> +struct aarch64_linux_hwbp_cap
> +{
> +  int bp_count;
> +  int wp_count;
> +  int max_wp_length;
> +};

Please document these two structures, including their various
fields (for the second one).

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

Empty line after variable declaration.

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

Same here.

> +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 (FALSE);
> +      return -1;

Use gdb_assert_not_reached or internal_error.

> +/* Initialize the hardware breakpoint control register value.  */
> +
> +static aarch64_hwbp_control_t
> +aarch64_hwbp_control_initialize (unsigned byte_address_select,
> +				 aarch64_hwbp_type hwbp_type,
> +				 int enable)
> +{
> +  gdb_assert ((byte_address_select & ~0xffU) == 0);
> +  gdb_assert (hwbp_type != aarch64_hwbp_break
> +	      || ((byte_address_select & 0xfU) != 0));
> +
> +  return (byte_address_select << 5) | (hwbp_type << 3) | (3 << 1) | enable;
> +}

The description and function name seem odd considering the
implementation (it does not seem to intialize anything).

> +static struct aarch64_linux_thread_points *
> +aarch64_linux_find_breakpoints_by_tid (int tid, int alloc_new)

This function needs some documentation.

> +static int
> +aarch64_hwbp_control_is_enabled (aarch64_hwbp_control_t control)

Same.

> +  if (ptrace (PTRACE_SETREGSET, tid,
> +	      watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
> +	      (void *)&iov))
                    ^^^ missing space after ')'

> +      const unsigned len = aarch64_watchpoint_length (pts->wpts[i].control);
> +      const CORE_ADDR addr_trap = (CORE_ADDR) siginfo.si_addr;
> +      const CORE_ADDR addr_watch = pts->wpts[i].address;
> +      if (aarch64_hwbp_control_is_enabled (pts->wpts[i].control)

Empty line after variable declarations.

> +static int
> +aarch64_linux_stopped_by_watchpoint (void)
> +{
> +  CORE_ADDR addr;
> +  return aarch64_linux_stopped_data_address (&current_target, &addr);

Same here...

-- 
Joel