From 1192f124a308601f5fef7a35715ccd6f904e7b17 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Sun, 24 Jan 2021 23:57:29 -0500 Subject: [PATCH] gdb: generalize commit_resume, avoid commit-resuming when threads have pending statuses The rationale for this patch comes from the ROCm port [1], the goal being to reduce the number of back and forths between GDB and the target when doing successive operations. I'll start with explaining the rationale and then go over the implementation. In the ROCm / GPU world, the term "wave" is somewhat equivalent to a "thread" in GDB. So if you read if from a GPU stand point, just s/thread/wave/. ROCdbgapi, the library used by GDB [2] to communicate with the GPU target, gives the illusion that it's possible for the debugger to control (start and stop) individual threads. But in reality, this is not how it works. Under the hood, all threads of a queue are controlled as a group. To stop one thread in a group of running ones, the state of all threads is retrieved from the GPU, all threads are destroyed, and all threads but the one we want to stop are re-created from the saved state. The net result, from the point of view of GDB, is that the library stopped one thread. The same thing goes if we want to resume one thread while others are running: the state of all running threads is retrieved from the GPU, they are all destroyed, and they are all re-created, including the thread we want to resume. This leads to some inefficiencies when combined with how GDB works, here are two examples: - Stopping all threads: because the target operates in non-stop mode, when the user interface mode is all-stop, GDB must stop all threads individually when presenting a stop. Let's suppose we have 1000 threads and the user does ^C. GDB asks the target to stop one thread. Behind the scenes, the library retrieves 1000 thread states and restores the 999 others still running ones. GDB asks the target to stop another one. The target retrieves 999 thread states and restores the 998 remaining ones. That means that to stop 1000 threads, we did 1000 back and forths with the GPU. It would have been much better to just retrieve the states once and stop there. - Resuming with pending events: suppose the 1000 threads hit a breakpoint at the same time. The breakpoint is conditional and evaluates to true for the first thread, to false for all others. GDB pulls one event (for the first thread) from the target, decides that it should present a stop, so stops all threads using stop_all_threads. All these other threads have a breakpoint event to report, which is saved in `thread_info::suspend::waitstatus` for later. When the user does "continue", GDB resumes that one thread that did hit the breakpoint. It then processes the pending events one by one as if they just arrived. It picks one, evaluates the condition to false, and resumes the thread. It picks another one, evaluates the condition to false, and resumes the thread. And so on. In between each resumption, there is a full state retrieval and re-creation. It would be much nicer if we could wait a little bit before sending those threads on the GPU, until it processed all those pending events. To address this kind of performance issue, ROCdbgapi has a concept called "forward progress required", which is a boolean state that allows its user (i.e. GDB) to say "I'm doing a bunch of operations, you can hold off putting the threads on the GPU until I'm done" (the "forward progress not required" state). Turning forward progress back on indicates to the library that all threads that are supposed to be running should now be really running on the GPU. It turns out that GDB has a similar concept, though not as general, commit_resume. One difference is that commit_resume is not stateful: the target can't look up "does the core need me to schedule resumed threads for execution right now". It is also specifically linked to the resume method, it is not used in other contexts. The target accumulates resumption requests through target_ops::resume calls, and then commits those resumptions when target_ops::commit_resume is called. The target has no way to check if it's ok to leave resumed threads stopped in other target methods. To bridge the gap, this patch generalizes the commit_resume concept in GDB to match the forward progress concept of ROCdbgapi. The current name (commit_resume) can be interpreted as "commit the previous resume calls". I renamed the concept to "commit_resumed", as in "commit the threads that are resumed". In the new version, we have two things: - the commit_resumed_state field in process_stratum_target: indicates whether GDB requires target stacks using this target to have resumed threads committed to the execution target/device. If false, an execution target is allowed to leave resumed threads un-committed at the end of whatever method it is executing. - the commit_resumed target method: called when commit_resumed_state transitions from false to true. While commit_resumed_state was false, the target may have left some resumed threads un-committed. This method being called tells it that it should commit them back to the execution device. Let's take the "Stopping all threads" scenario from above and see how it would work with the ROCm target with this change. Before stopping all threads, GDB would set the target's commit_resumed_state field to false. It would then ask the target to stop the first thread. The target would retrieve all threads' state from the GPU and mark that one as stopped. Since commit_resumed_state is false, it leaves all the other threads (still resumed) stopped. GDB would then proceed to call target_stop for all the other threads. Since resumed threads are not committed, this doesn't do any back and forth with the GPU. To simplify the implementation of targets, this patch makes it so that when calling certain target methods, the contract between the core and the targets guarantees that commit_resumed_state is false. This way, the target doesn't need two paths, one for commit_resumed_state == true and one for commit_resumed_state == false. It can just assert that commit_resumed_state is false and work with that assumption. This also helps catch places where we forgot to disable commit_resumed_state before calling the method, which represents a probable optimization opportunity. The commit adds assertions in the target method wrappers (target_resume and friends) to have some confidence that this contract between the core and the targets is respected. The scoped_disable_commit_resumed type is used to disable the commit resumed state of all process targets on construction, and selectively re-enable it on destruction (see below for criteria). Note that it only sets the process_stratum_target::commit_resumed_state flag. A subsequent call to maybe_call_commit_resumed_all_targets is necessary to call the commit_resumed method on all target stacks with process targets that got their commit_resumed_state flag turned back on. This separation is because we don't want to call the commit_resumed methods in scoped_disable_commit_resumed's destructor, as they may throw. On destruction, commit-resumed is not re-enabled for a given target if: 1. this target has no threads resumed, or 2. this target has at least one resumed thread with a pending status known to the core (saved in thread_info::suspend::waitstatus). The first point is not technically necessary, because a proper commit_resumed implementation would be a no-op if the target has no resumed threads. But since we have a flag do to a quick check, it shouldn't hurt. The second point is more important: together with the scoped_disable_commit_resumed instance added in fetch_inferior_event, it makes it so the "Resuming with pending events" described above is handled efficiently. Here's what happens in that case: 1. The user types "continue". 2. Upon destruction, the scoped_disable_commit_resumed in the `proceed` function does not enable commit-resumed, as it sees some threads have pending statuses. 3. fetch_inferior_event is called to handle another event, the breakpoint hit evaluates to false, and that thread is resumed. Because there are still more threads with pending statuses, the destructor of scoped_disable_commit_resumed in fetch_inferior_event still doesn't enable commit-resumed. 4. Rinse and repeat step 3, until the last pending status is handled by fetch_inferior_event. In that case, scoped_disable_commit_resumed's destructor sees there are no more threads with pending statues, so it asks the target to commit resumed threads. This allows us to avoid all unnecessary back and forths, there is a single commit_resumed call once all pending statuses are processed. This change required remote_target::remote_stop_ns to learn how to handle stopping threads that were resumed but pending vCont. The simplest example where that happens is when using the remote target in all-stop, but with "maint set target-non-stop on", to force it to operate in non-stop mode under the hood. If two threads hit a breakpoint at the same time, GDB will receive two stop replies. It will present the stop for one thread and save the other one in thread_info::suspend::waitstatus. Before this patch, when doing "continue", GDB first resumes the thread without a pending status: Sending packet: $vCont;c:p172651.172676#f3 It then consumes the pending status in the next fetch_inferior_event call: [infrun] do_target_wait_1: Using pending wait status status->kind = stopped, signal = GDB_SIGNAL_TRAP for Thread 1517137.1517137. [infrun] target_wait (-1.0.0, status) = [infrun] 1517137.1517137.0 [Thread 1517137.1517137], [infrun] status->kind = stopped, signal = GDB_SIGNAL_TRAP It then realizes it needs to stop all threads to present the stop, so stops the thread it just resumed: [infrun] stop_all_threads: Thread 1517137.1517137 not executing [infrun] stop_all_threads: Thread 1517137.1517174 executing, need stop remote_stop called Sending packet: $vCont;t:p172651.172676#04 This is an unnecessary resume/stop. With this patch, we don't commit resumed threads after proceeding, because of the pending status: [infrun] maybe_commit_resumed_all_process_targets: not requesting commit-resumed for target extended-remote, a thread has a pending waitstatus When GDB handles the pending status and stop_all_threads runs, we stop a resumed but pending vCont thread: remote_stop_ns: Enqueueing phony stop reply for thread pending vCont-resume (1520940, 1520976, 0) That thread was never actually resumed on the remote stub / gdbserver, so we shouldn't send a packet to the remote side asking to stop the thread. Note that there are paths that resume the target and then do a synchronous blocking wait, in sort of nested event loop, via wait_sync_command_done. For example, inferior function calls, or any run control command issued from a breakpoint command list. We handle that making wait_sync_command_one a "sync" point -- force forward progress, or IOW, force-enable commit-resumed state. gdb/ChangeLog: yyyy-mm-dd Simon Marchi Pedro Alves * infcmd.c (run_command_1, attach_command, detach_command) (interrupt_target_1): Use scoped_disable_commit_resumed. * infrun.c (do_target_resume): Remove target_commit_resume call. (commit_resume_all_targets): Remove. (maybe_set_commit_resumed_all_targets): New. (maybe_call_commit_resumed_all_targets): New. (enable_commit_resumed): New. (scoped_disable_commit_resumed::scoped_disable_commit_resumed) (scoped_disable_commit_resumed::~scoped_disable_commit_resumed) (scoped_disable_commit_resumed::reset) (scoped_disable_commit_resumed::reset_and_commit) (scoped_enable_commit_resumed::scoped_enable_commit_resumed) (scoped_enable_commit_resumed::~scoped_enable_commit_resumed): New. (proceed): Use scoped_disable_commit_resumed and maybe_call_commit_resumed_all_targets. (fetch_inferior_event): Use scoped_disable_commit_resumed. * infrun.h (struct scoped_disable_commit_resumed): New. (maybe_call_commit_resumed_all_process_targets): New. (struct scoped_enable_commit_resumed): New. * mi/mi-main.c (exec_continue): Use scoped_disable_commit_resumed. * process-stratum-target.h (class process_stratum_target): : New. * record-full.c (record_full_wait_1): Change commit_resumed_state around calling commit_resumed. * remote.c (class remote_target) : Rename to... : ... this. (struct stop_reply): Move up. (remote_target::commit_resume): Rename to... (remote_target::commit_resumed): ... this. Check if there is any thread pending vCont resume. (remote_target::remote_stop_ns): Generate stop replies for resumed but pending vCont threads. (remote_target::wait_ns): Add gdb_assert. * target-delegates.c: Regenerate. * target.c (target_wait, target_resume): Assert that the current process_stratum target isn't in commit-resumed state. (defer_target_commit_resume): Remove. (target_commit_resume): Remove. (target_commit_resumed): New. (make_scoped_defer_target_commit_resume): Remove. (target_stop): Assert that the current process_stratum target isn't in commit-resumed state. * target.h (struct target_ops) : Rename to ... : ... this. (target_commit_resume): Remove. (target_commit_resumed): New. (make_scoped_defer_target_commit_resume): Remove. * top.c (wait_sync_command_done): Use scoped_enable_commit_resumed. [1] https://github.com/ROCm-Developer-Tools/ROCgdb/ [2] https://github.com/ROCm-Developer-Tools/ROCdbgapi Change-Id: I836135531a29214b21695736deb0a81acf8cf566 --- gdb/ChangeLog | 55 +++++++++ gdb/infcmd.c | 16 +++ gdb/infrun.c | 226 ++++++++++++++++++++++++++++++++--- gdb/infrun.h | 98 +++++++++++++++ gdb/mi/mi-main.c | 4 + gdb/process-stratum-target.h | 32 +++++ gdb/record-full.c | 10 +- gdb/remote.c | 149 ++++++++++++++++++----- gdb/target-delegates.c | 18 +-- gdb/target.c | 27 ++--- gdb/target.h | 31 ++--- gdb/top.c | 7 ++ 12 files changed, 575 insertions(+), 98 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 7f60d3c01ab..e3ae1c4df75 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,58 @@ +2021-03-26 Simon Marchi + Pedro Alves + + * infcmd.c (run_command_1, attach_command, detach_command) + (interrupt_target_1): Use scoped_disable_commit_resumed. + * infrun.c (do_target_resume): Remove + target_commit_resume call. + (commit_resume_all_targets): Remove. + (maybe_set_commit_resumed_all_targets): New. + (maybe_call_commit_resumed_all_targets): New. + (enable_commit_resumed): New. + (scoped_disable_commit_resumed::scoped_disable_commit_resumed) + (scoped_disable_commit_resumed::~scoped_disable_commit_resumed) + (scoped_disable_commit_resumed::reset) + (scoped_disable_commit_resumed::reset_and_commit) + (scoped_enable_commit_resumed::scoped_enable_commit_resumed) + (scoped_enable_commit_resumed::~scoped_enable_commit_resumed): + New. + (proceed): Use scoped_disable_commit_resumed and + maybe_call_commit_resumed_all_targets. + (fetch_inferior_event): Use scoped_disable_commit_resumed. + * infrun.h (struct scoped_disable_commit_resumed): New. + (maybe_call_commit_resumed_all_process_targets): New. + (struct scoped_enable_commit_resumed): New. + * mi/mi-main.c (exec_continue): Use scoped_disable_commit_resumed. + * process-stratum-target.h (class process_stratum_target): + : New. + * record-full.c (record_full_wait_1): Change commit_resumed_state + around calling commit_resumed. + * remote.c (class remote_target) : Rename to... + : ... this. + (struct stop_reply): Move up. + (remote_target::commit_resume): Rename to... + (remote_target::commit_resumed): ... this. Check if there is any + thread pending vCont resume. + (remote_target::remote_stop_ns): Generate stop replies for resumed + but pending vCont threads. + (remote_target::wait_ns): Add gdb_assert. + * target-delegates.c: Regenerate. + * target.c (target_wait, target_resume): Assert that the current + process_stratum target isn't in commit-resumed state. + (defer_target_commit_resume): Remove. + (target_commit_resume): Remove. + (target_commit_resumed): New. + (make_scoped_defer_target_commit_resume): Remove. + (target_stop): Assert that the current process_stratum target + isn't in commit-resumed state. + * target.h (struct target_ops) : Rename to ... + : ... this. + (target_commit_resume): Remove. + (target_commit_resumed): New. + (make_scoped_defer_target_commit_resume): Remove. + * top.c (wait_sync_command_done): Use + scoped_enable_commit_resumed. + 2021-03-26 Pedro Alves * target.c (target_always_non_stop_p): Also check whether the diff --git a/gdb/infcmd.c b/gdb/infcmd.c index a6e9572fc7f..9b0186dd391 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -419,6 +419,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how) dont_repeat (); + scoped_disable_commit_resumed disable_commit_resumed ("running"); + kill_if_already_running (from_tty); init_wait_for_inferior (); @@ -538,6 +540,8 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how) /* Since there was no error, there's no need to finish the thread states here. */ finish_state.release (); + + disable_commit_resumed.reset_and_commit (); } static void @@ -2565,6 +2569,8 @@ attach_command (const char *args, int from_tty) dont_repeat (); /* Not for the faint of heart */ + scoped_disable_commit_resumed disable_commit_resumed ("attaching"); + if (gdbarch_has_global_solist (target_gdbarch ())) /* Don't complain if all processes share the same symbol space. */ @@ -2673,6 +2679,8 @@ attach_command (const char *args, int from_tty) } else attach_post_wait (args, from_tty, mode); + + disable_commit_resumed.reset_and_commit (); } /* We had just found out that the target was already attached to an @@ -2746,6 +2754,8 @@ detach_command (const char *args, int from_tty) if (inferior_ptid == null_ptid) error (_("The program is not being run.")); + scoped_disable_commit_resumed disable_commit_resumed ("detaching"); + query_if_trace_running (from_tty); disconnect_tracing (); @@ -2779,6 +2789,8 @@ detach_command (const char *args, int from_tty) if (!was_non_stop_p) restart_after_all_stop_detach (as_process_stratum_target (target_ref.get ())); + + disable_commit_resumed.reset_and_commit (); } /* Disconnect from the current target without resuming it (leaving it @@ -2827,6 +2839,8 @@ stop_current_target_threads_ns (ptid_t ptid) void interrupt_target_1 (bool all_threads) { + scoped_disable_commit_resumed disable_commit_resumed ("interrupting"); + if (non_stop) { if (all_threads) @@ -2844,6 +2858,8 @@ interrupt_target_1 (bool all_threads) } else target_interrupt (); + + disable_commit_resumed.reset_and_commit (); } /* interrupt [-a] diff --git a/gdb/infrun.c b/gdb/infrun.c index 20035a0f5e8..347eefbd0dd 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2173,8 +2173,6 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig) target_resume (resume_ptid, step, sig); - target_commit_resume (); - if (target_can_async_p ()) target_async (1); } @@ -2761,28 +2759,208 @@ schedlock_applies (struct thread_info *tp) execution_direction))); } -/* Calls target_commit_resume on all targets. */ +/* Set process_stratum_target::COMMIT_RESUMED_STATE in all target + stacks that have threads executing and don't have threads with + pending events. */ static void -commit_resume_all_targets () +maybe_set_commit_resumed_all_targets () +{ + for (inferior *inf : all_non_exited_inferiors ()) + { + process_stratum_target *proc_target = inf->process_target (); + + if (proc_target->commit_resumed_state) + { + /* We already set this in a previous iteration, via another + inferior sharing the process_stratum target. */ + continue; + } + + /* If the target has no resumed threads, it would be useless to + ask it to commit the resumed threads. */ + if (!proc_target->threads_executing) + { + infrun_debug_printf ("not requesting commit-resumed for target " + "%s, no resumed threads", + proc_target->shortname ()); + continue; + } + + /* As an optimization, if a thread from this target has some + status to report, handle it before requiring the target to + commit its resumed threads: handling the status might lead to + resuming more threads. */ + bool has_thread_with_pending_status = false; + for (thread_info *thread : all_non_exited_threads (proc_target)) + if (thread->resumed && thread->suspend.waitstatus_pending_p) + { + has_thread_with_pending_status = true; + break; + } + + if (has_thread_with_pending_status) + { + infrun_debug_printf ("not requesting commit-resumed for target %s, a" + " thread has a pending waitstatus", + proc_target->shortname ()); + continue; + } + + infrun_debug_printf ("enabling commit-resumed for target %s", + proc_target->shortname ()); + + proc_target->commit_resumed_state = true; + } +} + +/* See infrun.h. */ + +void +maybe_call_commit_resumed_all_targets () { scoped_restore_current_thread restore_thread; - /* Map between process_target and a representative inferior. This - is to avoid committing a resume in the same target more than - once. Resumptions must be idempotent, so this is an - optimization. */ - std::unordered_map conn_inf; + for (inferior *inf : all_non_exited_inferiors ()) + { + process_stratum_target *proc_target = inf->process_target (); + + if (!proc_target->commit_resumed_state) + continue; + + switch_to_inferior_no_thread (inf); + + infrun_debug_printf ("calling commit_resumed for target %s", + proc_target->shortname()); + + target_commit_resumed (); + } +} + +/* To track nesting of scoped_disable_commit_resumed objects, ensuring + that only the outermost one attempts to re-enable + commit-resumed. */ +static bool enable_commit_resumed = true; + +/* See infrun.h. */ + +scoped_disable_commit_resumed::scoped_disable_commit_resumed + (const char *reason) + : m_reason (reason), + m_prev_enable_commit_resumed (enable_commit_resumed) +{ + infrun_debug_printf ("reason=%s", m_reason); + + enable_commit_resumed = false; for (inferior *inf : all_non_exited_inferiors ()) - if (inf->has_execution ()) - conn_inf[inf->process_target ()] = inf; + { + process_stratum_target *proc_target = inf->process_target (); + + if (m_prev_enable_commit_resumed) + { + /* This is the outermost instance: force all + COMMIT_RESUMED_STATE to false. */ + proc_target->commit_resumed_state = false; + } + else + { + /* This is not the outermost instance, we expect + COMMIT_RESUMED_STATE to have been cleared by the + outermost instance. */ + gdb_assert (!proc_target->commit_resumed_state); + } + } +} - for (const auto &ci : conn_inf) +/* See infrun.h. */ + +void +scoped_disable_commit_resumed::reset () +{ + if (m_reset) + return; + m_reset = true; + + infrun_debug_printf ("reason=%s", m_reason); + + gdb_assert (!enable_commit_resumed); + + enable_commit_resumed = m_prev_enable_commit_resumed; + + if (m_prev_enable_commit_resumed) { - inferior *inf = ci.second; - switch_to_inferior_no_thread (inf); - target_commit_resume (); + /* This is the outermost instance, re-enable + COMMIT_RESUMED_STATE on the targets where it's possible. */ + maybe_set_commit_resumed_all_targets (); + } + else + { + /* This is not the outermost instance, we expect + COMMIT_RESUMED_STATE to still be false. */ + for (inferior *inf : all_non_exited_inferiors ()) + { + process_stratum_target *proc_target = inf->process_target (); + gdb_assert (!proc_target->commit_resumed_state); + } + } +} + +/* See infrun.h. */ + +scoped_disable_commit_resumed::~scoped_disable_commit_resumed () +{ + reset (); +} + +/* See infrun.h. */ + +void +scoped_disable_commit_resumed::reset_and_commit () +{ + reset (); + maybe_call_commit_resumed_all_targets (); +} + +/* See infrun.h. */ + +scoped_enable_commit_resumed::scoped_enable_commit_resumed + (const char *reason) + : m_reason (reason), + m_prev_enable_commit_resumed (enable_commit_resumed) +{ + infrun_debug_printf ("reason=%s", m_reason); + + if (!enable_commit_resumed) + { + enable_commit_resumed = true; + + /* Re-enable COMMIT_RESUMED_STATE on the targets where it's + possible. */ + maybe_set_commit_resumed_all_targets (); + + maybe_call_commit_resumed_all_targets (); + } +} + +/* See infrun.h. */ + +scoped_enable_commit_resumed::~scoped_enable_commit_resumed () +{ + infrun_debug_printf ("reason=%s", m_reason); + + gdb_assert (enable_commit_resumed); + + enable_commit_resumed = m_prev_enable_commit_resumed; + + if (!enable_commit_resumed) + { + /* Force all COMMIT_RESUMED_STATE back to false. */ + for (inferior *inf : all_non_exited_inferiors ()) + { + process_stratum_target *proc_target = inf->process_target (); + proc_target->commit_resumed_state = false; + } } } @@ -3006,7 +3184,7 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) cur_thr->prev_pc = regcache_read_pc_protected (regcache); { - scoped_restore save_defer_tc = make_scoped_defer_target_commit_resume (); + scoped_disable_commit_resumed disable_commit_resumed ("proceeding"); started = start_step_over (); @@ -3074,9 +3252,9 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) if (!ecs->wait_some_more) error (_("Command aborted.")); } - } - commit_resume_all_targets (); + disable_commit_resumed.reset_and_commit (); + } finish_state.release (); @@ -3878,8 +4056,16 @@ fetch_inferior_event () = make_scoped_restore (&execution_direction, target_execution_direction ()); + /* Allow targets to pause their resumed threads while we handle + the event. */ + scoped_disable_commit_resumed disable_commit_resumed ("handling event"); + if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG)) - return; + { + infrun_debug_printf ("do_target_wait returned no event"); + disable_commit_resumed.reset_and_commit (); + return; + } gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE); @@ -3970,6 +4156,8 @@ fetch_inferior_event () /* No error, don't finish the thread states yet. */ finish_state.release (); + disable_commit_resumed.reset_and_commit (); + /* This scope is used to ensure that readline callbacks are reinstalled here. */ } diff --git a/gdb/infrun.h b/gdb/infrun.h index e643c84dd80..220ccc79e8b 100644 --- a/gdb/infrun.h +++ b/gdb/infrun.h @@ -273,4 +273,102 @@ extern void all_uis_on_sync_execution_starting (void); detach. */ extern void restart_after_all_stop_detach (process_stratum_target *proc_target); +/* RAII object to temporarily disable the requirement for target + stacks to commit their resumed threads. + + On construction, set process_stratum_target::commit_resumed_state + to false for all process_stratum targets in all target + stacks. + + On destruction (or if reset_and_commit() is called), set + process_stratum_target::commit_resumed_state to true for all + process_stratum targets in all target stacks, except those that: + + - have no resumed threads + - have a resumed thread with a pending status + + target_commit_resumed is not called in the destructor, because its + implementations could throw, and we don't to swallow that error in + a destructor. Instead, the caller should call the + reset_and_commit_resumed() method so that an eventual exception can + propagate. "reset" in the method name refers to the fact that this + method has the same effect as the destructor, in addition to + committing resumes. + + The creation of nested scoped_disable_commit_resumed objects is + tracked, such that only the outermost instance actually does + something, for cases like this: + + void + inner_func () + { + scoped_disable_commit_resumed disable; + + // do stuff + + disable.reset_and_commit (); + } + + void + outer_func () + { + scoped_disable_commit_resumed disable; + + for (... each thread ...) + inner_func (); + + disable.reset_and_commit (); + } + + In this case, we don't want the `disable` destructor in + `inner_func` to require targets to commit resumed threads, so that + the `reset_and_commit()` call in `inner_func` doesn't actually + resume threads. */ + +struct scoped_disable_commit_resumed +{ + explicit scoped_disable_commit_resumed (const char *reason); + ~scoped_disable_commit_resumed (); + + DISABLE_COPY_AND_ASSIGN (scoped_disable_commit_resumed); + + /* Undoes the disabling done by the ctor, and calls + maybe_call_commit_resumed_all_targets(). */ + void reset_and_commit (); + +private: + /* Undoes the disabling done by the ctor. */ + void reset (); + + /* Whether this object has been reset. */ + bool m_reset = false; + + const char *m_reason; + bool m_prev_enable_commit_resumed; +}; + +/* Call target_commit_resumed method on all target stacks whose + process_stratum target layer has COMMIT_RESUME_STATE set. */ + +extern void maybe_call_commit_resumed_all_targets (); + +/* RAII object to temporarily enable the requirement for target stacks + to commit their resumed threads. This is the inverse of + scoped_disable_commit_resumed. The constructor calls the + maybe_call_commit_resumed_all_targets function itself, since it's + OK to throw from a constructor. */ + +struct scoped_enable_commit_resumed +{ + explicit scoped_enable_commit_resumed (const char *reason); + ~scoped_enable_commit_resumed (); + + DISABLE_COPY_AND_ASSIGN (scoped_enable_commit_resumed); + +private: + const char *m_reason; + bool m_prev_enable_commit_resumed; +}; + + #endif /* INFRUN_H */ diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index c6f1ab4e3f0..988db558be1 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -266,6 +266,8 @@ exec_continue (char **argv, int argc) { prepare_execution_command (current_inferior ()->top_target (), mi_async_p ()); + scoped_disable_commit_resumed disable_commit_resumed ("mi continue"); + if (non_stop) { /* In non-stop mode, 'resume' always resumes a single thread. @@ -311,6 +313,8 @@ exec_continue (char **argv, int argc) continue_1 (1); } } + + disable_commit_resumed.reset_and_commit (); } static void diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h index b513c26ffc2..6fddbba90c7 100644 --- a/gdb/process-stratum-target.h +++ b/gdb/process-stratum-target.h @@ -72,6 +72,38 @@ public: /* The connection number. Visible in "info connections". */ int connection_number = 0; + + /* Whether resumed threads must be committed to the target. + + When true, resumed threads must be committed to the execution + target. + + When false, the target may leave resumed threads stopped when + it's convenient or efficient to do so. When the core requires + resumed threads to be committed again, this is set back to true + and calls the `commit_resumed` method to allow the target to do + so. + + To simplify the implementation of targets, the following methods + are guaranteed to be called with COMMIT_RESUMED_STATE set to + false: + + - resume + - stop + - wait + + Knowing this, the target doesn't need to implement different + behaviors depending on the COMMIT_RESUMED_STATE, and can simply + assume that it is false. + + Targets can take advantage of this to batch resumption requests, + for example. In that case, the target doesn't actually resume in + its `resume` implementation. Instead, it takes note of the + resumption intent in `resume` and defers the actual resumption to + `commit_resumed`. For example, the remote target uses this to + coalesce multiple resumption requests in a single vCont + packet. */ + bool commit_resumed_state = false; }; /* Downcast TARGET to process_stratum_target. */ diff --git a/gdb/record-full.c b/gdb/record-full.c index 8310b7b4c25..23cbdcb58c4 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -1242,11 +1242,11 @@ record_full_wait_1 (struct target_ops *ops, break; } + process_stratum_target *proc_target + = current_inferior ()->process_target (); + if (gdbarch_software_single_step_p (gdbarch)) { - process_stratum_target *proc_target - = current_inferior ()->process_target (); - /* Try to insert the software single step breakpoint. If insert success, set step to 0. */ set_executing (proc_target, inferior_ptid, false); @@ -1263,7 +1263,9 @@ record_full_wait_1 (struct target_ops *ops, "issuing one more step in the " "target beneath\n"); ops->beneath ()->resume (ptid, step, GDB_SIGNAL_0); - ops->beneath ()->commit_resume (); + proc_target->commit_resumed_state = true; + proc_target->commit_resumed (); + proc_target->commit_resumed_state = false; continue; } } diff --git a/gdb/remote.c b/gdb/remote.c index 188f507febd..1036ffd5cad 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -426,7 +426,7 @@ public: void detach (inferior *, int) override; void disconnect (const char *, int) override; - void commit_resume () override; + void commit_resumed () override; void resume (ptid_t, int, enum gdb_signal) override; ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override; @@ -6499,6 +6499,36 @@ get_remote_inferior (inferior *inf) return static_cast (inf->priv.get ()); } +struct stop_reply : public notif_event +{ + ~stop_reply (); + + /* The identifier of the thread about this event */ + ptid_t ptid; + + /* The remote state this event is associated with. When the remote + connection, represented by a remote_state object, is closed, + all the associated stop_reply events should be released. */ + struct remote_state *rs; + + struct target_waitstatus ws; + + /* The architecture associated with the expedited registers. */ + gdbarch *arch; + + /* Expedited registers. This makes remote debugging a bit more + efficient for those targets that provide critical registers as + part of their normal status mechanism (as another roundtrip to + fetch them is avoided). */ + std::vector regcache; + + enum target_stop_reason stop_reason; + + CORE_ADDR watch_data_address; + + int core; +}; + /* Class 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. */ @@ -6603,7 +6633,7 @@ vcont_builder::push_action (ptid_t ptid, bool step, gdb_signal siggnal) /* to_commit_resume implementation. */ void -remote_target::commit_resume () +remote_target::commit_resumed () { int any_process_wildcard; int may_global_wildcard_vcont; @@ -6678,6 +6708,8 @@ remote_target::commit_resume () disable process and global wildcard resumes appropriately. */ check_pending_events_prevent_wildcard_vcont (&may_global_wildcard_vcont); + bool any_pending_vcont_resume = false; + for (thread_info *tp : all_non_exited_threads (this)) { remote_thread_info *priv = get_remote_thread_info (tp); @@ -6694,6 +6726,9 @@ remote_target::commit_resume () continue; } + if (priv->get_resume_state () == resume_state::RESUMED_PENDING_VCONT) + any_pending_vcont_resume = true; + /* 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. */ @@ -6701,6 +6736,11 @@ remote_target::commit_resume () may_global_wildcard_vcont = 0; } + /* We didn't have any resumed thread pending a vCont resume, so nothing to + do. */ + if (!any_pending_vcont_resume) + return; + /* 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 @@ -6721,6 +6761,13 @@ remote_target::commit_resume () gdb_assert (!thread_is_in_step_over_chain (tp)); + /* We should never be commit-resuming a thread that has a stop reply. + Otherwise, we would end up reporting a stop event for a thread while + it is running on the remote target. */ + remote_state *rs = get_remote_state (); + for (const auto &stop_reply : rs->stop_reply_queue) + gdb_assert (stop_reply->ptid != tp->ptid); + const resumed_pending_vcont_info &info = remote_thr->resumed_pending_vcont_info (); @@ -6786,6 +6833,74 @@ remote_target::remote_stop_ns (ptid_t ptid) char *p = rs->buf.data (); char *endp = p + get_remote_packet_size (); + /* If any thread that needs to stop was resumed but pending a vCont + resume, generate a phony stop_reply. However, first check + whether the thread wasn't resumed with a signal. Generating a + phony stop in that case would result in losing the signal. */ + bool needs_commit = false; + for (thread_info *tp : all_non_exited_threads (this, ptid)) + { + remote_thread_info *remote_thr = get_remote_thread_info (tp); + + if (remote_thr->get_resume_state () + == resume_state::RESUMED_PENDING_VCONT) + { + const resumed_pending_vcont_info &info + = remote_thr->resumed_pending_vcont_info (); + if (info.sig != GDB_SIGNAL_0) + { + /* This signal must be forwarded to the inferior. We + could commit-resume just this thread, but its simpler + to just commit-resume everything. */ + needs_commit = true; + break; + } + } + } + + if (needs_commit) + commit_resumed (); + else + for (thread_info *tp : all_non_exited_threads (this, ptid)) + { + remote_thread_info *remote_thr = get_remote_thread_info (tp); + + if (remote_thr->get_resume_state () + == resume_state::RESUMED_PENDING_VCONT) + { + remote_debug_printf ("Enqueueing phony stop reply for thread pending " + "vCont-resume (%d, %ld, %ld)", tp->ptid.pid(), + tp->ptid.lwp (), tp->ptid.tid ()); + + /* Check that the thread wasn't resumed with a signal. + Generating a phony stop would result in losing the + signal. */ + const resumed_pending_vcont_info &info + = remote_thr->resumed_pending_vcont_info (); + gdb_assert (info.sig == GDB_SIGNAL_0); + + stop_reply *sr = new stop_reply (); + sr->ptid = tp->ptid; + sr->rs = rs; + sr->ws.kind = TARGET_WAITKIND_STOPPED; + sr->ws.value.sig = GDB_SIGNAL_0; + sr->arch = tp->inf->gdbarch; + sr->stop_reason = TARGET_STOPPED_BY_NO_REASON; + sr->watch_data_address = 0; + sr->core = 0; + this->push_stop_reply (sr); + + /* Pretend that this thread was actually resumed on the + remote target, then stopped. If we leave it in the + RESUMED_PENDING_VCONT state and the commit_resumed + method is called while the stop reply is still in the + queue, we'll end up reporting a stop event to the core + for that thread while it is running on the remote + target... that would be bad. */ + remote_thr->set_resumed (); + } + } + /* FIXME: This supports_vCont_probed check is a workaround until packet_support is per-connection. */ if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN @@ -6990,36 +7105,6 @@ remote_console_output (const char *msg) gdb_stdtarg->flush (); } -struct stop_reply : public notif_event -{ - ~stop_reply (); - - /* The identifier of the thread about this event */ - ptid_t ptid; - - /* The remote state this event is associated with. When the remote - connection, represented by a remote_state object, is closed, - all the associated stop_reply events should be released. */ - struct remote_state *rs; - - struct target_waitstatus ws; - - /* The architecture associated with the expedited registers. */ - gdbarch *arch; - - /* Expedited registers. This makes remote debugging a bit more - efficient for those targets that provide critical registers as - part of their normal status mechanism (as another roundtrip to - fetch them is avoided). */ - std::vector regcache; - - enum target_stop_reason stop_reason; - - CORE_ADDR watch_data_address; - - int core; -}; - /* Return the length of the stop reply queue. */ int diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 74103bfa3fd..efbb31be02d 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -14,7 +14,7 @@ struct dummy_target : public target_ops void detach (inferior *arg0, int arg1) override; void disconnect (const char *arg0, int arg1) override; void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override; - void commit_resume () override; + void commit_resumed () override; ptid_t wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) override; void fetch_registers (struct regcache *arg0, int arg1) override; void store_registers (struct regcache *arg0, int arg1) override; @@ -188,7 +188,7 @@ struct debug_target : public target_ops void detach (inferior *arg0, int arg1) override; void disconnect (const char *arg0, int arg1) override; void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override; - void commit_resume () override; + void commit_resumed () override; ptid_t wait (ptid_t arg0, struct target_waitstatus *arg1, target_wait_flags arg2) override; void fetch_registers (struct regcache *arg0, int arg1) override; void store_registers (struct regcache *arg0, int arg1) override; @@ -447,22 +447,22 @@ debug_target::resume (ptid_t arg0, int arg1, enum gdb_signal arg2) } void -target_ops::commit_resume () +target_ops::commit_resumed () { - this->beneath ()->commit_resume (); + this->beneath ()->commit_resumed (); } void -dummy_target::commit_resume () +dummy_target::commit_resumed () { } void -debug_target::commit_resume () +debug_target::commit_resumed () { - fprintf_unfiltered (gdb_stdlog, "-> %s->commit_resume (...)\n", this->beneath ()->shortname ()); - this->beneath ()->commit_resume (); - fprintf_unfiltered (gdb_stdlog, "<- %s->commit_resume (", this->beneath ()->shortname ()); + fprintf_unfiltered (gdb_stdlog, "-> %s->commit_resumed (...)\n", this->beneath ()->shortname ()); + this->beneath ()->commit_resumed (); + fprintf_unfiltered (gdb_stdlog, "<- %s->commit_resumed (", this->beneath ()->shortname ()); fputs_unfiltered (")\n", gdb_stdlog); } diff --git a/gdb/target.c b/gdb/target.c index 51832c52b6a..0da035e1a67 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2597,6 +2597,9 @@ target_wait (ptid_t ptid, struct target_waitstatus *status, target_wait_flags options) { target_ops *target = current_inferior ()->top_target (); + process_stratum_target *proc_target = current_inferior ()->process_target (); + + gdb_assert (!proc_target->commit_resumed_state); if (!target->can_async_p ()) gdb_assert ((options & TARGET_WNOHANG) == 0); @@ -2653,6 +2656,7 @@ void target_resume (ptid_t ptid, int step, enum gdb_signal signal) { process_stratum_target *curr_target = current_inferior ()->process_target (); + gdb_assert (!curr_target->commit_resumed_state); target_dcache_invalidate (); @@ -2666,26 +2670,13 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) clear_inline_frame_state (curr_target, ptid); } -/* If true, target_commit_resume is a nop. */ -static int defer_target_commit_resume; - /* See target.h. */ void -target_commit_resume (void) -{ - if (defer_target_commit_resume) - return; - - current_inferior ()->top_target ()->commit_resume (); -} - -/* See target.h. */ - -scoped_restore_tmpl -make_scoped_defer_target_commit_resume () +target_commit_resumed () { - return make_scoped_restore (&defer_target_commit_resume, 1); + gdb_assert (current_inferior ()->process_target ()->commit_resumed_state); + current_inferior ()->top_target ()->commit_resumed (); } void @@ -3761,6 +3752,10 @@ target_update_thread_list (void) void target_stop (ptid_t ptid) { + process_stratum_target *proc_target = current_inferior ()->process_target (); + + gdb_assert (!proc_target->commit_resumed_state); + if (!may_stop) { warning (_("May not interrupt or stop the target, ignoring attempt")); diff --git a/gdb/target.h b/gdb/target.h index 5d14edb9f4c..0aef372a7e7 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -485,8 +485,15 @@ struct target_ops int TARGET_DEBUG_PRINTER (target_debug_print_step), enum gdb_signal) TARGET_DEFAULT_NORETURN (noprocess ()); - virtual void commit_resume () + + /* Ensure that all resumed threads are committed to the target. + + See the description of + process_stratum_target::commit_resumed_state for more + details. */ + virtual void commit_resumed () TARGET_DEFAULT_IGNORE (); + /* See target_wait's description. Note that implementations of this method must not assume that inferior_ptid on entry is pointing at the thread or inferior that ends up reporting an @@ -1463,23 +1470,11 @@ extern void target_disconnect (const char *, int); 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 reactivate - target_commit_resume on destruction, if it was previously - active. */ -extern scoped_restore_tmpl make_scoped_defer_target_commit_resume (); +/* Ensure that all resumed threads are committed to the target. + + See the description of process_stratum_target::commit_resumed_state + for more details. */ +extern void target_commit_resumed (); /* For target_read_memory see target/target.h. */ diff --git a/gdb/top.c b/gdb/top.c index 3be95079654..31b751fa262 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -517,6 +517,13 @@ wait_sync_command_done (void) scoped_restore save_ui = make_scoped_restore (¤t_ui); struct ui *ui = current_ui; + /* We're about to wait until the target stops after having resumed + it so must force-commit resumptions, in case we're being called + in some context where a scoped_disable_commit_resumed object is + active. I.e., this function is a commit-resumed sync/flush + point. */ + scoped_enable_commit_resumed enable ("sync wait"); + while (gdb_do_one_event () >= 0) if (ui->prompt_state != PROMPT_BLOCKED) break; -- 2.30.2