* [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions @ 2016-02-17 2:44 Pedro Alves 2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves ` (4 more replies) 0 siblings, 5 replies; 15+ messages in thread From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw) To: gdb-patches Since: [PATCH 00/18] Remote all-stop on top of non-stop https://www.sourceware.org/ml/gdb-patches/2015-10/msg00213.html The remote target supports "maint set target-non-stop on". However that's still not the default. I thought I'd first try to aggregate vCont packets, because with "maint set target-non-stop on", we go from: - All-stop / "maint set target-non-stop off" (gdb) c Continuing. (...) Sending packet: $vCont;c#a8.. to: - All-stop / "maint set target-non-stop on" (gdb) c Continuing. (...) Sending packet: $vCont;c:p2c45.2c45#7c...Packet received: OK Sending packet: $vCont;c:p2c45.2c4f#ad...Packet received: OK Sending packet: $vCont;c:p2c45.2c50#78...Packet received: OK ... one packet per thread ... After the series, we'll get back: (gdb) c Continuing. (...) Sending packet: $vCont;c#a8...Packet received: OK (Note the "OK", showing that that was indeed an async vCont resume.) along with other "wildcard" vCont packets like, "vCont;s:p1.1;c". Also pushed to the users/palves/vcont-coalesce-actions branch. Pedro Alves (5): gdb: Clean up remote.c:remote_resume gdb: Free inferior->priv when inferior exits gdb/doc: Clarify vCont packet description gdbserver: Leave already-vCont-resumed threads as they were gdb: Coalesce/aggregate (async) vCont packets/actions gdb/doc/gdb.texinfo | 32 ++- gdb/gdbserver/linux-low.c | 27 +++ gdb/gdbserver/server.c | 33 ++- gdb/gdbserver/server.h | 4 + gdb/inferior.c | 3 + gdb/inferior.h | 6 + gdb/infrun.c | 8 + gdb/record-btrace.c | 11 + gdb/record-full.c | 11 + gdb/remote.c | 566 ++++++++++++++++++++++++++++++++++++++++------ gdb/target-delegates.c | 26 +++ gdb/target.c | 29 +++ gdb/target.h | 46 +++- 13 files changed, 705 insertions(+), 97 deletions(-) -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/5] gdb: Clean up remote.c:remote_resume 2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves @ 2016-02-17 2:44 ` Pedro Alves 2016-02-17 11:45 ` Luis Machado 2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves ` (3 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw) To: gdb-patches Just some refactoring / TLC. Mainly split the old c/s/C/S packet handling to a separate function. gdb/ChangeLog: 2016-02-09 Pedro Alves <palves@redhat.com> * remote.c (remote_resume_with_hc): New function, factored out from ... (remote_resume): ... this. Always try vCont first. (remote_vcont_resume): Rename to ... (remote_resume_with_vcont): ... this. Bail out if execution direction is reverse. --- gdb/remote.c | 113 ++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 62 insertions(+), 51 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index fa97e1e..60e2dda 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) return p; } +/* Set the target running, using the packets that use Hc + (c/s/C/S). */ + +static void +remote_resume_with_hc (struct target_ops *ops, + ptid_t ptid, int step, enum gdb_signal siggnal) +{ + struct remote_state *rs = get_remote_state (); + struct thread_info *thread; + char *buf; + + rs->last_sent_signal = siggnal; + rs->last_sent_step = step; + + /* The c/s/C/S resume packets use Hc, so set the continue + thread. */ + if (ptid_equal (ptid, minus_one_ptid)) + set_continue_thread (any_thread_ptid); + else + set_continue_thread (ptid); + + ALL_NON_EXITED_THREADS (thread) + resume_clear_thread_private_info (thread); + + buf = rs->buf; + if (execution_direction == EXEC_REVERSE) + { + /* We don't pass signals to the target in reverse exec mode. */ + if (info_verbose && siggnal != GDB_SIGNAL_0) + warning (_(" - Can't pass signal %d to target in reverse: ignored."), + siggnal); + + if (step && packet_support (PACKET_bs) == PACKET_DISABLE) + error (_("Remote reverse-step not supported.")); + if (!step && packet_support (PACKET_bc) == PACKET_DISABLE) + error (_("Remote reverse-continue not supported.")); + + strcpy (buf, step ? "bs" : "bc"); + } + else if (siggnal != GDB_SIGNAL_0) + { + buf[0] = step ? 'S' : 'C'; + buf[1] = tohex (((int) siggnal >> 4) & 0xf); + buf[2] = tohex (((int) siggnal) & 0xf); + buf[3] = '\0'; + } + else + strcpy (buf, step ? "s" : "c"); + + putpkt (buf); +} + /* Resume the remote inferior by using a "vCont" packet. The thread to be resumed is PTID; STEP and SIGGNAL indicate whether the resumed thread should be single-stepped and/or signalled. If PTID @@ -5467,16 +5519,20 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) be stepped and/or signalled is given in the global INFERIOR_PTID. This function returns non-zero iff it resumes the inferior. - This function issues a strict subset of all possible vCont commands at the - moment. */ + This function issues a strict subset of all possible vCont commands + at the moment. */ static int -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal) +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal) { struct remote_state *rs = get_remote_state (); char *p; char *endp; + /* No reverse support (yet) for vCont. */ + if (execution_direction == EXEC_REVERSE) + return 0; + if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN) remote_vcont_probe (rs); @@ -5548,8 +5604,6 @@ remote_resume (struct target_ops *ops, ptid_t ptid, int step, enum gdb_signal siggnal) { struct remote_state *rs = get_remote_state (); - char *buf; - struct thread_info *thread; /* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN (explained in remote-notif.c:handle_notification) so @@ -5560,53 +5614,10 @@ remote_resume (struct target_ops *ops, if (!target_is_non_stop_p ()) remote_notif_process (rs->notif_state, ¬if_client_stop); - rs->last_sent_signal = siggnal; - rs->last_sent_step = step; - - /* The vCont packet doesn't need to specify threads via Hc. */ - /* No reverse support (yet) for vCont. */ - if (execution_direction != EXEC_REVERSE) - if (remote_vcont_resume (ptid, step, siggnal)) - goto done; - - /* All other supported resume packets do use Hc, so set the continue - thread. */ - if (ptid_equal (ptid, minus_one_ptid)) - set_continue_thread (any_thread_ptid); - else - set_continue_thread (ptid); - - ALL_NON_EXITED_THREADS (thread) - resume_clear_thread_private_info (thread); - - buf = rs->buf; - if (execution_direction == EXEC_REVERSE) - { - /* We don't pass signals to the target in reverse exec mode. */ - if (info_verbose && siggnal != GDB_SIGNAL_0) - warning (_(" - Can't pass signal %d to target in reverse: ignored."), - siggnal); - - if (step && packet_support (PACKET_bs) == PACKET_DISABLE) - error (_("Remote reverse-step not supported.")); - if (!step && packet_support (PACKET_bc) == PACKET_DISABLE) - error (_("Remote reverse-continue not supported.")); - - strcpy (buf, step ? "bs" : "bc"); - } - else if (siggnal != GDB_SIGNAL_0) - { - buf[0] = step ? 'S' : 'C'; - buf[1] = tohex (((int) siggnal >> 4) & 0xf); - buf[2] = tohex (((int) siggnal) & 0xf); - buf[3] = '\0'; - } - else - strcpy (buf, step ? "s" : "c"); - - putpkt (buf); + /* Prefer vCont, and fallback to s/c/S/C, which use Hc. */ + if (!remote_resume_with_vcont (ptid, step, siggnal)) + remote_resume_with_hc (ops, ptid, step, siggnal); - done: /* We are about to start executing the inferior, let's register it with the event loop. NOTE: this is the one place where all the execution commands end up. We could alternatively do this in each -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume 2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves @ 2016-02-17 11:45 ` Luis Machado 2016-02-17 12:32 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Luis Machado @ 2016-02-17 11:45 UTC (permalink / raw) To: Pedro Alves, gdb-patches Just nits. On 02/17/2016 12:44 AM, Pedro Alves wrote: > Just some refactoring / TLC. Mainly split the old c/s/C/S packet > handling to a separate function. > > gdb/ChangeLog: > 2016-02-09 Pedro Alves <palves@redhat.com> > > * remote.c (remote_resume_with_hc): New function, factored out > from ... > (remote_resume): ... this. Always try vCont first. > (remote_vcont_resume): Rename to ... > (remote_resume_with_vcont): ... this. Bail out if execution > direction is reverse. > --- > gdb/remote.c | 113 ++++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 62 insertions(+), 51 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index fa97e1e..60e2dda 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) > return p; > } > > +/* Set the target running, using the packets that use Hc > + (c/s/C/S). */ > + > +static void > +remote_resume_with_hc (struct target_ops *ops, > + ptid_t ptid, int step, enum gdb_signal siggnal) > +{ > + struct remote_state *rs = get_remote_state (); > + struct thread_info *thread; > + char *buf; > + > + rs->last_sent_signal = siggnal; > + rs->last_sent_step = step; > + > + /* The c/s/C/S resume packets use Hc, so set the continue > + thread. */ > + if (ptid_equal (ptid, minus_one_ptid)) > + set_continue_thread (any_thread_ptid); > + else > + set_continue_thread (ptid); > + > + ALL_NON_EXITED_THREADS (thread) > + resume_clear_thread_private_info (thread); > + > + buf = rs->buf; > + if (execution_direction == EXEC_REVERSE) > + { > + /* We don't pass signals to the target in reverse exec mode. */ > + if (info_verbose && siggnal != GDB_SIGNAL_0) > + warning (_(" - Can't pass signal %d to target in reverse: ignored."), > + siggnal); > + Even though it is existing code, this reads a bit odd. Should we update it to "... in reverse execution: ..." maybe? > + if (step && packet_support (PACKET_bs) == PACKET_DISABLE) > + error (_("Remote reverse-step not supported.")); > + if (!step && packet_support (PACKET_bc) == PACKET_DISABLE) > + error (_("Remote reverse-continue not supported.")); > + > + strcpy (buf, step ? "bs" : "bc"); > + } > + else if (siggnal != GDB_SIGNAL_0) > + { > + buf[0] = step ? 'S' : 'C'; > + buf[1] = tohex (((int) siggnal >> 4) & 0xf); > + buf[2] = tohex (((int) siggnal) & 0xf); > + buf[3] = '\0'; > + } > + else > + strcpy (buf, step ? "s" : "c"); > + > + putpkt (buf); > +} > + > /* Resume the remote inferior by using a "vCont" packet. The thread > to be resumed is PTID; STEP and SIGGNAL indicate whether the > resumed thread should be single-stepped and/or signalled. If PTID > @@ -5467,16 +5519,20 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) > be stepped and/or signalled is given in the global INFERIOR_PTID. > This function returns non-zero iff it resumes the inferior. > > - This function issues a strict subset of all possible vCont commands at the > - moment. */ > + This function issues a strict subset of all possible vCont commands > + at the moment. */ > > static int > -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal) > +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal) > { > struct remote_state *rs = get_remote_state (); > char *p; > char *endp; > > + /* No reverse support (yet) for vCont. */ > + if (execution_direction == EXEC_REVERSE) > + return 0; > + Same case as above. Also, do we need "(yet)"? > if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN) > remote_vcont_probe (rs); > > @@ -5548,8 +5604,6 @@ remote_resume (struct target_ops *ops, > ptid_t ptid, int step, enum gdb_signal siggnal) > { > struct remote_state *rs = get_remote_state (); > - char *buf; > - struct thread_info *thread; > > /* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN > (explained in remote-notif.c:handle_notification) so > @@ -5560,53 +5614,10 @@ remote_resume (struct target_ops *ops, > if (!target_is_non_stop_p ()) > remote_notif_process (rs->notif_state, ¬if_client_stop); > > - rs->last_sent_signal = siggnal; > - rs->last_sent_step = step; > - > - /* The vCont packet doesn't need to specify threads via Hc. */ > - /* No reverse support (yet) for vCont. */ > - if (execution_direction != EXEC_REVERSE) > - if (remote_vcont_resume (ptid, step, siggnal)) > - goto done; > - > - /* All other supported resume packets do use Hc, so set the continue > - thread. */ > - if (ptid_equal (ptid, minus_one_ptid)) > - set_continue_thread (any_thread_ptid); > - else > - set_continue_thread (ptid); > - > - ALL_NON_EXITED_THREADS (thread) > - resume_clear_thread_private_info (thread); > - > - buf = rs->buf; > - if (execution_direction == EXEC_REVERSE) > - { > - /* We don't pass signals to the target in reverse exec mode. */ > - if (info_verbose && siggnal != GDB_SIGNAL_0) > - warning (_(" - Can't pass signal %d to target in reverse: ignored."), > - siggnal); > - > - if (step && packet_support (PACKET_bs) == PACKET_DISABLE) > - error (_("Remote reverse-step not supported.")); > - if (!step && packet_support (PACKET_bc) == PACKET_DISABLE) > - error (_("Remote reverse-continue not supported.")); > - > - strcpy (buf, step ? "bs" : "bc"); > - } > - else if (siggnal != GDB_SIGNAL_0) > - { > - buf[0] = step ? 'S' : 'C'; > - buf[1] = tohex (((int) siggnal >> 4) & 0xf); > - buf[2] = tohex (((int) siggnal) & 0xf); > - buf[3] = '\0'; > - } > - else > - strcpy (buf, step ? "s" : "c"); > - > - putpkt (buf); > + /* Prefer vCont, and fallback to s/c/S/C, which use Hc. */ > + if (!remote_resume_with_vcont (ptid, step, siggnal)) > + remote_resume_with_hc (ops, ptid, step, siggnal); > > - done: > /* We are about to start executing the inferior, let's register it > with the event loop. NOTE: this is the one place where all the > execution commands end up. We could alternatively do this in each > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume 2016-02-17 11:45 ` Luis Machado @ 2016-02-17 12:32 ` Pedro Alves 2016-02-17 13:44 ` Luis Machado 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2016-02-17 12:32 UTC (permalink / raw) To: Luis Machado, gdb-patches On 02/17/2016 11:45 AM, Luis Machado wrote: > Just nits. > > On 02/17/2016 12:44 AM, Pedro Alves wrote: >> Just some refactoring / TLC. Mainly split the old c/s/C/S packet >> handling to a separate function. >> >> gdb/ChangeLog: >> 2016-02-09 Pedro Alves <palves@redhat.com> >> >> * remote.c (remote_resume_with_hc): New function, factored out >> from ... >> (remote_resume): ... this. Always try vCont first. >> (remote_vcont_resume): Rename to ... >> (remote_resume_with_vcont): ... this. Bail out if execution >> direction is reverse. >> --- >> gdb/remote.c | 113 ++++++++++++++++++++++++++++++++--------------------------- >> 1 file changed, 62 insertions(+), 51 deletions(-) >> >> diff --git a/gdb/remote.c b/gdb/remote.c >> index fa97e1e..60e2dda 100644 >> --- a/gdb/remote.c >> +++ b/gdb/remote.c >> @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) >> return p; >> } >> >> +/* Set the target running, using the packets that use Hc >> + (c/s/C/S). */ >> + >> +static void >> +remote_resume_with_hc (struct target_ops *ops, >> + ptid_t ptid, int step, enum gdb_signal siggnal) >> +{ >> + struct remote_state *rs = get_remote_state (); >> + struct thread_info *thread; >> + char *buf; >> + >> + rs->last_sent_signal = siggnal; >> + rs->last_sent_step = step; >> + >> + /* The c/s/C/S resume packets use Hc, so set the continue >> + thread. */ >> + if (ptid_equal (ptid, minus_one_ptid)) >> + set_continue_thread (any_thread_ptid); >> + else >> + set_continue_thread (ptid); >> + >> + ALL_NON_EXITED_THREADS (thread) >> + resume_clear_thread_private_info (thread); >> + >> + buf = rs->buf; >> + if (execution_direction == EXEC_REVERSE) >> + { >> + /* We don't pass signals to the target in reverse exec mode. */ >> + if (info_verbose && siggnal != GDB_SIGNAL_0) >> + warning (_(" - Can't pass signal %d to target in reverse: ignored."), >> + siggnal); >> + > > Even though it is existing code, this reads a bit odd. (Also, I have no idea what that unusual leading " - " is there.) > > Should we update it to "... in reverse execution: ..." maybe? Hmm, it'd still sound like a word is missing after execution, to me. I did 'grep reverse * | grep "\""' and found: reverse.c: error (_("Already in reverse mode. Use '%s' or 'set exec-dir forward'."), infcall.c: error (_("Cannot call functions in reverse mode.")); So maybe "... in reverse mode: ..." "... in reverse execution mode: ..." ? I'd rather leave it be in this patch though, since it's just a refactor with no UI change intended. >> static int >> -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal) >> +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal) >> { >> struct remote_state *rs = get_remote_state (); >> char *p; >> char *endp; >> >> + /* No reverse support (yet) for vCont. */ >> + if (execution_direction == EXEC_REVERSE) >> + return 0; >> + > > Same case as above. Also, do we need "(yet)"? How about: /* There are no vCont reverse-execution actions defined. */ if (execution_direction == EXEC_REVERSE) return 0; ? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume 2016-02-17 12:32 ` Pedro Alves @ 2016-02-17 13:44 ` Luis Machado 0 siblings, 0 replies; 15+ messages in thread From: Luis Machado @ 2016-02-17 13:44 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 02/17/2016 10:32 AM, Pedro Alves wrote: > On 02/17/2016 11:45 AM, Luis Machado wrote: >> Just nits. >> >> On 02/17/2016 12:44 AM, Pedro Alves wrote: >>> Just some refactoring / TLC. Mainly split the old c/s/C/S packet >>> handling to a separate function. >>> >>> gdb/ChangeLog: >>> 2016-02-09 Pedro Alves <palves@redhat.com> >>> >>> * remote.c (remote_resume_with_hc): New function, factored out >>> from ... >>> (remote_resume): ... this. Always try vCont first. >>> (remote_vcont_resume): Rename to ... >>> (remote_resume_with_vcont): ... this. Bail out if execution >>> direction is reverse. >>> --- >>> gdb/remote.c | 113 ++++++++++++++++++++++++++++++++--------------------------- >>> 1 file changed, 62 insertions(+), 51 deletions(-) >>> >>> diff --git a/gdb/remote.c b/gdb/remote.c >>> index fa97e1e..60e2dda 100644 >>> --- a/gdb/remote.c >>> +++ b/gdb/remote.c >>> @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) >>> return p; >>> } >>> >>> +/* Set the target running, using the packets that use Hc >>> + (c/s/C/S). */ >>> + >>> +static void >>> +remote_resume_with_hc (struct target_ops *ops, >>> + ptid_t ptid, int step, enum gdb_signal siggnal) >>> +{ >>> + struct remote_state *rs = get_remote_state (); >>> + struct thread_info *thread; >>> + char *buf; >>> + >>> + rs->last_sent_signal = siggnal; >>> + rs->last_sent_step = step; >>> + >>> + /* The c/s/C/S resume packets use Hc, so set the continue >>> + thread. */ >>> + if (ptid_equal (ptid, minus_one_ptid)) >>> + set_continue_thread (any_thread_ptid); >>> + else >>> + set_continue_thread (ptid); >>> + >>> + ALL_NON_EXITED_THREADS (thread) >>> + resume_clear_thread_private_info (thread); >>> + >>> + buf = rs->buf; >>> + if (execution_direction == EXEC_REVERSE) >>> + { >>> + /* We don't pass signals to the target in reverse exec mode. */ >>> + if (info_verbose && siggnal != GDB_SIGNAL_0) >>> + warning (_(" - Can't pass signal %d to target in reverse: ignored."), >>> + siggnal); >>> + >> >> Even though it is existing code, this reads a bit odd. > > (Also, I have no idea what that unusual leading " - " is there.) > >> >> Should we update it to "... in reverse execution: ..." maybe? > > Hmm, it'd still sound like a word is missing after execution, > to me. > > I did 'grep reverse * | grep "\""' and found: > > reverse.c: error (_("Already in reverse mode. Use '%s' or 'set exec-dir forward'."), > infcall.c: error (_("Cannot call functions in reverse mode.")); > > So maybe > > "... in reverse mode: ..." > "... in reverse execution mode: ..." > > ? > > I'd rather leave it be in this patch though, since it's > just a refactor with no UI change intended. > "... in reverse mode: ..." sounds good. I'm fine with leaving this be though. >>> static int >>> -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal) >>> +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal) >>> { >>> struct remote_state *rs = get_remote_state (); >>> char *p; >>> char *endp; >>> >>> + /* No reverse support (yet) for vCont. */ >>> + if (execution_direction == EXEC_REVERSE) >>> + return 0; >>> + >> >> Same case as above. Also, do we need "(yet)"? > > How about: > > /* There are no vCont reverse-execution actions defined. */ > if (execution_direction == EXEC_REVERSE) > return 0; > > ? That's good. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] gdb: Free inferior->priv when inferior exits 2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves 2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves @ 2016-02-17 2:44 ` Pedro Alves 2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves ` (2 subsequent siblings) 4 siblings, 0 replies; 15+ messages in thread From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw) To: gdb-patches (Where "exits" includes being killed or detached.) Nothing is clearing inferior->priv currently. This is a problem if we change the inferior's process_stratum targets in a single debug session. This field is currently only used by darwin-nat.c, but a follow up patch will make remote.c use it too. Without the fix, remote.c might end up mistaking the priv object allocated by darwin-nat.c with its own. (Found by inspection.) gdb/ChangeLog: 2016-02-16 Pedro Alves <palves@redhat.com> * inferior.c (exit_inferior_1): Free 'priv'. --- gdb/inferior.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gdb/inferior.c b/gdb/inferior.c index 45b3141..083dc63 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -255,6 +255,9 @@ exit_inferior_1 (struct inferior *inftoex, int silent) inf->pid = 0; inf->fake_pid_p = 0; + xfree (inf->priv); + inf->priv = NULL; + if (inf->vfork_parent != NULL) { inf->vfork_parent->vfork_child = NULL; -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/5] gdb/doc: Clarify vCont packet description 2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves 2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves 2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves @ 2016-02-17 2:44 ` Pedro Alves 2016-02-17 15:56 ` Eli Zaretskii 2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves 2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves 4 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2016-02-17 2:44 UTC (permalink / raw) To: gdb-patches Specifically, what happens with multiple actions that could match a thread, and what happens when we get a vCont action that matches a thread that was already running. E.g., what does: "vCont;s:2" "vCont;s:1;c" mean for thread 2. (Thread 2 continues stepping.) gdb/doc/ChangeLog: 2016-02-16 Pedro Alves <palves@redhat.com> * gdb.texinfo (Packets): Clarify vCont packets with multiple actions that match a thread, and what happens when an action matches a thread that is already running. --- gdb/doc/gdb.texinfo | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 9db234e..d936176 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -35321,13 +35321,17 @@ for success in non-stop mode (@pxref{Remote Non-Stop}) @cindex @samp{vCont} packet @anchor{vCont packet} Resume the inferior, specifying different actions for each thread. -If an action is specified with no @var{thread-id}, then it is applied to any -threads that don't have a specific action specified; if no default action is -specified then other threads should remain stopped in all-stop mode and -in their current state in non-stop mode. -Specifying multiple -default actions is an error; specifying no actions is also an error. -Thread IDs are specified using the syntax described in @ref{thread-id syntax}. + +For each inferior thread, the leftmost action with a matching +@var{thread-id} is applied. Threads that don't match any action +remain in their current state. Thread IDs are specified using the +syntax described in @ref{thread-id syntax}. If multiprocess +extensions (@pxref{multiprocess extensions}) are supported, actions +can be specified to match all threads in a process by using the +@samp{p@var{pid}.-1} form of the @var{thread-id}. An action with no +@var{thread-id} is called the default action and matches all threads. +Specifying multiple default actions is an error; specifying no actions +is also an error. Currently supported actions are: @@ -35372,11 +35376,17 @@ the corresponding stop reply should indicate that the thread has stopped with signal @samp{0}, regardless of whether the target uses some other signal as an implementation detail. +The server must ignore @samp{c}, @samp{C}, @samp{s}, @samp{S}, and +@samp{r} actions for threads that are already running. Conversely, +the server must ignore @samp{t} actions for threads that are already +stopped. + +@emph{Note:} In non-stop mode, a thread is considered running until +@value{GDBN} acknowleges an asynchronous stop notification for it with +the @samp{vStopped} packet (@pxref{Remote Non-Stop}). + The stub must support @samp{vCont} if it reports support for -multiprocess extensions (@pxref{multiprocess extensions}). Note that in -this case @samp{vCont} actions can be specified to apply to all threads -in a process by using the @samp{p@var{pid}.-1} form of the -@var{thread-id}. +multiprocess extensions (@pxref{multiprocess extensions}). Reply: @xref{Stop Reply Packets}, for the reply specifications. -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] gdb/doc: Clarify vCont packet description 2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves @ 2016-02-17 15:56 ` Eli Zaretskii 0 siblings, 0 replies; 15+ messages in thread From: Eli Zaretskii @ 2016-02-17 15:56 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > From: Pedro Alves <palves@redhat.com> > Date: Wed, 17 Feb 2016 02:44:49 +0000 > > gdb/doc/ChangeLog: > 2016-02-16 Pedro Alves <palves@redhat.com> > > * gdb.texinfo (Packets): Clarify vCont packets with multiple > actions that match a thread, and what happens when an action > matches a thread that is already running. OK, thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions 2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves ` (2 preceding siblings ...) 2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves @ 2016-02-17 2:51 ` Pedro Alves 2016-02-17 13:42 ` Luis Machado 2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves 4 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2016-02-17 2:51 UTC (permalink / raw) To: gdb-patches Currently, with "maint set target-non-stop on", that is, when gdb connects with the non-stop/asynchronous variant of the remote protocol, even with "set non-stop off", GDB always sends one vCont packet per thread resumed. This patch makes GDB aggregate and coalesce vCont packets, so we send vCont packets like "vCont;s:p1.1;c" in non-stop mode too. Basically, this is done by: - Adding a new target method target_do_resume that is called after calling target_resume one or more times. When resuming a batch of threads, we'll only call target_do_resume once after calling target_resume for all threads. - Making the remote target defer sending the actual vCont packet to target_do_resume. Special care must be taken to avoid sending a vCont action with a "wildcard" thread-id (all threads of process / all threads) when that would resume threads/processes that should not be resumed. See remote_do_resume comments for details. Unlike all-stop's remote_resume implementation, this handles the case of too many actions resulting in a too-big vCont packet, by flushing the vCont packet and starting a new one. E.g., imagining that the "c" action in: vCont;s:1;c overflows the packet buffer, we split the actions like: vCont;s:1 vCont;c Tested on x86_64 Fedora 20, with and without "maint set target-non-stop on". Also tested with a hack that makes remote_do_resume flush the vCont packet after every action appended (which caught a few bugs). gdb/ChangeLog: 2016-02-16 Pedro Alves <palves@redhat.com> * inferior.h (ALL_NON_EXITED_INFERIORS): New macro. * infrun.c (do_target_resume): Call target_do_resume. (proceed): Defer target_do_resume while looping over threads, resuming them. Call target_do_resume at the end. * record-btrace.c (record_btrace_do_resume): New function. (init_record_btrace_ops): Install it as to_do_resume method. * record-full.c (record_full_do_resume): New function. (record_full_wait_1): Call the beneath target's to_do_resume method. (init_record_full_ops): Install record_full_do_resume as to_do_resume method. * remote.c (struct private_thread_info) <last_resume_step, last_resume_sig, vcont_resumed>: New fields. (remote_add_thread): Set the new thread's vcont_resumed flag. (demand_private_info): Delete. (get_private_info_thread, get_private_info_ptid): New functions. (remote_update_thread_list): Adjust. (process_initial_stop_replies): Clear the thread's vcont_resumed flag. (remote_resume): If connected in non-stop mode, record the resume request and return early. (struct private_inferior): New. (struct vcont_builder): New. (vcont_builder_restart, vcont_builder_flush) (vcont_builder_push_action): New functions. (MAX_ACTION_SIZE): New macro. (remote_do_resume): New function. (thread_pending_fork_status, is_pending_fork_parent_thread): New functions. (check_pending_event_prevents_wildcard_vcont_callback) (check_pending_events_prevent_wildcard_vcont): New functions. (process_stop_reply): Adjust. Clear the thread's vcont_resumed flag. (init_remote_ops): Install remote_do_resume. * target-delegates.c: Regenerate. * target.c (defer_target_do_resume): New global. (target_do_resume, make_cleanup_defer_target_do_resume): New functions. * target.h (struct target_ops) <to_do_resume>: New field. (target_resume): Update comments. (target_do_resume): New declaration. --- gdb/inferior.h | 6 + gdb/infrun.c | 8 + gdb/record-btrace.c | 11 ++ gdb/record-full.c | 11 ++ gdb/remote.c | 453 ++++++++++++++++++++++++++++++++++++++++++++++--- gdb/target-delegates.c | 26 +++ gdb/target.c | 29 ++++ gdb/target.h | 46 +++-- 8 files changed, 556 insertions(+), 34 deletions(-) diff --git a/gdb/inferior.h b/gdb/inferior.h index 571d26a..42f6f6a 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -513,6 +513,12 @@ extern struct cleanup *save_current_inferior (void); #define ALL_INFERIORS(I) \ for ((I) = inferior_list; (I); (I) = (I)->next) +/* Traverse all non-exited inferiors. */ + +#define ALL_NON_EXITED_INFERIORS(I) \ + ALL_INFERIORS (I) \ + if ((I)->pid != 0) + extern struct inferior *inferior_list; /* Prune away automatically added inferiors that aren't required diff --git a/gdb/infrun.c b/gdb/infrun.c index 15210c9..f72937c 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig) target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass); target_resume (resume_ptid, step, sig); + + target_do_resume (); } /* Resume the inferior, but allow a QUIT. This is useful if the user @@ -2977,6 +2979,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) struct execution_control_state ecss; struct execution_control_state *ecs = &ecss; struct cleanup *old_chain; + struct cleanup *defer_resume_cleanup; int started; /* If we're stopped at a fork/vfork, follow the branch set by the @@ -3126,6 +3129,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) until the target stops again. */ tp->prev_pc = regcache_read_pc (regcache); + defer_resume_cleanup = make_cleanup_defer_target_do_resume (); + started = start_step_over (); if (step_over_info_valid_p ()) @@ -3190,6 +3195,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) error (_("Command aborted.")); } + do_cleanups (defer_resume_cleanup); + target_do_resume (); + discard_cleanups (old_chain); /* Tell the event loop to wait for it to stop. If the target diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 6a5ffa4..91d329b 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -2122,6 +2122,16 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step, } } +/* The to_do_resume method of target record-btrace. */ + +static void +record_btrace_do_resume (struct target_ops *ops) +{ + if ((execution_direction != EXEC_REVERSE) + && !record_btrace_is_replaying (ops, minus_one_ptid)) + ops->beneath->to_do_resume (ops->beneath); +} + /* Cancel resuming TP. */ static void @@ -2847,6 +2857,7 @@ init_record_btrace_ops (void) ops->to_get_unwinder = &record_btrace_to_get_unwinder; ops->to_get_tailcall_unwinder = &record_btrace_to_get_tailcall_unwinder; ops->to_resume = record_btrace_resume; + ops->to_do_resume = record_btrace_do_resume; ops->to_wait = record_btrace_wait; ops->to_stop = record_btrace_stop; ops->to_update_thread_list = record_btrace_update_thread_list; diff --git a/gdb/record-full.c b/gdb/record-full.c index e325869..b0955ef 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -1010,6 +1010,15 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step, target_async (1); } +/* "to_do_resume" method for process record target. */ + +static void +record_full_do_resume (struct target_ops *ops) +{ + if (!RECORD_FULL_IS_REPLAY) + ops->beneath->to_do_resume (ops->beneath); +} + static int record_full_get_sig = 0; /* SIGINT signal handler, registered by "to_wait" method. */ @@ -1180,6 +1189,7 @@ record_full_wait_1 (struct target_ops *ops, "target beneath\n"); ops->beneath->to_resume (ops->beneath, ptid, step, GDB_SIGNAL_0); + ops->beneath->to_do_resume (ops->beneath); continue; } } @@ -1956,6 +1966,7 @@ init_record_full_ops (void) record_full_ops.to_close = record_full_close; record_full_ops.to_async = record_full_async; record_full_ops.to_resume = record_full_resume; + record_full_ops.to_do_resume = record_full_do_resume; record_full_ops.to_wait = record_full_wait; record_full_ops.to_disconnect = record_disconnect; record_full_ops.to_detach = record_detach; diff --git a/gdb/remote.c b/gdb/remote.c index 60e2dda..453ce0f 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -441,6 +441,24 @@ struct private_thread_info /* This is set to the data address of the access causing the target to stop for a watchpoint. */ CORE_ADDR watch_data_address; + + /* Fields used by the vCont action coalescing implemented in + remote_resume / remote_do_resume. remote_resume stores each + thread's last resume request in these fields, so that a later + remote_do_resume knows which is the proper action for this thread + to include in the vCont packet. */ + + /* True if the last target_resume call for this thread was a step + request, false if a continue request. */ + int last_resume_step; + + /* The signal specified in the last target_resume call for this + thread. */ + enum gdb_signal last_resume_sig; + + /* Whether this thread was already vCont-resumed on the remote + side. */ + int vcont_resumed; }; static void @@ -1797,6 +1815,9 @@ remote_add_inferior (int fake_pid_p, int pid, int attached, return inf; } +static struct private_thread_info * + get_private_info_thread (struct thread_info *info); + /* Add thread PTID to GDB's thread list. Tag it as executing/running according to RUNNING. */ @@ -1804,6 +1825,7 @@ static void remote_add_thread (ptid_t ptid, int running) { struct remote_state *rs = get_remote_state (); + struct thread_info *thread; /* GDB historically didn't pull threads in the initial connection setup. If the remote target doesn't even have a concept of @@ -1812,10 +1834,11 @@ remote_add_thread (ptid_t ptid, int running) might be confusing to the user. Be silent then, preserving the age old behavior. */ if (rs->starting_up) - add_thread_silent (ptid); + thread = add_thread_silent (ptid); else - add_thread (ptid); + thread = add_thread (ptid); + get_private_info_thread (thread)->vcont_resumed = running; set_executing (ptid, running); set_running (ptid, running); } @@ -1904,25 +1927,40 @@ remote_notice_new_inferior (ptid_t currthread, int running) } } -/* Return the private thread data, creating it if necessary. */ +/* Return THREAD's private thread data, creating it if necessary. */ static struct private_thread_info * -demand_private_info (ptid_t ptid) +get_private_info_thread (struct thread_info *thread) { - struct thread_info *info = find_thread_ptid (ptid); - - gdb_assert (info); + gdb_assert (thread != NULL); - if (!info->priv) + if (thread->priv == NULL) { - info->priv = XNEW (struct private_thread_info); - info->private_dtor = free_private_thread_info; - info->priv->core = -1; - info->priv->extra = NULL; - info->priv->name = NULL; + struct private_thread_info *priv = XNEW (struct private_thread_info); + + thread->private_dtor = free_private_thread_info; + thread->priv = priv; + + priv->core = -1; + priv->extra = NULL; + priv->name = NULL; + priv->name = NULL; + priv->last_resume_step = 0; + priv->last_resume_sig = GDB_SIGNAL_0; + priv->vcont_resumed = 0; } - return info->priv; + return thread->priv; +} + +/* Return PTID's private thread data, creating it if necessary. */ + +static struct private_thread_info * +get_private_info_ptid (ptid_t ptid) +{ + struct thread_info *info = find_thread_ptid (ptid); + + return get_private_info_thread (info); } /* Call this function as a result of @@ -3266,7 +3304,7 @@ remote_update_thread_list (struct target_ops *ops) remote_notice_new_inferior (item->ptid, running); - info = demand_private_info (item->ptid); + info = get_private_info_ptid (item->ptid); info->core = item->core; info->extra = item->extra; item->extra = NULL; @@ -3900,6 +3938,7 @@ process_initial_stop_replies (int from_tty) set_executing (event_ptid, 0); set_running (event_ptid, 0); + thread->priv->vcont_resumed = 0; } /* "Notice" the new inferiors before anything related to @@ -5605,6 +5644,26 @@ remote_resume (struct target_ops *ops, { struct remote_state *rs = get_remote_state (); + /* When connected in non-stop mode, the core resumes threads + individually. Resuming remote threads directly in target_resume + would thus result in sending one packet per thread. Instead, to + minimize roundtrip latency, here we just store the resume + request; the actual remote resumption will be done in + target_do_resume / remote_do_resume, where we'll be able to do + vCont action coalescing. */ + if (target_is_non_stop_p () && execution_direction != EXEC_REVERSE) + { + struct private_thread_info *remote_thr; + + if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid)) + remote_thr = get_private_info_ptid (inferior_ptid); + else + remote_thr = get_private_info_ptid (ptid); + remote_thr->last_resume_step = step; + remote_thr->last_resume_sig = siggnal; + return; + } + /* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN (explained in remote-notif.c:handle_notification) so remote_notif_process is not called. We need find a place where @@ -5638,6 +5697,284 @@ remote_resume (struct target_ops *ops, if (!target_is_non_stop_p ()) rs->waiting_for_stop_reply = 1; } + +static void check_pending_events_prevent_wildcard_vcont + (int *may_global_wildcard_vcont); +static int is_pending_fork_parent_thread (struct thread_info *thread); + +/* Private per-inferior info for target remote processes. */ + +struct private_inferior +{ + /* Whether we can send a wildcard vCont for this process. */ + int may_wildcard_vcont; +}; + +/* Structure used to track the construction of a vCont packet in the + outgoing packet buffer. This is used to send multiple vCont + packets if we have more resumptions than would fit a single + packet. */ + +struct vcont_builder +{ + /* Pointer to the first action. P points here if no resumption has + been appended yet. */ + char *first_action; + + /* Where the next resumption will be appended. */ + char *p; + + /* The end of the buffer. Must never write past this. */ + char *endp; +}; + +/* Prepare the outgoing buffer for a new vCont packet. */ + +static void +vcont_builder_restart (struct vcont_builder *builder) +{ + struct remote_state *rs = get_remote_state (); + + builder->p = rs->buf; + builder->endp = rs->buf + get_remote_packet_size (); + builder->p += xsnprintf (builder->p, builder->endp - builder->p, "vCont"); + builder->first_action = builder->p; +} + +/* If the vCont packet being built has any action, send it to the + remote end. */ + +static void +vcont_builder_flush (struct vcont_builder *builder) +{ + struct remote_state *rs; + + if (builder->p == builder->first_action) + return; + + rs = get_remote_state (); + putpkt (rs->buf); + getpkt (&rs->buf, &rs->buf_size, 0); + if (strcmp (rs->buf, "OK") != 0) + error (_("Unexpected vCont reply in non-stop mode: %s"), rs->buf); +} + +/* The largest resumption is range-stepping, with its two addresses. + This is more than sufficient. If a new, bigger resumption is + created, it'll quickly trigger a failed assertion in + append_resumption (and we'll just bump this). */ +#define MAX_ACTION_SIZE 200 + +/* Append a new vCont action in the outgoing packet being built. If + the action doesn't fit the packet along with previous actions, push + what we've got so far to the remote end and start over a new vCont + packet (with the new action). */ + +static void +vcont_builder_push_action (struct vcont_builder *builder, + ptid_t ptid, int step, enum gdb_signal siggnal) +{ + char buf[MAX_ACTION_SIZE + 1]; + char *endp; + size_t rsize; + + endp = append_resumption (buf, buf + sizeof (buf), + ptid, step, siggnal); + + /* Check whether this new action would fit in the vCont packet along + with previous actions. If not, send what we've got so far and + start a new vCont packet. */ + rsize = endp - buf; + if (rsize > builder->endp - builder->p) + { + vcont_builder_flush (builder); + vcont_builder_restart (builder); + + /* Should now fit. */ + gdb_assert (rsize <= builder->endp - builder->p); + } + + memcpy (builder->p, buf, rsize); + builder->p += rsize; + *builder->p = '\0'; +} + +/* to_do_resume implementation. */ + +static void +remote_do_resume (struct target_ops *ops) +{ + struct remote_state *rs = get_remote_state (); + struct inferior *inf; + struct thread_info *tp; + int any_process_wildcard; + int may_global_wildcard_vcont; + struct vcont_builder vcont_builder; + + /* If connected in all-stop mode, we'd send the remote resume + request directly from remote_resume. Likewise if + reverse-debugging, as there are no defined vCont actions for + reverse execution. */ + if (!target_is_non_stop_p () || execution_direction == EXEC_REVERSE) + return; + + /* Try to send wildcard actions ("vCont;c" or "vCont;c:pPID.-1") + instead of resuming all threads of each process individually. + However, if any thread of a process must remain halted, we can't + send wildcard resumes and must send one action per thread. + + Care must be taken to not resume threads/processes the server + side already told us are stopped, but the core doesn't know about + yet, because the events are still in the vStopped notification + queue. For example: + + #1 => vCont s:p1.1;c + #2 <= OK + #3 <= %Stopped T05 p1.1 + #4 => vStopped + #5 <= T05 p1.2 + #6 => vStopped + #7 <= OK + #8 (infrun handles the stop for p1.1 and continues stepping) + #9 => vCont s:p1.1;c + + The last vCont above would resume thread p1.2 by mistake, because + the server has no idea that the event for p1.2 had not been + handled yet. + + The server side must similarly ignore resume actions for the + thread that has a pending %Stopped notification (and any other + threads with events pending), until GDB acks the notification + with vStopped. Otherwise, e.g., the following case is + mishandled: + + #1 => g (or any other packet) + #2 <= [registers] + #3 <= %Stopped T05 p1.2 + #4 => vCont s:p1.1;c + #5 <= OK + + Above, the server must not resume thread p1.2. GDB can't know + that p1.2 stopped until it acks the %Stopped notification, and + since from GDB's perspective all threads should be running, it + sends a "c" action. + + Finally, special care must also be given to handling fork/vfork + events. A (v)fork event actually tells us that two processes + stopped -- the parent and the child. Until we follow the fork, + we must not resume the child. Therefore, if we have a pending + fork follow, we must not send a global wildcard resume action + (vCont;c). We can still send process-wide wildcards though. */ + + /* Start by assuming a global wildcard (vCont;c) is possible. */ + may_global_wildcard_vcont = 1; + + /* And assume every process is individually wildcard-able too. */ + ALL_NON_EXITED_INFERIORS (inf) + { + if (inf->priv == NULL) + inf->priv = XNEW (struct private_inferior); + inf->priv->may_wildcard_vcont = 1; + } + + /* Check for any pending events (not reported or processed yet) and + disable process and global wildcard resumes appropriately. */ + check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont); + + ALL_NON_EXITED_THREADS (tp) + { + /* If a thread of a process is not meant to be resumed, then we + can't wildcard that process. */ + if (!tp->executing) + { + tp->inf->priv->may_wildcard_vcont = 0; + + /* And if we can't wildcard a process, we can't wildcard + everything either. */ + may_global_wildcard_vcont = 0; + continue; + } + + /* If a thread is the parent of an unfollowed fork, then we + can't do a global wildcard, as that would resume the fork + child. */ + if (is_pending_fork_parent_thread (tp)) + may_global_wildcard_vcont = 0; + } + + /* Now let's build the vCont packet(s). Actions must be appended + from narrower to wider scopes (thread -> process -> global). If + we end up with too many actions for a single packet vcont_builder + flushes the current vCont packet to the remote side and starts a + new one. */ + vcont_builder_restart (&vcont_builder); + + /* Threads first. */ + ALL_NON_EXITED_THREADS (tp) + { + struct private_thread_info *remote_thr = tp->priv; + + if (!tp->executing || remote_thr->vcont_resumed) + continue; + + gdb_assert (!thread_is_in_step_over_chain (tp)); + + if (!remote_thr->last_resume_step + && remote_thr->last_resume_sig == GDB_SIGNAL_0 + && tp->inf->priv->may_wildcard_vcont) + { + /* We'll send a wildcard resume instead. */ + remote_thr->vcont_resumed = 1; + continue; + } + + vcont_builder_push_action (&vcont_builder, tp->ptid, + remote_thr->last_resume_step, + remote_thr->last_resume_sig); + remote_thr->vcont_resumed = 1; + } + + /* Now check whether we can send any process-wide wildcard. This is + to avoid sending a global wildcard in the case nothing is + supposed to be resumed. */ + any_process_wildcard = 0; + + ALL_NON_EXITED_INFERIORS (inf) + { + if (inf->priv->may_wildcard_vcont) + { + any_process_wildcard = 1; + break; + } + } + + if (any_process_wildcard) + { + /* If all processes are wildcard-able, then send a single "c" + action, otherwise, send an "all (-1) threads of process" + continue action for each running process, if any. */ + if (may_global_wildcard_vcont) + { + vcont_builder_push_action (&vcont_builder, minus_one_ptid, + 0, GDB_SIGNAL_0); + } + else + { + ALL_NON_EXITED_INFERIORS (inf) + { + if (inf->priv->may_wildcard_vcont) + { + vcont_builder_push_action (&vcont_builder, + pid_to_ptid (inf->pid), + 0, GDB_SIGNAL_0); + } + } + } + } + + vcont_builder_flush (&vcont_builder); +} + \f /* Set up the signal handler for SIGINT, while the target is @@ -6120,7 +6457,7 @@ struct queue_iter_param struct stop_reply *output; }; -/* Determine if THREAD is a pending fork parent thread. ARG contains +/* Determine if THREAD_PTID is a pending fork parent thread. ARG contains the pid of the process that owns the threads we want to check, or -1 if we want to check all threads. */ @@ -6138,6 +6475,29 @@ is_pending_fork_parent (struct target_waitstatus *ws, int event_pid, return 0; } +/* Return the thread's pending status used to determine whether the + thread is a fork parent stopped at a fork event. */ + +static struct target_waitstatus * +thread_pending_fork_status (struct thread_info *thread) +{ + if (thread->suspend.waitstatus_pending_p) + return &thread->suspend.waitstatus; + else + return &thread->pending_follow; +} + +/* Determine if THREAD is a pending fork parent thread. */ + +static int +is_pending_fork_parent_thread (struct thread_info *thread) +{ + struct target_waitstatus *ws = thread_pending_fork_status (thread); + int pid = -1; + + return is_pending_fork_parent (ws, pid, thread->ptid); +} + /* Check whether EVENT is a fork event, and if it is, remove the fork child from the context list passed in DATA. */ @@ -6177,12 +6537,7 @@ remove_new_fork_children (struct threads_listing_context *context) fork child threads from the CONTEXT list. */ ALL_NON_EXITED_THREADS (thread) { - struct target_waitstatus *ws; - - if (thread->suspend.waitstatus_pending_p) - ws = &thread->suspend.waitstatus; - else - ws = &thread->pending_follow; + struct target_waitstatus *ws = thread_pending_fork_status (thread); if (is_pending_fork_parent (ws, pid, thread->ptid)) { @@ -6200,6 +6555,56 @@ remove_new_fork_children (struct threads_listing_context *context) remove_child_of_pending_fork, ¶m); } +/* Check whether EVENT would prevent a global or process wildcard + vCont action. */ + +static int +check_pending_event_prevents_wildcard_vcont_callback + (QUEUE (stop_reply_p) *q, + QUEUE_ITER (stop_reply_p) *iter, + stop_reply_p event, + void *data) +{ + struct inferior *inf; + int *may_global_wildcard_vcont = (int *) data; + + if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED + || event->ws.kind == TARGET_WAITKIND_NO_HISTORY) + return 1; + + if (event->ws.kind == TARGET_WAITKIND_FORKED + || event->ws.kind == TARGET_WAITKIND_VFORKED) + *may_global_wildcard_vcont = 0; + + inf = find_inferior_ptid (event->ptid); + + /* This may be the first time we heard about this process. + Regardless, we must not do a global wildcard resume, otherwise + we'd resume this process too. */ + *may_global_wildcard_vcont = 0; + if (inf != NULL) + inf->priv->may_wildcard_vcont = 0; + + return 1; +} + +/* Check whether any event pending in the vStopped queue would prevent + a global or process wildcard vCont action. Clear + *may_global_wildcard if we can't do a global wildcard (vCont;c), + and clear the event inferior's may_wildcard_vcont flag if we can do + a process-wide wildcard resume (vCont;c:pPID.-1). */ + +static void +check_pending_events_prevent_wildcard_vcont (int *may_global_wildcard) +{ + struct notif_client *notif = ¬if_client_stop; + + remote_notif_get_pending_events (notif); + QUEUE_iterate (stop_reply_p, stop_reply_queue, + check_pending_event_prevents_wildcard_vcont_callback, + may_global_wildcard); +} + /* Remove stop replies in the queue if its pid is equal to the given inferior's pid. */ @@ -6830,10 +7235,11 @@ process_stop_reply (struct stop_reply *stop_reply, } remote_notice_new_inferior (ptid, 0); - remote_thr = demand_private_info (ptid); + remote_thr = get_private_info_ptid (ptid); remote_thr->core = stop_reply->core; remote_thr->stop_reason = stop_reply->stop_reason; remote_thr->watch_data_address = stop_reply->watch_data_address; + remote_thr->vcont_resumed = 0; } stop_reply_xfree (stop_reply); @@ -13044,6 +13450,7 @@ Specify the serial device it is connected to\n\ remote_ops.to_detach = remote_detach; remote_ops.to_disconnect = remote_disconnect; remote_ops.to_resume = remote_resume; + remote_ops.to_do_resume = remote_do_resume; remote_ops.to_wait = remote_wait; remote_ops.to_fetch_registers = remote_fetch_registers; remote_ops.to_store_registers = remote_store_registers; diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index e21746f..67a7281 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -109,6 +109,28 @@ debug_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal ar fputs_unfiltered (")\n", gdb_stdlog); } +static void +delegate_do_resume (struct target_ops *self) +{ + self = self->beneath; + self->to_do_resume (self); +} + +static void +tdefault_do_resume (struct target_ops *self) +{ +} + +static void +debug_do_resume (struct target_ops *self) +{ + fprintf_unfiltered (gdb_stdlog, "-> %s->to_do_resume (...)\n", debug_target.to_shortname); + debug_target.to_do_resume (&debug_target); + fprintf_unfiltered (gdb_stdlog, "<- %s->to_do_resume (", debug_target.to_shortname); + target_debug_print_struct_target_ops_p (&debug_target); + fputs_unfiltered (")\n", gdb_stdlog); +} + static ptid_t delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3) { @@ -4086,6 +4108,8 @@ install_delegators (struct target_ops *ops) ops->to_disconnect = delegate_disconnect; if (ops->to_resume == NULL) ops->to_resume = delegate_resume; + if (ops->to_do_resume == NULL) + ops->to_do_resume = delegate_do_resume; if (ops->to_wait == NULL) ops->to_wait = delegate_wait; if (ops->to_fetch_registers == NULL) @@ -4389,6 +4413,7 @@ install_dummy_methods (struct target_ops *ops) ops->to_detach = tdefault_detach; ops->to_disconnect = tdefault_disconnect; ops->to_resume = tdefault_resume; + ops->to_do_resume = tdefault_do_resume; ops->to_wait = default_target_wait; ops->to_fetch_registers = tdefault_fetch_registers; ops->to_store_registers = tdefault_store_registers; @@ -4545,6 +4570,7 @@ init_debug_target (struct target_ops *ops) ops->to_detach = debug_detach; ops->to_disconnect = debug_disconnect; ops->to_resume = debug_resume; + ops->to_do_resume = debug_do_resume; ops->to_wait = debug_wait; ops->to_fetch_registers = debug_fetch_registers; ops->to_store_registers = debug_store_registers; diff --git a/gdb/target.c b/gdb/target.c index a0d5937..3de6e10 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2298,6 +2298,35 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) clear_inline_frame_state (ptid); } +/* If true, target_do_resume is a nop. */ + +static int defer_target_do_resume; + +/* See target.h. */ + +void +target_do_resume (void) +{ + struct target_ops *t; + + if (defer_target_do_resume) + return; + + current_target.to_do_resume (¤t_target); +} + +/* See target.h. */ + +struct cleanup * +make_cleanup_defer_target_do_resume (void) +{ + struct cleanup *old_chain; + + old_chain = make_cleanup_restore_integer (&defer_target_do_resume); + defer_target_do_resume = 1; + return old_chain; +} + void target_pass_signals (int numsigs, unsigned char *pass_signals) { diff --git a/gdb/target.h b/gdb/target.h index 7c8513a..c40d1c7 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -461,6 +461,8 @@ struct target_ops int TARGET_DEBUG_PRINTER (target_debug_print_step), enum gdb_signal) TARGET_DEFAULT_NORETURN (noprocess ()); + void (*to_do_resume) (struct target_ops *) + TARGET_DEFAULT_IGNORE (); ptid_t (*to_wait) (struct target_ops *, ptid_t, struct target_waitstatus *, int TARGET_DEBUG_PRINTER (target_debug_print_options)) @@ -1317,19 +1319,41 @@ extern void target_detach (const char *, int); extern void target_disconnect (const char *, int); -/* Resume execution of the target process PTID (or a group of - threads). STEP says whether to hardware single-step or to run free; - SIGGNAL is the signal to be given to the target, or GDB_SIGNAL_0 for no - signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific - PTID means `step/resume only this process id'. A wildcard PTID - (all threads, or all threads of process) means `step/resume - INFERIOR_PTID, and let other threads (for which the wildcard PTID - matches) resume with their 'thread->suspend.stop_signal' signal - (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal - if in "no pass" state. */ - +/* Resume execution (or prepare for execution) of a target thread, + process or all processes. STEP says whether to hardware + single-step or to run free; SIGGNAL is the signal to be given to + the target, or GDB_SIGNAL_0 for no signal. The caller may not pass + GDB_SIGNAL_DEFAULT. A specific PTID means `step/resume only this + process id'. A wildcard PTID (all threads, or all threads of + process) means `step/resume INFERIOR_PTID, and let other threads + (for which the wildcard PTID matches) resume with their + 'thread->suspend.stop_signal' signal (usually GDB_SIGNAL_0) if it + is in "pass" state, or with no signal if in "no pass" state. + + In order to efficiently handle batches of resumption requests, + targets may implement this method such that it records the + resumption request, but defers the actual resumption to the + target_do_resume method implementation. See target_do_resume + below. */ extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal); +/* Commit a series of resumption requests previously prepared with + target_resume calls. + + GDB always call target_do_resume after calling target_resume or + more times. A target may thus use this method in coordination with + the target_resume method to batch target-side resumption requests. + In that case, the target doesn't actually resume in its + target_resume implementation. Instead, it prepares the resumption + in target_resume, and defers the actual resumption to + target_do_resume. E.g., the remote target uses this to coalesce + multiple resumption requests in a single vCont packet. */ +extern void target_do_resume (void); + +/* Setup to defer target_do_resume calls, and return a cleanup that + reactivates target_do_resume, if it was previously active. */ +struct cleanup *make_cleanup_defer_target_do_resume (void); + /* Wait for process pid to do something. PTID = -1 to wait for any pid to do something. Return pid of child, or -1 in case of error; store status through argument pointer STATUS. Note that it is -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions 2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves @ 2016-02-17 13:42 ` Luis Machado 2016-10-26 15:35 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Luis Machado @ 2016-02-17 13:42 UTC (permalink / raw) To: Pedro Alves, gdb-patches I missed 5/5... Mostly nits, overall it looks sane to me. On 02/17/2016 12:44 AM, Pedro Alves wrote: > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 15210c9..f72937c 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig) > target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass); > > target_resume (resume_ptid, step, sig); > + > + target_do_resume (); > } > I'm looking at the sequence of function names and they don't look too clear. do_target_resume/target_resume/target_do_resume. Should we have better names for these functions? Ones that make it more explicit what each function is doing and the fact that we are potentially defering resumptions? Like "queue_resume_actions" for target_resume or "commit_resumption_actions" for target_do_resume? > @@ -6200,6 +6555,56 @@ remove_new_fork_children (struct threads_listing_context *context) > remove_child_of_pending_fork, ¶m); > } > > +/* Check whether EVENT would prevent a global or process wildcard > + vCont action. */ > + > +static int > +check_pending_event_prevents_wildcard_vcont_callback > + (QUEUE (stop_reply_p) *q, > + QUEUE_ITER (stop_reply_p) *iter, > + stop_reply_p event, > + void *data) > +{ > + struct inferior *inf; > + int *may_global_wildcard_vcont = (int *) data; > + > + if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED > + || event->ws.kind == TARGET_WAITKIND_NO_HISTORY) > + return 1; > + > + if (event->ws.kind == TARGET_WAITKIND_FORKED > + || event->ws.kind == TARGET_WAITKIND_VFORKED) > + *may_global_wildcard_vcont = 0; > + > + inf = find_inferior_ptid (event->ptid); > + > + /* This may be the first time we heard about this process. > + Regardless, we must not do a global wildcard resume, otherwise > + we'd resume this process too. */ > + *may_global_wildcard_vcont = 0; > + if (inf != NULL) > + inf->priv->may_wildcard_vcont = 0; > + > + return 1; > +} > + > +/* Check whether any event pending in the vStopped queue would prevent > + a global or process wildcard vCont action. Clear > + *may_global_wildcard if we can't do a global wildcard (vCont;c), > + and clear the event inferior's may_wildcard_vcont flag if we can do > + a process-wide wildcard resume (vCont;c:pPID.-1). */ > + ... inferior's may_wildcard_vcont flag if we can do ... Can do or can't do? > diff --git a/gdb/target.h b/gdb/target.h > index 7c8513a..c40d1c7 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -461,6 +461,8 @@ struct target_ops > int TARGET_DEBUG_PRINTER (target_debug_print_step), > enum gdb_signal) > TARGET_DEFAULT_NORETURN (noprocess ()); > + void (*to_do_resume) (struct target_ops *) > + TARGET_DEFAULT_IGNORE (); > ptid_t (*to_wait) (struct target_ops *, > ptid_t, struct target_waitstatus *, > int TARGET_DEBUG_PRINTER (target_debug_print_options)) > @@ -1317,19 +1319,41 @@ extern void target_detach (const char *, int); > > extern void target_disconnect (const char *, int); > > -/* Resume execution of the target process PTID (or a group of > - threads). STEP says whether to hardware single-step or to run free; > - SIGGNAL is the signal to be given to the target, or GDB_SIGNAL_0 for no > - signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific > - PTID means `step/resume only this process id'. A wildcard PTID > - (all threads, or all threads of process) means `step/resume > - INFERIOR_PTID, and let other threads (for which the wildcard PTID > - matches) resume with their 'thread->suspend.stop_signal' signal > - (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal > - if in "no pass" state. */ > - > +/* Resume execution (or prepare for execution) of a target thread, > + process or all processes. STEP says whether to hardware > + single-step or to run free; SIGGNAL is the signal to be given to > + the target, or GDB_SIGNAL_0 for no signal. The caller may not pass > + GDB_SIGNAL_DEFAULT. A specific PTID means `step/resume only this > + process id'. A wildcard PTID (all threads, or all threads of > + process) means `step/resume INFERIOR_PTID, and let other threads > + (for which the wildcard PTID matches) resume with their > + 'thread->suspend.stop_signal' signal (usually GDB_SIGNAL_0) if it > + is in "pass" state, or with no signal if in "no pass" state. > + > + In order to efficiently handle batches of resumption requests, > + targets may implement this method such that it records the > + resumption request, but defers the actual resumption to the > + target_do_resume method implementation. See target_do_resume > + below. */ > extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal); > > +/* Commit a series of resumption requests previously prepared with > + target_resume calls. > + > + GDB always call target_do_resume after calling target_resume or > + more times. A target may thus use this method in coordination with "GDB always calls ... target_resume one or more times."? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions 2016-02-17 13:42 ` Luis Machado @ 2016-10-26 15:35 ` Pedro Alves 0 siblings, 0 replies; 15+ messages in thread From: Pedro Alves @ 2016-10-26 15:35 UTC (permalink / raw) To: Luis Machado, gdb-patches Hi there. I've finally got back to looking at this... On 02/17/2016 01:42 PM, Luis Machado wrote: > I missed 5/5... Mostly nits, overall it looks sane to me. > > On 02/17/2016 12:44 AM, Pedro Alves wrote: >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 15210c9..f72937c 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -2357,6 +2357,8 @@ do_target_resume (ptid_t resume_ptid, int step, >> enum gdb_signal sig) >> target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass); >> >> target_resume (resume_ptid, step, sig); >> + >> + target_do_resume (); >> } >> > > I'm looking at the sequence of function names and they don't look too > clear. > > do_target_resume/target_resume/target_do_resume. > > Should we have better names for these functions? Ones that make it more > explicit what each function is doing and the fact that we are > potentially defering resumptions? Like "queue_resume_actions" for > target_resume or "commit_resumption_actions" for target_do_resume? You're right. I'm not looking forward to changing target_resume, since that's a target method implemented by all targets. But I changed target_do_resume to target_commit_resume. >> +/* Check whether any event pending in the vStopped queue would prevent >> + a global or process wildcard vCont action. Clear >> + *may_global_wildcard if we can't do a global wildcard (vCont;c), >> + and clear the event inferior's may_wildcard_vcont flag if we can do >> + a process-wide wildcard resume (vCont;c:pPID.-1). */ >> + > > ... inferior's may_wildcard_vcont flag if we can do ... > > Can do or can't do? Can't. Fixed. >> +/* Commit a series of resumption requests previously prepared with >> + target_resume calls. >> + >> + GDB always call target_do_resume after calling target_resume or >> + more times. A target may thus use this method in coordination with > > > "GDB always calls ... target_resume one or more times."? Fixed too. Below's what I pushed to master. This added a new cleanup, which would better done with a scoped_restore nowadays. I'll fix that as a follow up. From 85ad3aaf403d2104c82010494d3d4a93a36e2e6f Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 26 Oct 2016 11:08:28 +0100 Subject: [PATCH] gdb: Coalesce/aggregate (async) vCont packets/actions Currently, with "maint set target-non-stop on", that is, when gdb connects with the non-stop/asynchronous variant of the remote protocol, even with "set non-stop off", GDB always sends one vCont packet per thread resumed. This patch makes GDB aggregate and coalesce vCont packets, so we send vCont packets like "vCont;s:p1.1;c" in non-stop mode too. Basically, this is done by: - Adding a new target method target_commit_resume that is called after calling target_resume one or more times. When resuming a batch of threads, we'll only call target_commit_resume once after calling target_resume for all threads. - Making the remote target defer sending the actual vCont packet to target_commit_resume. Special care must be taken to avoid sending a vCont action with a "wildcard" thread-id (all threads of process / all threads) when that would resume threads/processes that should not be resumed. See remote_commit_resume comments for details. Unlike all-stop's remote_resume implementation, this handles the case of too many actions resulting in a too-big vCont packet, by flushing the vCont packet and starting a new one. E.g., imagining that the "c" action in: vCont;s:1;c overflows the packet buffer, we split the actions like: vCont;s:1 vCont;c Tested on x86_64 Fedora 20, with and without "maint set target-non-stop on". Also tested with a hack that makes remote_commit_resume flush the vCont packet after every action appended (which caught a few bugs). gdb/ChangeLog: 2016-10-26 Pedro Alves <palves@redhat.com> * inferior.h (ALL_NON_EXITED_INFERIORS): New macro. * infrun.c (do_target_resume): Call target_commit_resume. (proceed): Defer target_commit_resume while looping over threads, resuming them. Call target_commit_resume at the end. * record-btrace.c (record_btrace_commit_resume): New function. (init_record_btrace_ops): Install it as to_commit_resume method. * record-full.c (record_full_commit_resume): New function. (record_full_wait_1): Call the beneath target's to_commit_resume method. (init_record_full_ops): Install record_full_commit_resume as to_commit_resume method. * remote.c (struct private_thread_info) <last_resume_step, last_resume_sig, vcont_resumed>: New fields. (remote_add_thread): Set the new thread's vcont_resumed flag. (demand_private_info): Delete. (get_private_info_thread, get_private_info_ptid): New functions. (remote_update_thread_list): Adjust. (process_initial_stop_replies): Clear the thread's vcont_resumed flag. (remote_resume): If connected in non-stop mode, record the resume request and return early. (struct private_inferior): New. (struct vcont_builder): New. (vcont_builder_restart, vcont_builder_flush) (vcont_builder_push_action): New functions. (MAX_ACTION_SIZE): New macro. (remote_commit_resume): New function. (thread_pending_fork_status, is_pending_fork_parent_thread): New functions. (check_pending_event_prevents_wildcard_vcont_callback) (check_pending_events_prevent_wildcard_vcont): New functions. (process_stop_reply): Adjust. Clear the thread's vcont_resumed flag. (init_remote_ops): Install remote_commit_resume. * target-delegates.c: Regenerate. * target.c (defer_target_commit_resume): New global. (target_commit_resume, make_cleanup_defer_target_commit_resume): New functions. * target.h (struct target_ops) <to_commit_resume>: New field. (target_resume): Update comments. (target_commit_resume): New declaration. --- gdb/ChangeLog | 44 +++++ gdb/inferior.h | 6 + gdb/infrun.c | 8 + gdb/record-btrace.c | 11 ++ gdb/record-full.c | 11 ++ gdb/remote.c | 452 ++++++++++++++++++++++++++++++++++++++++++++++--- gdb/target-delegates.c | 26 +++ gdb/target.c | 28 +++ gdb/target.h | 47 +++-- 9 files changed, 599 insertions(+), 34 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index f8296b6..6e24938 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,49 @@ 2016-10-26 Pedro Alves <palves@redhat.com> + * inferior.h (ALL_NON_EXITED_INFERIORS): New macro. + * infrun.c (do_target_resume): Call target_commit_resume. + (proceed): Defer target_commit_resume while looping over threads, + resuming them. Call target_commit_resume at the end. + * record-btrace.c (record_btrace_commit_resume): New function. + (init_record_btrace_ops): Install it as to_commit_resume method. + * record-full.c (record_full_commit_resume): New function. + (record_full_wait_1): Call the beneath target's to_commit_resume + method. + (init_record_full_ops): Install record_full_commit_resume as + to_commit_resume method. + * remote.c (struct private_thread_info) <last_resume_step, + last_resume_sig, vcont_resumed>: New fields. + (remote_add_thread): Set the new thread's vcont_resumed flag. + (demand_private_info): Delete. + (get_private_info_thread, get_private_info_ptid): New functions. + (remote_update_thread_list): Adjust. + (process_initial_stop_replies): Clear the thread's vcont_resumed + flag. + (remote_resume): If connected in non-stop mode, record the resume + request and return early. + (struct private_inferior): New. + (struct vcont_builder): New. + (vcont_builder_restart, vcont_builder_flush) + (vcont_builder_push_action): New functions. + (MAX_ACTION_SIZE): New macro. + (remote_commit_resume): New function. + (thread_pending_fork_status, is_pending_fork_parent_thread): New + functions. + (check_pending_event_prevents_wildcard_vcont_callback) + (check_pending_events_prevent_wildcard_vcont): New functions. + (process_stop_reply): Adjust. Clear the thread's vcont_resumed + flag. + (init_remote_ops): Install remote_commit_resume. + * target-delegates.c: Regenerate. + * target.c (defer_target_commit_resume): New global. + (target_commit_resume, make_cleanup_defer_target_commit_resume): + New functions. + * target.h (struct target_ops) <to_commit_resume>: New field. + (target_resume): Update comments. + (target_commit_resume): New declaration. + +2016-10-26 Pedro Alves <palves@redhat.com> + * inferior.c (exit_inferior_1): Free 'priv'. 2016-10-26 Pedro Alves <palves@redhat.com> diff --git a/gdb/inferior.h b/gdb/inferior.h index 54c6f65..300cc10 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -513,6 +513,12 @@ extern struct cleanup *save_current_inferior (void); #define ALL_INFERIORS(I) \ for ((I) = inferior_list; (I); (I) = (I)->next) +/* Traverse all non-exited inferiors. */ + +#define ALL_NON_EXITED_INFERIORS(I) \ + ALL_INFERIORS (I) \ + if ((I)->pid != 0) + extern struct inferior *inferior_list; /* Prune away automatically added inferiors that aren't required diff --git a/gdb/infrun.c b/gdb/infrun.c index 00bba16..5e62472 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2366,6 +2366,8 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig) target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass); target_resume (resume_ptid, step, sig); + + target_commit_resume (); } /* Resume the inferior, but allow a QUIT. This is useful if the user @@ -2984,6 +2986,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) struct execution_control_state ecss; struct execution_control_state *ecs = &ecss; struct cleanup *old_chain; + struct cleanup *defer_resume_cleanup; int started; /* If we're stopped at a fork/vfork, follow the branch set by the @@ -3125,6 +3128,8 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) until the target stops again. */ tp->prev_pc = regcache_read_pc (regcache); + defer_resume_cleanup = make_cleanup_defer_target_commit_resume (); + started = start_step_over (); if (step_over_info_valid_p ()) @@ -3189,6 +3194,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) error (_("Command aborted.")); } + do_cleanups (defer_resume_cleanup); + target_commit_resume (); + discard_cleanups (old_chain); /* Tell the event loop to wait for it to stop. If the target diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 257d0b0..4808129 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -2150,6 +2150,16 @@ record_btrace_resume (struct target_ops *ops, ptid_t ptid, int step, } } +/* The to_commit_resume method of target record-btrace. */ + +static void +record_btrace_commit_resume (struct target_ops *ops) +{ + if ((execution_direction != EXEC_REVERSE) + && !record_btrace_is_replaying (ops, minus_one_ptid)) + ops->beneath->to_commit_resume (ops->beneath); +} + /* Cancel resuming TP. */ static void @@ -2875,6 +2885,7 @@ init_record_btrace_ops (void) ops->to_get_unwinder = &record_btrace_to_get_unwinder; ops->to_get_tailcall_unwinder = &record_btrace_to_get_tailcall_unwinder; ops->to_resume = record_btrace_resume; + ops->to_commit_resume = record_btrace_commit_resume; ops->to_wait = record_btrace_wait; ops->to_stop = record_btrace_stop; ops->to_update_thread_list = record_btrace_update_thread_list; diff --git a/gdb/record-full.c b/gdb/record-full.c index e4dd55b..50f235d 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -1002,6 +1002,15 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step, target_async (1); } +/* "to_commit_resume" method for process record target. */ + +static void +record_full_commit_resume (struct target_ops *ops) +{ + if (!RECORD_FULL_IS_REPLAY) + ops->beneath->to_commit_resume (ops->beneath); +} + static int record_full_get_sig = 0; /* SIGINT signal handler, registered by "to_wait" method. */ @@ -1172,6 +1181,7 @@ record_full_wait_1 (struct target_ops *ops, "target beneath\n"); ops->beneath->to_resume (ops->beneath, ptid, step, GDB_SIGNAL_0); + ops->beneath->to_commit_resume (ops->beneath); continue; } } @@ -1975,6 +1985,7 @@ init_record_full_ops (void) record_full_ops.to_close = record_full_close; record_full_ops.to_async = record_full_async; record_full_ops.to_resume = record_full_resume; + record_full_ops.to_commit_resume = record_full_commit_resume; record_full_ops.to_wait = record_full_wait; record_full_ops.to_disconnect = record_disconnect; record_full_ops.to_detach = record_detach; diff --git a/gdb/remote.c b/gdb/remote.c index 96f0ad5..517e36d 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -452,6 +452,24 @@ struct private_thread_info /* This is set to the data address of the access causing the target to stop for a watchpoint. */ CORE_ADDR watch_data_address; + + /* Fields used by the vCont action coalescing implemented in + remote_resume / remote_commit_resume. remote_resume stores each + thread's last resume request in these fields, so that a later + remote_commit_resume knows which is the proper action for this + thread to include in the vCont packet. */ + + /* True if the last target_resume call for this thread was a step + request, false if a continue request. */ + int last_resume_step; + + /* The signal specified in the last target_resume call for this + thread. */ + enum gdb_signal last_resume_sig; + + /* Whether this thread was already vCont-resumed on the remote + side. */ + int vcont_resumed; }; static void @@ -1805,6 +1823,9 @@ remote_add_inferior (int fake_pid_p, int pid, int attached, return inf; } +static struct private_thread_info * + get_private_info_thread (struct thread_info *info); + /* Add thread PTID to GDB's thread list. Tag it as executing/running according to RUNNING. */ @@ -1812,6 +1833,7 @@ static void remote_add_thread (ptid_t ptid, int running, int executing) { struct remote_state *rs = get_remote_state (); + struct thread_info *thread; /* GDB historically didn't pull threads in the initial connection setup. If the remote target doesn't even have a concept of @@ -1820,10 +1842,11 @@ remote_add_thread (ptid_t ptid, int running, int executing) might be confusing to the user. Be silent then, preserving the age old behavior. */ if (rs->starting_up) - add_thread_silent (ptid); + thread = add_thread_silent (ptid); else - add_thread (ptid); + thread = add_thread (ptid); + get_private_info_thread (thread)->vcont_resumed = executing; set_executing (ptid, executing); set_running (ptid, running); } @@ -1918,25 +1941,40 @@ remote_notice_new_inferior (ptid_t currthread, int executing) } } -/* Return the private thread data, creating it if necessary. */ +/* Return THREAD's private thread data, creating it if necessary. */ static struct private_thread_info * -demand_private_info (ptid_t ptid) +get_private_info_thread (struct thread_info *thread) { - struct thread_info *info = find_thread_ptid (ptid); + gdb_assert (thread != NULL); - gdb_assert (info); - - if (!info->priv) + if (thread->priv == NULL) { - info->priv = XNEW (struct private_thread_info); - info->private_dtor = free_private_thread_info; - info->priv->core = -1; - info->priv->extra = NULL; - info->priv->name = NULL; + struct private_thread_info *priv = XNEW (struct private_thread_info); + + thread->private_dtor = free_private_thread_info; + thread->priv = priv; + + priv->core = -1; + priv->extra = NULL; + priv->name = NULL; + priv->name = NULL; + priv->last_resume_step = 0; + priv->last_resume_sig = GDB_SIGNAL_0; + priv->vcont_resumed = 0; } - return info->priv; + return thread->priv; +} + +/* Return PTID's private thread data, creating it if necessary. */ + +static struct private_thread_info * +get_private_info_ptid (ptid_t ptid) +{ + struct thread_info *info = find_thread_ptid (ptid); + + return get_private_info_thread (info); } /* Call this function as a result of @@ -3280,7 +3318,7 @@ remote_update_thread_list (struct target_ops *ops) remote_notice_new_inferior (item->ptid, executing); - info = demand_private_info (item->ptid); + info = get_private_info_ptid (item->ptid); info->core = item->core; info->extra = item->extra; item->extra = NULL; @@ -3913,6 +3951,7 @@ process_initial_stop_replies (int from_tty) set_executing (event_ptid, 0); set_running (event_ptid, 0); + thread->priv->vcont_resumed = 0; } /* "Notice" the new inferiors before anything related to @@ -5701,6 +5740,26 @@ remote_resume (struct target_ops *ops, { struct remote_state *rs = get_remote_state (); + /* When connected in non-stop mode, the core resumes threads + individually. Resuming remote threads directly in target_resume + would thus result in sending one packet per thread. Instead, to + minimize roundtrip latency, here we just store the resume + request; the actual remote resumption will be done in + target_commit_resume / remote_commit_resume, where we'll be able + to do vCont action coalescing. */ + if (target_is_non_stop_p () && execution_direction != EXEC_REVERSE) + { + struct private_thread_info *remote_thr; + + if (ptid_equal (minus_one_ptid, ptid) || ptid_is_pid (ptid)) + remote_thr = get_private_info_ptid (inferior_ptid); + else + remote_thr = get_private_info_ptid (ptid); + remote_thr->last_resume_step = step; + remote_thr->last_resume_sig = siggnal; + return; + } + /* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN (explained in remote-notif.c:handle_notification) so remote_notif_process is not called. We need find a place where @@ -5736,6 +5795,283 @@ remote_resume (struct target_ops *ops, if (!target_is_non_stop_p ()) rs->waiting_for_stop_reply = 1; } + +static void check_pending_events_prevent_wildcard_vcont + (int *may_global_wildcard_vcont); +static int is_pending_fork_parent_thread (struct thread_info *thread); + +/* Private per-inferior info for target remote processes. */ + +struct private_inferior +{ + /* Whether we can send a wildcard vCont for this process. */ + int may_wildcard_vcont; +}; + +/* Structure used to track the construction of a vCont packet in the + outgoing packet buffer. This is used to send multiple vCont + packets if we have more actions than would fit a single packet. */ + +struct vcont_builder +{ + /* Pointer to the first action. P points here if no action has been + appended yet. */ + char *first_action; + + /* Where the next action will be appended. */ + char *p; + + /* The end of the buffer. Must never write past this. */ + char *endp; +}; + +/* Prepare the outgoing buffer for a new vCont packet. */ + +static void +vcont_builder_restart (struct vcont_builder *builder) +{ + struct remote_state *rs = get_remote_state (); + + builder->p = rs->buf; + builder->endp = rs->buf + get_remote_packet_size (); + builder->p += xsnprintf (builder->p, builder->endp - builder->p, "vCont"); + builder->first_action = builder->p; +} + +/* If the vCont packet being built has any action, send it to the + remote end. */ + +static void +vcont_builder_flush (struct vcont_builder *builder) +{ + struct remote_state *rs; + + if (builder->p == builder->first_action) + return; + + rs = get_remote_state (); + putpkt (rs->buf); + getpkt (&rs->buf, &rs->buf_size, 0); + if (strcmp (rs->buf, "OK") != 0) + error (_("Unexpected vCont reply in non-stop mode: %s"), rs->buf); +} + +/* The largest action is range-stepping, with its two addresses. This + is more than sufficient. If a new, bigger action is created, it'll + quickly trigger a failed assertion in append_resumption (and we'll + just bump this). */ +#define MAX_ACTION_SIZE 200 + +/* Append a new vCont action in the outgoing packet being built. If + the action doesn't fit the packet along with previous actions, push + what we've got so far to the remote end and start over a new vCont + packet (with the new action). */ + +static void +vcont_builder_push_action (struct vcont_builder *builder, + ptid_t ptid, int step, enum gdb_signal siggnal) +{ + char buf[MAX_ACTION_SIZE + 1]; + char *endp; + size_t rsize; + + endp = append_resumption (buf, buf + sizeof (buf), + ptid, step, siggnal); + + /* Check whether this new action would fit in the vCont packet along + with previous actions. If not, send what we've got so far and + start a new vCont packet. */ + rsize = endp - buf; + if (rsize > builder->endp - builder->p) + { + vcont_builder_flush (builder); + vcont_builder_restart (builder); + + /* Should now fit. */ + gdb_assert (rsize <= builder->endp - builder->p); + } + + memcpy (builder->p, buf, rsize); + builder->p += rsize; + *builder->p = '\0'; +} + +/* to_commit_resume implementation. */ + +static void +remote_commit_resume (struct target_ops *ops) +{ + struct remote_state *rs = get_remote_state (); + struct inferior *inf; + struct thread_info *tp; + int any_process_wildcard; + int may_global_wildcard_vcont; + struct vcont_builder vcont_builder; + + /* If connected in all-stop mode, we'd send the remote resume + request directly from remote_resume. Likewise if + reverse-debugging, as there are no defined vCont actions for + reverse execution. */ + if (!target_is_non_stop_p () || execution_direction == EXEC_REVERSE) + return; + + /* Try to send wildcard actions ("vCont;c" or "vCont;c:pPID.-1") + instead of resuming all threads of each process individually. + However, if any thread of a process must remain halted, we can't + send wildcard resumes and must send one action per thread. + + Care must be taken to not resume threads/processes the server + side already told us are stopped, but the core doesn't know about + yet, because the events are still in the vStopped notification + queue. For example: + + #1 => vCont s:p1.1;c + #2 <= OK + #3 <= %Stopped T05 p1.1 + #4 => vStopped + #5 <= T05 p1.2 + #6 => vStopped + #7 <= OK + #8 (infrun handles the stop for p1.1 and continues stepping) + #9 => vCont s:p1.1;c + + The last vCont above would resume thread p1.2 by mistake, because + the server has no idea that the event for p1.2 had not been + handled yet. + + The server side must similarly ignore resume actions for the + thread that has a pending %Stopped notification (and any other + threads with events pending), until GDB acks the notification + with vStopped. Otherwise, e.g., the following case is + mishandled: + + #1 => g (or any other packet) + #2 <= [registers] + #3 <= %Stopped T05 p1.2 + #4 => vCont s:p1.1;c + #5 <= OK + + Above, the server must not resume thread p1.2. GDB can't know + that p1.2 stopped until it acks the %Stopped notification, and + since from GDB's perspective all threads should be running, it + sends a "c" action. + + Finally, special care must also be given to handling fork/vfork + events. A (v)fork event actually tells us that two processes + stopped -- the parent and the child. Until we follow the fork, + we must not resume the child. Therefore, if we have a pending + fork follow, we must not send a global wildcard resume action + (vCont;c). We can still send process-wide wildcards though. */ + + /* Start by assuming a global wildcard (vCont;c) is possible. */ + may_global_wildcard_vcont = 1; + + /* And assume every process is individually wildcard-able too. */ + ALL_NON_EXITED_INFERIORS (inf) + { + if (inf->priv == NULL) + inf->priv = XNEW (struct private_inferior); + inf->priv->may_wildcard_vcont = 1; + } + + /* Check for any pending events (not reported or processed yet) and + disable process and global wildcard resumes appropriately. */ + check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont); + + ALL_NON_EXITED_THREADS (tp) + { + /* If a thread of a process is not meant to be resumed, then we + can't wildcard that process. */ + if (!tp->executing) + { + tp->inf->priv->may_wildcard_vcont = 0; + + /* And if we can't wildcard a process, we can't wildcard + everything either. */ + may_global_wildcard_vcont = 0; + continue; + } + + /* If a thread is the parent of an unfollowed fork, then we + can't do a global wildcard, as that would resume the fork + child. */ + if (is_pending_fork_parent_thread (tp)) + may_global_wildcard_vcont = 0; + } + + /* Now let's build the vCont packet(s). Actions must be appended + from narrower to wider scopes (thread -> process -> global). If + we end up with too many actions for a single packet vcont_builder + flushes the current vCont packet to the remote side and starts a + new one. */ + vcont_builder_restart (&vcont_builder); + + /* Threads first. */ + ALL_NON_EXITED_THREADS (tp) + { + struct private_thread_info *remote_thr = tp->priv; + + if (!tp->executing || remote_thr->vcont_resumed) + continue; + + gdb_assert (!thread_is_in_step_over_chain (tp)); + + if (!remote_thr->last_resume_step + && remote_thr->last_resume_sig == GDB_SIGNAL_0 + && tp->inf->priv->may_wildcard_vcont) + { + /* We'll send a wildcard resume instead. */ + remote_thr->vcont_resumed = 1; + continue; + } + + vcont_builder_push_action (&vcont_builder, tp->ptid, + remote_thr->last_resume_step, + remote_thr->last_resume_sig); + remote_thr->vcont_resumed = 1; + } + + /* Now check whether we can send any process-wide wildcard. This is + to avoid sending a global wildcard in the case nothing is + supposed to be resumed. */ + any_process_wildcard = 0; + + ALL_NON_EXITED_INFERIORS (inf) + { + if (inf->priv->may_wildcard_vcont) + { + any_process_wildcard = 1; + break; + } + } + + if (any_process_wildcard) + { + /* If all processes are wildcard-able, then send a single "c" + action, otherwise, send an "all (-1) threads of process" + continue action for each running process, if any. */ + if (may_global_wildcard_vcont) + { + vcont_builder_push_action (&vcont_builder, minus_one_ptid, + 0, GDB_SIGNAL_0); + } + else + { + ALL_NON_EXITED_INFERIORS (inf) + { + if (inf->priv->may_wildcard_vcont) + { + vcont_builder_push_action (&vcont_builder, + pid_to_ptid (inf->pid), + 0, GDB_SIGNAL_0); + } + } + } + } + + vcont_builder_flush (&vcont_builder); +} + \f /* Non-stop version of target_stop. Uses `vCont;t' to stop a remote @@ -6102,7 +6438,7 @@ struct queue_iter_param struct stop_reply *output; }; -/* Determine if THREAD is a pending fork parent thread. ARG contains +/* Determine if THREAD_PTID is a pending fork parent thread. ARG contains the pid of the process that owns the threads we want to check, or -1 if we want to check all threads. */ @@ -6120,6 +6456,29 @@ is_pending_fork_parent (struct target_waitstatus *ws, int event_pid, return 0; } +/* Return the thread's pending status used to determine whether the + thread is a fork parent stopped at a fork event. */ + +static struct target_waitstatus * +thread_pending_fork_status (struct thread_info *thread) +{ + if (thread->suspend.waitstatus_pending_p) + return &thread->suspend.waitstatus; + else + return &thread->pending_follow; +} + +/* Determine if THREAD is a pending fork parent thread. */ + +static int +is_pending_fork_parent_thread (struct thread_info *thread) +{ + struct target_waitstatus *ws = thread_pending_fork_status (thread); + int pid = -1; + + return is_pending_fork_parent (ws, pid, thread->ptid); +} + /* Check whether EVENT is a fork event, and if it is, remove the fork child from the context list passed in DATA. */ @@ -6159,12 +6518,7 @@ remove_new_fork_children (struct threads_listing_context *context) fork child threads from the CONTEXT list. */ ALL_NON_EXITED_THREADS (thread) { - struct target_waitstatus *ws; - - if (thread->suspend.waitstatus_pending_p) - ws = &thread->suspend.waitstatus; - else - ws = &thread->pending_follow; + struct target_waitstatus *ws = thread_pending_fork_status (thread); if (is_pending_fork_parent (ws, pid, thread->ptid)) { @@ -6182,6 +6536,56 @@ remove_new_fork_children (struct threads_listing_context *context) remove_child_of_pending_fork, ¶m); } +/* Check whether EVENT would prevent a global or process wildcard + vCont action. */ + +static int +check_pending_event_prevents_wildcard_vcont_callback + (QUEUE (stop_reply_p) *q, + QUEUE_ITER (stop_reply_p) *iter, + stop_reply_p event, + void *data) +{ + struct inferior *inf; + int *may_global_wildcard_vcont = (int *) data; + + if (event->ws.kind == TARGET_WAITKIND_NO_RESUMED + || event->ws.kind == TARGET_WAITKIND_NO_HISTORY) + return 1; + + if (event->ws.kind == TARGET_WAITKIND_FORKED + || event->ws.kind == TARGET_WAITKIND_VFORKED) + *may_global_wildcard_vcont = 0; + + inf = find_inferior_ptid (event->ptid); + + /* This may be the first time we heard about this process. + Regardless, we must not do a global wildcard resume, otherwise + we'd resume this process too. */ + *may_global_wildcard_vcont = 0; + if (inf != NULL) + inf->priv->may_wildcard_vcont = 0; + + return 1; +} + +/* Check whether any event pending in the vStopped queue would prevent + a global or process wildcard vCont action. Clear + *may_global_wildcard if we can't do a global wildcard (vCont;c), + and clear the event inferior's may_wildcard_vcont flag if we can't + do a process-wide wildcard resume (vCont;c:pPID.-1). */ + +static void +check_pending_events_prevent_wildcard_vcont (int *may_global_wildcard) +{ + struct notif_client *notif = ¬if_client_stop; + + remote_notif_get_pending_events (notif); + QUEUE_iterate (stop_reply_p, stop_reply_queue, + check_pending_event_prevents_wildcard_vcont_callback, + may_global_wildcard); +} + /* Remove stop replies in the queue if its pid is equal to the given inferior's pid. */ @@ -6812,10 +7216,11 @@ process_stop_reply (struct stop_reply *stop_reply, } remote_notice_new_inferior (ptid, 0); - remote_thr = demand_private_info (ptid); + remote_thr = get_private_info_ptid (ptid); remote_thr->core = stop_reply->core; remote_thr->stop_reason = stop_reply->stop_reason; remote_thr->watch_data_address = stop_reply->watch_data_address; + remote_thr->vcont_resumed = 0; } stop_reply_xfree (stop_reply); @@ -13114,6 +13519,7 @@ Specify the serial device it is connected to\n\ remote_ops.to_detach = remote_detach; remote_ops.to_disconnect = remote_disconnect; remote_ops.to_resume = remote_resume; + remote_ops.to_commit_resume = remote_commit_resume; remote_ops.to_wait = remote_wait; remote_ops.to_fetch_registers = remote_fetch_registers; remote_ops.to_store_registers = remote_store_registers; diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 57e7939..73e45dd 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -109,6 +109,28 @@ debug_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal ar fputs_unfiltered (")\n", gdb_stdlog); } +static void +delegate_commit_resume (struct target_ops *self) +{ + self = self->beneath; + self->to_commit_resume (self); +} + +static void +tdefault_commit_resume (struct target_ops *self) +{ +} + +static void +debug_commit_resume (struct target_ops *self) +{ + fprintf_unfiltered (gdb_stdlog, "-> %s->to_commit_resume (...)\n", debug_target.to_shortname); + debug_target.to_commit_resume (&debug_target); + fprintf_unfiltered (gdb_stdlog, "<- %s->to_commit_resume (", debug_target.to_shortname); + target_debug_print_struct_target_ops_p (&debug_target); + fputs_unfiltered (")\n", gdb_stdlog); +} + static ptid_t delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3) { @@ -4108,6 +4130,8 @@ install_delegators (struct target_ops *ops) ops->to_disconnect = delegate_disconnect; if (ops->to_resume == NULL) ops->to_resume = delegate_resume; + if (ops->to_commit_resume == NULL) + ops->to_commit_resume = delegate_commit_resume; if (ops->to_wait == NULL) ops->to_wait = delegate_wait; if (ops->to_fetch_registers == NULL) @@ -4413,6 +4437,7 @@ install_dummy_methods (struct target_ops *ops) ops->to_detach = tdefault_detach; ops->to_disconnect = tdefault_disconnect; ops->to_resume = tdefault_resume; + ops->to_commit_resume = tdefault_commit_resume; ops->to_wait = default_target_wait; ops->to_fetch_registers = tdefault_fetch_registers; ops->to_store_registers = tdefault_store_registers; @@ -4570,6 +4595,7 @@ init_debug_target (struct target_ops *ops) ops->to_detach = debug_detach; ops->to_disconnect = debug_disconnect; ops->to_resume = debug_resume; + ops->to_commit_resume = debug_commit_resume; ops->to_wait = debug_wait; ops->to_fetch_registers = debug_fetch_registers; ops->to_store_registers = debug_store_registers; diff --git a/gdb/target.c b/gdb/target.c index cb89e75..246d292 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2329,6 +2329,34 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) clear_inline_frame_state (ptid); } +/* If true, target_commit_resume is a nop. */ +static int defer_target_commit_resume; + +/* See target.h. */ + +void +target_commit_resume (void) +{ + struct target_ops *t; + + if (defer_target_commit_resume) + return; + + current_target.to_commit_resume (¤t_target); +} + +/* See target.h. */ + +struct cleanup * +make_cleanup_defer_target_commit_resume (void) +{ + struct cleanup *old_chain; + + old_chain = make_cleanup_restore_integer (&defer_target_commit_resume); + defer_target_commit_resume = 1; + return old_chain; +} + void target_pass_signals (int numsigs, unsigned char *pass_signals) { diff --git a/gdb/target.h b/gdb/target.h index 176f332..a54b3db 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -461,6 +461,8 @@ struct target_ops int TARGET_DEBUG_PRINTER (target_debug_print_step), enum gdb_signal) TARGET_DEFAULT_NORETURN (noprocess ()); + void (*to_commit_resume) (struct target_ops *) + TARGET_DEFAULT_IGNORE (); ptid_t (*to_wait) (struct target_ops *, ptid_t, struct target_waitstatus *, int TARGET_DEBUG_PRINTER (target_debug_print_options)) @@ -1328,19 +1330,42 @@ extern void target_detach (const char *, int); extern void target_disconnect (const char *, int); -/* Resume execution of the target process PTID (or a group of - threads). STEP says whether to hardware single-step or to run free; - SIGGNAL is the signal to be given to the target, or GDB_SIGNAL_0 for no - signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific - PTID means `step/resume only this process id'. A wildcard PTID - (all threads, or all threads of process) means `step/resume - INFERIOR_PTID, and let other threads (for which the wildcard PTID - matches) resume with their 'thread->suspend.stop_signal' signal - (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal - if in "no pass" state. */ - +/* Resume execution (or prepare for execution) of a target thread, + process or all processes. STEP says whether to hardware + single-step or to run free; SIGGNAL is the signal to be given to + the target, or GDB_SIGNAL_0 for no signal. The caller may not pass + GDB_SIGNAL_DEFAULT. A specific PTID means `step/resume only this + process id'. A wildcard PTID (all threads, or all threads of + process) means `step/resume INFERIOR_PTID, and let other threads + (for which the wildcard PTID matches) resume with their + 'thread->suspend.stop_signal' signal (usually GDB_SIGNAL_0) if it + is in "pass" state, or with no signal if in "no pass" state. + + In order to efficiently handle batches of resumption requests, + targets may implement this method such that it records the + resumption request, but defers the actual resumption to the + target_commit_resume method implementation. See + target_commit_resume below. */ extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal); +/* Commit a series of resumption requests previously prepared with + target_resume calls. + + GDB always calls target_commit_resume after calling target_resume + one or more times. A target may thus use this method in + coordination with the target_resume method to batch target-side + resumption requests. In that case, the target doesn't actually + resume in its target_resume implementation. Instead, it prepares + the resumption in target_resume, and defers the actual resumption + to target_commit_resume. E.g., the remote target uses this to + coalesce multiple resumption requests in a single vCont packet. */ +extern void target_commit_resume (); + +/* Setup to defer target_commit_resume calls, and return a cleanup + that reactivates target_commit_resume, if it was previously + active. */ +struct cleanup *make_cleanup_defer_target_commit_resume (); + /* For target_read_memory see target/target.h. */ /* The default target_ops::to_wait implementation. */ -- 2.5.5 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were 2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves ` (3 preceding siblings ...) 2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves @ 2016-02-17 2:51 ` Pedro Alves 2016-02-17 11:46 ` Luis Machado 4 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2016-02-17 2:51 UTC (permalink / raw) To: gdb-patches Currently GDB never sends more than one action per vCont packet, when connected in non-stop mode. A follow up patch will change that, and it exposed a gdbserver problem with the vCont handling. For example, this in non-stop mode: => vCont;s:p1.1;c <= OK Should be equivalent to: => vCont;s:p1.1 <= OK => vCont;c <= OK But gdbserver currently doesn't handle this. In the latter case, "vCont;c" makes gdbserver clobber the previous step request. This patch fixes that. Note the server side must ignore resume actions for the thread that has a pending %Stopped notification (and any other threads with events pending), until GDB acks the notification with vStopped. Otherwise, e.g., the following case is mishandled: #1 => g (or any other packet) #2 <= [registers] #3 <= %Stopped T05 thread:p1.2 #4 => vCont s:p1.1;c #5 <= OK Above, the server must not resume thread p1.2 when it processes the vCont. GDB can't know that p1.2 stopped until it acks the %Stopped notification. (Otherwise it wouldn't send a default "c" action.) (The vCont documentation already specifies this.) Finally, special care must also be given to handling fork/vfork events. A (v)fork event actually tells us that two processes stopped -- the parent and the child. Until we follow the fork, we must not resume the child. Therefore, if we have a pending fork follow, we must not send a global wildcard resume action (vCont;c). We can still send process-wide wildcards though. (The comments above will be added as code comments to gdb in a follow up patch.) gdb/gdbserver/ChangeLog: 2016-02-16 Pedro Alves <palves@redhat.com> * linux-low.c (linux_set_resume_request): Ignore resume requests for already-resumed threads. * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): New functions. * server.h (in_queued_stop_replies): New declaration. --- gdb/gdbserver/linux-low.c | 27 +++++++++++++++++++++++++++ gdb/gdbserver/server.c | 33 ++++++++++++++++++++++++++++++++- gdb/gdbserver/server.h | 4 ++++ 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 8b025bd..2cac4c0 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -4465,6 +4465,33 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg) continue; } + /* Ignore (wildcard) resume requests for already-resumed + requests. */ + if (r->resume[ndx].kind != resume_stop + && thread->last_resume_kind != resume_stop) + { + if (debug_threads) + debug_printf ("already %s LWP %ld at GDB's request\n", + (thread->last_resume_kind + == resume_step) + ? "stepping" + : "continuing", + lwpid_of (thread)); + continue; + } + + /* If the thread has a pending event that has already been + reported to GDBserver core, but GDB has not pulled the + event out of the vStopped queue yet, likewise, ignore the + (wildcard) resume request. */ + if (in_queued_stop_replies (entry->id)) + { + if (debug_threads) + debug_printf ("not resuming LWP %ld: has queued stop reply\n", + lwpid_of (thread)); + continue; + } + lwp->resume = &r->resume[ndx]; thread->last_resume_kind = lwp->resume->kind; diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index ef715e7..660ee5b 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -193,6 +193,38 @@ vstop_notif_reply (struct notif_event *event, char *own_buf) prepare_resume_reply (own_buf, vstop->ptid, &vstop->status); } +/* QUEUE_iterate callback helper for in_queued_stop_replies. */ + +static int +in_queued_stop_replies_ptid (QUEUE (notif_event_p) *q, + QUEUE_ITER (notif_event_p) *iter, + struct notif_event *event, + void *data) +{ + ptid_t filter_ptid = *(ptid_t *) data; + struct vstop_notif *vstop_event = (struct vstop_notif *) event; + + if (ptid_match (vstop_event->ptid, filter_ptid)) + return 0; + + /* Don't resume fork children that GDB does not know about yet. */ + if ((vstop_event->status.kind == TARGET_WAITKIND_FORKED + || vstop_event->status.kind == TARGET_WAITKIND_VFORKED) + && ptid_match (vstop_event->status.value.related_pid, filter_ptid)) + return 0; + + return 1; +} + +/* See server.h. */ + +int +in_queued_stop_replies (ptid_t ptid) +{ + return !QUEUE_iterate (notif_event_p, notif_stop.queue, + in_queued_stop_replies_ptid, &ptid); +} + struct notif_server notif_stop = { "vStopped", "Stop", NULL, vstop_notif_reply, @@ -2949,7 +2981,6 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) if (startswith (own_buf, "vCont;")) { - require_running (own_buf); handle_v_cont (own_buf); return; } diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h index 3d78fb3..68a3670 100644 --- a/gdb/gdbserver/server.h +++ b/gdb/gdbserver/server.h @@ -119,6 +119,10 @@ extern int handle_target_event (int err, gdb_client_data client_data); /* Get rid of the currently pending stop replies that match PTID. */ extern void discard_queued_stop_replies (ptid_t ptid); +/* Returns true if there's a pending stop reply that matches PTID in + the vStopped notifications queue. */ +extern int in_queued_stop_replies (ptid_t ptid); + #include "remote-utils.h" #include "utils.h" -- 1.9.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were 2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves @ 2016-02-17 11:46 ` Luis Machado 2016-02-17 12:32 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Luis Machado @ 2016-02-17 11:46 UTC (permalink / raw) To: Pedro Alves, gdb-patches On 02/17/2016 12:44 AM, Pedro Alves wrote: > Currently GDB never sends more than one action per vCont packet, when > connected in non-stop mode. A follow up patch will change that, and > it exposed a gdbserver problem with the vCont handling. > > For example, this in non-stop mode: > > => vCont;s:p1.1;c > <= OK > > Should be equivalent to: > > => vCont;s:p1.1 > <= OK > => vCont;c > <= OK > > But gdbserver currently doesn't handle this. In the latter case, > "vCont;c" makes gdbserver clobber the previous step request. This > patch fixes that. > > Note the server side must ignore resume actions for the thread that > has a pending %Stopped notification (and any other threads with events > pending), until GDB acks the notification with vStopped. Otherwise, > e.g., the following case is mishandled: > > #1 => g (or any other packet) > #2 <= [registers] > #3 <= %Stopped T05 thread:p1.2 > #4 => vCont s:p1.1;c > #5 <= OK > > Above, the server must not resume thread p1.2 when it processes the > vCont. GDB can't know that p1.2 stopped until it acks the %Stopped > notification. (Otherwise it wouldn't send a default "c" action.) > > (The vCont documentation already specifies this.) > > Finally, special care must also be given to handling fork/vfork > events. A (v)fork event actually tells us that two processes stopped > -- the parent and the child. Until we follow the fork, we must not > resume the child. Therefore, if we have a pending fork follow, we > must not send a global wildcard resume action (vCont;c). We can still > send process-wide wildcards though. > > (The comments above will be added as code comments to gdb in a follow > up patch.) > > gdb/gdbserver/ChangeLog: > 2016-02-16 Pedro Alves <palves@redhat.com> > > * linux-low.c (linux_set_resume_request): Ignore resume requests > for already-resumed threads. > * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): > New functions. > * server.h (in_queued_stop_replies): New declaration. > --- > gdb/gdbserver/linux-low.c | 27 +++++++++++++++++++++++++++ > gdb/gdbserver/server.c | 33 ++++++++++++++++++++++++++++++++- > gdb/gdbserver/server.h | 4 ++++ > 3 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index 8b025bd..2cac4c0 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -4465,6 +4465,33 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg) > continue; > } > > + /* Ignore (wildcard) resume requests for already-resumed > + requests. */ For already-resumed requests or threads? Looked a little confusing. If you really meant "requests", then we may need to adjust the wording a bit, like "for requests that have already been acknowledged.". The rest of the series looks good to me. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were 2016-02-17 11:46 ` Luis Machado @ 2016-02-17 12:32 ` Pedro Alves 2016-10-26 15:39 ` Pedro Alves 0 siblings, 1 reply; 15+ messages in thread From: Pedro Alves @ 2016-02-17 12:32 UTC (permalink / raw) To: Luis Machado, gdb-patches On 02/17/2016 11:46 AM, Luis Machado wrote: > On 02/17/2016 12:44 AM, Pedro Alves wrote: >> Currently GDB never sends more than one action per vCont packet, when >> connected in non-stop mode. A follow up patch will change that, and >> it exposed a gdbserver problem with the vCont handling. >> >> For example, this in non-stop mode: >> >> => vCont;s:p1.1;c >> <= OK >> >> Should be equivalent to: >> >> => vCont;s:p1.1 >> <= OK >> => vCont;c >> <= OK >> >> But gdbserver currently doesn't handle this. In the latter case, >> "vCont;c" makes gdbserver clobber the previous step request. This >> patch fixes that. >> >> Note the server side must ignore resume actions for the thread that >> has a pending %Stopped notification (and any other threads with events >> pending), until GDB acks the notification with vStopped. Otherwise, >> e.g., the following case is mishandled: >> >> #1 => g (or any other packet) >> #2 <= [registers] >> #3 <= %Stopped T05 thread:p1.2 >> #4 => vCont s:p1.1;c >> #5 <= OK >> >> Above, the server must not resume thread p1.2 when it processes the >> vCont. GDB can't know that p1.2 stopped until it acks the %Stopped >> notification. (Otherwise it wouldn't send a default "c" action.) >> >> (The vCont documentation already specifies this.) >> >> Finally, special care must also be given to handling fork/vfork >> events. A (v)fork event actually tells us that two processes stopped >> -- the parent and the child. Until we follow the fork, we must not >> resume the child. Therefore, if we have a pending fork follow, we >> must not send a global wildcard resume action (vCont;c). We can still >> send process-wide wildcards though. >> >> (The comments above will be added as code comments to gdb in a follow >> up patch.) >> >> gdb/gdbserver/ChangeLog: >> 2016-02-16 Pedro Alves <palves@redhat.com> >> >> * linux-low.c (linux_set_resume_request): Ignore resume requests >> for already-resumed threads. >> * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): >> New functions. >> * server.h (in_queued_stop_replies): New declaration. >> --- >> gdb/gdbserver/linux-low.c | 27 +++++++++++++++++++++++++++ >> gdb/gdbserver/server.c | 33 ++++++++++++++++++++++++++++++++- >> gdb/gdbserver/server.h | 4 ++++ >> 3 files changed, 63 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c >> index 8b025bd..2cac4c0 100644 >> --- a/gdb/gdbserver/linux-low.c >> +++ b/gdb/gdbserver/linux-low.c >> @@ -4465,6 +4465,33 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg) >> continue; >> } >> >> + /* Ignore (wildcard) resume requests for already-resumed >> + requests. */ > > For already-resumed requests or threads? Looked a little confusing. Whoops, I meant "already-resumed threads". Fixed locally. > > If you really meant "requests", then we may need to adjust the wording a > bit, like "for requests that have already been acknowledged.". > > The rest of the series looks good to me. Great, thanks! -- Pedro Alves ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were 2016-02-17 12:32 ` Pedro Alves @ 2016-10-26 15:39 ` Pedro Alves 0 siblings, 0 replies; 15+ messages in thread From: Pedro Alves @ 2016-10-26 15:39 UTC (permalink / raw) To: Luis Machado, gdb-patches On 02/17/2016 12:32 PM, Pedro Alves wrote: > On 02/17/2016 11:46 AM, Luis Machado wrote: >> The rest of the series looks good to me. > > Great, thanks! > This is the version that I pushed. The fork parent/child handling needed an extra tweak, to consider the case of the fork event not having migrated out of linux-low.c yet. This was exposed by sporadic failures in the gdb.threads/forking-threads-plus-breakpoint.exp test. From 5a04c4cf5df6d13596e79e7b84520cbe245a5a4d Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 26 Oct 2016 16:17:25 +0100 Subject: [PATCH] gdbserver: Leave already-vCont-resumed threads as they were Currently GDB never sends more than one action per vCont packet, when connected in non-stop mode. A follow up patch will change that, and it exposed a gdbserver problem with the vCont handling. For example, this in non-stop mode: => vCont;s:p1.1;c <= OK Should be equivalent to: => vCont;s:p1.1 <= OK => vCont;c <= OK But gdbserver currently doesn't handle this. In the latter case, "vCont;c" makes gdbserver clobber the previous step request. This patch fixes that. Note the server side must ignore resume actions for the thread that has a pending %Stopped notification (and any other threads with events pending), until GDB acks the notification with vStopped. Otherwise, e.g., the following case is mishandled: #1 => g (or any other packet) #2 <= [registers] #3 <= %Stopped T05 thread:p1.2 #4 => vCont s:p1.1;c #5 <= OK Above, the server must not resume thread p1.2 when it processes the vCont. GDB can't know that p1.2 stopped until it acks the %Stopped notification. (Otherwise it wouldn't send a default "c" action.) (The vCont documentation already specifies this.) Finally, special care must also be given to handling fork/vfork events. A (v)fork event actually tells us that two processes stopped -- the parent and the child. Until we follow the fork, we must not resume the child. Therefore, if we have a pending fork follow, we must not send a global wildcard resume action (vCont;c). We can still send process-wide wildcards though. (The comments above will be added as code comments to gdb in a follow up patch.) gdb/gdbserver/ChangeLog: 2016-10-26 Pedro Alves <palves@redhat.com> * linux-low.c (handle_extended_wait): Link parent/child fork threads. (linux_wait_1): Unlink them. (linux_set_resume_request): Ignore resume requests for already-resumed and unhandled fork child threads. * linux-low.h (struct lwp_info) <fork_relative>: New field. * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): New functions. (handle_v_requests) <vCont>: Don't call require_running. * server.h (in_queued_stop_replies): New declaration. --- gdb/gdbserver/ChangeLog | 13 +++++++++++ gdb/gdbserver/linux-low.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++ gdb/gdbserver/linux-low.h | 6 +++++ gdb/gdbserver/server.c | 33 +++++++++++++++++++++++++- gdb/gdbserver/server.h | 4 ++++ 5 files changed, 114 insertions(+), 1 deletion(-) diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog index 437bb4c..1a9c4e5 100644 --- a/gdb/gdbserver/ChangeLog +++ b/gdb/gdbserver/ChangeLog @@ -1,3 +1,16 @@ +2016-10-26 Pedro Alves <palves@redhat.com> + + * linux-low.c (handle_extended_wait): Link parent/child fork + threads. + (linux_wait_1): Unlink them. + (linux_set_resume_request): Ignore resume requests for + already-resumed and unhandled fork child threads. + * linux-low.h (struct lwp_info) <fork_relative>: New field. + * server.c (in_queued_stop_replies_ptid, in_queued_stop_replies): + New functions. + (handle_v_requests) <vCont>: Don't call require_running. + * server.h (in_queued_stop_replies): New declaration. + 2016-10-24 Yao Qi <yao.qi@linaro.org> PR server/20733 diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 94c5bbe..f43ce7e 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -589,6 +589,11 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) event_lwp->status_pending_p = 1; event_lwp->status_pending = wstat; + /* Link the threads until the parent event is passed on to + higher layers. */ + event_lwp->fork_relative = child_lwp; + child_lwp->fork_relative = event_lwp; + /* If the parent thread is doing step-over with single-step breakpoints, the list of single-step breakpoints are cloned from the parent's. Remove them from the child process. @@ -3853,6 +3858,15 @@ linux_wait_1 (ptid_t ptid, { /* If the reported event is an exit, fork, vfork or exec, let GDB know. */ + + /* Break the unreported fork relationship chain. */ + if (event_child->waitstatus.kind == TARGET_WAITKIND_FORKED + || event_child->waitstatus.kind == TARGET_WAITKIND_VFORKED) + { + event_child->fork_relative->fork_relative = NULL; + event_child->fork_relative = NULL; + } + *ourstatus = event_child->waitstatus; /* Clear the event lwp's waitstatus since we handled it already. */ event_child->waitstatus.kind = TARGET_WAITKIND_IGNORE; @@ -4654,6 +4668,51 @@ linux_set_resume_request (struct inferior_list_entry *entry, void *arg) continue; } + /* Ignore (wildcard) resume requests for already-resumed + threads. */ + if (r->resume[ndx].kind != resume_stop + && thread->last_resume_kind != resume_stop) + { + if (debug_threads) + debug_printf ("already %s LWP %ld at GDB's request\n", + (thread->last_resume_kind + == resume_step) + ? "stepping" + : "continuing", + lwpid_of (thread)); + continue; + } + + /* Don't let wildcard resumes resume fork children that GDB + does not yet know are new fork children. */ + if (lwp->fork_relative != NULL) + { + struct inferior_list_entry *inf, *tmp; + struct lwp_info *rel = lwp->fork_relative; + + if (rel->status_pending_p + && (rel->waitstatus.kind == TARGET_WAITKIND_FORKED + || rel->waitstatus.kind == TARGET_WAITKIND_VFORKED)) + { + if (debug_threads) + debug_printf ("not resuming LWP %ld: has queued stop reply\n", + lwpid_of (thread)); + continue; + } + } + + /* If the thread has a pending event that has already been + reported to GDBserver core, but GDB has not pulled the + event out of the vStopped queue yet, likewise, ignore the + (wildcard) resume request. */ + if (in_queued_stop_replies (entry->id)) + { + if (debug_threads) + debug_printf ("not resuming LWP %ld: has queued stop reply\n", + lwpid_of (thread)); + continue; + } + lwp->resume = &r->resume[ndx]; thread->last_resume_kind = lwp->resume->kind; diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h index 5057e66..476816d 100644 --- a/gdb/gdbserver/linux-low.h +++ b/gdb/gdbserver/linux-low.h @@ -301,6 +301,12 @@ struct lwp_info information or exit status until it can be reported to GDB. */ struct target_waitstatus waitstatus; + /* A pointer to the fork child/parent relative. Valid only while + the parent fork event is not reported to higher layers. Used to + avoid wildcard vCont actions resuming a fork child before GDB is + notified about the parent's fork event. */ + struct lwp_info *fork_relative; + /* When stopped is set, this is where the lwp last stopped, with decr_pc_after_break already accounted for. If the LWP is running, this is the address at which the lwp was resumed. */ diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index 2996e19..3f9ff2b 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -193,6 +193,38 @@ vstop_notif_reply (struct notif_event *event, char *own_buf) prepare_resume_reply (own_buf, vstop->ptid, &vstop->status); } +/* QUEUE_iterate callback helper for in_queued_stop_replies. */ + +static int +in_queued_stop_replies_ptid (QUEUE (notif_event_p) *q, + QUEUE_ITER (notif_event_p) *iter, + struct notif_event *event, + void *data) +{ + ptid_t filter_ptid = *(ptid_t *) data; + struct vstop_notif *vstop_event = (struct vstop_notif *) event; + + if (ptid_match (vstop_event->ptid, filter_ptid)) + return 0; + + /* Don't resume fork children that GDB does not know about yet. */ + if ((vstop_event->status.kind == TARGET_WAITKIND_FORKED + || vstop_event->status.kind == TARGET_WAITKIND_VFORKED) + && ptid_match (vstop_event->status.value.related_pid, filter_ptid)) + return 0; + + return 1; +} + +/* See server.h. */ + +int +in_queued_stop_replies (ptid_t ptid) +{ + return !QUEUE_iterate (notif_event_p, notif_stop.queue, + in_queued_stop_replies_ptid, &ptid); +} + struct notif_server notif_stop = { "vStopped", "Stop", NULL, vstop_notif_reply, @@ -2947,7 +2979,6 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len) if (startswith (own_buf, "vCont;")) { - require_running (own_buf); handle_v_cont (own_buf); return; } diff --git a/gdb/gdbserver/server.h b/gdb/gdbserver/server.h index 51b2191..f56c0f5 100644 --- a/gdb/gdbserver/server.h +++ b/gdb/gdbserver/server.h @@ -123,6 +123,10 @@ extern int handle_target_event (int err, gdb_client_data client_data); /* Get rid of the currently pending stop replies that match PTID. */ extern void discard_queued_stop_replies (ptid_t ptid); +/* Returns true if there's a pending stop reply that matches PTID in + the vStopped notifications queue. */ +extern int in_queued_stop_replies (ptid_t ptid); + #include "remote-utils.h" #include "utils.h" -- 2.5.5 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-10-26 15:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-17 2:44 [PATCH 0/5] Coalesce/aggregate (async) vCont packets/actions Pedro Alves 2016-02-17 2:44 ` [PATCH 1/5] gdb: Clean up remote.c:remote_resume Pedro Alves 2016-02-17 11:45 ` Luis Machado 2016-02-17 12:32 ` Pedro Alves 2016-02-17 13:44 ` Luis Machado 2016-02-17 2:44 ` [PATCH 2/5] gdb: Free inferior->priv when inferior exits Pedro Alves 2016-02-17 2:44 ` [PATCH 3/5] gdb/doc: Clarify vCont packet description Pedro Alves 2016-02-17 15:56 ` Eli Zaretskii 2016-02-17 2:51 ` [PATCH 5/5] gdb: Coalesce/aggregate (async) vCont packets/actions Pedro Alves 2016-02-17 13:42 ` Luis Machado 2016-10-26 15:35 ` Pedro Alves 2016-02-17 2:51 ` [PATCH 4/5] gdbserver: Leave already-vCont-resumed threads as they were Pedro Alves 2016-02-17 11:46 ` Luis Machado 2016-02-17 12:32 ` Pedro Alves 2016-10-26 15:39 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).