* [PATCH 0/7] Range stepping
@ 2013-03-11 12:52 Yao Qi
2013-03-11 12:53 ` [PATCH 4/7] range stepping: gdb Yao Qi
` (8 more replies)
0 siblings, 9 replies; 44+ messages in thread
From: Yao Qi @ 2013-03-11 12:52 UTC (permalink / raw)
To: gdb-patches
Hi,
When user types "next" command on the following line of source,
a = b + c + d * e - a;
GDB will single step every instruction belongs to this source line
and stop when program goes to the next line, the following is the
assembly of this source line
0x08048434 <+65>: mov 0x1c(%esp),%eax
0x08048438 <+69>: mov 0x30(%esp),%edx
0x0804843c <+73>: add %eax,%edx
0x0804843e <+75>: mov 0x18(%esp),%eax
0x08048442 <+79>: imul 0x2c(%esp),%eax
0x08048447 <+84>: add %edx,%eax
0x08048449 <+86>: sub 0x34(%esp),%eax
0x0804844d <+90>: mov %eax,0x34(%esp)
0x08048451 <+94>: mov 0x1c(%esp),%eax
the following will happen between GDB and GDBserver in remote
debugging,
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:3c840408;thread:p2e13.2e13;core:1;
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:3e840408;thread:p2e13.2e13;core:2;
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:42840408;thread:p2e13.2e13;core:2;
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:47840408;thread:p2e13.2e13;core:0;
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:49840408;thread:p2e13.2e13;core:0;
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:4d840408;thread:p2e13.2e13;core:0;
--> vCont;s:p2e13.2e13;c
<-- T0505:68efffbf;04:30efffbf;08:51840408;thread:p2e13.2e13;core:0;
There are some round-trip between GDB and GDBserver. Isn't it boring
and inefficient? so GDB has to say something
different to GDBserver, that is, "keep stepping and report a stop
to me until program goes out of the range of [0x08048434, 0x08048451)"
then, the RSP traffic looks like this,
--> vCont;r8048434,8048451:p2db0.2db0;c
<-- T0505:68efffbf;04:30efffbf;08:51840408;thread:p2db0.2db0;core:1;
As we can see, GDB tells GDBserver a range of stepping, and GDBserver
only sends stop reply back when programs goes out of the range.
The number of packets is reduced dramatically, and as a result,
the performance of stepping a source line is improved. We call it
"range stepping".
In this patch series, we add range stepping support in GDBserver on
x86/linux and in GDB. GDB is able to send "vCont;r" packet for other
remote stubs if "vCont;r" is supported. We tested GDB with GDBserver
on x86_64-linux, and also test mips-sde-elf GDB with a stub understands
range stepping. No regressions.
Is it OK after the release branch is created?
--
1.7.7.6
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH 4/7] range stepping: gdb 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-05-14 18:31 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi ` (7 subsequent siblings) 8 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches This patch teaches GDB to send 'vCont;r' packet when appropriate. GDB has to check whether the target understands 'r' action of 'vCont' packet. We also make use of fields 'step_range_start' and 'step_range_end' of 'struct thread_control_state' to compose the range of the stepping. gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * remote.c (struct support_v_cont) <r>: New field. (remote_vcont_probe): Handle 'r' action of 'vCont' packet. (append_resumption): Send 'vCont;r' for range stepping. --- gdb/remote.c | 36 +++++++++++++++++++++++++++++++++++- 1 files changed, 35 insertions(+), 1 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index ae0ae7e..84e09b0 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -253,6 +253,8 @@ struct support_v_cont { /* True if the stub reports support for vCont;t. */ int t; + /* True if the stub reports support for vCont;r. */ + int r; }; /* Description of the remote protocol state for the currently @@ -4644,6 +4646,7 @@ remote_vcont_probe (struct remote_state *rs) support_c = 0; support_C = 0; rs->support_vCont.t = 0; + rs->support_vCont.r = 0; while (p && *p == ';') { p++; @@ -4657,6 +4660,8 @@ remote_vcont_probe (struct remote_state *rs) support_C = 1; else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) rs->support_vCont.t = 1; + else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0)) + rs->support_vCont.r = 1; p = strchr (p, ';'); } @@ -4689,7 +4694,36 @@ append_resumption (char *p, char *endp, if (step && siggnal != GDB_SIGNAL_0) p += xsnprintf (p, endp - p, ";S%02x", siggnal); else if (step) - p += xsnprintf (p, endp - p, ";s"); + { + struct thread_info *tp = NULL; + CORE_ADDR pc; + + gdb_assert (!ptid_equal (ptid, minus_one_ptid)); + + if (ptid_is_pid (ptid)) + tp = find_thread_ptid (inferior_ptid); + else + tp = find_thread_ptid (ptid); + gdb_assert (tp != NULL); + + pc = regcache_read_pc (get_thread_regcache (ptid)); + if (rs->support_vCont.r /* Target supports step range. */ + /* Can't do range stepping for all threads of a process + 'pPID.-1'. */ + && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) + /* Not step over a breakpoint. */ + && !tp->control.trap_expected + /* We have a range to single step. */ + && THREAD_WITHIN_SINGLE_STEP_RANGE (tp, pc)) + { + p += xsnprintf (p, endp - p, ";r"); + p += hexnumstr (p, tp->control.step_range_start); + p += xsnprintf (p, endp - p, ","); + p += hexnumstr (p, tp->control.step_range_end); + } + else + p += xsnprintf (p, endp - p, ";s"); + } else if (siggnal != GDB_SIGNAL_0) p += xsnprintf (p, endp - p, ";C%02x", siggnal); else -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] range stepping: gdb 2013-03-11 12:53 ` [PATCH 4/7] range stepping: gdb Yao Qi @ 2013-05-14 18:31 ` Pedro Alves 2013-05-15 8:07 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-05-14 18:31 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 03/11/2013 12:51 PM, Yao Qi wrote: > @@ -4689,7 +4694,36 @@ append_resumption (char *p, char *endp, > if (step && siggnal != GDB_SIGNAL_0) > p += xsnprintf (p, endp - p, ";S%02x", siggnal); > else if (step) > - p += xsnprintf (p, endp - p, ";s"); > + { > + struct thread_info *tp = NULL; > + CORE_ADDR pc; > + > + gdb_assert (!ptid_equal (ptid, minus_one_ptid)); I think you'll trip on this in case the target supports vCont but not actually listing thread ids (magic_null_ptid). > + > + if (ptid_is_pid (ptid)) > + tp = find_thread_ptid (inferior_ptid); > + else > + tp = find_thread_ptid (ptid); > + gdb_assert (tp != NULL); > + > + pc = regcache_read_pc (get_thread_regcache (ptid)); > + if (rs->support_vCont.r /* Target supports step range. */ > + /* Can't do range stepping for all threads of a process > + 'pPID.-1'. */ > + && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) > + /* Not step over a breakpoint. */ > + && !tp->control.trap_expected > + /* We have a range to single step. */ > + && THREAD_WITHIN_SINGLE_STEP_RANGE (tp, pc)) This is problematic. It's better to _not_ have target itself decide when to range step or to single step, and peeking at "infrun-owned" variables. For example, with software watchpoints, GDB needs to have control of single-steps, in order to evaluate the watchpoints at after each instruction is executed, so trap_expected isn't enough. (My v3 adds a test for that, that v2 fails.) I dislike the design of using PC checks here too :-/. That seems fragile, and potentially inefficient (considering GDB ever sending more than one range action per packet, that might end up fetching registers for threads unnecessarily). IMO, it's better to have the core run control (infrun.c/infcmd.c) decide when to range step or single-step. > + { > + p += xsnprintf (p, endp - p, ";r"); > + p += hexnumstr (p, tp->control.step_range_start); > + p += xsnprintf (p, endp - p, ","); > + p += hexnumstr (p, tp->control.step_range_end); > + } > + else > + p += xsnprintf (p, endp - p, ";s"); > + } > else if (siggnal != GDB_SIGNAL_0) > p += xsnprintf (p, endp - p, ";C%02x", siggnal); > else > Thanks, -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] range stepping: gdb 2013-05-14 18:31 ` Pedro Alves @ 2013-05-15 8:07 ` Yao Qi 2013-05-20 17:59 ` Pedro Alves 0 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-05-15 8:07 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 05/15/2013 02:31 AM, Pedro Alves wrote: > This is problematic. It's better to_not_ have target itself decide > when to range step or to single step, and peeking at "infrun-owned" > variables. For example, with software watchpoints, GDB needs to have control > of single-steps, in order to evaluate the watchpoints at after each > instruction is executed, so trap_expected isn't enough. (My v3 adds a test for > that, that v2 fails.) I didn't take software watchpoint into account in V2, so probably some problems there. > I dislike the design of using PC checks here too :-/. That > seems fragile, and potentially inefficient (considering GDB ever > sending more than one range action per packet, that might end up > fetching registers for threads unnecessarily). IMO, it's better to have Sorry, I don't understand why it is inefficient. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] range stepping: gdb 2013-05-15 8:07 ` Yao Qi @ 2013-05-20 17:59 ` Pedro Alves 0 siblings, 0 replies; 44+ messages in thread From: Pedro Alves @ 2013-05-20 17:59 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 05/15/2013 09:07 AM, Yao Qi wrote: >> I dislike the design of using PC checks here too :-/. That >> seems fragile, and potentially inefficient (considering GDB ever >> sending more than one range action per packet, that might end up >> fetching registers for threads unnecessarily). IMO, it's better to have > > Sorry, I don't understand why it is inefficient. I meant, thinking forward, if GDB ever sends 100 vCont;r's in the same packet, checking the PC potentially forces fetching the current registers for 100 threads, if GDB doesn't have them (or no longer has them) cached at that point, with the associated extra RSP rountrips/traffic that causes. It seems better not to have a design that forces this refetch. The fragility aspect was a much stronger concern though. I ran the testsuite with an assertion in remote.c to catch the cases of when the PC would be out of the step range. It caught things like displaced stepping, stepping through the dynamic linker, stepping through inlines, and probably others I don't recall right now. Along with software watchpoints, this made wary of other cases (not controlled by trap_expected) where complex run control might end up requiring the target reporting a stop after a single instruction step even if the thread was in the step range to begin with. -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi 2013-03-11 12:53 ` [PATCH 4/7] range stepping: gdb Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-03-11 17:05 ` Eli Zaretskii 2013-05-14 18:31 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 6/7] range stepping: test case Yao Qi ` (6 subsequent siblings) 8 siblings, 2 replies; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * remote.c (use_range_stepping): New. (remote_vcont_probe): Set 'use_range_stepping' if the remote target supports range stepping. (append_resumption): Use 'vCont;r' packet if 'use_range_stepping' is true. (maint_show_range_stepping): New. (maint_set_range_stepping): New. (_initialize_remote): Call add_setshow_boolean_cmd to register commands 'maint set range-stepping' and 'maint show range-stepping'. gdb/doc: 2013-03-11 Yao Qi <yao@codesourcery.com> * gdb.texinfo (Maintenance Commands): Document commands 'maint set range-stepping' and 'maint show range-stepping'. --- gdb/doc/gdb.texinfo | 7 +++++ gdb/remote.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletions(-) diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index a607166..7c06120 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -35547,6 +35547,13 @@ data in a @file{gmon.out} file, be sure to move it to a safe location. Configuring with @samp{--enable-profiling} arranges for @value{GDBN} to be compiled with the @samp{-pg} compiler option. +@kindex maint set range-stepping +@kindex maint show range-stepping +@cindex range-stepping +@item maint set range-stepping +@itemx maint show range-stepping +Control whether to do stepping in an address range. + @kindex maint set show-debug-regs @kindex maint show show-debug-regs @cindex hardware debug registers diff --git a/gdb/remote.c b/gdb/remote.c index 84e09b0..de7391f 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -257,6 +257,10 @@ struct support_v_cont int r; }; +/* A flag on GDB is willing to use range-stepping or not. */ + +static int use_range_stepping = 0; + /* Description of the remote protocol state for the currently connected target. This is per-target state, and independent of the selected architecture. */ @@ -4647,6 +4651,7 @@ remote_vcont_probe (struct remote_state *rs) support_C = 0; rs->support_vCont.t = 0; rs->support_vCont.r = 0; + use_range_stepping = 0; while (p && *p == ';') { p++; @@ -4661,7 +4666,10 @@ remote_vcont_probe (struct remote_state *rs) else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) rs->support_vCont.t = 1; else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0)) - rs->support_vCont.r = 1; + { + rs->support_vCont.r = 1; + use_range_stepping = 1; + } p = strchr (p, ';'); } @@ -4708,6 +4716,8 @@ append_resumption (char *p, char *endp, pc = regcache_read_pc (get_thread_regcache (ptid)); if (rs->support_vCont.r /* Target supports step range. */ + /* GDB is willing to do range stepping. */ + && use_range_stepping /* Can't do range stepping for all threads of a process 'pPID.-1'. */ && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) @@ -11488,6 +11498,45 @@ remote_upload_trace_state_variables (struct uploaded_tsv **utsvp) return 0; } +static void +maint_show_range_stepping (struct ui_file *file, int from_tty, + struct cmd_list_element *c, + const char *value) +{ + fprintf_filtered (file, + _("Debugger's willingness to do range-stepping " + "is %s.\n"), value); +} + +static void +maint_set_range_stepping (char *ignore_args, int from_tty, + struct cmd_list_element *c) +{ + /* Check range stepping is supported when turns it on. */ + if (use_range_stepping) + { + if (remote_desc != NULL) + { + struct remote_state *rs = get_remote_state (); + + if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN) + remote_vcont_probe (rs); + + if (remote_protocol_packets[PACKET_vCont].support == PACKET_DISABLE + || !rs->support_vCont.r) + { + use_range_stepping = 0; + error (_("Range stepping is not supported")); + } + } + else + { + use_range_stepping = 0; + error (_("Range stepping is not supported")); + } + } +} + void _initialize_remote (void) { @@ -11873,6 +11922,19 @@ Set the remote pathname for \"run\""), _("\ Show the remote pathname for \"run\""), NULL, NULL, NULL, &remote_set_cmdlist, &remote_show_cmdlist); + add_setshow_boolean_cmd ("range-stepping", class_maintenance, + &use_range_stepping, _("\ +Enable or disable range-stepping."), _("\ +Show whether range-stepping is enabled."), _("\ +If On, GDB will tell the target to do stepping a range of address.\n\ +This will speed up stepping a line of source file.\n\ +If off, GDB will not use it, even if such is supported by the \n\ +target"), + maint_set_range_stepping, + maint_show_range_stepping, + &maintenance_set_cmdlist, + &maintenance_show_cmdlist); + /* Eventually initialize fileio. See fileio.c */ initialize_remote_fileio (remote_set_cmdlist, remote_show_cmdlist); -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-03-11 12:53 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi @ 2013-03-11 17:05 ` Eli Zaretskii 2013-03-18 3:10 ` Yao Qi 2013-05-14 18:31 ` Pedro Alves 1 sibling, 1 reply; 44+ messages in thread From: Eli Zaretskii @ 2013-03-11 17:05 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > From: Yao Qi <yao@codesourcery.com> > Date: Mon, 11 Mar 2013 20:51:29 +0800 > > +@kindex maint set range-stepping > +@kindex maint show range-stepping > +@cindex range-stepping > +@item maint set range-stepping > +@itemx maint show range-stepping > +Control whether to do stepping in an address range. Why is this a maintenance command? Anyway, the documentation part is OK. > + add_setshow_boolean_cmd ("range-stepping", class_maintenance, > + &use_range_stepping, _("\ > +Enable or disable range-stepping."), _("\ > +Show whether range-stepping is enabled."), _("\ > +If On, GDB will tell the target to do stepping a range of address.\n\ I suggest If ON, GDB will send a single packet to a target telling it to keep single-stepping as long as PC is inside a certain range of addresses. > +This will speed up stepping a line of source file.\n\ > +If off, GDB will not use it, even if such is supported by the \n\ "will not use this feature". "It" is ambiguous here. Thanks. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-03-11 17:05 ` Eli Zaretskii @ 2013-03-18 3:10 ` Yao Qi 2013-03-18 5:39 ` Eli Zaretskii 0 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-03-18 3:10 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches On 03/12/2013 01:04 AM, Eli Zaretskii wrote: >> +@kindex maint set range-stepping >> >+@kindex maint show range-stepping >> >+@cindex range-stepping >> >+@item maint set range-stepping >> >+@itemx maint show range-stepping >> >+Control whether to do stepping in an address range. > Why is this a maintenance command? Don't have any special reasons to do this. The range stepping should be always on if target supports, and it can be turned off for some "maintenance purpose", so I set this command as a maintenance command. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-03-18 3:10 ` Yao Qi @ 2013-03-18 5:39 ` Eli Zaretskii 0 siblings, 0 replies; 44+ messages in thread From: Eli Zaretskii @ 2013-03-18 5:39 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > Date: Mon, 18 Mar 2013 10:08:37 +0800 > From: Yao Qi <yao@codesourcery.com> > CC: <gdb-patches@sourceware.org> > > On 03/12/2013 01:04 AM, Eli Zaretskii wrote: > >> +@kindex maint set range-stepping > >> >+@kindex maint show range-stepping > >> >+@cindex range-stepping > >> >+@item maint set range-stepping > >> >+@itemx maint show range-stepping > >> >+Control whether to do stepping in an address range. > > Why is this a maintenance command? > > Don't have any special reasons to do this. The range stepping should be > always on if target supports, and it can be turned off for some > "maintenance purpose", so I set this command as a maintenance command. What would be the possible reasons to turn this feature off? Once we have the reasons, we could try thinking whether or not they are limited to GDB maintenance. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-03-11 12:53 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi 2013-03-11 17:05 ` Eli Zaretskii @ 2013-05-14 18:31 ` Pedro Alves 1 sibling, 0 replies; 44+ messages in thread From: Pedro Alves @ 2013-05-14 18:31 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 03/11/2013 12:51 PM, Yao Qi wrote: > @@ -4647,6 +4651,7 @@ remote_vcont_probe (struct remote_state *rs) > support_C = 0; > rs->support_vCont.t = 0; > rs->support_vCont.r = 0; > + use_range_stepping = 0; > while (p && *p == ';') > { > p++; > @@ -4661,7 +4666,10 @@ remote_vcont_probe (struct remote_state *rs) > else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) > rs->support_vCont.t = 1; > else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0)) > - rs->support_vCont.r = 1; > + { > + rs->support_vCont.r = 1; > + use_range_stepping = 1; > + } I don't think this is a good approach. :-( If the user does "maint set range-stepping", and then connects, she'll reasonably assume range stepping will be disabled. She could have a stub that has known broken range stepping, for example. However, with this approach, if the target supports it, range stepping will always end up forced enabled as soon as GDB sends the first vCont packet, even if the user set range stepping off. So I think the "maint set range-stepping" toggle should be enabled by default, and it should only represent GDB's willingness to use the feature. > > p = strchr (p, ';'); > } > @@ -4708,6 +4716,8 @@ append_resumption (char *p, char *endp, > > pc = regcache_read_pc (get_thread_regcache (ptid)); > if (rs->support_vCont.r /* Target supports step range. */ > + /* GDB is willing to do range stepping. */ > + && use_range_stepping > /* Can't do range stepping for all threads of a process > 'pPID.-1'. */ > && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) > @@ -11488,6 +11498,45 @@ remote_upload_trace_state_variables (struct uploaded_tsv **utsvp) > return 0; > } > > +static void > +maint_show_range_stepping (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, > + const char *value) > +{ > + fprintf_filtered (file, > + _("Debugger's willingness to do range-stepping " > + "is %s.\n"), value); > +} > + > +static void > +maint_set_range_stepping (char *ignore_args, int from_tty, > + struct cmd_list_element *c) > +{ > + /* Check range stepping is supported when turns it on. */ > + if (use_range_stepping) > + { > + if (remote_desc != NULL) > + { > + struct remote_state *rs = get_remote_state (); > + > + if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN) > + remote_vcont_probe (rs); > + > + if (remote_protocol_packets[PACKET_vCont].support == PACKET_DISABLE > + || !rs->support_vCont.r) If we check instead for PACKET_ENABLE && rs->support_vCont.r here, > + { > + use_range_stepping = 0; > + error (_("Range stepping is not supported")); > + } > + } > + else > + { > + use_range_stepping = 0; > + error (_("Range stepping is not supported")); > + } Then we can merge these two errors. Following up on the "willingness-only" idea, I'd rather make this a warning though. In v3, I've make moved the setting out of "maint". -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 6/7] range stepping: test case 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi 2013-03-11 12:53 ` [PATCH 4/7] range stepping: gdb Yao Qi 2013-03-11 12:53 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-05-14 18:32 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi ` (5 subsequent siblings) 8 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches This test case is used to verify range stepping is used by means of checking the rsp packets. gdb/testsuite: 2013-03-11 Yao Qi <yao@codesourcery.com> * lib/range-stepping-support.exp: New. * gdb.base/range-stepping.c: New. * gdb.base/range-stepping.exp: New. * gdb.trace/range-stepping.c: New. * gdb.trace/range-stepping.exp: New. --- gdb/testsuite/gdb.base/range-stepping.c | 69 ++++++++ gdb/testsuite/gdb.base/range-stepping.exp | 220 ++++++++++++++++++++++++++ gdb/testsuite/gdb.trace/range-stepping.c | 61 +++++++ gdb/testsuite/gdb.trace/range-stepping.exp | 88 ++++++++++ gdb/testsuite/lib/range-stepping-support.exp | 55 +++++++ 5 files changed, 493 insertions(+), 0 deletions(-) create mode 100644 gdb/testsuite/gdb.base/range-stepping.c create mode 100644 gdb/testsuite/gdb.base/range-stepping.exp create mode 100644 gdb/testsuite/gdb.trace/range-stepping.c create mode 100644 gdb/testsuite/gdb.trace/range-stepping.exp create mode 100644 gdb/testsuite/lib/range-stepping-support.exp diff --git a/gdb/testsuite/gdb.base/range-stepping.c b/gdb/testsuite/gdb.base/range-stepping.c new file mode 100644 index 0000000..930cb8d --- /dev/null +++ b/gdb/testsuite/gdb.base/range-stepping.c @@ -0,0 +1,69 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2013 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +static int +func1 (int a, int b) +{ + int r = a * b; + + r =+ (a | b); + r =+ (a - b); + + return r; +} + +int +main(void) +{ + int a = 0; + int b = 1; + int c = 2; + int d = 3; + int e = 4; + double d1 = 1.0; + double d2 = 2.0; + + /* A line of source will be generated to a number of + instructions by compiler. */ + a = b + c + d * e - a; /* location 1 */ + + /* To compose multiple instructions before and after + 'function call' or 'branch' instruction. This line + of source will be generated to the following instructions, + +addr1: + insn1; + insn2; + ... + call func1; + ... + insn3; +addr2: + insn4; */ + e = 10 + func1 (a + b, c * d); /* location 2 */ + + e = 10 + func1 (a + b, c * d); + /* Generate a range that includes a loop in it. */ + for (a = 0, e = 0; a < 15; a++) { e += a;} + /* Generate a range that includes a loop in it. */ + for (a = 0, e = 0; a < 15; a++) { e += a;} + /* Generate a range that includes a loop, which is time consuming. + Variable C is used to terminate the loop earlier when GDB + wants. */ + for (c = 1, a = 0; a < 65535 && c; a++) {for (b = 0; b < 65535 && c; b++) { d1 = d2 * a / b; d2 = d1 * a;}} + return 0; +} diff --git a/gdb/testsuite/gdb.base/range-stepping.exp b/gdb/testsuite/gdb.base/range-stepping.exp new file mode 100644 index 0000000..90083d6 --- /dev/null +++ b/gdb/testsuite/gdb.base/range-stepping.exp @@ -0,0 +1,220 @@ +# Copyright 2013 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +load_lib "range-stepping-support.exp" + +standard_testfile .c +set executable $testfile + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \ + executable {debug nowarnings}] != "" } { + untested ${testfile}.exp + return -1 +} + +clean_restart $executable + +if ![runto_main] { + fail "Can't run to main" + return -1 +} + +# Check range stepping is supported by target or not. +set test "range-stepping is supported" +gdb_test_multiple "maintenance show range-stepping" "show range-stepping" { + -re "Debugger's willingness to do range-stepping is off.*\r\n$gdb_prompt $" { + unsupported $test + return -1 + } + -re "Debugger's willingness to do range-stepping is on.*\r\n$gdb_prompt $" { + pass $test + } +} + +# Check range stepping can step a range of multiple instructions. + +with_test_prefix "multi insns" { + global executable + global hex decimal + global gdb_prompt + + gdb_breakpoint [gdb_get_line_number "location 1"] + gdb_continue_to_breakpoint "location 1" + + set pc_before_stepping "" + set test "pc before stepping" + gdb_test_multiple "print/x \$pc" $test { + -re "\\\$$decimal = (\[^\r\n\]*)\r\n$gdb_prompt $" { + set pc_before_stepping $expect_out(1,string) + pass $test + } + } + + # When execute command "next", GDB should send one vCont;s and + # vCont;r and receive two stop replies. + # --> vCont;s (step over breakpoint) + # <-- T0505:XXX + # --> vCont;rSTART,END (do range stepping) + # <-- T0505:XXX + exec_cmd_and_check_rsp_packets "next" 1 1 + + set pc_after_stepping "" + set msg "pc before stepping" + gdb_test_multiple "print/x \$pc" $msg { + -re "\\\$$decimal = (\[^\r\n\]*)\r\n$gdb_prompt $" { + set pc_after_stepping $expect_out(1,string) + pass $msg + } + } + + # At least, there are two instructions between + # PC_BEFORE_STEPPING and PC_AFTER_STEPPING. + gdb_test "disassemble ${pc_before_stepping},${pc_after_stepping}" \ + "${hex} <main\\+${decimal}>:.*${hex} <main\\+${decimal}>:.*" \ + "stepped multiple insns" +} + +# Check range stepping can step over a function. + +with_test_prefix "step over func" { + global srcfile + + set line_num [gdb_get_line_number "location 2"] + gdb_test "where" "main \\(\\) at .*${srcfile}:${line_num}.*" + + # It is expected to get three stops and two 'vCont;r'. In the C + # code, the line of C source produces the following instructions, + # + # addr1: + # insn1; + # insn2; + # ... + # call func1; + # addr2: + # ... + # insn3; + # addr3: + # insn4; + # + # Something like this will happen: + # --> vCont;rADDR1,ADDR3 (do range stepping from ADDR1 to ADDR3) + # <-- T0505:XXXX + # (inferior is single-stepping to func, which is out of the range) + # --> $Z0,ADDR2 + # --> vCont;c (set step-resume breakpoint on ADDR2, and resume) + # <-- T0505:XXXX (inferior stops at ADD2) + # --> vCont;rADDR1,ADDR3 (continues to do range stepping) + # <-- T0505:XXXX + exec_cmd_and_check_rsp_packets "next" 0 2 +} + +# Check range stepping works well with breakpoint. + +with_test_prefix "breakpoint" { + gdb_breakpoint "func1" + # Something like this will happen: + # --> vCont;rADDR1,ADDR3 + # <-- T0505:XXXX + # (inferior is single-stepping to func1, which is out of the range) + # --> $Z0,ADDR2 + # --> vCont;c (set step-resume breakpoint on ADDR2, and resume) + # <-- T0505:XXXX (inferior hits the breakpoint on fun1) + exec_cmd_and_check_rsp_packets "next" 0 1 + + gdb_test "backtrace" "#0 .* func1 .*#1 .* main .*" \ + "backtrace from func1" + # The range stepping was interrupted by a breakpoint, but the + # state related to range stepping should be cleared. + exec_cmd_and_check_rsp_packets "stepi" 1 0 + gdb_test "finish" ".*" + + gdb_test "next" ".*" + delete_breakpoints +} + +# Check range stepping works well with a loop in the range. + +with_test_prefix "loop" { + + # GDB should send one vCont;r and receive one stop reply. + # --> vCont;rSTART,END (do range stepping) + # <-- T0505:XXX + exec_cmd_and_check_rsp_packets "next" 0 1 + + # Check loop is done. + gdb_test "print a" "\\$${decimal} = 15\[\r\n\].*" + gdb_test "print e" "\\$${decimal} = 105\[\r\n\].*" +} + +# Check range stepping works well when PC has already within the loop +# body.. + +with_test_prefix "loop" { + # Stepi into the loop body. 15 is large enough to make sure + # program goes to loop body. + gdb_test "stepi 15" ".*" + # GDB should send one vCont;r and receive one stop reply. + # --> vCont;rSTART,END (do range stepping) + # <-- T0505:XXX + exec_cmd_and_check_rsp_packets "next" 0 1 + + # Check loop is done. + gdb_test "print a" "\\$${decimal} = 15\[\r\n\].*" + gdb_test "print e" "\\$${decimal} = 105\[\r\n\].*" +} + +# Check range stepping works well when it is interrupted by ctrl-c. + +with_test_prefix "interrupt" { + gdb_test_no_output "set debug remote 1" + + send_gdb "next\n" + sleep 1 + send_gdb "\003" + + # GDB should send one vCont;r and receive one stop reply about + # SIGINT. + # --> vCont;rSTART,END (do range stepping) + # <-- T0205:XXX + + set vcont_r_counter 0 + + set test "send gdb ctrl-c" + gdb_expect { + -re "vCont;r\[^\r\n\]*\.\.\." { + incr vcont_r_counter + exp_continue + } + -re "Program received signal SIGINT.*$gdb_prompt $" { + pass $test + } + timeout { fail "${test} (timeout)" } + eof { fail "${test} (eof)" } + } + gdb_test_no_output "set debug remote 0" + + # Check the number of 'vCont;r' packets. + if { $vcont_r_counter == 1 } { + pass "${test}: 1 vCont;r" + } else { + fail "${test}: 1 vCont;r" + } + + # Terminate the loop earlier and continue to do range stepping. + gdb_test "set variable c = 0" + exec_cmd_and_check_rsp_packets "next" 0 1 +} + +return 0 diff --git a/gdb/testsuite/gdb.trace/range-stepping.c b/gdb/testsuite/gdb.trace/range-stepping.c new file mode 100644 index 0000000..50db5a0 --- /dev/null +++ b/gdb/testsuite/gdb.trace/range-stepping.c @@ -0,0 +1,61 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2013 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifdef SYMBOL_PREFIX +#define SYMBOL(str) SYMBOL_PREFIX #str +#else +#define SYMBOL(str) #str +#endif + +/* Called from asm. */ +static void __attribute__((used)) +func2 (void) +{} + +static int +func1 (int a, int b) +{ + int r = a * b; + + /* `set_point' is the label where we'll set tracepoint at. The insn + at the label must the large enough to fit a fast tracepoint + jump. */ + asm (" .global " SYMBOL(set_point) "\n" + SYMBOL(set_point) ":\n" +#if (defined __x86_64__ || defined __i386__) + " call " SYMBOL(func2) "\n" +#endif + ); + + r =+ (a | b); + r =+ (a - b); + + return r; +} + +int +main(void) +{ + int a = 0; + int b = 1; + int c = 2; + int d = 3; + int e = 4; + + e = 10 + func1 (a + b, c * d); /* location 1 */ + return 0; +} diff --git a/gdb/testsuite/gdb.trace/range-stepping.exp b/gdb/testsuite/gdb.trace/range-stepping.exp new file mode 100644 index 0000000..6ff3ffa --- /dev/null +++ b/gdb/testsuite/gdb.trace/range-stepping.exp @@ -0,0 +1,88 @@ +# Copyright 2013 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +load_lib "trace-support.exp" +load_lib "range-stepping-support.exp" + +standard_testfile +set executable $testfile +set expfile $testfile.exp + +if [prepare_for_testing $expfile $executable $srcfile \ + {debug nowarnings}] { + untested "failed to prepare for trace tests" + return -1 +} + +if ![runto_main] { + fail "Can't run to main to check for trace support" + return -1 +} + +if ![gdb_target_supports_trace] { + unsupported "target does not support trace" + return -1; +} + +# Check range stepping works well with tracepoint. +proc range_stepping_with_tracepoint { type } { + with_test_prefix "${type}" { + + gdb_breakpoint [gdb_get_line_number "location 1"] + gdb_continue_to_breakpoint "location 1" + delete_breakpoints + + gdb_test "${type} set_point" ".*" + gdb_test_no_output "tstart" + + # GDB will step over function func1, in which a tracepoint is + # set. GDB will send two vCont;r packets because calling to + # func1 is out of the range. However, tracepoint itself + # shouldn't have any effect on range stepping. + exec_cmd_and_check_rsp_packets "next" 0 2 + gdb_test_no_output "tstop" + gdb_test "tfind" "Found trace frame 0.*" "tfind 0" + gdb_test "tfind" \ + "Target failed to find requested trace frame.*" \ + "tfind 1" + + delete_breakpoints + } +} + +range_stepping_with_tracepoint "trace" + +set libipa [get_in_proc_agent] +gdb_load_shlibs $libipa + +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \ + executable [list debug nowarnings shlib=$libipa] ] != "" } { + untested "failed to compile ftrace tests" + return -1 +} + +clean_restart ${executable} + +if ![runto_main] { + fail "Can't run to main for ftrace tests" + return 0 +} + +gdb_reinitialize_dir $srcdir/$subdir +if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } { + untested "Could not find IPA lib loaded" +} else { + range_stepping_with_tracepoint "ftrace" +} diff --git a/gdb/testsuite/lib/range-stepping-support.exp b/gdb/testsuite/lib/range-stepping-support.exp new file mode 100644 index 0000000..c46e92d --- /dev/null +++ b/gdb/testsuite/lib/range-stepping-support.exp @@ -0,0 +1,55 @@ +# Copyright 2013 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Execute command CMD and check the expected RSP packets +# (vCont;s and vCont;r) during range stepping. + +proc exec_cmd_and_check_rsp_packets { cmd vcont_s vcont_r} { + global gdb_prompt + + gdb_test_no_output "set debug remote 1" "" + + set test "range stepping" + set vcont_r_counter 0 + set vcont_s_counter 0 + gdb_test_multiple $cmd $test { + -re "vCont;s\[^\r\n\]*Packet received: T\[\[:xdigit:\]\]\[\[:xdigit:\]\]" { + incr vcont_s_counter + exp_continue + } + -re "vCont;r\[^\r\n\]*Packet received: T\[\[:xdigit:\]\]\[\[:xdigit:\]\]" { + incr vcont_r_counter + exp_continue + } + -re "$gdb_prompt $" { + pass $test + } + } + gdb_test_no_output "set debug remote 0" "" + + # Check the number of 'vCont;r' packets. + if { $vcont_r_counter == ${vcont_r} } { + pass "${test}: ${vcont_r} vCont;r" + } else { + fail "${test}: ${vcont_r} vCont;r" + } + + # Check the number of 'vCont;s' packets. + if { $vcont_s_counter == ${vcont_s} } { + pass "${test}: ${vcont_s} vCont;s" + } else { + fail "${test}: ${vcont_s} vCont;s" + } +} -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 6/7] range stepping: test case 2013-03-11 12:53 ` [PATCH 6/7] range stepping: test case Yao Qi @ 2013-05-14 18:32 ` Pedro Alves 2013-05-15 8:27 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-05-14 18:32 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches Hi Yao, Thanks a lot for writing this. On 03/11/2013 12:51 PM, Yao Qi wrote: > This test case is used to verify range stepping is used by means of > checking the rsp packets. I agree with the idea. There's not much black box style testing we can do, as GDB is supposed to work the same from the user's perspective, with or without range stepping. I prefer this over the convenience variable hack. If we ever make range stepping work on a target !remote, we can switch to look at "set debug infrun 1" output or some new "set debug range-step" output. I didn't understand your stated reason in v2 for switching away from this. If it was that expect buffer overflows, v3 has a tweak that likely fixes it. > + > +static int > +func1 (int a, int b) > +{ > + int r = a * b; > + > + r =+ (a | b); > + r =+ (a - b); "=+" very much looks like a typo for "+=" (and appears elsewhere too). > + > + return r; > +} > + > +int > +main(void) > +{ > + int a = 0; > + int b = 1; > + int c = 2; > + int d = 3; > + int e = 4; > + double d1 = 1.0; > + double d2 = 2.0; It's probably a good idea to make these volatile, > + > + /* A line of source will be generated to a number of > + instructions by compiler. */ > + a = b + c + d * e - a; /* location 1 */ in case the compiler folds these, even at -O0. We really want to make sure these are multiple instructions. I think that for softfp targets, the compiler will end up emitting calls (which break the tests) for these uses of floats, though I have left those in v3 too. > + > + /* To compose multiple instructions before and after > + 'function call' or 'branch' instruction. This line > + of source will be generated to the following instructions, > + > +addr1: > + insn1; > + insn2; > + ... > + call func1; > + ... > + insn3; > +addr2: > + insn4; */ > + e = 10 + func1 (a + b, c * d); /* location 2 */ > + > + e = 10 + func1 (a + b, c * d); > + /* Generate a range that includes a loop in it. */ > + for (a = 0, e = 0; a < 15; a++) { e += a;} > + /* Generate a range that includes a loop in it. */ > + for (a = 0, e = 0; a < 15; a++) { e += a;} > + /* Generate a range that includes a loop, which is time consuming. > + Variable C is used to terminate the loop earlier when GDB > + wants. */ > + for (c = 1, a = 0; a < 65535 && c; a++) {for (b = 0; b < 65535 && c; b++) { d1 = d2 * a / b; d2 = d1 * a;}} We can put these multi-statement lines behind macros, so we can still write them in multiple lines, making it easier to read. > + > +standard_testfile .c .c is the default. > + # When execute command "next", GDB should send one vCont;s and > + # vCont;r and receive two stop replies. > + # --> vCont;s (step over breakpoint) > + # <-- T0505:XXX The second 05 in "T0505" is a register number. Best leave it out. > + # GDB should send one vCont;r and receive one stop reply about > + # SIGINT. > + # --> vCont;rSTART,END (do range stepping) > + # <-- T0205:XXX > + > + set vcont_r_counter 0 > + > + set test "send gdb ctrl-c" > + gdb_expect { I didn't see why this needed to be gdb_expect instead of gdb_test_multiple. > + -re "vCont;r\[^\r\n\]*\.\.\." { > + incr vcont_r_counter > + exp_continue > + } > + -re "Program received signal SIGINT.*$gdb_prompt $" { > + pass $test > + } > + timeout { fail "${test} (timeout)" } > + eof { fail "${test} (eof)" } > + } > + gdb_test_no_output "set debug remote 0" > + > + # Check the number of 'vCont;r' packets. > + if { $vcont_r_counter == 1 } { > + pass "${test}: 1 vCont;r" > + } else { > + fail "${test}: 1 vCont;r" > + } > + > + # Terminate the loop earlier and continue to do range stepping. > + gdb_test "set variable c = 0" > + exec_cmd_and_check_rsp_packets "next" 0 1 > +} > + > +static int > +func1 (int a, int b) > +{ > + int r = a * b; > + > + /* `set_point' is the label where we'll set tracepoint at. The insn > + at the label must the large enough to fit a fast tracepoint > + jump. */ > + asm (" .global " SYMBOL(set_point) "\n" > + SYMBOL(set_point) ":\n" > +#if (defined __x86_64__ || defined __i386__) > + " call " SYMBOL(func2) "\n" > +#endif > + ); > + > + r =+ (a | b); > + r =+ (a - b); > + > + return r; > +} > + > +int > +main(void) > +{ > + int a = 0; > + int b = 1; > + int c = 2; > + int d = 3; > + int e = 4; > + > + e = 10 + func1 (a + b, c * d); /* location 1 */ > + return 0; > +} > +# Check range stepping works well with tracepoint. > +proc range_stepping_with_tracepoint { type } { > + with_test_prefix "${type}" { > + > + gdb_breakpoint [gdb_get_line_number "location 1"] > + gdb_continue_to_breakpoint "location 1" > + delete_breakpoints > + > + gdb_test "${type} set_point" ".*" > + gdb_test_no_output "tstart" > + > + # GDB will step over function func1, in which a tracepoint is > + # set. GDB will send two vCont;r packets because calling to > + # func1 is out of the range. However, tracepoint itself > + # shouldn't have any effect on range stepping. I think we'd be more certain the tracepoint doesn't affect range stepping if it is set in the middle of a line that is a single range. I changed the test's source to do a 5-byte nop instead of a "call" in the asm part (and removed func1). > + exec_cmd_and_check_rsp_packets "next" 0 2 > + gdb_test_no_output "tstop" > + gdb_test "tfind" "Found trace frame 0.*" "tfind 0" > + gdb_test "tfind" \ > + "Target failed to find requested trace frame.*" \ > + "tfind 1" > + > + delete_breakpoints > + } > +} > + > +proc exec_cmd_and_check_rsp_packets { cmd vcont_s vcont_r} { > + global gdb_prompt > + > + gdb_test_no_output "set debug remote 1" "" > + > + set test "range stepping" > + set vcont_r_counter 0 > + set vcont_s_counter 0 > + gdb_test_multiple $cmd $test { > + -re "vCont;s\[^\r\n\]*Packet received: T\[\[:xdigit:\]\]\[\[:xdigit:\]\]" { > + incr vcont_s_counter > + exp_continue > + } > + -re "vCont;r\[^\r\n\]*Packet received: T\[\[:xdigit:\]\]\[\[:xdigit:\]\]" { > + incr vcont_r_counter > + exp_continue > + } > + -re "$gdb_prompt $" { > + pass $test > + } > + } > + gdb_test_no_output "set debug remote 0" "" > + > + # Check the number of 'vCont;r' packets. > + if { $vcont_r_counter == ${vcont_r} } { > + pass "${test}: ${vcont_r} vCont;r" > + } else { > + fail "${test}: ${vcont_r} vCont;r" > + } > + > + # Check the number of 'vCont;s' packets. > + if { $vcont_s_counter == ${vcont_s} } { > + pass "${test}: ${vcont_s} vCont;s" > + } else { > + fail "${test}: ${vcont_s} vCont;s" > + } This generates three PASS/FAILs per test. I'd prefer merging them into one. In v3, I tweaked a lot many little details in formatting and wording, but the essence is mostly the same. -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 6/7] range stepping: test case 2013-05-14 18:32 ` Pedro Alves @ 2013-05-15 8:27 ` Yao Qi 2013-05-20 18:29 ` Pedro Alves 0 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-05-15 8:27 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 05/15/2013 02:32 AM, Pedro Alves wrote: > I prefer this over the convenience variable hack. If we ever > make range stepping work on a target !remote, we can switch to > look at "set debug infrun 1" output or some new "set debug range-step" > output. I didn't understand your stated reason in v2 for > switching away from this. If it was that expect buffer > overflows, v3 has a tweak that likely fixes it. In our tests, when running tests with a problematic stub supporting range stepping, the range stepping of the stub falls back to the single stepping. This will generate a huge number of rsp packets, and the gdb.log will blow up to several giga-byte. We use convince variable to avoid this problem. On the other hand, I don't think it is a good idea to peek the GDB internal states by checking some rsp packets and personally I prefer the way that GDB is able to expose some internal states by some means (command "maint" and convince variables, for example). -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 6/7] range stepping: test case 2013-05-15 8:27 ` Yao Qi @ 2013-05-20 18:29 ` Pedro Alves 2013-05-22 14:01 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-05-20 18:29 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 05/15/2013 09:27 AM, Yao Qi wrote: > On 05/15/2013 02:32 AM, Pedro Alves wrote: >> I prefer this over the convenience variable hack. If we ever >> make range stepping work on a target !remote, we can switch to >> look at "set debug infrun 1" output or some new "set debug range-step" >> output. I didn't understand your stated reason in v2 for >> switching away from this. If it was that expect buffer >> overflows, v3 has a tweak that likely fixes it. > > In our tests, when running tests with a problematic stub supporting range > stepping, the range stepping of the stub falls back to the single stepping. > This will generate a huge number of rsp packets, and the gdb.log will blow up > to several giga-byte. I see. Thanks, that's much more detailed info than just saying "it's unsafe". I'm guessing the huge number of RSP packets comes from that big loop in the test: /* Generate a range that includes a loop, which is time consuming. Variable C is used to terminate the loop earlier when GDB wants. */ for (c = 1, a = 0; a < 65535 && c; a++) {for (b = 0; b < 65535 && c; b++) { d1 = d2 * a / b; d2 = d1 * We could skip most of the range stepping tests if e.g., the test that steps the short line FAILs: /* A line of source will be generated to a number of instructions by compiler. */ a = b + c + d * e - a; /* location 1 */ WDYT? > We use convince variable to avoid this problem. On the other > hand, I don't think it is a good idea to peek the GDB internal states by > checking some rsp packets and personally I prefer the way that GDB is able to expose > some internal states by some means (command "maint" and convince variables, for example). I don't like using a convenience variable for this, as those are visible to the user (show convenience). "maint" seems better. But I'm not certain of it. An issue I have with it is that from the log you just see the condensed report of what happened (sent 4 vCont;r, should have been 3), while to diagnose the issue you'll most likely need to get more info than that ("Okay, what was really sent? What were the ranges? Were there signals interrupting the range? Etc."). I have a suspicion we'll end up needing to end up with "set debug infrun 1" on, and look at that too for some of the trickier cases, and end up with different number of expected ranges depending on target on some cases. The actual difference between v1 -> v2 wrt to the RSP packets vs convenience var was surprisingly smaller than I anticipated. So how about we go with RSP first, and see how things go from there? -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 6/7] range stepping: test case 2013-05-20 18:29 ` Pedro Alves @ 2013-05-22 14:01 ` Yao Qi 0 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-05-22 14:01 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 05/21/2013 02:28 AM, Pedro Alves wrote: > I see. Thanks, that's much more detailed info than just saying "it's unsafe". > > I'm guessing the huge number of RSP packets comes from that big loop in the > test: > > /* Generate a range that includes a loop, which is time consuming. > Variable C is used to terminate the loop earlier when GDB > wants. */ > for (c = 1, a = 0; a < 65535 && c; a++) {for (b = 0; b < 65535 && c; b++) { d1 = d2 * a / b; d2 = d1 * > > We could skip most of the range stepping tests if e.g., the > test that steps the short line FAILs: > > /* A line of source will be generated to a number of > instructions by compiler. */ > a = b + c + d * e - a; /* location 1 */ > > WDYT? Pedro, That is a good idea. It works for my internal stub! I'll post a delta patch on top of yours. > >> >We use convince variable to avoid this problem. On the other >> >hand, I don't think it is a good idea to peek the GDB internal states by >> >checking some rsp packets and personally I prefer the way that GDB is able to expose >> >some internal states by some means (command "maint" and convince variables, for example). > I don't like using a convenience variable for this, as those are visible > to the user (show convenience). > > "maint" seems better. But I'm not certain of it. An issue I have with it > is that from the log you just see the condensed report of what happened > (sent 4 vCont;r, should have been 3), while to diagnose the issue you'll > most likely need to get more info than that ("Okay, what was really sent? > What were the ranges? Were there signals interrupting the range? Etc."). I > have a suspicion we'll end up needing to end up with "set debug infrun 1" > on, and look at that too for some of the trickier cases, and end up with > different number of expected ranges depending on target on some cases. > > The actual difference between v1 -> v2 wrt to the RSP packets vs > convenience var was surprisingly smaller than I anticipated. So how > about we go with RSP first, and see how things go from there? I agree. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi ` (2 preceding siblings ...) 2013-03-11 12:53 ` [PATCH 6/7] range stepping: test case Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-05-14 19:24 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 2/7] Move rs->support_vCont_t to a separate struct Yao Qi ` (4 subsequent siblings) 8 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches This patch moves code into a macro, which will be used in my following patches. gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * gdbthread.h (THREAD_WITHIN_SINGLE_STEP_RANGE): New macro. * infrun.c (handle_inferior_event): Use it. --- gdb/gdbthread.h | 6 ++++++ gdb/infrun.c | 6 ++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 824e4d0..6f47006 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -37,6 +37,12 @@ enum thread_state THREAD_EXITED, }; +/* PC is within the range of single stepping of thread THR. */ + +#define THREAD_WITHIN_SINGLE_STEP_RANGE(THR, PC) \ + ((PC) >= (THR)->control.step_range_start \ + && (PC) < (THR)->control.step_range_end) + /* Inferior thread specific part of `struct infcall_control_state'. Inferior process counterpart is `struct inferior_control_state'. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index 1cf30fe..5bc276c 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4340,8 +4340,7 @@ process_event_stop_test: if (ecs->event_thread->control.step_range_end != 0 && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_0 - && (ecs->event_thread->control.step_range_start <= stop_pc - && stop_pc < ecs->event_thread->control.step_range_end) + && THREAD_WITHIN_SINGLE_STEP_RANGE (ecs->event_thread, stop_pc) && frame_id_eq (get_stack_frame_id (frame), ecs->event_thread->control.step_stack_frame_id) && ecs->event_thread->control.step_resume_breakpoint == NULL) @@ -4710,8 +4709,7 @@ process_event_stop_test: through a function epilogue and therefore must detect when the current-frame changes in the middle of a line. */ - if (stop_pc >= ecs->event_thread->control.step_range_start - && stop_pc < ecs->event_thread->control.step_range_end + if (THREAD_WITHIN_SINGLE_STEP_RANGE (ecs->event_thread, stop_pc) && (execution_direction != EXEC_REVERSE || frame_id_eq (get_frame_id (frame), ecs->event_thread->control.step_frame_id))) -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE 2013-03-11 12:53 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi @ 2013-05-14 19:24 ` Pedro Alves 0 siblings, 0 replies; 44+ messages in thread From: Pedro Alves @ 2013-05-14 19:24 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches Hi Yao, Finally I'm replying to this series. As I said before, I ended up working on the code, and I'll post a v3 next. I'm still replying to the individual patches, to point out some of the issues I found. On 03/11/2013 12:51 PM, Yao Qi wrote: > This patch moves code into a macro, which will be used in my > following patches. I agree with the idea. > 2013-03-11 Yao Qi <yao@codesourcery.com> > > * gdbthread.h (THREAD_WITHIN_SINGLE_STEP_RANGE): New macro. > * infrun.c (handle_inferior_event): Use it. However, the gdbserver patch does: +/* Return true if THREAD is in range stepping. PC is the value of pc + of THREAD. */ + +int +thread_in_range_stepping_p (struct thread_info *thread, CORE_ADDR pc) +{ + return (pc >= thread->start && pc < thread->end); } If we have code doing the exact same at the same conceptual level in both gdb and gdbserver, I'd rather it is similar. -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 2/7] Move rs->support_vCont_t to a separate struct 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi ` (3 preceding siblings ...) 2013-03-11 12:53 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-03-11 12:53 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi ` (3 subsequent siblings) 8 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches This is a refactor patch to move field 'support_vCont_t' to a separate struct 'support_v_cont', to prepare for adding more fields in it in the next patch. gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * remote.c (struct support_v_cont): New. (struct remote_state) <support_vCont_t>: Remove. <support_vCont>: New field. (remote_vcont_probe, remote_stop_ns): Update. --- gdb/remote.c | 18 +++++++++++++----- 1 files changed, 13 insertions(+), 5 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 4978fca..ae0ae7e 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -247,6 +247,14 @@ static struct cmd_list_element *remote_cmdlist; static struct cmd_list_element *remote_set_cmdlist; static struct cmd_list_element *remote_show_cmdlist; +/* The status of the stub supports for various vCont; packets. */ + +struct support_v_cont +{ + /* True if the stub reports support for vCont;t. */ + int t; +}; + /* Description of the remote protocol state for the currently connected target. This is per-target state, and independent of the selected architecture. */ @@ -304,8 +312,8 @@ struct remote_state /* True if the stub reports support for non-stop mode. */ int non_stop_aware; - /* True if the stub reports support for vCont;t. */ - int support_vCont_t; + /* The status of the stub supports for various vCont; packets. */ + struct support_v_cont support_vCont; /* True if the stub reports support for conditional tracepoints. */ int cond_tracepoints; @@ -4635,7 +4643,7 @@ remote_vcont_probe (struct remote_state *rs) support_S = 0; support_c = 0; support_C = 0; - rs->support_vCont_t = 0; + rs->support_vCont.t = 0; while (p && *p == ';') { p++; @@ -4648,7 +4656,7 @@ remote_vcont_probe (struct remote_state *rs) else if (*p == 'C' && (*(p + 1) == ';' || *(p + 1) == 0)) support_C = 1; else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) - rs->support_vCont_t = 1; + rs->support_vCont.t = 1; p = strchr (p, ';'); } @@ -4997,7 +5005,7 @@ remote_stop_ns (ptid_t ptid) if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN) remote_vcont_probe (rs); - if (!rs->support_vCont_t) + if (!rs->support_vCont.t) error (_("Remote server does not support stopping threads")); if (ptid_equal (ptid, minus_one_ptid) -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 7/7] range stepping: doc and NEWS 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi ` (4 preceding siblings ...) 2013-03-11 12:53 ` [PATCH 2/7] Move rs->support_vCont_t to a separate struct Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-03-11 13:38 ` Abid, Hafiz ` (2 more replies) 2013-03-11 12:53 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi ` (2 subsequent siblings) 8 siblings, 3 replies; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches gdb/doc: 2013-03-11 Yao Qi <yao@codesourcery.com> * gdb.texinfo (Packets): Document about 'vCont;r'. gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * NEWS: Mention range stepping, new packet and new commands. --- gdb/NEWS | 12 ++++++++++++ gdb/doc/gdb.texinfo | 5 +++++ 2 files changed, 17 insertions(+), 0 deletions(-) diff --git a/gdb/NEWS b/gdb/NEWS index 99b8add..836638c 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -89,6 +89,10 @@ catch signal maint info bfds List the BFDs known to GDB. +maint set range-stepping +maint show range-stepping + Control and show whether do range stepping. + python-interactive [command] pi [command] Start a Python interactive prompt, or evaluate the optional command @@ -164,11 +168,19 @@ show filename-display feature to be enabled. For more information, see: http://fedoraproject.org/wiki/Features/MiniDebugInfo +* GDB now supports range stepping, which improves the performance of + single stepping over a source line by reducing the number of control + packets from GDB. + * New remote packets QTBuffer:size Set the size of trace buffer. The remote stub reports support for this packet to gdb's qSupported query. +vCont;r + Tell the remote stub to do range stepping in an address range. The remote + stub reports a stop reply when the program goes out of the range or is + stopped due to other reasons, such as hitting a breakpoint. *** Changes in GDB 7.5 diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 7c06120..b505cda 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -36261,6 +36261,11 @@ Step. Step with signal @var{sig}. The signal @var{sig} should be two hex digits. @item t Stop. +@item r @var{start},@var{end} +Step repeatedly while the PC is within the range [@var{start}, +@var{end}). Note that a stop reply may be sent at any point even if +the PC is within the stepping range; for example, it is permissible to +implement this packet in a degenerate way as a single step operation. @end table The optional argument @var{addr} normally associated with the -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] range stepping: doc and NEWS 2013-03-11 12:53 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi @ 2013-03-11 13:38 ` Abid, Hafiz 2013-03-11 17:01 ` Eli Zaretskii 2013-05-14 18:32 ` Pedro Alves 2 siblings, 0 replies; 44+ messages in thread From: Abid, Hafiz @ 2013-03-11 13:38 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 11/03/13 12:51:31, Yao Qi wrote: > gdb/doc: > > 2013-03-11 Yao Qi <yao@codesourcery.com> > > * gdb.texinfo (Packets): Document about 'vCont;r'. > > gdb: > > 2013-03-11 Yao Qi <yao@codesourcery.com> > > * NEWS: Mention range stepping, new packet and new > commands. > --- > gdb/NEWS | 12 ++++++++++++ > gdb/doc/gdb.texinfo | 5 +++++ > 2 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index 99b8add..836638c 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -89,6 +89,10 @@ catch signal > maint info bfds > List the BFDs known to GDB. > > +maint set range-stepping > +maint show range-stepping > + Control and show whether do range stepping. What about having a to here. ^^ > + > python-interactive [command] > pi [command] > Start a Python interactive prompt, or evaluate the optional command > @@ -164,11 +168,19 @@ show filename-display > feature to be enabled. For more information, see: > http://fedoraproject.org/wiki/Features/MiniDebugInfo > > +* GDB now supports range stepping, which improves the performance of > + single stepping over a source line by reducing the number of > control > + packets from GDB. > + > * New remote packets > > QTBuffer:size > Set the size of trace buffer. The remote stub reports support > for this > packet to gdb's qSupported query. > +vCont;r > + Tell the remote stub to do range stepping in an address range. > The remote > + stub reports a stop reply when the program goes out of the range > or is > + stopped due to other reasons, such as hitting a breakpoint. > > *** Changes in GDB 7.5 > > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 7c06120..b505cda 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -36261,6 +36261,11 @@ Step. > Step with signal @var{sig}. The signal @var{sig} should be two hex > digits. > @item t > Stop. > +@item r @var{start},@var{end} > +Step repeatedly while the PC is within the range [@var{start}, > +@var{end}). Note that a stop reply may be sent at any point even if > +the PC is within the stepping range; for example, it is permissible > to > +implement this packet in a degenerate way as a single step operation. > @end table > > The optional argument @var{addr} normally associated with the > -- > 1.7.7.6 > > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] range stepping: doc and NEWS 2013-03-11 12:53 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi 2013-03-11 13:38 ` Abid, Hafiz @ 2013-03-11 17:01 ` Eli Zaretskii 2013-05-14 18:32 ` Pedro Alves 2 siblings, 0 replies; 44+ messages in thread From: Eli Zaretskii @ 2013-03-11 17:01 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > From: Yao Qi <yao@codesourcery.com> > Date: Mon, 11 Mar 2013 20:51:31 +0800 > > +maint set range-stepping > +maint show range-stepping > + Control and show whether do range stepping. ^^^^ "to do" OK with that change. Thanks. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] range stepping: doc and NEWS 2013-03-11 12:53 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi 2013-03-11 13:38 ` Abid, Hafiz 2013-03-11 17:01 ` Eli Zaretskii @ 2013-05-14 18:32 ` Pedro Alves 2 siblings, 0 replies; 44+ messages in thread From: Pedro Alves @ 2013-05-14 18:32 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 03/11/2013 12:51 PM, Yao Qi wrote: > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 7c06120..b505cda 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -36261,6 +36261,11 @@ Step. > Step with signal @var{sig}. The signal @var{sig} should be two hex digits. > @item t > Stop. > +@item r @var{start},@var{end} > +Step repeatedly while the PC is within the range [@var{start}, > +@var{end}). Note that a stop reply may be sent at any point even if > +the PC is within the stepping range; for example, it is permissible to > +implement this packet in a degenerate way as a single step operation. > @end table I think we should be explicit on what it means when start==end. -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 3/7] range stepping: gdbserver on x86/linux 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi ` (5 preceding siblings ...) 2013-03-11 12:53 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi @ 2013-03-11 12:53 ` Yao Qi 2013-05-14 18:30 ` Pedro Alves 2013-03-14 20:12 ` [PATCH 0/7] Range stepping Pedro Alves 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi 8 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-03-11 12:53 UTC (permalink / raw) To: gdb-patches This patch adds the support of range stepping in GDBserver for x86/linux. gdb/gdbserver: 2013-03-11 Yao Qi <yao@codesourcery.com> * gdbthread.h (struct thread_info) <start, end>: New fields. (thread_in_range_stepping_p): Declare. (thread_clear_range_stepping): Declare. * inferiors.c (thread_clear_range_stepping): New. (add_thread): Call thread_clear_range_stepping. (thread_in_range_stepping_p): New. * linux-low.c (linux_wait_1): Call thread_in_range_stepping_p to check if report stop to GDB. Call thread_clear_range_stepping. (linux_supports_range_stepping): New. (linux_target_ops): Initialize 'supports_range_stepping' to linux_supports_range_stepping. * linux-low.h (struct linux_target_ops) <supports_range_stepping>: New field. * linux-x86-low.c (x86_supports_range_stepping): New. (the_low_target): Initialize 'supports_range_stepping' to * server.c (handle_v_cont): Handle action 'r'. (handle_v_requests): Append ";r" if target supports range stepping. * target.h (struct target_ops) <supports_range_stepping>>: New field. (target_supports_range_stepping): New macro. --- gdb/gdbserver/gdbthread.h | 6 ++++++ gdb/gdbserver/inferiors.c | 19 +++++++++++++++++++ gdb/gdbserver/linux-low.c | 38 +++++++++++++++++++++++++++++++++++--- gdb/gdbserver/linux-low.h | 2 ++ gdb/gdbserver/linux-x86-low.c | 7 +++++++ gdb/gdbserver/server.c | 40 ++++++++++++++++++++++++++++++++++++++++ gdb/gdbserver/target.h | 7 +++++++ 7 files changed, 116 insertions(+), 3 deletions(-) diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h index 85951d2..86542dd 100644 --- a/gdb/gdbserver/gdbthread.h +++ b/gdb/gdbserver/gdbthread.h @@ -57,12 +57,18 @@ struct thread_info Each item in the list holds the current step of the while-stepping action. */ struct wstep_state *while_stepping; + + /* The start and end address of range stepping. */ + CORE_ADDR start, end; }; extern struct inferior_list all_threads; void remove_thread (struct thread_info *thread); void add_thread (ptid_t ptid, void *target_data); +int thread_in_range_stepping_p (struct thread_info *thread, + CORE_ADDR pc); +void thread_clear_range_stepping (struct thread_info *thread); struct thread_info *find_thread_ptid (ptid_t ptid); struct thread_info *gdb_id_to_thread (unsigned int); diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c index ba3c6cd..8d85af4 100644 --- a/gdb/gdbserver/inferiors.c +++ b/gdb/gdbserver/inferiors.c @@ -85,6 +85,15 @@ remove_inferior (struct inferior_list *list, list->tail = *cur; } +/* Clear the state of range stepping of THREAD. */ + +void +thread_clear_range_stepping (struct thread_info *thread) +{ + thread->start = 0; + thread->end = 0; +} + void add_thread (ptid_t thread_id, void *target_data) { @@ -103,6 +112,16 @@ add_thread (ptid_t thread_id, void *target_data) new_thread->target_data = target_data; set_inferior_regcache_data (new_thread, new_register_cache ()); + thread_clear_range_stepping (new_thread); +} + +/* Return true if THREAD is in range stepping. PC is the value of pc + of THREAD. */ + +int +thread_in_range_stepping_p (struct thread_info *thread, CORE_ADDR pc) +{ + return (pc >= thread->start && pc < thread->end); } ptid_t diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 5f03628..813a018 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -2311,6 +2311,7 @@ linux_wait_1 (ptid_t ptid, int maybe_internal_trap; int report_to_gdb; int trace_event; + int in_range_stepping; /* Translate generic target options into linux options. */ options = __WALL; @@ -2320,6 +2321,7 @@ linux_wait_1 (ptid_t ptid, retry: bp_explains_trap = 0; trace_event = 0; + in_range_stepping = 0; ourstatus->kind = TARGET_WAITKIND_IGNORE; /* If we were only supposed to resume one thread, only wait for @@ -2613,6 +2615,9 @@ Check if we're already there.\n", goto retry; } + in_range_stepping + = thread_in_range_stepping_p (current_inferior, + event_child->stop_pc); /* If GDB wanted this thread to single step, we always want to report the SIGTRAP, and let GDB handle it. Watchpoints should always be reported. So should signals we can't explain. A @@ -2622,9 +2627,12 @@ Check if we're already there.\n", internal breakpoint and still reporting the event to GDB. If we don't, we're out of luck, GDB won't see the breakpoint hit. */ report_to_gdb = (!maybe_internal_trap - || current_inferior->last_resume_kind == resume_step + /* Report the event back to GDB if the inferior is + stepping and not in range stepping. */ + || (current_inferior->last_resume_kind == resume_step + && !in_range_stepping) || event_child->stopped_by_watchpoint - || (!step_over_finished + || (!step_over_finished && !in_range_stepping && !bp_explains_trap && !trace_event) || (gdb_breakpoint_here (event_child->stop_pc) && gdb_condition_true_at_breakpoint (event_child->stop_pc) @@ -2645,6 +2653,12 @@ Check if we're already there.\n", fprintf (stderr, "Step-over finished.\n"); if (trace_event) fprintf (stderr, "Tracepoint event.\n"); + if (thread_in_range_stepping_p (current_inferior, + event_child->stop_pc)) + fprintf (stderr, "Range stepping pc 0x%s [0x%s, 0x%s).\n", + paddress (event_child->stop_pc), + paddress (current_inferior->start), + paddress (current_inferior->end)); } /* We're not reporting this breakpoint to GDB, so apply the @@ -2676,7 +2690,13 @@ Check if we're already there.\n", if (debug_threads) { if (current_inferior->last_resume_kind == resume_step) - fprintf (stderr, "GDB wanted to single-step, reporting event.\n"); + { + if (!thread_in_range_stepping_p (current_inferior, + event_child->stop_pc)) + fprintf (stderr, "Range stepping is done, reporting event.\n"); + else + fprintf (stderr, "GDB wanted to single-step, reporting event.\n"); + } if (event_child->stopped_by_watchpoint) fprintf (stderr, "Stopped by watchpoint.\n"); if (gdb_breakpoint_here (event_child->stop_pc)) @@ -2685,6 +2705,8 @@ Check if we're already there.\n", fprintf (stderr, "Hit a non-gdbserver trap event.\n"); } + thread_clear_range_stepping (current_inferior); + /* Alright, we're going to report a stop. */ if (!non_stop && !stabilizing_threads) @@ -5081,6 +5103,15 @@ linux_supports_agent (void) return 1; } +static int +linux_supports_range_stepping (void) +{ + if (*the_low_target.supports_range_stepping == NULL) + return 0; + + return (*the_low_target.supports_range_stepping) (); +} + /* Enumerate spufs IDs for process PID. */ static int spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len) @@ -5885,6 +5916,7 @@ static struct target_ops linux_target_ops = { linux_get_min_fast_tracepoint_insn_len, linux_qxfer_libraries_svr4, linux_supports_agent, + linux_supports_range_stepping, }; static void diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h index 27dd3b5..5d3e8cf 100644 --- a/gdb/gdbserver/linux-low.h +++ b/gdb/gdbserver/linux-low.h @@ -168,6 +168,8 @@ struct linux_target_ops for use as a fast tracepoint. */ int (*get_min_fast_tracepoint_insn_len) (void); + /* Returns true if the low target supports range stepping. */ + int (*supports_range_stepping) (void); }; extern struct linux_target_ops the_low_target; diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c index 31657d3..1d1df95 100644 --- a/gdb/gdbserver/linux-x86-low.c +++ b/gdb/gdbserver/linux-x86-low.c @@ -3175,6 +3175,12 @@ x86_emit_ops (void) return &i386_emit_ops; } +static int +x86_supports_range_stepping (void) +{ + return 1; +} + /* This is initialized assuming an amd64 target. x86_arch_setup will correct it for i386 or amd64 targets. */ @@ -3214,4 +3220,5 @@ struct linux_target_ops the_low_target = x86_install_fast_tracepoint_jump_pad, x86_emit_ops, x86_get_min_fast_tracepoint_insn_len, + x86_supports_range_stepping, }; diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index 922a5bf..0b330d1 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -1871,10 +1871,15 @@ handle_v_cont (char *own_buf) p = &own_buf[5]; while (*p) { + CORE_ADDR start = 0; + CORE_ADDR end = 0; + p++; if (p[0] == 's' || p[0] == 'S') resume_info[i].kind = resume_step; + else if (p[0] == 'r') + resume_info[i].kind = resume_step; else if (p[0] == 'c' || p[0] == 'C') resume_info[i].kind = resume_continue; else if (p[0] == 't') @@ -1894,6 +1899,21 @@ handle_v_cont (char *own_buf) goto err; resume_info[i].sig = gdb_signal_to_host (sig); } + else if (p[0] == 'r') + { + char *p1; + + p = p + 1; + p1 = strchr (p, ','); + decode_address (&start, p, p1 - p); + + p = p1 + 1; + p1 = strchr (p, ':'); + decode_address (&end, p, p1 - p); + + resume_info[i].sig = 0; + p = p1; + } else { resume_info[i].sig = 0; @@ -1919,6 +1939,21 @@ handle_v_cont (char *own_buf) goto err; resume_info[i].thread = ptid; + if (end > 0) + { + struct thread_info *tp = find_thread_ptid (ptid); + + /* GDB should not send range stepping for all threads of + a process, like 'vCont;rSTART,END:pPID.-1', TP can't + be NULL. */ + gdb_assert (tp != NULL); + + tp->start = start; + tp->end = end; + + start = 0; + end = 0; + } i++; } @@ -2142,6 +2177,11 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) if (strncmp (own_buf, "vCont?", 6) == 0) { strcpy (own_buf, "vCont;c;C;s;S;t"); + if (target_supports_range_stepping ()) + { + own_buf = own_buf + strlen (own_buf); + strcpy (own_buf, ";r"); + } return; } } diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h index cc9a910..b1dd3b1 100644 --- a/gdb/gdbserver/target.h +++ b/gdb/gdbserver/target.h @@ -397,6 +397,9 @@ struct target_ops /* Return true if target supports debugging agent. */ int (*supports_agent) (void); + + /* Return true if target supports range stepping. */ + int (*supports_range_stepping) (void); }; extern struct target_ops *the_target; @@ -520,6 +523,10 @@ int kill_inferior (int); (the_target->supports_agent ? \ (*the_target->supports_agent) () : 0) +#define target_supports_range_stepping() \ + (the_target->supports_range_stepping ? \ + (*the_target->supports_range_stepping) () : 0) + /* Start non-stop mode, returns 0 on success, -1 on failure. */ int start_non_stop (int nonstop); -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/7] range stepping: gdbserver on x86/linux 2013-03-11 12:53 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi @ 2013-05-14 18:30 ` Pedro Alves 2013-05-15 7:40 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-05-14 18:30 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 03/11/2013 12:51 PM, Yao Qi wrote: > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index 922a5bf..0b330d1 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -1871,10 +1871,15 @@ handle_v_cont (char *own_buf) > p = &own_buf[5]; > while (*p) > { > + CORE_ADDR start = 0; > + CORE_ADDR end = 0; > + > p++; > > if (p[0] == 's' || p[0] == 'S') > resume_info[i].kind = resume_step; > + else if (p[0] == 'r') > + resume_info[i].kind = resume_step; > else if (p[0] == 'c' || p[0] == 'C') > resume_info[i].kind = resume_continue; > else if (p[0] == 't') > @@ -1894,6 +1899,21 @@ handle_v_cont (char *own_buf) > goto err; > resume_info[i].sig = gdb_signal_to_host (sig); > } > + else if (p[0] == 'r') > + { > + char *p1; > + > + p = p + 1; > + p1 = strchr (p, ','); > + decode_address (&start, p, p1 - p); > + > + p = p1 + 1; > + p1 = strchr (p, ':'); > + decode_address (&end, p, p1 - p); > + > + resume_info[i].sig = 0; > + p = p1; > + } > else > { > resume_info[i].sig = 0; > @@ -1919,6 +1939,21 @@ handle_v_cont (char *own_buf) > goto err; > > resume_info[i].thread = ptid; > + if (end > 0) > + { > + struct thread_info *tp = find_thread_ptid (ptid); > + > + /* GDB should not send range stepping for all threads of > + a process, like 'vCont;rSTART,END:pPID.-1', TP can't > + be NULL. */ > + gdb_assert (tp != NULL); > + > + tp->start = start; > + tp->end = end; > + > + start = 0; > + end = 0; > + } My main issues with this are: - it assumes GDB will ever only send one r action per vCont. I'd much rather we don't bake in that assumption. - GDBserver should not gdb_assert for unexpected input it can't control. - It seems like this leaves threads with stale step ranges. Because linux-low.c only clears the stepping range of the thread that is reporting the event (here): > @@ -2685,6 +2705,8 @@ Check if we're already there.\n", > fprintf (stderr, "Hit a non-gdbserver trap event.\n"); > } > > + thread_clear_range_stepping (current_inferior); > + > /* Alright, we're going to report a stop. */ > > if (!non_stop && !stabilizing_threads) > @@ -5081,6 +5103,15 @@ linux_supports_agent (void) > return 1; > } That is, e.g.,: - user "step"s thread 1 (vCont;r) - thread 2 hits breakpoint (step range of thread 2 is cleared) - user goes back to thread 1, and does "si". - GDB sends vCont;s, but thread 1 still has the stale step range from the previous vCont;r. We should probably have an explicit test for this, though I haven't added one myself (at least yet). -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/7] range stepping: gdbserver on x86/linux 2013-05-14 18:30 ` Pedro Alves @ 2013-05-15 7:40 ` Yao Qi 2013-05-20 18:00 ` Pedro Alves 0 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-05-15 7:40 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 05/15/2013 02:30 AM, Pedro Alves wrote: > - it assumes GDB will ever only send one r action per vCont. I'd much > rather we don't bake in that assumption. > I am afraid not. It works for multiple r actions in one vCont. > - It seems like this leaves threads with stale step ranges. Because > linux-low.c only clears the stepping range of the thread that is reporting > the event (here): OK. Looks we need to call thread_clear_range_stepping for the thread we parsed in handle_v_cont first. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/7] range stepping: gdbserver on x86/linux 2013-05-15 7:40 ` Yao Qi @ 2013-05-20 18:00 ` Pedro Alves 2013-05-22 10:06 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-05-20 18:00 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 05/15/2013 08:40 AM, Yao Qi wrote: > On 05/15/2013 02:30 AM, Pedro Alves wrote: >> - it assumes GDB will ever only send one r action per vCont. I'd much >> rather we don't bake in that assumption. >> > > I am afraid not. It works for multiple r actions in one vCont. Oh, sorry about that. :-/ That wasn't actually the issue I had noticed initially, and then when I went back to compose the email, I had already forgotten and got it mixed up. The issue was actually with: if (end > 0) { struct thread_info *tp = find_thread_ptid (ptid); /* GDB should not send range stepping for all threads of a process, like 'vCont;rSTART,END:pPID.-1', TP can't be NULL. */ gdb_assert (tp != NULL); I think it's best not to error on this as nothing in the protocol actually prohibits it, and we might take advantage of it at some point. The way gdbserver handles vCont requests currently is by letting the target match the ptid to whatever thread/lwp. That means leaving server.c only knowing about how to parse the rsp and construct a thread_resume, and then pass that down to the target. Conceivably, a target might not need to keep the step range anywhere, if it has something like a PTRACE_STEP_RANGE at the kernel level. (There's PTRACE_BLOCKSTEP, but it's not the same). -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/7] range stepping: gdbserver on x86/linux 2013-05-20 18:00 ` Pedro Alves @ 2013-05-22 10:06 ` Yao Qi 0 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-05-22 10:06 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 05/21/2013 02:00 AM, Pedro Alves wrote: > The issue was actually with: > > if (end > 0) > { > struct thread_info *tp = find_thread_ptid (ptid); > > /* GDB should not send range stepping for all threads of > a process, like 'vCont;rSTART,END:pPID.-1', TP can't > be NULL. */ > gdb_assert (tp != NULL); > > I think it's best not to error on this as nothing in the protocol actually > prohibits it, and we might take advantage of it at some point. The way OK, looks I abused gdb_assert some times, :). > gdbserver handles vCont requests currently is by letting the target > match the ptid to whatever thread/lwp. That means leaving server.c > only knowing about how to parse the rsp and construct a thread_resume, > and then pass that down to the target. Conceivably, a target might > not need to keep the step range anywhere, if it has something like > a PTRACE_STEP_RANGE at the kernel level. (There's PTRACE_BLOCKSTEP, > but it's not the same). Yeah, that makes sense to me. I didn't realize such abstraction when reading the code. Thanks for your explanation. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/7] Range stepping 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi ` (6 preceding siblings ...) 2013-03-11 12:53 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi @ 2013-03-14 20:12 ` Pedro Alves 2013-03-15 19:54 ` Pedro Alves 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi 8 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-03-14 20:12 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches Just a FYI, so it doesn't seem this is stalled too, I'm looking at/playing with this series. Thanks much for doing this. It's a long overdue feature. I'll reply back with a review shortly (today/tomorrow). -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/7] Range stepping 2013-03-14 20:12 ` [PATCH 0/7] Range stepping Pedro Alves @ 2013-03-15 19:54 ` Pedro Alves 2013-03-22 2:25 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Pedro Alves @ 2013-03-15 19:54 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 03/14/2013 08:12 PM, Pedro Alves wrote: > Just a FYI, so it doesn't seem this is stalled too, > I'm looking at/playing with this series. > > Thanks much for doing this. It's a long overdue feature. > > I'll reply back with a review shortly (today/tomorrow). I ended up spending the afternoon around it, tweaking a few things. More early next week. Thanks again! -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/7] Range stepping 2013-03-15 19:54 ` Pedro Alves @ 2013-03-22 2:25 ` Yao Qi 2013-03-22 20:24 ` Pedro Alves 0 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-03-22 2:25 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 03/16/2013 03:44 AM, Pedro Alves wrote: > I ended up spending the afternoon around it, tweaking > a few things. More early next week. Pedro, Are your comments ready? -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/7] Range stepping 2013-03-22 2:25 ` Yao Qi @ 2013-03-22 20:24 ` Pedro Alves 0 siblings, 0 replies; 44+ messages in thread From: Pedro Alves @ 2013-03-22 20:24 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 03/22/2013 02:16 AM, Yao Qi wrote: > On 03/16/2013 03:44 AM, Pedro Alves wrote: >> I ended up spending the afternoon around it, tweaking >> a few things. More early next week. > > Pedro, > Are your comments ready? Sorry, not yet. It's at the top of my TODO just below 7.6 issues. I did hack on this more a couple days early this week, but then spent the rest of the week mostly fixing the integer command regressions for 7.6, and reviewing patches that block the release. -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 0/7 V2] Range stepping 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi ` (7 preceding siblings ...) 2013-03-14 20:12 ` [PATCH 0/7] Range stepping Pedro Alves @ 2013-04-11 6:16 ` Yao Qi 2013-04-11 6:17 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi ` (7 more replies) 8 siblings, 8 replies; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:16 UTC (permalink / raw) To: gdb-patches Hello, This is the V2 of range stepping series. V1 can be found here <http://sourceware.org/ml/gdb-patches/2013-03/msg00450.html>. There are some updates in V2: - Rebase patches on GDB trunk, and resolve conflicts. - Use phex_nz instead of hexnumstr to get the address. It fixes a bug on mips target when sending address to the remote target. - Define an internalvar range_stepping_counter and increment it each time GDB does range-stepping, so that in the test, we don't have to turn 'remote debug' on and parse the RSP packets. During the test, we find that it is unsafe to turn the debugging output on, because it may blow up the gdb.log. Patch 1,2,3,7 are unchanged. The whole series is tested on x86_64-linux, native and gdbserver. gdb/NEWS | 12 ++ gdb/doc/gdb.texinfo | 15 ++ gdb/gdbserver/gdbthread.h | 6 + gdb/gdbserver/inferiors.c | 19 +++ gdb/gdbserver/linux-low.c | 38 +++++- gdb/gdbserver/linux-low.h | 2 + gdb/gdbserver/linux-x86-low.c | 7 + gdb/gdbserver/server.c | 40 +++++ gdb/gdbserver/target.h | 6 + gdb/gdbthread.h | 6 + gdb/infrun.c | 6 +- gdb/remote.c | 127 +++++++++++++++- gdb/testsuite/gdb.base/range-stepping.c | 69 +++++++++ gdb/testsuite/gdb.base/range-stepping.exp | 222 ++++++++++++++++++++++++++++ gdb/testsuite/gdb.trace/range-stepping.c | 61 ++++++++ gdb/testsuite/gdb.trace/range-stepping.exp | 89 +++++++++++ 16 files changed, 712 insertions(+), 13 deletions(-) create mode 100644 gdb/testsuite/gdb.base/range-stepping.c create mode 100644 gdb/testsuite/gdb.base/range-stepping.exp create mode 100644 gdb/testsuite/gdb.trace/range-stepping.c create mode 100644 gdb/testsuite/gdb.trace/range-stepping.exp -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi @ 2013-04-11 6:17 ` Yao Qi 2013-04-11 6:17 ` [PATCH 2/7] Move rs->support_vCont_t to a separate struct Yao Qi ` (6 subsequent siblings) 7 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:17 UTC (permalink / raw) To: gdb-patches This patch moves code into a macro, which will be used in my following patches. gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * gdbthread.h (THREAD_WITHIN_SINGLE_STEP_RANGE): New macro. * infrun.c (handle_inferior_event): Use it. --- gdb/gdbthread.h | 6 ++++++ gdb/infrun.c | 6 ++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 0846322..3525de6 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -38,6 +38,12 @@ enum thread_state THREAD_EXITED, }; +/* PC is within the range of single stepping of thread THR. */ + +#define THREAD_WITHIN_SINGLE_STEP_RANGE(THR, PC) \ + ((PC) >= (THR)->control.step_range_start \ + && (PC) < (THR)->control.step_range_end) + /* Inferior thread specific part of `struct infcall_control_state'. Inferior process counterpart is `struct inferior_control_state'. */ diff --git a/gdb/infrun.c b/gdb/infrun.c index 7031ecc..6876dd6 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -4341,8 +4341,7 @@ process_event_stop_test: if (ecs->event_thread->control.step_range_end != 0 && ecs->event_thread->suspend.stop_signal != GDB_SIGNAL_0 - && (ecs->event_thread->control.step_range_start <= stop_pc - && stop_pc < ecs->event_thread->control.step_range_end) + && THREAD_WITHIN_SINGLE_STEP_RANGE (ecs->event_thread, stop_pc) && frame_id_eq (get_stack_frame_id (frame), ecs->event_thread->control.step_stack_frame_id) && ecs->event_thread->control.step_resume_breakpoint == NULL) @@ -4711,8 +4710,7 @@ process_event_stop_test: through a function epilogue and therefore must detect when the current-frame changes in the middle of a line. */ - if (stop_pc >= ecs->event_thread->control.step_range_start - && stop_pc < ecs->event_thread->control.step_range_end + if (THREAD_WITHIN_SINGLE_STEP_RANGE (ecs->event_thread, stop_pc) && (execution_direction != EXEC_REVERSE || frame_id_eq (get_frame_id (frame), ecs->event_thread->control.step_frame_id))) -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 2/7] Move rs->support_vCont_t to a separate struct 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi 2013-04-11 6:17 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi @ 2013-04-11 6:17 ` Yao Qi 2013-04-11 6:18 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi ` (5 subsequent siblings) 7 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:17 UTC (permalink / raw) To: gdb-patches This is a refactor patch to move field 'support_vCont_t' to a separate struct 'support_v_cont', to prepare for adding more fields in it in the next patch. gdb: 2013-03-11 Yao Qi <yao@codesourcery.com> * remote.c (struct support_v_cont): New. (struct remote_state) <support_vCont_t>: Remove. <support_vCont>: New field. (remote_vcont_probe, remote_stop_ns): Update. --- gdb/remote.c | 18 +++++++++++++----- 1 files changed, 13 insertions(+), 5 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index de075c8..f6a3f4c 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -250,6 +250,14 @@ static struct cmd_list_element *remote_cmdlist; static struct cmd_list_element *remote_set_cmdlist; static struct cmd_list_element *remote_show_cmdlist; +/* The status of the stub supports for various vCont; packets. */ + +struct support_v_cont +{ + /* True if the stub reports support for vCont;t. */ + int t; +}; + /* Description of the remote protocol state for the currently connected target. This is per-target state, and independent of the selected architecture. */ @@ -307,8 +315,8 @@ struct remote_state /* True if the stub reports support for non-stop mode. */ int non_stop_aware; - /* True if the stub reports support for vCont;t. */ - int support_vCont_t; + /* The status of the stub supports for various vCont; packets. */ + struct support_v_cont support_vCont; /* True if the stub reports support for conditional tracepoints. */ int cond_tracepoints; @@ -4640,7 +4648,7 @@ remote_vcont_probe (struct remote_state *rs) support_S = 0; support_c = 0; support_C = 0; - rs->support_vCont_t = 0; + rs->support_vCont.t = 0; while (p && *p == ';') { p++; @@ -4653,7 +4661,7 @@ remote_vcont_probe (struct remote_state *rs) else if (*p == 'C' && (*(p + 1) == ';' || *(p + 1) == 0)) support_C = 1; else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) - rs->support_vCont_t = 1; + rs->support_vCont.t = 1; p = strchr (p, ';'); } @@ -5002,7 +5010,7 @@ remote_stop_ns (ptid_t ptid) if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN) remote_vcont_probe (rs); - if (!rs->support_vCont_t) + if (!rs->support_vCont.t) error (_("Remote server does not support stopping threads")); if (ptid_equal (ptid, minus_one_ptid) -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 3/7] range stepping: gdbserver on x86/linux 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi 2013-04-11 6:17 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi 2013-04-11 6:17 ` [PATCH 2/7] Move rs->support_vCont_t to a separate struct Yao Qi @ 2013-04-11 6:18 ` Yao Qi 2013-04-11 6:19 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi ` (4 subsequent siblings) 7 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:18 UTC (permalink / raw) To: gdb-patches This patch adds the support of range stepping in GDBserver for x86/linux. gdb/gdbserver: 2013-03-11 Yao Qi <yao@codesourcery.com> * gdbthread.h (struct thread_info) <start, end>: New fields. (thread_in_range_stepping_p): Declare. (thread_clear_range_stepping): Declare. * inferiors.c (thread_clear_range_stepping): New. (add_thread): Call thread_clear_range_stepping. (thread_in_range_stepping_p): New. * linux-low.c (linux_wait_1): Call thread_in_range_stepping_p to check if report stop to GDB. Call thread_clear_range_stepping. (linux_supports_range_stepping): New. (linux_target_ops): Initialize 'supports_range_stepping' to linux_supports_range_stepping. * linux-low.h (struct linux_target_ops) <supports_range_stepping>: New field. * linux-x86-low.c (x86_supports_range_stepping): New. (the_low_target): Initialize 'supports_range_stepping' to * server.c (handle_v_cont): Handle action 'r'. (handle_v_requests): Append ";r" if target supports range stepping. * target.h (struct target_ops) <supports_range_stepping>>: New field. (target_supports_range_stepping): New macro. --- gdb/gdbserver/gdbthread.h | 6 ++++++ gdb/gdbserver/inferiors.c | 19 +++++++++++++++++++ gdb/gdbserver/linux-low.c | 38 +++++++++++++++++++++++++++++++++++--- gdb/gdbserver/linux-low.h | 2 ++ gdb/gdbserver/linux-x86-low.c | 7 +++++++ gdb/gdbserver/server.c | 40 ++++++++++++++++++++++++++++++++++++++++ gdb/gdbserver/target.h | 6 ++++++ 7 files changed, 115 insertions(+), 3 deletions(-) diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h index 5d4955b..fbd768b 100644 --- a/gdb/gdbserver/gdbthread.h +++ b/gdb/gdbserver/gdbthread.h @@ -62,12 +62,18 @@ struct thread_info /* Branch trace target information for this thread. */ struct btrace_target_info *btrace; + + /* The start and end address of range stepping. */ + CORE_ADDR start, end; }; extern struct inferior_list all_threads; void remove_thread (struct thread_info *thread); void add_thread (ptid_t ptid, void *target_data); +int thread_in_range_stepping_p (struct thread_info *thread, + CORE_ADDR pc); +void thread_clear_range_stepping (struct thread_info *thread); struct thread_info *find_thread_ptid (ptid_t ptid); struct thread_info *gdb_id_to_thread (unsigned int); diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c index 6953d0e..2184a7b 100644 --- a/gdb/gdbserver/inferiors.c +++ b/gdb/gdbserver/inferiors.c @@ -85,6 +85,15 @@ remove_inferior (struct inferior_list *list, list->tail = *cur; } +/* Clear the state of range stepping of THREAD. */ + +void +thread_clear_range_stepping (struct thread_info *thread) +{ + thread->start = 0; + thread->end = 0; +} + void add_thread (ptid_t thread_id, void *target_data) { @@ -103,6 +112,16 @@ add_thread (ptid_t thread_id, void *target_data) new_thread->target_data = target_data; set_inferior_regcache_data (new_thread, new_register_cache ()); + thread_clear_range_stepping (new_thread); +} + +/* Return true if THREAD is in range stepping. PC is the value of pc + of THREAD. */ + +int +thread_in_range_stepping_p (struct thread_info *thread, CORE_ADDR pc) +{ + return (pc >= thread->start && pc < thread->end); } ptid_t diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 72c51e0..e9879cc 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -2313,6 +2313,7 @@ linux_wait_1 (ptid_t ptid, int maybe_internal_trap; int report_to_gdb; int trace_event; + int in_range_stepping; /* Translate generic target options into linux options. */ options = __WALL; @@ -2322,6 +2323,7 @@ linux_wait_1 (ptid_t ptid, retry: bp_explains_trap = 0; trace_event = 0; + in_range_stepping = 0; ourstatus->kind = TARGET_WAITKIND_IGNORE; /* If we were only supposed to resume one thread, only wait for @@ -2615,6 +2617,9 @@ Check if we're already there.\n", goto retry; } + in_range_stepping + = thread_in_range_stepping_p (current_inferior, + event_child->stop_pc); /* If GDB wanted this thread to single step, we always want to report the SIGTRAP, and let GDB handle it. Watchpoints should always be reported. So should signals we can't explain. A @@ -2624,9 +2629,12 @@ Check if we're already there.\n", internal breakpoint and still reporting the event to GDB. If we don't, we're out of luck, GDB won't see the breakpoint hit. */ report_to_gdb = (!maybe_internal_trap - || current_inferior->last_resume_kind == resume_step + /* Report the event back to GDB if the inferior is + stepping and not in range stepping. */ + || (current_inferior->last_resume_kind == resume_step + && !in_range_stepping) || event_child->stopped_by_watchpoint - || (!step_over_finished + || (!step_over_finished && !in_range_stepping && !bp_explains_trap && !trace_event) || (gdb_breakpoint_here (event_child->stop_pc) && gdb_condition_true_at_breakpoint (event_child->stop_pc) @@ -2647,6 +2655,12 @@ Check if we're already there.\n", fprintf (stderr, "Step-over finished.\n"); if (trace_event) fprintf (stderr, "Tracepoint event.\n"); + if (thread_in_range_stepping_p (current_inferior, + event_child->stop_pc)) + fprintf (stderr, "Range stepping pc 0x%s [0x%s, 0x%s).\n", + paddress (event_child->stop_pc), + paddress (current_inferior->start), + paddress (current_inferior->end)); } /* We're not reporting this breakpoint to GDB, so apply the @@ -2678,7 +2692,13 @@ Check if we're already there.\n", if (debug_threads) { if (current_inferior->last_resume_kind == resume_step) - fprintf (stderr, "GDB wanted to single-step, reporting event.\n"); + { + if (!thread_in_range_stepping_p (current_inferior, + event_child->stop_pc)) + fprintf (stderr, "Range stepping is done, reporting event.\n"); + else + fprintf (stderr, "GDB wanted to single-step, reporting event.\n"); + } if (event_child->stopped_by_watchpoint) fprintf (stderr, "Stopped by watchpoint.\n"); if (gdb_breakpoint_here (event_child->stop_pc)) @@ -2687,6 +2707,8 @@ Check if we're already there.\n", fprintf (stderr, "Hit a non-gdbserver trap event.\n"); } + thread_clear_range_stepping (current_inferior); + /* Alright, we're going to report a stop. */ if (!non_stop && !stabilizing_threads) @@ -5083,6 +5105,15 @@ linux_supports_agent (void) return 1; } +static int +linux_supports_range_stepping (void) +{ + if (*the_low_target.supports_range_stepping == NULL) + return 0; + + return (*the_low_target.supports_range_stepping) (); +} + /* Enumerate spufs IDs for process PID. */ static int spu_enumerate_spu_ids (long pid, unsigned char *buf, CORE_ADDR offset, int len) @@ -5939,6 +5970,7 @@ static struct target_ops linux_target_ops = { NULL, NULL, #endif + linux_supports_range_stepping, }; static void diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h index 27dd3b5..5d3e8cf 100644 --- a/gdb/gdbserver/linux-low.h +++ b/gdb/gdbserver/linux-low.h @@ -168,6 +168,8 @@ struct linux_target_ops for use as a fast tracepoint. */ int (*get_min_fast_tracepoint_insn_len) (void); + /* Returns true if the low target supports range stepping. */ + int (*supports_range_stepping) (void); }; extern struct linux_target_ops the_low_target; diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c index 31657d3..1d1df95 100644 --- a/gdb/gdbserver/linux-x86-low.c +++ b/gdb/gdbserver/linux-x86-low.c @@ -3175,6 +3175,12 @@ x86_emit_ops (void) return &i386_emit_ops; } +static int +x86_supports_range_stepping (void) +{ + return 1; +} + /* This is initialized assuming an amd64 target. x86_arch_setup will correct it for i386 or amd64 targets. */ @@ -3214,4 +3220,5 @@ struct linux_target_ops the_low_target = x86_install_fast_tracepoint_jump_pad, x86_emit_ops, x86_get_min_fast_tracepoint_insn_len, + x86_supports_range_stepping, }; diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index 6bb36d8..aa4ddf6 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -2040,10 +2040,15 @@ handle_v_cont (char *own_buf) p = &own_buf[5]; while (*p) { + CORE_ADDR start = 0; + CORE_ADDR end = 0; + p++; if (p[0] == 's' || p[0] == 'S') resume_info[i].kind = resume_step; + else if (p[0] == 'r') + resume_info[i].kind = resume_step; else if (p[0] == 'c' || p[0] == 'C') resume_info[i].kind = resume_continue; else if (p[0] == 't') @@ -2063,6 +2068,21 @@ handle_v_cont (char *own_buf) goto err; resume_info[i].sig = gdb_signal_to_host (sig); } + else if (p[0] == 'r') + { + char *p1; + + p = p + 1; + p1 = strchr (p, ','); + decode_address (&start, p, p1 - p); + + p = p1 + 1; + p1 = strchr (p, ':'); + decode_address (&end, p, p1 - p); + + resume_info[i].sig = 0; + p = p1; + } else { resume_info[i].sig = 0; @@ -2088,6 +2108,21 @@ handle_v_cont (char *own_buf) goto err; resume_info[i].thread = ptid; + if (end > 0) + { + struct thread_info *tp = find_thread_ptid (ptid); + + /* GDB should not send range stepping for all threads of + a process, like 'vCont;rSTART,END:pPID.-1', TP can't + be NULL. */ + gdb_assert (tp != NULL); + + tp->start = start; + tp->end = end; + + start = 0; + end = 0; + } i++; } @@ -2311,6 +2346,11 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) if (strncmp (own_buf, "vCont?", 6) == 0) { strcpy (own_buf, "vCont;c;C;s;S;t"); + if (target_supports_range_stepping ()) + { + own_buf = own_buf + strlen (own_buf); + strcpy (own_buf, ";r"); + } return; } } diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h index f257459..8c0790e 100644 --- a/gdb/gdbserver/target.h +++ b/gdb/gdbserver/target.h @@ -414,6 +414,8 @@ struct target_ops to break a cyclic dependency. */ void (*read_btrace) (struct btrace_target_info *, struct buffer *, int type); + /* Return true if target supports range stepping. */ + int (*supports_range_stepping) (void); }; extern struct target_ops *the_target; @@ -549,6 +551,10 @@ int kill_inferior (int); #define target_read_btrace(tinfo, buffer, type) \ (*the_target->read_btrace) (tinfo, buffer, type) +#define target_supports_range_stepping() \ + (the_target->supports_range_stepping ? \ + (*the_target->supports_range_stepping) () : 0) + /* Start non-stop mode, returns 0 on success, -1 on failure. */ int start_non_stop (int nonstop); -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi ` (2 preceding siblings ...) 2013-04-11 6:18 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi @ 2013-04-11 6:19 ` Yao Qi 2013-04-11 23:00 ` Eli Zaretskii 2013-04-11 6:19 ` [PATCH 4/7] range stepping: gdb Yao Qi ` (3 subsequent siblings) 7 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:19 UTC (permalink / raw) To: gdb-patches On 03/18/2013 11:45 AM, Eli Zaretskii wrote: >> Don't have any special reasons to do this. The range stepping should be >> >always on if target supports, and it can be turned off for some >> >"maintenance purpose", so I set this command as a maintenance >> >command. > What would be the possible reasons to turn this feature off? > > Once we have the reasons, we could try thinking whether or not they > are limited to GDB maintenance. Range-stepping is useful to speed up some operations, so users don't have to turn it off. The reason turning it off could be that the remote stub has a bug in supporting range stepping, so we have to disable it in GDB side. I considered to name the command to "set remote range-stepping", but range-stepping is not specific to remote target, and it can be implemented on native target as well. So "set remote range-stepping" is not a good choice. I also considered "set range-stepping", but unable to find a reasonable place for documentation on it. In V2, I don't update the command name. gdb: 2013-04-11 Yao Qi <yao@codesourcery.com> * remote.c (use_range_stepping): New. (remote_vcont_probe): Set 'use_range_stepping' if the remote target supports range stepping. (append_resumption): Use 'vCont;r' packet if 'use_range_stepping' is true. (maint_show_range_stepping): New. (maint_set_range_stepping): New. (_initialize_remote): Call add_setshow_boolean_cmd to register commands 'maint set range-stepping' and 'maint show range-stepping'. gdb/doc: 2013-04-11 Yao Qi <yao@codesourcery.com> * gdb.texinfo (Maintenance Commands): Document commands 'maint set range-stepping' and 'maint show range-stepping' and convenience variable '$range_stepping_counter'. --- gdb/doc/gdb.texinfo | 10 ++++++++ gdb/remote.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 1 deletions(-) diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index bf7e25e..9292b94 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -35620,6 +35620,16 @@ data in a @file{gmon.out} file, be sure to move it to a safe location. Configuring with @samp{--enable-profiling} arranges for @value{GDBN} to be compiled with the @samp{-pg} compiler option. +@kindex maint set range-stepping +@kindex maint show range-stepping +@cindex range-stepping +@item maint set range-stepping +@itemx maint show range-stepping +@vindex $range_stepping_counter +Control whether to do stepping in an address range. The debugger convenience +variable @samp{$range_stepping_counter} contains the number of range stepping +@value{GDBN} has performed. + @kindex maint set show-debug-regs @kindex maint show show-debug-regs @cindex hardware debug registers diff --git a/gdb/remote.c b/gdb/remote.c index a063698..7022275 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -260,6 +260,10 @@ struct support_v_cont int r; }; +/* A flag on GDB is willing to use range-stepping or not. */ + +static int use_range_stepping = 0; + /* Description of the remote protocol state for the currently connected target. This is per-target state, and independent of the selected architecture. */ @@ -4652,6 +4656,7 @@ remote_vcont_probe (struct remote_state *rs) support_C = 0; rs->support_vCont.t = 0; rs->support_vCont.r = 0; + use_range_stepping = 0; while (p && *p == ';') { p++; @@ -4666,7 +4671,10 @@ remote_vcont_probe (struct remote_state *rs) else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) rs->support_vCont.t = 1; else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0)) - rs->support_vCont.r = 1; + { + rs->support_vCont.r = 1; + use_range_stepping = 1; + } p = strchr (p, ';'); } @@ -4713,6 +4721,8 @@ append_resumption (char *p, char *endp, pc = regcache_read_pc (get_thread_regcache (ptid)); if (rs->support_vCont.r /* Target supports step range. */ + /* GDB is willing to do range stepping. */ + && use_range_stepping /* Can't do range stepping for all threads of a process 'pPID.-1'. */ && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) @@ -11697,6 +11707,45 @@ remote_upload_trace_state_variables (struct uploaded_tsv **utsvp) return 0; } +static void +maint_show_range_stepping (struct ui_file *file, int from_tty, + struct cmd_list_element *c, + const char *value) +{ + fprintf_filtered (file, + _("Debugger's willingness to do range-stepping " + "is %s.\n"), value); +} + +static void +maint_set_range_stepping (char *ignore_args, int from_tty, + struct cmd_list_element *c) +{ + /* Check range stepping is supported when turns it on. */ + if (use_range_stepping) + { + if (remote_desc != NULL) + { + struct remote_state *rs = get_remote_state (); + + if (remote_protocol_packets[PACKET_vCont].support == PACKET_SUPPORT_UNKNOWN) + remote_vcont_probe (rs); + + if (remote_protocol_packets[PACKET_vCont].support == PACKET_DISABLE + || !rs->support_vCont.r) + { + use_range_stepping = 0; + error (_("Range stepping is not supported")); + } + } + else + { + use_range_stepping = 0; + error (_("Range stepping is not supported")); + } + } +} + void _initialize_remote (void) { @@ -12094,6 +12143,19 @@ Set the remote pathname for \"run\""), _("\ Show the remote pathname for \"run\""), NULL, NULL, NULL, &remote_set_cmdlist, &remote_show_cmdlist); + add_setshow_boolean_cmd ("range-stepping", class_maintenance, + &use_range_stepping, _("\ +Enable or disable range-stepping."), _("\ +Show whether range-stepping is enabled."), _("\ +If On, GDB will tell the target to do stepping a range of address.\n\ +This will speed up stepping a line of source file.\n\ +If off, GDB will not use it, even if such is supported by the \n\ +target"), + maint_set_range_stepping, + maint_show_range_stepping, + &maintenance_set_cmdlist, + &maintenance_show_cmdlist); + /* Eventually initialize fileio. See fileio.c */ initialize_remote_fileio (remote_set_cmdlist, remote_show_cmdlist); -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] range stepping: New command 'maint set range stepping' 2013-04-11 6:19 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi @ 2013-04-11 23:00 ` Eli Zaretskii 0 siblings, 0 replies; 44+ messages in thread From: Eli Zaretskii @ 2013-04-11 23:00 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > From: Yao Qi <yao@codesourcery.com> > Date: Thu, 11 Apr 2013 10:43:40 +0800 > > On 03/18/2013 11:45 AM, Eli Zaretskii wrote: > >> Don't have any special reasons to do this. The range stepping should be > >> >always on if target supports, and it can be turned off for some > >> >"maintenance purpose", so I set this command as a maintenance > >> >command. > > What would be the possible reasons to turn this feature off? > > > > Once we have the reasons, we could try thinking whether or not they > > are limited to GDB maintenance. > > Range-stepping is useful to speed up some operations, so users don't > have to turn it off. The reason turning it off could be that the > remote stub has a bug in supporting range stepping, so we have to > disable it in GDB side. To me, this means that the command is not really a maintenance command. It is a user option, albeit one that is not expected to be used frequently. > I considered to name the command to "set remote range-stepping", but > range-stepping is not specific to remote target, and it can be > implemented on native target as well. So "set remote range-stepping" > is not a good choice. I also considered "set range-stepping", but > unable to find a reasonable place for documentation on it. How about "Continuing and Stepping"? > In V2, I don't update the command name. > +@kindex maint set range-stepping > +@kindex maint show range-stepping > +@cindex range-stepping > +@item maint set range-stepping > +@itemx maint show range-stepping > +@vindex $range_stepping_counter > +Control whether to do stepping in an address range. The debugger convenience > +variable @samp{$range_stepping_counter} contains the number of range stepping > +@value{GDBN} has performed. This is OK, but it doesn't really explain what the setting does. The doc string of the command is much better: > + add_setshow_boolean_cmd ("range-stepping", class_maintenance, > + &use_range_stepping, _("\ > +Enable or disable range-stepping."), _("\ > +Show whether range-stepping is enabled."), _("\ > +If On, GDB will tell the target to do stepping a range of address.\n\ > +This will speed up stepping a line of source file.\n\ > +If off, GDB will not use it, even if such is supported by the \n\ > +target"), Except that "do stepping a range of address" is not really correct. How about this rewording: If On, GDB will instruct the target to single-step through all the instructions in a range of addresses, and report back only once at the end. This reduces the amount of communication during stepping. ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 4/7] range stepping: gdb 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi ` (3 preceding siblings ...) 2013-04-11 6:19 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi @ 2013-04-11 6:19 ` Yao Qi 2013-04-11 13:22 ` Yao Qi 2013-04-11 6:38 ` [PATCH 6/7] range stepping: test case Yao Qi ` (2 subsequent siblings) 7 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:19 UTC (permalink / raw) To: gdb-patches This patch teaches GDB to send 'vCont;r' packet when appropriate. GDB has to check whether the target understand 'r' action of 'vCont' packet. We also make use of fields 'step_range_start' and 'step_range_end' of 'struct thread_control_state' to compose the range of the stepping. In V2, there are several changes: - Use phex_nz instead of hexnumstr to get the address. - Define an internalvar range_stepping_counter and increment it each time GDB does range-stepping. gdb: 2013-04-10 Yao Qi <yao@codesourcery.com> * remote.c (struct support_v_cont) <r>: New field. (remote_vcont_probe): Handle 'r' action of 'vCont' packet. (append_resumption): Send 'vCont;r' for range stepping and increment internalvar range_stepping_counter. --- gdb/remote.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 46 insertions(+), 1 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index f6a3f4c..a063698 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -256,6 +256,8 @@ struct support_v_cont { /* True if the stub reports support for vCont;t. */ int t; + /* True if the stub reports support for vCont;r. */ + int r; }; /* Description of the remote protocol state for the currently @@ -4649,6 +4651,7 @@ remote_vcont_probe (struct remote_state *rs) support_c = 0; support_C = 0; rs->support_vCont.t = 0; + rs->support_vCont.r = 0; while (p && *p == ';') { p++; @@ -4662,6 +4665,8 @@ remote_vcont_probe (struct remote_state *rs) support_C = 1; else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) rs->support_vCont.t = 1; + else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0)) + rs->support_vCont.r = 1; p = strchr (p, ';'); } @@ -4694,7 +4699,47 @@ append_resumption (char *p, char *endp, if (step && siggnal != GDB_SIGNAL_0) p += xsnprintf (p, endp - p, ";S%02x", siggnal); else if (step) - p += xsnprintf (p, endp - p, ";s"); + { + struct thread_info *tp = NULL; + CORE_ADDR pc; + + gdb_assert (!ptid_equal (ptid, minus_one_ptid)); + + if (ptid_is_pid (ptid)) + tp = find_thread_ptid (inferior_ptid); + else + tp = find_thread_ptid (ptid); + gdb_assert (tp != NULL); + + pc = regcache_read_pc (get_thread_regcache (ptid)); + if (rs->support_vCont.r /* Target supports step range. */ + /* Can't do range stepping for all threads of a process + 'pPID.-1'. */ + && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) + /* Not step over a breakpoint. */ + && !tp->control.trap_expected + /* We have a range to single step. */ + && THREAD_WITHIN_SINGLE_STEP_RANGE (tp, pc)) + { + int addr_size = gdbarch_addr_bit (target_gdbarch ()) / 8; + struct internalvar *range_stepping_counter + = lookup_internalvar ("range_stepping_counter"); + LONGEST result = 0; + + p += xsnprintf (p, endp - p, ";r%s,%s", + phex_nz (tp->control.step_range_start, + addr_size), + phex_nz (tp->control.step_range_end, + addr_size)); + + get_internalvar_integer (range_stepping_counter, &result); + set_internalvar_integer (range_stepping_counter, + result + 1); + + } + else + p += xsnprintf (p, endp - p, ";s"); + } else if (siggnal != GDB_SIGNAL_0) p += xsnprintf (p, endp - p, ";C%02x", siggnal); else -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] range stepping: gdb 2013-04-11 6:19 ` [PATCH 4/7] range stepping: gdb Yao Qi @ 2013-04-11 13:22 ` Yao Qi 2013-04-12 12:35 ` Yao Qi 0 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-04-11 13:22 UTC (permalink / raw) To: gdb-patches On 04/11/2013 10:43 AM, Yao Qi wrote: > - Use phex_nz instead of hexnumstr to get the address. ..... > + > + p += xsnprintf (p, endp - p, ";r%s,%s", > + phex_nz (tp->control.step_range_start, > + addr_size), > + phex_nz (tp->control.step_range_end, > + addr_size)); It is incorrect to copy address to buffer via phex_nz here and somewhere else in remote.c. We should use remote_address_masked first to mask the CORE_ADDR and then call hexnumstr to copy address to buffer. Here is an updated one. This patch teaches GDB to send 'vCont;r' packet when appropriate. GDB has to check whether the target understand 'r' action of 'vCont' packet. We also make use of fields 'step_range_start' and 'step_range_end' of 'struct thread_control_state' to compose the range of the stepping. In V2, there are several changes: - Call remote_address_masked first before call hexnumstr to get the address. - Define an internalvar range_stepping_counter and increment it each time GDB does range-stepping. -- Yao (é½å°§) gdb: 2013-04-10 Yao Qi <yao@codesourcery.com> * remote.c (struct support_v_cont) <r>: New field. (remote_vcont_probe): Handle 'r' action of 'vCont' packet. (append_resumption): Send 'vCont;r' for range stepping and increment internalvar range_stepping_counter. --- gdb/remote.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 48 insertions(+), 1 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index f6a3f4c..4158a09 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -256,6 +256,8 @@ struct support_v_cont { /* True if the stub reports support for vCont;t. */ int t; + /* True if the stub reports support for vCont;r. */ + int r; }; /* Description of the remote protocol state for the currently @@ -4649,6 +4651,7 @@ remote_vcont_probe (struct remote_state *rs) support_c = 0; support_C = 0; rs->support_vCont.t = 0; + rs->support_vCont.r = 0; while (p && *p == ';') { p++; @@ -4662,6 +4665,8 @@ remote_vcont_probe (struct remote_state *rs) support_C = 1; else if (*p == 't' && (*(p + 1) == ';' || *(p + 1) == 0)) rs->support_vCont.t = 1; + else if (*p == 'r' && (*(p + 1) == ';' || *(p + 1) == 0)) + rs->support_vCont.r = 1; p = strchr (p, ';'); } @@ -4694,7 +4699,49 @@ append_resumption (char *p, char *endp, if (step && siggnal != GDB_SIGNAL_0) p += xsnprintf (p, endp - p, ";S%02x", siggnal); else if (step) - p += xsnprintf (p, endp - p, ";s"); + { + struct thread_info *tp = NULL; + CORE_ADDR pc; + + gdb_assert (!ptid_equal (ptid, minus_one_ptid)); + + if (ptid_is_pid (ptid)) + tp = find_thread_ptid (inferior_ptid); + else + tp = find_thread_ptid (ptid); + gdb_assert (tp != NULL); + + pc = regcache_read_pc (get_thread_regcache (ptid)); + if (rs->support_vCont.r /* Target supports step range. */ + /* Can't do range stepping for all threads of a process + 'pPID.-1'. */ + && !(remote_multi_process_p (rs) && ptid_is_pid (ptid)) + /* Not step over a breakpoint. */ + && !tp->control.trap_expected + /* We have a range to single step. */ + && THREAD_WITHIN_SINGLE_STEP_RANGE (tp, pc)) + { + struct internalvar *range_stepping_counter + = lookup_internalvar ("range_stepping_counter"); + LONGEST result = 0; + CORE_ADDR addr; + + p += xsnprintf (p, endp - p, ";r"); + + addr = tp->control.step_range_start; + p += hexnumstr (p, (ULONGEST) remote_address_masked (addr)); + p += xsnprintf (p, endp - p, ","); + addr = tp->control.step_range_end; + p += hexnumstr (p, (ULONGEST) remote_address_masked (addr)); + + get_internalvar_integer (range_stepping_counter, &result); + set_internalvar_integer (range_stepping_counter, + result + 1); + + } + else + p += xsnprintf (p, endp - p, ";s"); + } else if (siggnal != GDB_SIGNAL_0) p += xsnprintf (p, endp - p, ";C%02x", siggnal); else -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/7] range stepping: gdb 2013-04-11 13:22 ` Yao Qi @ 2013-04-12 12:35 ` Yao Qi 0 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-04-12 12:35 UTC (permalink / raw) To: gdb-patches On 04/11/2013 03:36 PM, Yao Qi wrote: > It is incorrect to copy address to buffer via phex_nz here and > somewhere else in remote.c. We should use remote_address_masked first > to mask the CORE_ADDR and then call hexnumstr to copy address to > buffer. Here is an updated one. After a further thought, I decide to withdraw the updated patch 4/7, and keep using the original V2 patch 4/7. The comments of variable 'remote_address_size' say: /* This variable sets the number of bits in an address that are to be sent in a memory ("M" or "m") packet. Normally, after stripping leading zeros, the entire address would be sent. This variable restricts the address to REMOTE_ADDRESS_SIZE bits. HISTORY: The initial implementation of remote.c restricted the address sent in memory packets to ``host::sizeof long'' bytes - (typically 32 bits). Consequently, for 64 bit targets, the upper 32 bits of an address was never sent. Since fixing this bug may cause a break in some remote targets this variable is principly provided to facilitate backward compatibility. */ static unsigned int remote_address_size; variable 'remote_address_size' has only effect on memory packet and breakpoint packet (it is not documented, but actually used). Address in these packets should be masked by remote_address_masked, and address in new packets should not be, because the stub supports these new packets should be able to parse address correctly. -- Yao (é½å°§) ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 6/7] range stepping: test case 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi ` (4 preceding siblings ...) 2013-04-11 6:19 ` [PATCH 4/7] range stepping: gdb Yao Qi @ 2013-04-11 6:38 ` Yao Qi 2013-04-11 7:30 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi 2013-04-12 20:48 ` [PATCH 0/7 V2] Range stepping Pedro Alves 7 siblings, 0 replies; 44+ messages in thread From: Yao Qi @ 2013-04-11 6:38 UTC (permalink / raw) To: gdb-patches This test case is used to verify range stepping is used by means of checking the internalvar '$range_stepping_counter'. In V2, the test cases check the value of '$range_stepping_counter' instead of parsing RSP packets. gdb/testsuite: 2013-04-10 Yao Qi <yao@codesourcery.com> * gdb.base/range-stepping.c: New. * gdb.base/range-stepping.exp: New. * gdb.trace/range-stepping.c: New. * gdb.trace/range-stepping.exp: New. --- gdb/testsuite/gdb.base/range-stepping.c | 69 +++++++++ gdb/testsuite/gdb.base/range-stepping.exp | 222 ++++++++++++++++++++++++++++ gdb/testsuite/gdb.trace/range-stepping.c | 61 ++++++++ gdb/testsuite/gdb.trace/range-stepping.exp | 89 +++++++++++ 4 files changed, 441 insertions(+), 0 deletions(-) create mode 100644 gdb/testsuite/gdb.base/range-stepping.c create mode 100644 gdb/testsuite/gdb.base/range-stepping.exp create mode 100644 gdb/testsuite/gdb.trace/range-stepping.c create mode 100644 gdb/testsuite/gdb.trace/range-stepping.exp diff --git a/gdb/testsuite/gdb.base/range-stepping.c b/gdb/testsuite/gdb.base/range-stepping.c new file mode 100644 index 0000000..930cb8d --- /dev/null +++ b/gdb/testsuite/gdb.base/range-stepping.c @@ -0,0 +1,69 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2013 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +static int +func1 (int a, int b) +{ + int r = a * b; + + r =+ (a | b); + r =+ (a - b); + + return r; +} + +int +main(void) +{ + int a = 0; + int b = 1; + int c = 2; + int d = 3; + int e = 4; + double d1 = 1.0; + double d2 = 2.0; + + /* A line of source will be generated to a number of + instructions by compiler. */ + a = b + c + d * e - a; /* location 1 */ + + /* To compose multiple instructions before and after + 'function call' or 'branch' instruction. This line + of source will be generated to the following instructions, + +addr1: + insn1; + insn2; + ... + call func1; + ... + insn3; +addr2: + insn4; */ + e = 10 + func1 (a + b, c * d); /* location 2 */ + + e = 10 + func1 (a + b, c * d); + /* Generate a range that includes a loop in it. */ + for (a = 0, e = 0; a < 15; a++) { e += a;} + /* Generate a range that includes a loop in it. */ + for (a = 0, e = 0; a < 15; a++) { e += a;} + /* Generate a range that includes a loop, which is time consuming. + Variable C is used to terminate the loop earlier when GDB + wants. */ + for (c = 1, a = 0; a < 65535 && c; a++) {for (b = 0; b < 65535 && c; b++) { d1 = d2 * a / b; d2 = d1 * a;}} + return 0; +} diff --git a/gdb/testsuite/gdb.base/range-stepping.exp b/gdb/testsuite/gdb.base/range-stepping.exp new file mode 100644 index 0000000..0f2a2dd --- /dev/null +++ b/gdb/testsuite/gdb.base/range-stepping.exp @@ -0,0 +1,222 @@ +# Copyright 2013 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +standard_testfile .c +set executable $testfile + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" \ + executable {debug nowarnings}] != "" } { + untested ${testfile}.exp + return -1 +} + +clean_restart $executable + +if ![runto_main] { + fail "Can't run to main" + return -1 +} + +# Check range stepping is supported by target or not. +set test "range-stepping is supported" +gdb_test_multiple "maintenance show range-stepping" "show range-stepping" { + -re "Debugger's willingness to do range-stepping is off.*\r\n$gdb_prompt $" { + unsupported $test + return -1 + } + -re "Debugger's willingness to do range-stepping is on.*\r\n$gdb_prompt $" { + pass $test + } +} + +# Check range stepping can step a range of multiple instructions. + +with_test_prefix "multi insns" { + global executable + global hex decimal + global gdb_prompt + + gdb_breakpoint [gdb_get_line_number "location 1"] + gdb_continue_to_breakpoint "location 1" + + set pc_before_stepping "" + set test "pc before stepping" + gdb_test_multiple "print/x \$pc" $test { + -re "\\\$$decimal = (\[^\r\n\]*)\r\n$gdb_prompt $" { + set pc_before_stepping $expect_out(1,string) + pass $test + } + } + + # When execute command "next", GDB should send one vCont;s and + # vCont;r and receive two stop replies. + # --> vCont;s (step over breakpoint) + # <-- T0505:XXX + # --> vCont;rSTART,END (do range stepping) + # <-- T0505:XXX + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "next" + gdb_test "p \$range_stepping_counter" " = 1" + + set pc_after_stepping "" + set msg "pc before stepping" + gdb_test_multiple "print/x \$pc" $msg { + -re "\\\$$decimal = (\[^\r\n\]*)\r\n$gdb_prompt $" { + set pc_after_stepping $expect_out(1,string) + pass $msg + } + } + + # At least, there are two instructions between + # PC_BEFORE_STEPPING and PC_AFTER_STEPPING. + gdb_test "disassemble ${pc_before_stepping},${pc_after_stepping}" \ + "${hex} <main\\+${decimal}>:.*${hex} <main\\+${decimal}>:.*" \ + "stepped multiple insns" +} + +# Check range stepping can step over a function. + +with_test_prefix "step over func" { + global srcfile + + set line_num [gdb_get_line_number "location 2"] + gdb_test "where" "main \\(\\) at .*${srcfile}:${line_num}.*" + + # It is expected to get three stops and two 'vCont;r'. In the C + # code, the line of C source produces the following instructions, + # + # addr1: + # insn1; + # insn2; + # ... + # call func1; + # addr2: + # ... + # insn3; + # addr3: + # insn4; + # + # Something like this will happen: + # --> vCont;rADDR1,ADDR3 (do range stepping from ADDR1 to ADDR3) + # <-- T0505:XXXX + # (inferior is single-stepping to func, which is out of the range) + # --> $Z0,ADDR2 + # --> vCont;c (set step-resume breakpoint on ADDR2, and resume) + # <-- T0505:XXXX (inferior stops at ADD2) + # --> vCont;rADDR1,ADDR3 (continues to do range stepping) + # <-- T0505:XXXX + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "next" + gdb_test "p \$range_stepping_counter" " = 2" +} + +# Check range stepping works well with breakpoint. + +with_test_prefix "breakpoint" { + gdb_breakpoint "func1" + # Something like this will happen: + # --> vCont;rADDR1,ADDR3 + # <-- T0505:XXXX + # (inferior is single-stepping to func1, which is out of the range) + # --> $Z0,ADDR2 + # --> vCont;c (set step-resume breakpoint on ADDR2, and resume) + # <-- T0505:XXXX (inferior hits the breakpoint on fun1) + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "next" "." "" + gdb_test "p \$range_stepping_counter" " = 1" \ + "range-stepping-counter is 1" + + gdb_test "backtrace" "#0 .* func1 .*#1 .* main .*" \ + "backtrace from func1" + # The range stepping was interrupted by a breakpoint, but the + # state related to range stepping should be cleared. + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "stepi" + gdb_test "p \$range_stepping_counter" " = 0" \ + "range-stepping-counter is 0" + gdb_test "finish" ".*" + + gdb_test "next" ".*" + delete_breakpoints +} + +# Check range stepping works well with a loop in the range. + +with_test_prefix "loop" { + + # GDB should send one vCont;r and receive one stop reply. + # --> vCont;rSTART,END (do range stepping) + # <-- T0505:XXX + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "next" + gdb_test "p \$range_stepping_counter" " = 1" + + # Check loop is done. + gdb_test "print a" "\\$${decimal} = 15\[\r\n\].*" + gdb_test "print e" "\\$${decimal} = 105\[\r\n\].*" +} + +# Check range stepping works well when PC has already within the loop +# body. + +with_test_prefix "loop" { + # Stepi into the loop body. 15 is large enough to make sure + # program goes to loop body. + gdb_test "stepi 15" ".*" + # GDB should send one vCont;r and receive one stop reply. + # --> vCont;rSTART,END (do range stepping) + # <-- T0505:XXX + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "next" + gdb_test "p \$range_stepping_counter" " = 1" + + # Check loop is done. + gdb_test "print a" "\\$${decimal} = 15\[\r\n\].*" + gdb_test "print e" "\\$${decimal} = 105\[\r\n\].*" +} + +# Check range stepping works well when it is interrupted by ctrl-c. + +with_test_prefix "interrupt" { + gdb_test_no_output "set \$range_stepping_counter = 0" "" + + send_gdb "next\n" + sleep 1 + send_gdb "\003" + + # GDB should send one vCont;r and receive one stop reply about + # SIGINT. + # --> vCont;rSTART,END (do range stepping) + # <-- T0205:XXX + + set test "send gdb ctrl-c" + gdb_expect { + -re "Program received signal SIGINT.*$gdb_prompt $" { + pass $test + } + timeout { fail "${test} (timeout)" } + eof { fail "${test} (eof)" } + } + gdb_test "p \$range_stepping_counter" " = 1" \ + "range-stepping-counter is 1" + + # Terminate the loop earlier and continue to do range stepping. + gdb_test "set variable c = 0" + gdb_test "next" + gdb_test "p \$range_stepping_counter" " = 2" \ + "range-stepping-counter is 2" +} + +return 0 diff --git a/gdb/testsuite/gdb.trace/range-stepping.c b/gdb/testsuite/gdb.trace/range-stepping.c new file mode 100644 index 0000000..50db5a0 --- /dev/null +++ b/gdb/testsuite/gdb.trace/range-stepping.c @@ -0,0 +1,61 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2013 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifdef SYMBOL_PREFIX +#define SYMBOL(str) SYMBOL_PREFIX #str +#else +#define SYMBOL(str) #str +#endif + +/* Called from asm. */ +static void __attribute__((used)) +func2 (void) +{} + +static int +func1 (int a, int b) +{ + int r = a * b; + + /* `set_point' is the label where we'll set tracepoint at. The insn + at the label must the large enough to fit a fast tracepoint + jump. */ + asm (" .global " SYMBOL(set_point) "\n" + SYMBOL(set_point) ":\n" +#if (defined __x86_64__ || defined __i386__) + " call " SYMBOL(func2) "\n" +#endif + ); + + r =+ (a | b); + r =+ (a - b); + + return r; +} + +int +main(void) +{ + int a = 0; + int b = 1; + int c = 2; + int d = 3; + int e = 4; + + e = 10 + func1 (a + b, c * d); /* location 1 */ + return 0; +} diff --git a/gdb/testsuite/gdb.trace/range-stepping.exp b/gdb/testsuite/gdb.trace/range-stepping.exp new file mode 100644 index 0000000..22c705d --- /dev/null +++ b/gdb/testsuite/gdb.trace/range-stepping.exp @@ -0,0 +1,89 @@ +# Copyright 2013 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +load_lib "trace-support.exp" + +standard_testfile +set executable $testfile +set expfile $testfile.exp + +if [prepare_for_testing $expfile $executable $srcfile \ + {debug nowarnings}] { + untested "failed to prepare for trace tests" + return -1 +} + +if ![runto_main] { + fail "Can't run to main to check for trace support" + return -1 +} + +if ![gdb_target_supports_trace] { + unsupported "target does not support trace" + return -1; +} + +# Check range stepping works well with tracepoint. +proc range_stepping_with_tracepoint { type } { + with_test_prefix "${type}" { + + gdb_breakpoint [gdb_get_line_number "location 1"] + gdb_continue_to_breakpoint "location 1" + delete_breakpoints + + gdb_test "${type} set_point" ".*" + gdb_test_no_output "tstart" + + # GDB will step over function func1, in which a tracepoint is + # set. GDB will send two vCont;r packets because calling to + # func1 is out of the range. However, tracepoint itself + # shouldn't have any effect on range stepping. + gdb_test_no_output "set \$range_stepping_counter = 0" "" + gdb_test "next" + gdb_test "p \$range_stepping_counter" " = 2" + gdb_test_no_output "tstop" + gdb_test "tfind" "Found trace frame 0.*" "tfind 0" + gdb_test "tfind" \ + "Target failed to find requested trace frame.*" \ + "tfind 1" + + delete_breakpoints + } +} + +range_stepping_with_tracepoint "trace" + +set libipa [get_in_proc_agent] +gdb_load_shlibs $libipa + +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \ + executable [list debug nowarnings shlib=$libipa] ] != "" } { + untested "failed to compile ftrace tests" + return -1 +} + +clean_restart ${executable} + +if ![runto_main] { + fail "Can't run to main for ftrace tests" + return 0 +} + +gdb_reinitialize_dir $srcdir/$subdir +if { [gdb_test "info sharedlibrary" ".*${libipa}.*" "IPA loaded"] != 0 } { + untested "Could not find IPA lib loaded" +} else { + range_stepping_with_tracepoint "ftrace" +} -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 7/7] range stepping: doc and NEWS 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi ` (5 preceding siblings ...) 2013-04-11 6:38 ` [PATCH 6/7] range stepping: test case Yao Qi @ 2013-04-11 7:30 ` Yao Qi 2013-04-11 23:00 ` Eli Zaretskii 2013-04-12 20:48 ` [PATCH 0/7 V2] Range stepping Pedro Alves 7 siblings, 1 reply; 44+ messages in thread From: Yao Qi @ 2013-04-11 7:30 UTC (permalink / raw) To: gdb-patches It was reviewed and approved by Eli here <http://sourceware.org/ml/gdb-patches/2013-03/msg00475.html> gdb/doc: 2013-04-10 Yao Qi <yao@codesourcery.com> * gdb.texinfo (Packets): Document about 'vCont;r'. gdb: 2013-04-10 Yao Qi <yao@codesourcery.com> * NEWS: Mention range stepping, new packet and new commands. --- gdb/NEWS | 12 ++++++++++++ gdb/doc/gdb.texinfo | 5 +++++ 2 files changed, 17 insertions(+), 0 deletions(-) diff --git a/gdb/NEWS b/gdb/NEWS index 6f202e2..0368c10 100644 --- a/gdb/NEWS +++ b/gdb/NEWS @@ -10,6 +10,10 @@ maint set|show per-command time maint set|show per-command symtab Enable display of per-command gdb resource usage. +maint set range-stepping +maint show range-stepping + Control and show whether to do range stepping. + * New options set remote trace-status-packet @@ -240,11 +244,19 @@ show debug notification feature to be enabled. For more information, see: http://fedoraproject.org/wiki/Features/MiniDebugInfo +* GDB now supports range stepping, which improves the performance of + single stepping over a source line by reducing the number of control + packets from GDB. + * New remote packets QTBuffer:size Set the size of trace buffer. The remote stub reports support for this packet to gdb's qSupported query. +vCont;r + Tell the remote stub to do range stepping in an address range. The remote + stub reports a stop reply when the program goes out of the range or is + stopped due to other reasons, such as hitting a breakpoint. Qbtrace:bts Enable Branch Trace Store (BTS)-based branch tracing for the current diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 9292b94..16059dc 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -36375,6 +36375,11 @@ Step. Step with signal @var{sig}. The signal @var{sig} should be two hex digits. @item t Stop. +@item r @var{start},@var{end} +Step repeatedly while the PC is within the range [@var{start}, +@var{end}). Note that a stop reply may be sent at any point even if +the PC is within the stepping range; for example, it is permissible to +implement this packet in a degenerate way as a single step operation. @end table The optional argument @var{addr} normally associated with the -- 1.7.7.6 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] range stepping: doc and NEWS 2013-04-11 7:30 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi @ 2013-04-11 23:00 ` Eli Zaretskii 0 siblings, 0 replies; 44+ messages in thread From: Eli Zaretskii @ 2013-04-11 23:00 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches > From: Yao Qi <yao@codesourcery.com> > Date: Thu, 11 Apr 2013 10:43:42 +0800 > > 2013-04-10 Yao Qi <yao@codesourcery.com> > > * NEWS: Mention range stepping, new packet and new > commands. OK for this part. > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 9292b94..16059dc 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -36375,6 +36375,11 @@ Step. > Step with signal @var{sig}. The signal @var{sig} should be two hex digits. > @item t > Stop. > +@item r @var{start},@var{end} > +Step repeatedly while the PC is within the range [@var{start}, > +@var{end}). Note that a stop reply may be sent at any point even if > +the PC is within the stepping range; for example, it is permissible to > +implement this packet in a degenerate way as a single step operation. > @end table This is also OK. Thanks. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/7 V2] Range stepping 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi ` (6 preceding siblings ...) 2013-04-11 7:30 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi @ 2013-04-12 20:48 ` Pedro Alves 7 siblings, 0 replies; 44+ messages in thread From: Pedro Alves @ 2013-04-12 20:48 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 04/11/2013 03:43 AM, Yao Qi wrote: > This is the V2 of range stepping series. V1 can be found here > <http://sourceware.org/ml/gdb-patches/2013-03/msg00450.html>. > > There are some updates in V2: > > - Rebase patches on GDB trunk, and resolve conflicts. > - Use phex_nz instead of hexnumstr to get the address. It fixes a > bug on mips target when sending address to the remote target. > - Define an internalvar range_stepping_counter and increment it each > time GDB does range-stepping, so that in the test, we don't have to > turn 'remote debug' on and parse the RSP packets. During the test, > we find that it is unsafe to turn the debugging output on, because it > may blow up the gdb.log. > > Patch 1,2,3,7 are unchanged. > > The whole series is tested on x86_64-linux, native and gdbserver. Thanks! As I'm no longer much distracted with 7.6 things or that dreaded -Wpointer-sign thing, I've resumed review/hacking on this. I really really want to get this over with, but please bear with me. :-) -- Pedro Alves ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2013-05-22 14:01 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-03-11 12:52 [PATCH 0/7] Range stepping Yao Qi 2013-03-11 12:53 ` [PATCH 4/7] range stepping: gdb Yao Qi 2013-05-14 18:31 ` Pedro Alves 2013-05-15 8:07 ` Yao Qi 2013-05-20 17:59 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi 2013-03-11 17:05 ` Eli Zaretskii 2013-03-18 3:10 ` Yao Qi 2013-03-18 5:39 ` Eli Zaretskii 2013-05-14 18:31 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 6/7] range stepping: test case Yao Qi 2013-05-14 18:32 ` Pedro Alves 2013-05-15 8:27 ` Yao Qi 2013-05-20 18:29 ` Pedro Alves 2013-05-22 14:01 ` Yao Qi 2013-03-11 12:53 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi 2013-05-14 19:24 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 2/7] Move rs->support_vCont_t to a separate struct Yao Qi 2013-03-11 12:53 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi 2013-03-11 13:38 ` Abid, Hafiz 2013-03-11 17:01 ` Eli Zaretskii 2013-05-14 18:32 ` Pedro Alves 2013-03-11 12:53 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi 2013-05-14 18:30 ` Pedro Alves 2013-05-15 7:40 ` Yao Qi 2013-05-20 18:00 ` Pedro Alves 2013-05-22 10:06 ` Yao Qi 2013-03-14 20:12 ` [PATCH 0/7] Range stepping Pedro Alves 2013-03-15 19:54 ` Pedro Alves 2013-03-22 2:25 ` Yao Qi 2013-03-22 20:24 ` Pedro Alves 2013-04-11 6:16 ` [PATCH 0/7 V2] " Yao Qi 2013-04-11 6:17 ` [PATCH 1/7] New macro THREAD_WITHIN_SINGLE_STEP_RANGE Yao Qi 2013-04-11 6:17 ` [PATCH 2/7] Move rs->support_vCont_t to a separate struct Yao Qi 2013-04-11 6:18 ` [PATCH 3/7] range stepping: gdbserver on x86/linux Yao Qi 2013-04-11 6:19 ` [PATCH 5/7] range stepping: New command 'maint set range stepping' Yao Qi 2013-04-11 23:00 ` Eli Zaretskii 2013-04-11 6:19 ` [PATCH 4/7] range stepping: gdb Yao Qi 2013-04-11 13:22 ` Yao Qi 2013-04-12 12:35 ` Yao Qi 2013-04-11 6:38 ` [PATCH 6/7] range stepping: test case Yao Qi 2013-04-11 7:30 ` [PATCH 7/7] range stepping: doc and NEWS Yao Qi 2013-04-11 23:00 ` Eli Zaretskii 2013-04-12 20:48 ` [PATCH 0/7 V2] Range stepping Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox