From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1551) id 2DB51385781D; Fri, 29 Apr 2022 11:37:59 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2DB51385781D Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Pedro Alves To: gdb-cvs@sourceware.org Subject: [binutils-gdb] Slightly tweak and clarify target_resume's interface X-Act-Checkin: binutils-gdb X-Git-Author: Pedro Alves X-Git-Refname: refs/heads/master X-Git-Oldrev: 8a2ef851861706a113a080a1ff3d91c81449704d X-Git-Newrev: d51926f06a7f4854bebdd71dcb0a78dbaa2f4168 Message-Id: <20220429113759.2DB51385781D@sourceware.org> Date: Fri, 29 Apr 2022 11:37:59 +0000 (GMT) X-BeenThere: gdb-cvs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Apr 2022 11:37:59 -0000 https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3Dd51926f06a7f= 4854bebdd71dcb0a78dbaa2f4168 commit d51926f06a7f4854bebdd71dcb0a78dbaa2f4168 Author: Pedro Alves Date: Thu Apr 21 14:20:36 2022 +0100 Slightly tweak and clarify target_resume's interface =20 The current target_resume interface is a bit odd & non-intuitive. I've found myself explaining it a couple times the recent past, while reviewing patches that assumed STEP/SIGNAL always applied to the passed in PTID. It goes like this today: =20 - if the passed in PTID is a thread, then the step/signal request is for that thread. =20 - otherwise, if PTID is a wildcard (all threads or all threads of process), the step/signal request is for inferior_ptid, and PTID indicates which set of threads run free. =20 Because GDB always switches the current thread to "leader" thread being resumed/stepped/signalled, we can simplify this a bit to: =20 - step/signal are always for inferior_ptid. =20 - PTID indicates the set of threads that run free. =20 Still not ideal, but it's a minimal change and at least there are no special cases this way. =20 That's what this patch does. It renames the PTID parameter to SCOPE_PTID, adds some assertions to target_resume, and tweaks target_resume's description. In addition, it also renames PTID to SCOPE_PTID in the remote and linux-nat targets, and simplifies their implementation a little bit. Other targets could do the same, but they don't have to. =20 Change-Id: I02a2ec2ab3a3e9b191de1e9a84f55c17cab7daaf Diff: --- gdb/linux-nat.c | 29 ++++++++++------------------- gdb/remote.c | 52 +++++++++++++++++++++++++--------------------------- gdb/target.c | 13 ++++++++----- gdb/target.h | 31 ++++++++++++++++++++----------- 4 files changed, 63 insertions(+), 62 deletions(-) diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index dff8d07d3f7..740cc0ddfc0 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -1595,32 +1595,22 @@ resume_set_callback (struct lwp_info *lp) } =20 void -linux_nat_target::resume (ptid_t ptid, int step, enum gdb_signal signo) +linux_nat_target::resume (ptid_t scope_ptid, int step, enum gdb_signal sig= no) { struct lwp_info *lp; - int resume_many; =20 linux_nat_debug_printf ("Preparing to %s %s, %s, inferior_ptid %s", step ? "step" : "resume", - ptid.to_string ().c_str (), + scope_ptid.to_string ().c_str (), (signo !=3D GDB_SIGNAL_0 ? strsignal (gdb_signal_to_host (signo)) : "0"), inferior_ptid.to_string ().c_str ()); =20 - /* A specific PTID means `step only this process id'. */ - resume_many =3D (minus_one_ptid =3D=3D ptid - || ptid.is_pid ()); - /* Mark the lwps we're resuming as resumed and update their last_resume_kind to resume_continue. */ - iterate_over_lwps (ptid, resume_set_callback); + iterate_over_lwps (scope_ptid, resume_set_callback); =20 - /* See if it's the current inferior that should be handled - specially. */ - if (resume_many) - lp =3D find_lwp_pid (inferior_ptid); - else - lp =3D find_lwp_pid (ptid); + lp =3D find_lwp_pid (inferior_ptid); gdb_assert (lp !=3D NULL); =20 /* Remember if we're stepping. */ @@ -1669,11 +1659,12 @@ linux_nat_target::resume (ptid_t ptid, int step, en= um gdb_signal signo) return; } =20 - if (resume_many) - iterate_over_lwps (ptid, [=3D] (struct lwp_info *info) - { - return linux_nat_resume_callback (info, lp); - }); + /* No use iterating unless we're resuming other threads. */ + if (scope_ptid !=3D lp->ptid) + iterate_over_lwps (scope_ptid, [=3D] (struct lwp_info *info) + { + return linux_nat_resume_callback (info, lp); + }); =20 linux_nat_debug_printf ("%s %s, %s (resume event thread)", step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT", diff --git a/gdb/remote.c b/gdb/remote.c index 562cc586f2b..ff98024cd78 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -737,7 +737,7 @@ public: /* Remote specific methods. */ =20 char *append_resumption (char *p, char *endp, ptid_t ptid, int step, gdb_signal siggnal); - int remote_resume_with_vcont (ptid_t ptid, int step, + int remote_resume_with_vcont (ptid_t scope_ptid, int step, gdb_signal siggnal); =20 thread_info *add_current_inferior_and_thread (const char *wait_status); @@ -6274,9 +6274,8 @@ remote_target::remote_vcont_probe () thread to be resumed is PTID; STEP and SIGGNAL indicate whether the resumed thread should be single-stepped and/or signalled. If PTID equals minus_one_ptid, then all threads are resumed; if PTID - represents a process, then all threads of the process are resumed; - the thread to be stepped and/or signalled is given in the global - INFERIOR_PTID. */ + represents a process, then all threads of the process are + resumed. */ =20 char * remote_target::append_resumption (char *p, char *endp, @@ -6433,18 +6432,15 @@ remote_target::remote_resume_with_hc (ptid_t ptid, = int step, putpkt (buf); } =20 -/* 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 - equals minus_one_ptid, then all threads are resumed; the thread to - be stepped and/or signalled is given in the global INFERIOR_PTID. - This function returns non-zero iff it resumes the inferior. +/* Resume the remote inferior by using a "vCont" packet. SCOPE_PTID, + STEP, and SIGGNAL have the same meaning as in target_resume. This + function returns non-zero iff it resumes the inferior. =20 This function issues a strict subset of all possible vCont commands at the moment. */ =20 int -remote_target::remote_resume_with_vcont (ptid_t ptid, int step, +remote_target::remote_resume_with_vcont (ptid_t scope_ptid, int step, enum gdb_signal siggnal) { struct remote_state *rs =3D get_remote_state (); @@ -6470,7 +6466,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid,= int step, =20 p +=3D xsnprintf (p, endp - p, "vCont"); =20 - if (ptid =3D=3D magic_null_ptid) + if (scope_ptid =3D=3D magic_null_ptid) { /* MAGIC_NULL_PTID means that we don't have any active threads, so we don't have any TID numbers the inferior will @@ -6478,7 +6474,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid,= int step, a TID. */ append_resumption (p, endp, minus_one_ptid, step, siggnal); } - else if (ptid =3D=3D minus_one_ptid || ptid.is_pid ()) + else if (scope_ptid =3D=3D minus_one_ptid || scope_ptid.is_pid ()) { /* Resume all threads (of all processes, or of a single process), with preference for INFERIOR_PTID. This assumes @@ -6492,15 +6488,15 @@ remote_target::remote_resume_with_vcont (ptid_t pti= d, int step, =20 /* Also pass down any pending signaled resumption for other threads not the current. */ - p =3D append_pending_thread_resumptions (p, endp, ptid); + p =3D append_pending_thread_resumptions (p, endp, scope_ptid); =20 /* And continue others without a signal. */ - append_resumption (p, endp, ptid, /*step=3D*/ 0, GDB_SIGNAL_0); + append_resumption (p, endp, scope_ptid, /*step=3D*/ 0, GDB_SIGNAL_0); } else { - /* Scheduler locking; resume only PTID. */ - append_resumption (p, endp, ptid, step, siggnal); + /* Scheduler locking; resume only SCOPE_PTID. */ + append_resumption (p, endp, scope_ptid, step, siggnal); } =20 gdb_assert (strlen (rs->buf.data ()) < get_remote_packet_size ()); @@ -6523,7 +6519,7 @@ remote_target::remote_resume_with_vcont (ptid_t ptid,= int step, /* Tell the remote machine to resume. */ =20 void -remote_target::resume (ptid_t ptid, int step, enum gdb_signal siggnal) +remote_target::resume (ptid_t scope_ptid, int step, enum gdb_signal siggna= l) { struct remote_state *rs =3D get_remote_state (); =20 @@ -6536,18 +6532,20 @@ remote_target::resume (ptid_t ptid, int step, enum = gdb_signal siggnal) able to do vCont action coalescing. */ if (target_is_non_stop_p () && ::execution_direction !=3D EXEC_REVERSE) { - remote_thread_info *remote_thr; - - if (minus_one_ptid =3D=3D ptid || ptid.is_pid ()) - remote_thr =3D get_remote_thread_info (this, inferior_ptid); - else - remote_thr =3D get_remote_thread_info (this, ptid); + remote_thread_info *remote_thr + =3D get_remote_thread_info (inferior_thread ()); =20 /* We don't expect the core to ask to resume an already resumed (from its point of view) thread. */ gdb_assert (remote_thr->get_resume_state () =3D=3D resume_state::NOT= _RESUMED); =20 remote_thr->set_resumed_pending_vcont (step, siggnal); + + /* There's actually nothing that says that the core can't + request a wildcard resume in non-stop mode, though. It's + just that we know it doesn't currently, so we don't bother + with it. */ + gdb_assert (scope_ptid =3D=3D inferior_ptid); return; } =20 @@ -6563,11 +6561,11 @@ remote_target::resume (ptid_t ptid, int step, enum = gdb_signal siggnal) rs->last_resume_exec_dir =3D ::execution_direction; =20 /* Prefer vCont, and fallback to s/c/S/C, which use Hc. */ - if (!remote_resume_with_vcont (ptid, step, siggnal)) - remote_resume_with_hc (ptid, step, siggnal); + if (!remote_resume_with_vcont (scope_ptid, step, siggnal)) + remote_resume_with_hc (scope_ptid, step, siggnal); =20 /* Update resumed state tracked by the remote target. */ - for (thread_info *tp : all_non_exited_threads (this, ptid)) + for (thread_info *tp : all_non_exited_threads (this, scope_ptid)) get_remote_thread_info (tp)->set_resumed (); =20 /* We've just told the target to resume. The remote server will diff --git a/gdb/target.c b/gdb/target.c index 7d291fd4d0d..1063f8086bc 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2655,21 +2655,24 @@ target_thread_info_to_thread_handle (struct thread_= info *tip) } =20 void -target_resume (ptid_t ptid, int step, enum gdb_signal signal) +target_resume (ptid_t scope_ptid, int step, enum gdb_signal signal) { process_stratum_target *curr_target =3D current_inferior ()->process_tar= get (); gdb_assert (!curr_target->commit_resumed_state); =20 + gdb_assert (inferior_ptid !=3D null_ptid); + gdb_assert (inferior_ptid.matches (scope_ptid)); + target_dcache_invalidate (); =20 - current_inferior ()->top_target ()->resume (ptid, step, signal); + current_inferior ()->top_target ()->resume (scope_ptid, step, signal); =20 - registers_changed_ptid (curr_target, ptid); + registers_changed_ptid (curr_target, scope_ptid); /* We only set the internal executing state here. The user/frontend running state is set at a higher level. This also clears the thread's stop_pc as side effect. */ - set_executing (curr_target, ptid, true); - clear_inline_frame_state (curr_target, ptid); + set_executing (curr_target, scope_ptid, true); + clear_inline_frame_state (curr_target, scope_ptid); =20 if (target_can_async_p ()) target_async (1); diff --git a/gdb/target.h b/gdb/target.h index 093178af0bc..f77dbf05113 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -1471,23 +1471,32 @@ extern void target_detach (inferior *inf, int from_= tty); =20 extern void target_disconnect (const char *, int); =20 -/* 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. +/* Resume execution (or prepare for execution) of the current thread + (INFERIOR_PTID), while optionally letting other threads of the + current process or all processes run free. + + STEP says whether to hardware single-step the current thread or to + let it run free; SIGNAL is the signal to be given to the current + thread, or GDB_SIGNAL_0 for no signal. The caller may not pass + GDB_SIGNAL_DEFAULT. + + SCOPE_PTID indicates the resumption scope. I.e., which threads + (other than the current) run free. If resuming a single thread, + SCOPE_PTID is the same thread as the current thread. A wildcard + SCOPE_PTID (all threads, or all threads of process) lets threads + other than the current (for which the wildcard SCOPE_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. Note neither STEP nor SIGNAL apply to any thread + other than the current. =20 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); +extern void target_resume (ptid_t scope_ptid, + int step, enum gdb_signal signal); =20 /* Ensure that all resumed threads are committed to the target.