* [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep @ 2023-06-12 19:21 Tom Tromey 2023-06-12 19:21 ` [PATCH 1/2] " Tom Tromey ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Tom Tromey @ 2023-06-12 19:21 UTC (permalink / raw) To: gdb-patches This series implements a suggestion that Pedro made back in 2018. The idea has been kicking around internally here at AdaCore since then, and I finally got around to writing the patch. Tested on x86-64 Fedora 36. I've also run it through the AdaCore internal test suite. --- Tom Tromey (2): Call target_can_do_single_step from maybe_software_singlestep Disabling hardware single step in gdbserver gdb/arm-linux-tdep.c | 5 ----- gdb/gdbarch-gen.h | 12 +++--------- gdb/gdbarch_components.py | 12 +++--------- gdb/infrun.c | 12 +++++++++--- gdbserver/server.cc | 12 ++++++++---- gdbserver/server.h | 1 + 6 files changed, 24 insertions(+), 30 deletions(-) --- base-commit: 2e3aff27623b20b08ac58f8eaf73e97e58b4e67c change-id: 20230612-sw-single-step-946fd4d856f2 Best regards, -- Tom Tromey <tromey@adacore.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] Call target_can_do_single_step from maybe_software_singlestep 2023-06-12 19:21 [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey @ 2023-06-12 19:21 ` Tom Tromey 2023-11-16 10:12 ` Andrew Burgess 2023-06-12 19:21 ` [PATCH 2/2] Disabling hardware single step in gdbserver Tom Tromey 2023-09-07 17:59 ` [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey 2 siblings, 1 reply; 7+ messages in thread From: Tom Tromey @ 2023-06-12 19:21 UTC (permalink / raw) To: gdb-patches When the PikeOS osabi sniffer was added, Pedro suggested that a target could omit stepping from its vCont? reply packet to tell gdb that software single-step must be used: https://sourceware.org/legacy-ml/gdb-patches/2018-09/msg00312.html This patch implements this idea by moving the call to target_can_do_single_step into maybe_software_singlestep. I've also removed some FIXME comments from gdbarch_components.py, and slightly updated the documentation for gdbarch_software_single_step. I think these comments are somewhat obsolete now that target_can_do_single_step exists -- the current approach isn't exactly what the comments intended, but on the other hand, it exists and works. --- gdb/arm-linux-tdep.c | 5 ----- gdb/gdbarch-gen.h | 12 +++--------- gdb/gdbarch_components.py | 12 +++--------- gdb/infrun.c | 12 +++++++++--- 4 files changed, 15 insertions(+), 26 deletions(-) diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c index dfa816990ff..f4da996728b 100644 --- a/gdb/arm-linux-tdep.c +++ b/gdb/arm-linux-tdep.c @@ -923,11 +923,6 @@ arm_linux_software_single_step (struct regcache *regcache) struct gdbarch *gdbarch = regcache->arch (); struct arm_get_next_pcs next_pcs_ctx; - /* If the target does have hardware single step, GDB doesn't have - to bother software single step. */ - if (target_can_do_single_step () == 1) - return {}; - arm_get_next_pcs_ctor (&next_pcs_ctx, &arm_linux_get_next_pcs_ops, gdbarch_byte_order (gdbarch), diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h index 101b1b73636..1d86879f00a 100644 --- a/gdb/gdbarch-gen.h +++ b/gdb/gdbarch-gen.h @@ -721,16 +721,10 @@ extern void set_gdbarch_get_memtag (struct gdbarch *gdbarch, gdbarch_get_memtag_ extern CORE_ADDR gdbarch_memtag_granule_size (struct gdbarch *gdbarch); extern void set_gdbarch_memtag_granule_size (struct gdbarch *gdbarch, CORE_ADDR memtag_granule_size); -/* FIXME/cagney/2001-01-18: This should be split in two. A target method that - indicates if the target needs software single step. An ISA method to - implement it. +/* Return a vector of addresses at which the software single step + breakpoints should be inserted. An empty vector means software single + step is not used. - FIXME/cagney/2001-01-18: The logic is backwards. It should be asking if the - target can single step. If not, then implement single step using breakpoints. - - Return a vector of addresses on which the software single step - breakpoints should be inserted. NULL means software single step is - not used. Multiple breakpoints may be inserted for some instructions such as conditional branch. However, each implementation must always evaluate the condition and only put the breakpoint at the branch destination if diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py index 23e5789327c..7f2380ae0a7 100644 --- a/gdb/gdbarch_components.py +++ b/gdb/gdbarch_components.py @@ -1301,16 +1301,10 @@ For a non-zero value, this represents the number of bytes of memory per tag. Function( comment=""" -FIXME/cagney/2001-01-18: This should be split in two. A target method that -indicates if the target needs software single step. An ISA method to -implement it. +Return a vector of addresses at which the software single step +breakpoints should be inserted. An empty vector means software single +step is not used. -FIXME/cagney/2001-01-18: The logic is backwards. It should be asking if the -target can single step. If not, then implement single step using breakpoints. - -Return a vector of addresses on which the software single step -breakpoints should be inserted. NULL means software single step is -not used. Multiple breakpoints may be inserted for some instructions such as conditional branch. However, each implementation must always evaluate the condition and only put the breakpoint at the branch destination if diff --git a/gdb/infrun.c b/gdb/infrun.c index 58da1cef29e..d1dce33bd3a 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2242,9 +2242,15 @@ maybe_software_singlestep (struct gdbarch *gdbarch) { bool hw_step = true; - if (execution_direction == EXEC_FORWARD - && gdbarch_software_single_step_p (gdbarch)) - hw_step = !insert_single_step_breakpoints (gdbarch); + if (execution_direction == EXEC_FORWARD) + { + if (target_can_do_single_step () == 1) + { + /* The target definitely has hardware single step. */ + } + else if (gdbarch_software_single_step_p (gdbarch)) + hw_step = !insert_single_step_breakpoints (gdbarch); + } return hw_step; } -- 2.40.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] Call target_can_do_single_step from maybe_software_singlestep 2023-06-12 19:21 ` [PATCH 1/2] " Tom Tromey @ 2023-11-16 10:12 ` Andrew Burgess 0 siblings, 0 replies; 7+ messages in thread From: Andrew Burgess @ 2023-11-16 10:12 UTC (permalink / raw) To: Tom Tromey, gdb-patches Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: > When the PikeOS osabi sniffer was added, Pedro suggested that a target > could omit stepping from its vCont? reply packet to tell gdb that > software single-step must be used: > > https://sourceware.org/legacy-ml/gdb-patches/2018-09/msg00312.html > > This patch implements this idea by moving the call to > target_can_do_single_step into maybe_software_singlestep. So this changes the behaviour for record-full.c, which includes two calls to insert_single_step_breakpoints, which bypasses maybe_software_singlestep. If I'm reading the code correctly, after this change, we'll switch from using h/w stepping to s/w stepping. I'm assuming this isn't an intentional change as it's not mentioned. I'm tempted to suggest that the two calls to insert_single_step_breakpoints in record-full.c should really be calling maybe_software_singlestep; I don't believe we can be recording in a backward direction -- that makes no sense, we record forward, and use the record to move backward. And other than a duplicate call to gdbarch_software_single_step_p, calling maybe_software_singlestep seems otherwise harmless .... and after this patch we'd pick up the target_can_do_single_step call, which is probably what we want. Of course, if we did change record-full.c to use maybe_software_singlestep then there would only be one call to insert_single_step_breakpoints, so we could possibly fold this into maybe_software_singlestep? Thanks, Andrew > > I've also removed some FIXME comments from gdbarch_components.py, and > slightly updated the documentation for gdbarch_software_single_step. > I think these comments are somewhat obsolete now that > target_can_do_single_step exists -- the current approach isn't exactly > what the comments intended, but on the other hand, it exists and > works. > --- > gdb/arm-linux-tdep.c | 5 ----- > gdb/gdbarch-gen.h | 12 +++--------- > gdb/gdbarch_components.py | 12 +++--------- > gdb/infrun.c | 12 +++++++++--- > 4 files changed, 15 insertions(+), 26 deletions(-) > > diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c > index dfa816990ff..f4da996728b 100644 > --- a/gdb/arm-linux-tdep.c > +++ b/gdb/arm-linux-tdep.c > @@ -923,11 +923,6 @@ arm_linux_software_single_step (struct regcache *regcache) > struct gdbarch *gdbarch = regcache->arch (); > struct arm_get_next_pcs next_pcs_ctx; > > - /* If the target does have hardware single step, GDB doesn't have > - to bother software single step. */ > - if (target_can_do_single_step () == 1) > - return {}; > - > arm_get_next_pcs_ctor (&next_pcs_ctx, > &arm_linux_get_next_pcs_ops, > gdbarch_byte_order (gdbarch), > diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h > index 101b1b73636..1d86879f00a 100644 > --- a/gdb/gdbarch-gen.h > +++ b/gdb/gdbarch-gen.h > @@ -721,16 +721,10 @@ extern void set_gdbarch_get_memtag (struct gdbarch *gdbarch, gdbarch_get_memtag_ > extern CORE_ADDR gdbarch_memtag_granule_size (struct gdbarch *gdbarch); > extern void set_gdbarch_memtag_granule_size (struct gdbarch *gdbarch, CORE_ADDR memtag_granule_size); > > -/* FIXME/cagney/2001-01-18: This should be split in two. A target method that > - indicates if the target needs software single step. An ISA method to > - implement it. > +/* Return a vector of addresses at which the software single step > + breakpoints should be inserted. An empty vector means software single > + step is not used. > > - FIXME/cagney/2001-01-18: The logic is backwards. It should be asking if the > - target can single step. If not, then implement single step using breakpoints. > - > - Return a vector of addresses on which the software single step > - breakpoints should be inserted. NULL means software single step is > - not used. > Multiple breakpoints may be inserted for some instructions such as > conditional branch. However, each implementation must always evaluate > the condition and only put the breakpoint at the branch destination if > diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py > index 23e5789327c..7f2380ae0a7 100644 > --- a/gdb/gdbarch_components.py > +++ b/gdb/gdbarch_components.py > @@ -1301,16 +1301,10 @@ For a non-zero value, this represents the number of bytes of memory per tag. > > Function( > comment=""" > -FIXME/cagney/2001-01-18: This should be split in two. A target method that > -indicates if the target needs software single step. An ISA method to > -implement it. > +Return a vector of addresses at which the software single step > +breakpoints should be inserted. An empty vector means software single > +step is not used. > > -FIXME/cagney/2001-01-18: The logic is backwards. It should be asking if the > -target can single step. If not, then implement single step using breakpoints. > - > -Return a vector of addresses on which the software single step > -breakpoints should be inserted. NULL means software single step is > -not used. > Multiple breakpoints may be inserted for some instructions such as > conditional branch. However, each implementation must always evaluate > the condition and only put the breakpoint at the branch destination if > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 58da1cef29e..d1dce33bd3a 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -2242,9 +2242,15 @@ maybe_software_singlestep (struct gdbarch *gdbarch) > { > bool hw_step = true; > > - if (execution_direction == EXEC_FORWARD > - && gdbarch_software_single_step_p (gdbarch)) > - hw_step = !insert_single_step_breakpoints (gdbarch); > + if (execution_direction == EXEC_FORWARD) > + { > + if (target_can_do_single_step () == 1) > + { > + /* The target definitely has hardware single step. */ > + } > + else if (gdbarch_software_single_step_p (gdbarch)) > + hw_step = !insert_single_step_breakpoints (gdbarch); > + } > > return hw_step; > } > > -- > 2.40.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] Disabling hardware single step in gdbserver 2023-06-12 19:21 [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey 2023-06-12 19:21 ` [PATCH 1/2] " Tom Tromey @ 2023-06-12 19:21 ` Tom Tromey 2023-11-16 10:14 ` Andrew Burgess 2023-09-07 17:59 ` [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey 2 siblings, 1 reply; 7+ messages in thread From: Tom Tromey @ 2023-06-12 19:21 UTC (permalink / raw) To: gdb-patches This patch gives gdbserver the ability to omit the 's' reply to 'vCont?'. This tells gdb that hardware single-step is definitely not supported, causing it to fall back to using software single-step. This is useful for testing the earlier change to maybe_software_singlestep. --- gdbserver/server.cc | 12 ++++++++---- gdbserver/server.h | 1 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/gdbserver/server.cc b/gdbserver/server.cc index c57270175b4..afc18a87770 100644 --- a/gdbserver/server.cc +++ b/gdbserver/server.cc @@ -135,6 +135,7 @@ unsigned long signal_pid; in gdbserver, for the sake of testing GDB against stubs that don't support them. */ bool disable_packet_vCont; +bool disable_packet_vCont_step; bool disable_packet_Tthread; bool disable_packet_qC; bool disable_packet_qfThreadInfo; @@ -3125,9 +3126,10 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) { strcpy (own_buf, "vCont;c;C;t"); - if (target_supports_hardware_single_step () - || target_supports_software_single_step () - || !cs.vCont_supported) + if (!disable_packet_vCont_step + && (target_supports_hardware_single_step () + || target_supports_software_single_step () + || !cs.vCont_supported)) { /* If target supports single step either by hardware or by software, add actions s and S to the list of supported @@ -3456,7 +3458,7 @@ gdbserver_usage (FILE *stream) " --disable-packet=OPT1[,OPT2,...]\n" " Disable support for RSP packets or features.\n" " Options:\n" - " vCont, T, Tthread, qC, qfThreadInfo and \n" + " vCont, vConts, T, Tthread, qC, qfThreadInfo and\n" " threads (disable all threading packets).\n" "\n" "For more information, consult the GDB manual (available as on-line \n" @@ -3766,6 +3768,8 @@ captured_main (int argc, char *argv[]) { if (strcmp ("vCont", tok) == 0) disable_packet_vCont = true; + else if (strcmp ("vConts", tok) == 0) + disable_packet_vCont_step = true; else if (strcmp ("Tthread", tok) == 0) disable_packet_Tthread = true; else if (strcmp ("qC", tok) == 0) diff --git a/gdbserver/server.h b/gdbserver/server.h index fde7dfe7060..70fd8ff8b55 100644 --- a/gdbserver/server.h +++ b/gdbserver/server.h @@ -68,6 +68,7 @@ void initialize_low (); extern bool server_waiting; extern bool disable_packet_vCont; +extern bool disable_packet_vCont_step; extern bool disable_packet_Tthread; extern bool disable_packet_qC; extern bool disable_packet_qfThreadInfo; -- 2.40.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] Disabling hardware single step in gdbserver 2023-06-12 19:21 ` [PATCH 2/2] Disabling hardware single step in gdbserver Tom Tromey @ 2023-11-16 10:14 ` Andrew Burgess 0 siblings, 0 replies; 7+ messages in thread From: Andrew Burgess @ 2023-11-16 10:14 UTC (permalink / raw) To: Tom Tromey, gdb-patches Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: > This patch gives gdbserver the ability to omit the 's' reply to > 'vCont?'. This tells gdb that hardware single-step is definitely not > supported, causing it to fall back to using software single-step. > This is useful for testing the earlier change to > maybe_software_singlestep. LGTM. Approved-By: Andrew Burgess <aburgess@redhat.com> Thanks, Andrew > --- > gdbserver/server.cc | 12 ++++++++---- > gdbserver/server.h | 1 + > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/gdbserver/server.cc b/gdbserver/server.cc > index c57270175b4..afc18a87770 100644 > --- a/gdbserver/server.cc > +++ b/gdbserver/server.cc > @@ -135,6 +135,7 @@ unsigned long signal_pid; > in gdbserver, for the sake of testing GDB against stubs that don't > support them. */ > bool disable_packet_vCont; > +bool disable_packet_vCont_step; > bool disable_packet_Tthread; > bool disable_packet_qC; > bool disable_packet_qfThreadInfo; > @@ -3125,9 +3126,10 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) > { > strcpy (own_buf, "vCont;c;C;t"); > > - if (target_supports_hardware_single_step () > - || target_supports_software_single_step () > - || !cs.vCont_supported) > + if (!disable_packet_vCont_step > + && (target_supports_hardware_single_step () > + || target_supports_software_single_step () > + || !cs.vCont_supported)) > { > /* If target supports single step either by hardware or by > software, add actions s and S to the list of supported > @@ -3456,7 +3458,7 @@ gdbserver_usage (FILE *stream) > " --disable-packet=OPT1[,OPT2,...]\n" > " Disable support for RSP packets or features.\n" > " Options:\n" > - " vCont, T, Tthread, qC, qfThreadInfo and \n" > + " vCont, vConts, T, Tthread, qC, qfThreadInfo and\n" > " threads (disable all threading packets).\n" > "\n" > "For more information, consult the GDB manual (available as on-line \n" > @@ -3766,6 +3768,8 @@ captured_main (int argc, char *argv[]) > { > if (strcmp ("vCont", tok) == 0) > disable_packet_vCont = true; > + else if (strcmp ("vConts", tok) == 0) > + disable_packet_vCont_step = true; > else if (strcmp ("Tthread", tok) == 0) > disable_packet_Tthread = true; > else if (strcmp ("qC", tok) == 0) > diff --git a/gdbserver/server.h b/gdbserver/server.h > index fde7dfe7060..70fd8ff8b55 100644 > --- a/gdbserver/server.h > +++ b/gdbserver/server.h > @@ -68,6 +68,7 @@ void initialize_low (); > extern bool server_waiting; > > extern bool disable_packet_vCont; > +extern bool disable_packet_vCont_step; > extern bool disable_packet_Tthread; > extern bool disable_packet_qC; > extern bool disable_packet_qfThreadInfo; > > -- > 2.40.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep 2023-06-12 19:21 [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey 2023-06-12 19:21 ` [PATCH 1/2] " Tom Tromey 2023-06-12 19:21 ` [PATCH 2/2] Disabling hardware single step in gdbserver Tom Tromey @ 2023-09-07 17:59 ` Tom Tromey 2023-11-15 19:01 ` Tom Tromey 2 siblings, 1 reply; 7+ messages in thread From: Tom Tromey @ 2023-09-07 17:59 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey >>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: Tom> This series implements a suggestion that Pedro made back in 2018. The Tom> idea has been kicking around internally here at AdaCore since then, Tom> and I finally got around to writing the patch. Tom> Tested on x86-64 Fedora 36. I've also run it through the AdaCore Tom> internal test suite. Normally I just land patches after a couple weeks or so, but I think these probably should get a review. thanks, Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep 2023-09-07 17:59 ` [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey @ 2023-11-15 19:01 ` Tom Tromey 0 siblings, 0 replies; 7+ messages in thread From: Tom Tromey @ 2023-11-15 19:01 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey >>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: >>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes: Tom> This series implements a suggestion that Pedro made back in 2018. The Tom> idea has been kicking around internally here at AdaCore since then, Tom> and I finally got around to writing the patch. Tom> Tested on x86-64 Fedora 36. I've also run it through the AdaCore Tom> internal test suite. Tom> Normally I just land patches after a couple weeks or so, but I think Tom> these probably should get a review. Pinging this one again. Tom ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-16 10:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-12 19:21 [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey 2023-06-12 19:21 ` [PATCH 1/2] " Tom Tromey 2023-11-16 10:12 ` Andrew Burgess 2023-06-12 19:21 ` [PATCH 2/2] Disabling hardware single step in gdbserver Tom Tromey 2023-11-16 10:14 ` Andrew Burgess 2023-09-07 17:59 ` [PATCH 0/2] Call target_can_do_single_step from maybe_software_singlestep Tom Tromey 2023-11-15 19:01 ` Tom Tromey
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).