* [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread @ 2018-11-20 11:56 Alan Hayward 2018-11-29 14:05 ` [Ping][PATCH] " Alan Hayward 2018-11-29 14:32 ` [PATCH] " Pedro Alves 0 siblings, 2 replies; 4+ messages in thread From: Alan Hayward @ 2018-11-20 11:56 UTC (permalink / raw) To: gdb-patches; +Cc: nd, Alan Hayward [Note: I hope to eventually isolate the exact kernel/hardware issue and raise a bug elsewhere against that.] 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++) + if (addr[i] != 0 || ctrl[i] != 0) + return true; + + return false; +} + /* Print the values of the cached breakpoint/watchpoint registers. */ void diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h index 1940b06a89..37107a888c 100644 --- 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); + 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); } -- 2.17.2 (Apple Git-113) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Ping][PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread 2018-11-20 11:56 [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread Alan Hayward @ 2018-11-29 14:05 ` Alan Hayward 2018-11-29 14:32 ` [PATCH] " Pedro Alves 1 sibling, 0 replies; 4+ messages in thread From: Alan Hayward @ 2018-11-29 14:05 UTC (permalink / raw) To: gdb-patches; +Cc: nd Ping! > On 20 Nov 2018, at 11:56, Alan Hayward <Alan.Hayward@arm.com> wrote: > > [Note: I hope to eventually isolate the exact kernel/hardware issue and > raise a bug elsewhere against that.] > > 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++) > + if (addr[i] != 0 || ctrl[i] != 0) > + return true; > + > + return false; > +} > + > /* Print the values of the cached breakpoint/watchpoint registers. */ > > void > diff --git a/gdb/nat/aarch64-linux-hw-point.h b/gdb/nat/aarch64-linux-hw-point.h > index 1940b06a89..37107a888c 100644 > --- 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); > + > 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); > } > -- > 2.17.2 (Apple Git-113) > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread 2018-11-20 11:56 [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread Alan Hayward 2018-11-29 14:05 ` [Ping][PATCH] " Alan Hayward @ 2018-11-29 14:32 ` Pedro Alves 2018-11-29 15:24 ` Alan Hayward 1 sibling, 1 reply; 4+ messages in thread From: Pedro Alves @ 2018-11-29 14:32 UTC (permalink / raw) To: Alan Hayward, gdb-patches; +Cc: nd 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread 2018-11-29 14:32 ` [PATCH] " Pedro Alves @ 2018-11-29 15:24 ` Alan Hayward 0 siblings, 0 replies; 4+ messages in thread From: Alan Hayward @ 2018-11-29 15:24 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches, nd > On 29 Nov 2018, at 14:32, Pedro Alves <palves@redhat.com> wrote: > > 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. > Here’s where it gets more interesting... We recently got some newly purchased boxes of (allegedly) the same hardware. With the same kernel versions installed, identical gdb binary, and same versions of the linked libraries, this racy behaviour doesn’t happen. Ideally, my next step would be to get one of the older boxes re-imaged in exactly the same way as the newer boxes and see if the issue vanishes (or not). Sadly, that’s not possible right now (it’s a shared resource). Once they free up I’ll be able to investigate more. Also, if we can’t reproduce on newer hardware then it lowers the urgency down my todo list a bit too. > 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++) Agreed. This was due to me copying from aarch64_linux_set_debug_regs and just keeping to the existing style. > > >> --- 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); > Ok. > > >> + >> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-29 15:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-20 11:56 [PATCH] AArch64: Racy: Don't set empty set of hardware BPs/WPs on new thread Alan Hayward 2018-11-29 14:05 ` [Ping][PATCH] " Alan Hayward 2018-11-29 14:32 ` [PATCH] " Pedro Alves 2018-11-29 15:24 ` Alan Hayward
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).