This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread
- From: Pedro Alves <palves at redhat dot com>
- To: Alan Hayward <Alan dot Hayward at arm dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Cc: nd <nd at arm dot com>
- Date: Thu, 29 Nov 2018 14:32:16 +0000
- Subject: Re: [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread
- References: <20181120115602.30606-1-alan.hayward@arm.com>
On 11/20/2018 11:56 AM, Alan Hayward wrote:
> [Note: I hope to eventually isolate the exact kernel/hardware issue and
> raise a bug elsewhere against that.]
Any luck with that so far?
A fix that makes a race window a bit narrower, while still
leaving it open, risks pushing down / delaying a proper fix,
papering over the issue, and making the bug harder to reproduce.
But I won't object, especially since this seems to reduce number
of syscalls gdb issues, so could be seen as optimization and
good thing on its own.
LGTM, with nits below addressed.
>
> On some heavily loaded AArch64 boxes, GDB will sometimes hang forever when
> the inferior creates a thread. This hang happens inside the kernel during
> the ptrace call to set hardware watchpoints or hardware breakpoints.
> Currently, GDB will always set hw wp/bp at the start of each thread even if
> there are none set in the process.
>
> This patch works around the issue by avoiding setting hw wp/bp if there
> are none set for the process.
>
> On an effected machine, this fix drastically reduces the racy nature of the
> gdb.threads test set. I ran the entire gdb test suite across all processors
> for 100 iterations, then ran the results through the racy tests script.
> Without the patch, 58 .exp files in gdb.threads were marked as racy. After
> the patch this reduced to the same ~14 tests as the non effected boxes.
>
> Clearly GDB will still be subject to hangs on an effect box if hw wp/bp's are
> used prior to creating inferior threads on a heavily loaded system.
>
> To enable this in gdbserver, the sequence in gdbserver add_lwp() is switched
> to the same as gdb order as gdb, to ensure the thread is registered before
> calling new_thread(). This allows aarch64_linux_new_thread() to read the
> ptid.
>
> gdb/ChangeLog:
>
> 2018-11-20 Alan Hayward <alan.hayward@arm.com>
>
> * nat/aarch64-linux-hw-point.c
> (aarch64_linux_any_set_debug_regs_state): New function.
> * nat/aarch64-linux-hw-point.h
> (aarch64_linux_any_set_debug_regs_state): New declaration.
> * nat/aarch64-linux.c (aarch64_linux_new_thread): Check if any
> BPs or WPs are set.
>
> gdb/gdbserver/ChangeLog:
>
> 2018-11-20 Alan Hayward <alan.hayward@arm.com>
>
> * linux-low.c (add_lwp): Switch ordering.
> ---
> gdb/gdbserver/linux-low.c | 4 ++--
> gdb/nat/aarch64-linux-hw-point.c | 23 +++++++++++++++++++++++
> gdb/nat/aarch64-linux-hw-point.h | 6 ++++++
> gdb/nat/aarch64-linux.c | 15 ++++++++++-----
> 4 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 701f3e863c..1c336efade 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -951,11 +951,11 @@ add_lwp (ptid_t ptid)
>
> lwp->waitstatus.kind = TARGET_WAITKIND_IGNORE;
>
> + lwp->thread = add_thread (ptid, lwp);
> +
> if (the_low_target.new_thread != NULL)
> the_low_target.new_thread (lwp);
>
> - lwp->thread = add_thread (ptid, lwp);
> -
> return lwp;
> }
>
> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
> index 18b5af2d49..7f96fa2b7a 100644
> --- a/gdb/nat/aarch64-linux-hw-point.c
> +++ b/gdb/nat/aarch64-linux-hw-point.c
> @@ -717,6 +717,29 @@ aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state,
> }
> }
>
> +/* See nat/aarch64-linux-hw-point.h. */
> +
> +bool
> +aarch64_linux_any_set_debug_regs_state (struct aarch64_debug_reg_state *state,
> + bool watchpoint)
> +{
> + int i, count;
> + const CORE_ADDR *addr;
> + const unsigned int *ctrl;
> +
> + count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;
> + addr = watchpoint ? state->dr_addr_wp : state->dr_addr_bp;
> + ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
> + if (count == 0)
> + return false;
> +
> + for (i = 0; i < count; i++)
You could declare+initialize at the same time. I.e.:
int count = watchpoint ? aarch64_num_wp_regs : aarch64_num_bp_regs;
if (count == 0)
return false;
CORE_ADDR *addr = watchpoint ? state->dr_addr_wp : state->dr_addr_bp;
unsigned int ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
for (int i = 0; i < count; i++)
> --- a/gdb/nat/aarch64-linux-hw-point.h
> +++ b/gdb/nat/aarch64-linux-hw-point.h
> @@ -182,6 +182,12 @@ int aarch64_handle_watchpoint (enum target_hw_bp_type type, CORE_ADDR addr,
> void aarch64_linux_set_debug_regs (struct aarch64_debug_reg_state *state,
> int tid, int watchpoint);
>
> +/* Return TRUE if there are any hardware breakpoints. If WATCHPOINT is TRUE,
> + check hardware watchpoints instead. */
> +bool
> +aarch64_linux_any_set_debug_regs_state (struct aarch64_debug_reg_state *state,
> + bool watchpoint);
Function name only goes on first column in definitions.
See e.g., extension.h:
bool aarch64_linux_any_set_debug_regs_state
(struct aarch64_debug_reg_state *state, bool watchpoint);
Or you could drop "struct" to fit under 80 cols:
bool aarch64_linux_any_set_debug_regs_state (aarch64_debug_reg_state *state,
bool watchpoint);
> +
> void aarch64_show_debug_reg_state (struct aarch64_debug_reg_state *state,
> const char *func, CORE_ADDR addr,
> int len, enum target_hw_bp_type type);
> diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
> index 0f2c8011ea..48c59f6288 100644
> --- a/gdb/nat/aarch64-linux.c
> +++ b/gdb/nat/aarch64-linux.c
> @@ -73,13 +73,18 @@ aarch64_linux_prepare_to_resume (struct lwp_info *lwp)
> void
> aarch64_linux_new_thread (struct lwp_info *lwp)
> {
> + ptid_t ptid = ptid_of_lwp (lwp);
> + struct aarch64_debug_reg_state *state
> + = aarch64_get_debug_reg_state (ptid.pid ());
> struct arch_lwp_info *info = XNEW (struct arch_lwp_info);
>
> - /* Mark that all the hardware breakpoint/watchpoint register pairs
> - for this thread need to be initialized (with data from
> - aarch_process_info.debug_reg_state). */
> - DR_MARK_ALL_CHANGED (info->dr_changed_bp, aarch64_num_bp_regs);
> - DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs);
> + /* If there are hardware breakpoints/watchpoints in the process then mark that
> + all the hardware breakpoint/watchpoint register pairs for this thread need
> + to be initialized (with data from aarch_process_info.debug_reg_state). */
> + if (aarch64_linux_any_set_debug_regs_state (state, false))
> + DR_MARK_ALL_CHANGED (info->dr_changed_bp, aarch64_num_bp_regs);
> + if (aarch64_linux_any_set_debug_regs_state (state, true))
> + DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs);
>
> lwp_set_arch_private_info (lwp, info);
> }
>
Thanks,
Pedro Alves