Re: [PATCH] AArch64 GDBSERVER

[Pedro Alves <palves at redhat dot com>]

First off, thanks.  This look really good.  It uses all
the good modern mechanisms, which makes me quite happy with it.

A few minor remarks follow, otherwise, it's almost ready.

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

> Proposed ChangeLog:
> 
> 2013-01-22  Jim MacArthur  <jim.macarthur@arm.com>
>             Marcus Shawcroft  <marcus.shawcroft@arm.com>
>             Nigel Stephens  <nigel.stephens@arm.com>
>             Yufeng Zhang  <yufeng.zhang@arm.com>
> 
>     gdb/
> 
>         * configure.tgt: Set build_gdbserver=yes for the aarch64*-*-linux*
>         targets.

	* configure.tgt (aarch64*-*-linux*): Set build_gdbserver=yes.

> 
>     gdb/gdbserver/
> 
>         * Makefile.in: Add AArch64.
>         * configure.srv: Add AArch64.
>         * linux-aarch64-low.c: New file.
>         * linux-low.c: For various 'ptrace' calls, cast '0's as the 3rd
>         and 4th arguments to PTRACE_ARG3_TYPE and PTRACE_ARG4_TYPE respectively.

You'll need to be more detailed, per the GNU Coding Standards:

	* Makefile.in (clean): Remove aarch64.c and aarch64-without-fpu.c.
	...

Please see the numerous existing entries in the file.


> +#define TRAP_HWBKPT 0x0004

Is this really needed?  We tend to avoid putting in these
compat defines unless the minimum released kernel that we
support for a given arch missed them.

> +/* Maximum number of hardware breakpoints/watchpoints.
> +   N.B.  when change, especially increase, the numbers, also make sure
> +   the type dr_changed_t still be wide enough, i.e. the number of its
> +   bits is equal to or larger than the maximum value of the two
> +   macros.  */

This could use a grammar review, I think.

> +
> +#define AARCH64_HBP_MAX_NUM 16
> +#define AARCH64_HWP_MAX_NUM 16
> +


> +static CORE_ADDR
> +aarch64_get_pc (struct regcache *regcache)
> +{
> +  unsigned long pc;
> +  collect_register_by_name (regcache, "pc", &pc);

Empty line between declaration and statement.  Here and
elsewhere.

> +  if (debug_threads)
> +    fprintf (stderr, "stop pc is %08lx\n", pc);
> +  return pc;
> +}
> +


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

"break" after return is dead code.  Please put each
statement on its own line:

    case 0x01:
	return 1;


> +    }
> +}
> +


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

No space after *.

> +			  int *next_len_p)
> +{


+
> +  if (aligned_addr_p)

GDB's style, recently carved in the internals manual, is:

  if (aligned_addr_p != NULL)

etc.

> +    *aligned_addr_p = aligned_addr;
> +  if (aligned_len_p)
> +    *aligned_len_p = aligned_len;
> +  if (next_addr_p)
> +    *next_addr_p = addr;
> +  if (next_len_p)
> +    *next_len_p = len;
> +}
> +



> +  if (ptrace (PTRACE_SETREGSET, tid,
> +	      watchpoint ? NT_ARM_HW_WATCH : NT_ARM_HW_BREAK,
> +	      (void *)&iov))

Space after cast missing.

> +    error (_("Unexpected error setting hardware debug registers"));
> +}
> +


> +static struct aarch64_debug_reg_state *
> +aarch64_get_debug_reg_state ()
> +{
> +  int pid;
> +  struct lwp_info *thread;
> +  struct process_info *proc;
> +
> +  thread = get_thread_lwp (current_inferior);
> +  pid = pid_of (thread);
> +  proc = find_process_pid (pid);
> +

All this can be simplified as:

  proc = get_thread_process (current_inferior);

and then even further:

  proc = current_process ();

> +  return &proc->private->arch_private->debug_reg_state;
> +}
> +


> +  /* Find the entry that matches the ADDR and CTRL.  */
> +  for (i = 0; i < num_regs; ++i)
> +    if (dr_addr_p[i] == addr && dr_ctrl_p[i] == ctrl)
> +      {
> +	gdb_assert (dr_ref_count[i] != 0);
> +	break;

"break" after gdb_assert is dead code.

> +      }
> +



> +static int
> +aarch64_handle_breakpoint (enum target_point_type type, CORE_ADDR addr,
> +			   int len, int is_insert)
> +{
> +  struct aarch64_debug_reg_state *state;
> +
> +  /* The hardware breakpoint on AArch64 should always be 4-byte
> +     aligned.  */
> +  if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len))
> +    {
> +      gdb_assert (0);
> +      return -1;

The return is dead code.  But, in this case, it seems you
can get here if GDB does send over such an unaligned
breakpoint request (e.g., user does "b *0x...1", perhaps).  As
such, this should be rejected with error, not assert, as it's
not a gdbserver bug.

> +    }
> +
> +  state = aarch64_get_debug_reg_state ();
> +
> +  if (is_insert)
> +    return aarch64_dr_state_insert_one_point (state, type, addr, len);
> +  else
> +    return aarch64_dr_state_remove_one_point (state, type, addr, len);
> +}
> +


> +      if (debug_hw_points)
> +	fprintf (stderr,
> + "handle_unaligned_watchpoint: is_insert: %d\n"
> + "                             aligned_addr: 0x%llx, aligned_len: %d\n"
> + "                                next_addr: 0x%llx,    next_len: %d\n",
> +		 is_insert, aligned_addr, aligned_len, addr, len);

Printing CORE_ADDR -> paddress, not %llx.

> +
> +      if (ret != 0)
> +	return ret;
> +    }
> +
> +  return 0;
> +}
> +



> +
> +static void
> +aarch64_arch_setup (void)
> +{
> +  int pid;
> +  struct iovec iov;
> +  struct user_hwdebug_state dreg_state;
> +
> +  init_registers_aarch64 ();
> +
> +  pid = lwpid_of (get_thread_lwp (current_inferior));
> +  iov.iov_base = &dreg_state;
> +  iov.iov_len = sizeof (dreg_state);
> +
> +  /* Get hardware watchpoint register info.  */
> +  if (ptrace (PTRACE_GETREGSET, pid, 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)
> +	error ("arch_setup fails: num_wp_regs (%d) exceeds the maximum (%d)",
> +	       aarch64_num_wp_regs, AARCH64_HBP_MAX_NUM);

Should this be a bit more graceful?  IOW, maybe warn, and
leave aarch64_num_wp_regs set to the maximum supported, I think?
Otherwise, an old binary on a new kernel will just stop working
with a hard error.

> +    }
> +  else
> +    error ("arch_setup fails: unable to get debug register resource info");

Likewise, here set num_XX to 0, for the el cheapo variant without
watchpoint resources?

> +
> +  /* Get hardware breakpoint register info.  */
> +  if (ptrace (PTRACE_GETREGSET, pid, NT_ARM_HW_BREAK, &iov) == 0
> +      && AARCH64_DEBUG_ARCH (dreg_state.dbg_info) == AARCH64_DEBUG_ARCH_V8)
> +    {
> +      aarch64_num_bp_regs = AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info);
> +      if (aarch64_num_bp_regs > AARCH64_HBP_MAX_NUM)
> +	error ("arch_setup fails: num_bp_regs (%d) exceeds the maximum (%d)",
> +	       aarch64_num_bp_regs, AARCH64_HBP_MAX_NUM);
> +    }
> +  else
> +    error ("arch_setup fails: unable to get debug register resource info");
> +}
> +
> +struct regset_info target_regsets[] = {

Brace goes on new line.

> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
> +    sizeof (struct user_pt_regs), GENERAL_REGS,
> +    aarch64_fill_gregset, aarch64_store_gregset },
> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
> +    sizeof (struct user_fpsimd_state), FP_REGS,
> +    aarch64_fill_fpregset, aarch64_store_fpregset
> +  },
> +  { 0, 0, 0, -1, -1, NULL, NULL }
> +};
> +
> +struct linux_target_ops the_low_target = {

Ditto.

>    if (pid == 0)
>      {
> -      ptrace (PTRACE_TRACEME, 0, 0, 0);
> +      ptrace (PTRACE_TRACEME, 0, (PTRACE_ARG3_TYPE) 0, (PTRACE_ARG4_TYPE) 0);

These (and a few others below) are most probably unnecessary
since PTRACE_TRACEME ignores its pid, addr and data, but it can't hurt to
be consistent.  So OK.  This is the sort of change that if you
split into a separate patch, it can be quickly approved, and gotten out
of the way.  Can you do that please?


> @@ -4492,15 +4502,15 @@ linux_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr, int len)
>    if (debug_threads)
>      {
>        /* Dump up to four bytes.  */
> -      unsigned int val = * (unsigned int *) myaddr;
> +      unsigned val = * (unsigned *) myaddr;

Curious.  Any reason?

>        if (len == 1)
>  	val = val & 0xff;
>        else if (len == 2)
>  	val = val & 0xffff;
>        else if (len == 3)
>  	val = val & 0xffffff;
> -      fprintf (stderr, "Writing %0*x to 0x%08lx\n", 2 * ((len < 4) ? len : 4),
> -	       val, (long)memaddr);
> +      fprintf (stderr, "Writing len=%d 0x%0*x to 0x%08lx\n", len,
> +	       2 * ((len < 4) ? len : 4), val, (long) memaddr);

Ok.  Though unrelated to Aarch64...

>      }
>  

>        ret = my_waitpid (child_pid, &status, 0);
>        if (ret != child_pid)
>  	warning ("linux_test_for_tracefork: failed to wait for killed child");
>        else if (!WIFSIGNALED (status))
> -	warning ("linux_test_for_tracefork: unexpected wait status 0x%x from "
> -		 "killed child", status);
> +	warning ("linux_test_for_tracefork: unexpected wait status "
> +		 "0x%x waitint=0x%x x=0x%x x=0x%x from killed child",
> +		 status, __WAIT_INT (status), WIFSIGNALED (status),
> +		 ((signed char)(((__WAIT_INT (status) & 0x7f) + 1) >> 1)) > 0);
>  

Not sure __WAIT_INT is kosher on all the libcs we support building
gdbserver with.  Please split these non Aarch64 bits into separate
changes/patches/threads.

-- 
Pedro Alves