This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] AArch64 GDBSERVER
- From: Pedro Alves <palves at redhat dot com>
- To: Marcus Shawcroft <marcus dot shawcroft at arm dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 01 Feb 2013 16:51:21 +0000
- Subject: Re: [PATCH] AArch64 GDBSERVER
- References: <50FED7B9.5080801@arm.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