This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Support for HWbreak/watchpoint accross fork/vfork on arm-native
- From: Pedro Alves <palves at redhat dot com>
- To: Omair Javaid <omair dot javaid at linaro dot org>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 07 Feb 2014 15:32:49 +0000
- Subject: Re: [PATCH] Support for HWbreak/watchpoint accross fork/vfork on arm-native
- Authentication-results: sourceware.org; auth=none
- References: <1391119790-6580-1-git-send-email-omair dot javaid at linaro dot org> <52EAD0DC dot 506 at linaro dot org>
On 01/30/2014 10:23 PM, Omair Javaid wrote:
> Accidentally missed the patch description and changelog:
>
> This patch updates arm native support for hardware breakpoints and watchpoints
> to accommodate watchpoint/breakpoints across fork/vfork. Previously for arm
> native a record of breakpoints at thread level was being kept which has been
> changed to a process level record to come in sync with gdb-linux calls to
> threads and process creation/destruction hooks.
Thanks.
This also changes the hwbreak/watchpoint insertion mechanism
to the modern way of marking debug registers as needing update, but
only really updating on resume, which is necessary for supporting
watchpoints in non-stop mode (the current code tries to poke at
running threads with ptrace, which fails). Please update the
commit log to mention this.
The patch looks overall mostly good, but I have a few comments
below.
> ---
> gdb/arm-linux-nat.c | 417 ++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 273 insertions(+), 144 deletions(-)
>
> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
> index 6f56634..9a756c1 100644
> --- a/gdb/arm-linux-nat.c
> +++ b/gdb/arm-linux-nat.c
> @@ -787,70 +787,120 @@ struct arm_linux_hw_breakpoint
> arm_hwbp_control_t control;
> };
>
> -/* Structure containing arrays of the break and watch points which are have
> - active in each thread.
> -
> - The Linux ptrace interface to hardware break-/watch-points presents the
> - values in a vector centred around 0 (which is used fo generic information).
> - Positive indicies refer to breakpoint addresses/control registers, negative
> - indices to watchpoint addresses/control registers.
> -
> - The Linux vector is indexed as follows:
> - -((i << 1) + 2): Control register for watchpoint i.
> - -((i << 1) + 1): Address register for watchpoint i.
> - 0: Information register.
> - ((i << 1) + 1): Address register for breakpoint i.
> - ((i << 1) + 2): Control register for breakpoint i.
Why remove this ptrace comment? It looks quite useful to me.
> -
> - This structure is used as a per-thread cache of the state stored by the
> - kernel, so that we don't need to keep calling into the kernel to find a
> - free breakpoint.
> -
> - We treat break-/watch-points with their enable bit clear as being deleted.
> - */
> -typedef struct arm_linux_thread_points
> +/* Since we cannot dynamically allocate subfields of arm_linux_process_info,
> + assume a maximum number of supported break-/watchpoints. */
> +#define MAX_BPTS 16
> +#define MAX_WPTS 16
I saw no assertion making sure these maximums aren't
overflowed. see aarch64_linux_get_debug_reg_capacity.
> + /* Linked list. */
> + struct arm_linux_process_info *next;
> + /* The process identifier. */
> + pid_t pid;
> + /* Hardware breakpoints for this process. */
> + struct arm_linux_hw_breakpoint bpts[MAX_BPTS];
> + /* Hardware watchpoints for this process. */
> + struct arm_linux_hw_breakpoint wpts[MAX_WPTS];
It'd be a little cleaner if these two array were moved
to a separate structure, like e.g., in the aarch64 port.
> +/* Get hardware breakpoint state for process PID. */
>
> - return t;
> +static struct arm_linux_hw_breakpoint *
> +arm_linux_get_hwbreak_state (pid_t pid)
> +{
> + return arm_linux_process_info_get (pid)->bpts;
> +}
> +
> +/* Get watchpoint state for process PID. */
> +
> +static struct arm_linux_hw_breakpoint *
> +arm_linux_get_watch_state (pid_t pid)
> +{
> + return arm_linux_process_info_get (pid)->wpts;
> }
Then these two could be a single:
static struct arm_linux_hw_breakpoint *
arm_linux_get_point_state (pid_t pid)
{
return &arm_linux_process_info_get (pid)->state;
}
Users could then do:
arm_linux_get_point_state (pid)->wpts;
arm_linux_get_point_state (pid)->bpts;
> +static int
> +update_registers_callback (struct lwp_info *lwp, void *arg)
> +{
> + struct update_registers_data *data = (struct update_registers_data *) arg;
> +
> + /* Force iterate_over_lwps to return matched lwp_info*. */
> + if (arg == NULL)
> + return 1;
I don't understand this. It seems nothing passes a NULL arg.
> +
> + if (lwp->arch_private == NULL)
> + lwp->arch_private = XCNEW (struct arch_lwp_info);
> +
> + /* The actual update is done later just before resuming the lwp,
> + we just mark that the registers need updating. */
> + if (data->watch)
> + lwp->arch_private->wpts_changed[data->index] = 1;
> + else
> + lwp->arch_private->bpts_changed[data->index] = 1;
> +
> + /* If the lwp isn't stopped, force it to momentarily pause, so
> + we can update its breakpoint registers. */
> + if (!lwp->stopped)
> + linux_stop_lwp (lwp);
> +
> + return 0;
> +}
> if (watchpoint)
> {
> count = arm_linux_get_hw_watchpoint_count ();
> - bpts = t->wpts;
> - dir = -1;
> + bpts = arm_linux_get_watch_state(pid);
Space before parens.
> }
> else
> {
> count = arm_linux_get_hw_breakpoint_count ();
> - bpts = t->bpts;
> - dir = 1;
> + bpts = arm_linux_get_hwbreak_state(pid);
Ditto.
> }
> if (watchpoint)
> {
> count = arm_linux_get_hw_watchpoint_count ();
> - bpts = t->wpts;
> - dir = -1;
> + bpts = arm_linux_get_watch_state(pid);
Ditto.
> }
> else
> {
> count = arm_linux_get_hw_breakpoint_count ();
> - bpts = t->bpts;
> - dir = 1;
> + bpts = arm_linux_get_hwbreak_state(pid);
Ditto.
> }
>
> +arm_linux_prepare_to_resume (struct lwp_info *lwp)
> {
...
> + for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++)
> + if (arm_lwp_info->bpts_changed[i])
> + {
> + errno = 0;
> + pid = ptid_get_lwp (lwp->ptid);
> + bpts = arm_linux_get_hwbreak_state (ptid_get_pid (lwp->ptid));
This should probably be moved out of the loop. As is it's doing
a linear lookup per iteration.
> - }
> + if (arm_hwbp_control_is_enabled (bpts[i].control))
> + if (ptrace (PTRACE_SETHBPREGS, pid,
> + (PTRACE_TYPE_ARG3) ((i << 1) + 1), &bpts[i].address) < 0)
> + perror_with_name ("Unexpected error setting breakpoint address");
Missing _() for i18n. The old code had it.
>
> - if (t == NULL)
> - return;
> + if (bpts[i].control != 0)
> + if (ptrace (PTRACE_SETHBPREGS, pid,
> + (PTRACE_TYPE_ARG3) ((i << 1) + 2), &bpts[i].control) < 0)
> + perror_with_name ("Unexpected error setting breakpoint");
Ditto.
>
> - VEC_unordered_remove (arm_linux_thread_points_p, arm_threads, i);
> + arm_lwp_info->bpts_changed[i] = 0;
> + }
>
> - xfree (t->bpts);
> - xfree (t->wpts);
> - xfree (t);
> - }
> + for (i = 0; i < arm_linux_get_hw_watchpoint_count (); i++)
> + if (arm_lwp_info->wpts_changed[i])
> + {
> + errno = 0;
> + pid = ptid_get_lwp (lwp->ptid);
> + wpts = arm_linux_get_watch_state (ptid_get_pid (lwp->ptid));
Likewise as comment above.
> +
> + if (arm_hwbp_control_is_enabled (wpts[i].control))
> + if (ptrace (PTRACE_SETHBPREGS, pid,
> + (PTRACE_TYPE_ARG3) -((i << 1) + 1), &wpts[i].address) < 0)
> + perror_with_name ("Unexpected error setting watchpoint address");
Ditto, etc.
> +static void
> +arm_linux_new_fork (struct lwp_info *parent, pid_t child_pid)
> +{
> + parent_pid = ptid_get_pid (parent->ptid);
> + parent_point_state = arm_linux_get_hwbreak_state (parent_pid);
> + child_point_state = arm_linux_get_hwbreak_state (child_pid);
> + memcpy (child_point_state, parent_point_state,
> + sizeof (struct arm_linux_hw_breakpoint) * MAX_BPTS);
> +
> + parent_point_state = arm_linux_get_watch_state (parent_pid);
> + child_point_state = arm_linux_get_watch_state (child_pid);
> + memcpy (child_point_state, parent_point_state,
> + sizeof (struct arm_linux_hw_breakpoint) * MAX_BPTS);
The latter should be MAX_WPTS. But with the earlier proposed change,
these two would simply turn into a single:
parent_pid = ptid_get_pid (parent->ptid);
parent_point_state = arm_linux_get_point_state (parent_pid);
child_point_state = arm_linux_get_point_state (child_pid);
*child_point_state = *parent_point_state;