From b4b1a226df8d51da9594200ad803ad303c15cd36 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Mon, 6 Jul 2020 15:53:28 -0400 Subject: [PATCH] gdb: defer commit resume until all available events are consumed Rationale --------- Let's say you have multiple threads hitting a conditional breakpoint at the same time, and all of these are going to evaluate to false. All these threads will need to be resumed. Currently, GDB fetches one target event (one SIGTRAP representing the breakpoint hit) and decides that the thread should be resumed. It calls resume and commit_resume immediately. It then fetches the second target event, and does the same, until it went through all threads. The result is therefore something like: - consume event for thread A - resume thread A - commit resume (affects thread A) - consume event for thread B - resume thread B - commit resume (affects thread B) - consume event for thread C - resume thread C - commit resume (affects thread C) For targets where it's beneficial to group resumptions requests (most likely those that implement target_ops::commit_resume), it would be much better to have: - consume event for thread A - resume thread A - consume event for thread B - resume thread B - consume event for thread C - resume thread C - commit resume (affects threads A, B and C) Implementation details ---------------------- To achieve this, this patch adds another check in maybe_set_commit_resumed_all_targets to avoid setting the commit-resumed flag of targets that readily have events to provide to infrun. To determine if a target has events readily available to report, this patch adds an `has_pending_events` target_ops method. The method returns a simple bool to say whether or not it has pending events to report. Testing ======= To test this, I start GDBserver with a program that spawns multiple threads: $ ../gdbserver/gdbserver --once :1234 ~/src/many-threads-stepping-over-breakpoints/many-threads-stepping-over-breakpoints I then connect with GDB and install a conditional breakpoint that always evaluates to false (and force the evaluation to be done by GDB): $ ./gdb -nx --data-directory=data-directory \ /home/simark/src/many-threads-stepping-over-breakpoints/many-threads-stepping-over-breakpoints \ -ex "set breakpoint condition-evaluation host" \ -ex "set pag off" \ -ex "set confirm off" \ -ex "maint set target-non-stop on" \ -ex "tar rem :1234" \ -ex "tb main" \ -ex "b 13 if 0" \ -ex c \ -ex "set debug infrun" \ -ex "set debug remote 1" \ -ex "set debug displaced" I then do "continue" and look at the log. The remote target receives a bunch of stop notifications for all threads that have hit the breakpoint. infrun consumes and processes one event, decides it should not cause a stop, prepares a displaced step, after which we should see: [infrun] maybe_set_commit_resumed_all_process_targets: not requesting commit-resumed for target remote, target has pending events Same for a second thread (since we have 2 displaced step buffers). For the following threads, their displaced step is deferred since there are no more buffers available. After consuming the last event the remote target has to offer, we get: [infrun] maybe_set_commit_resumed_all_process_targets: enabling commit-resumed for target remote [infrun] maybe_call_commit_resumed_all_process_targets: calling commit_resumed for target remote [remote] Sending packet: $vCont;s:p14d16b.14d1b1;s:p14d16b.14d1b2#55 [remote] Packet received: OK Without the patch, there would have been one vCont;s just after each prepared displaced step. gdb/ChangeLog: yyyy-mm-dd Simon Marchi Pedro Alves * async-event.c (async_event_handler_marked): New. * async-event.h (async_event_handler_marked): Declare. * infrun.c (maybe_set_commit_resumed_all_targets): Switch to inferior before calling target method. Don't commit-resumed if target_has_pending_events is true. * remote.c (remote_target::has_pending_events): New. * target-delegates.c: Regenerate. * target.c (target_has_pending_events): New. * target.h (target_ops::has_pending_events): New target method. (target_has_pending_events): New. Change-Id: I18112ba19a1ff4986530c660f530d847bb4a1f1d --- gdb/ChangeLog | 15 +++++++++++++++ gdb/async-event.c | 8 ++++++++ gdb/async-event.h | 3 +++ gdb/infrun.c | 12 ++++++++++++ gdb/remote.c | 21 +++++++++++++++++++++ gdb/target-delegates.c | 27 +++++++++++++++++++++++++++ gdb/target.c | 8 ++++++++ gdb/target.h | 14 ++++++++++++++ 8 files changed, 108 insertions(+) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index e3ae1c4df75..fa4d5b7d24c 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,18 @@ +2021-03-26 Simon Marchi + Pedro Alves + + * async-event.c: Include "infrun.h". + (async_event_handler_marked): New. + * async-event.h (async_event_handler_marked): Declare. + * infrun.c (maybe_set_commit_resumed_all_targets): Switch to + inferior before calling target method. Don't commit-resumed if + target_has_pending_events is true. + * remote.c (remote_target::has_pending_events): New. + * target-delegates.c: Regenerate. + * target.c (target_has_pending_events): New. + * target.h (target_ops::has_pending_events): New target method. + (target_has_pending_events): New. + 2021-03-26 Simon Marchi Pedro Alves diff --git a/gdb/async-event.c b/gdb/async-event.c index c4ab7318179..7b1abfe65f8 100644 --- a/gdb/async-event.c +++ b/gdb/async-event.c @@ -308,6 +308,14 @@ clear_async_event_handler (async_event_handler *async_handler_ptr) async_handler_ptr->ready = 0; } +/* See event-loop.h. */ + +bool +async_event_handler_marked (async_event_handler *handler) +{ + return handler->ready; +} + /* Check if asynchronous event handlers are ready, and call the handler function for one that is. */ diff --git a/gdb/async-event.h b/gdb/async-event.h index 9d96235b38d..47759d5c2d3 100644 --- a/gdb/async-event.h +++ b/gdb/async-event.h @@ -78,6 +78,9 @@ extern void loop. */ extern void mark_async_event_handler (struct async_event_handler *handler); +/* Return true if HANDLER is marked. */ +extern bool async_event_handler_marked (async_event_handler *handler); + /* Mark the handler (ASYNC_HANDLER_PTR) as NOT ready. */ extern void clear_async_event_handler (struct async_event_handler *handler); diff --git a/gdb/infrun.c b/gdb/infrun.c index 347eefbd0dd..6176fa94fe3 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -2766,6 +2766,8 @@ schedlock_applies (struct thread_info *tp) static void maybe_set_commit_resumed_all_targets () { + scoped_restore_current_thread restore_thread; + for (inferior *inf : all_non_exited_inferiors ()) { process_stratum_target *proc_target = inf->process_target (); @@ -2807,6 +2809,16 @@ maybe_set_commit_resumed_all_targets () continue; } + switch_to_inferior_no_thread (inf); + + if (target_has_pending_events ()) + { + infrun_debug_printf ("not requesting commit-resumed for target %s, " + "target has pending events", + proc_target->shortname ()); + continue; + } + infrun_debug_printf ("enabling commit-resumed for target %s", proc_target->shortname ()); diff --git a/gdb/remote.c b/gdb/remote.c index 1036ffd5cad..fd0ad9c74cb 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -429,6 +429,7 @@ public: 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; + bool has_pending_events () override; void fetch_registers (struct regcache *, int) override; void store_registers (struct regcache *, int) override; @@ -6820,6 +6821,26 @@ remote_target::commit_resumed () vcont_builder.flush (); } +/* Implementation of target_has_pending_events. */ + +bool +remote_target::has_pending_events () +{ + if (target_can_async_p ()) + { + remote_state *rs = get_remote_state (); + + if (async_event_handler_marked (rs->remote_async_inferior_event_token)) + return true; + + /* Note that BUFCNT can be negative, indicating sticky + error. */ + if (rs->remote_desc->bufcnt != 0) + return true; + } + return false; +} + /* Non-stop version of target_stop. Uses `vCont;t' to stop a remote diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index efbb31be02d..cc8c64a4f1e 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -84,6 +84,7 @@ struct dummy_target : public target_ops bool is_async_p () override; void async (int arg0) override; int async_wait_fd () override; + bool has_pending_events () override; void thread_events (int arg0) override; bool supports_non_stop () override; bool always_non_stop_p () override; @@ -258,6 +259,7 @@ struct debug_target : public target_ops bool is_async_p () override; void async (int arg0) override; int async_wait_fd () override; + bool has_pending_events () override; void thread_events (int arg0) override; bool supports_non_stop () override; bool always_non_stop_p () override; @@ -2199,6 +2201,31 @@ debug_target::async_wait_fd () return result; } +bool +target_ops::has_pending_events () +{ + return this->beneath ()->has_pending_events (); +} + +bool +dummy_target::has_pending_events () +{ + return false; +} + +bool +debug_target::has_pending_events () +{ + bool result; + fprintf_unfiltered (gdb_stdlog, "-> %s->has_pending_events (...)\n", this->beneath ()->shortname ()); + result = this->beneath ()->has_pending_events (); + fprintf_unfiltered (gdb_stdlog, "<- %s->has_pending_events (", this->beneath ()->shortname ()); + fputs_unfiltered (") = ", gdb_stdlog); + target_debug_print_bool (result); + fputs_unfiltered ("\n", gdb_stdlog); + return result; +} + void target_ops::thread_events (int arg0) { diff --git a/gdb/target.c b/gdb/target.c index 0da035e1a67..995e7ef1dac 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2679,6 +2679,14 @@ target_commit_resumed () current_inferior ()->top_target ()->commit_resumed (); } +/* See target.h. */ + +bool +target_has_pending_events () +{ + return current_inferior ()->top_target ()->has_pending_events (); +} + void target_pass_signals (gdb::array_view pass_signals) { diff --git a/gdb/target.h b/gdb/target.h index 0aef372a7e7..adae49dc329 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -719,6 +719,15 @@ struct target_ops TARGET_DEFAULT_NORETURN (tcomplain ()); virtual int async_wait_fd () TARGET_DEFAULT_NORETURN (noprocess ()); + /* Return true if the target has pending events to report to the + core. If true, then GDB avoids resuming the target until all + pending events are consumed, so that multiple resumptions can + be coalesced as an optimization. Most targets can't tell + whether they have pending events without calling target_wait, + so we default to returning false. The only downside is that a + potential optimization is missed. */ + virtual bool has_pending_events () + TARGET_DEFAULT_RETURN (false); virtual void thread_events (int) TARGET_DEFAULT_IGNORE (); /* This method must be implemented in some situations. See the @@ -1485,6 +1494,11 @@ extern ptid_t default_target_wait (struct target_ops *ops, struct target_waitstatus *status, target_wait_flags options); +/* Return true if the target has pending events to report to the core. + See target_ops::has_pending_events(). */ + +extern bool target_has_pending_events (); + /* Fetch at least register REGNO, or all regs if regno == -1. No result. */ extern void target_fetch_registers (struct regcache *regcache, int regno); -- 2.30.2