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  <simon.marchi@efficios.com>
	    Pedro Alves  <pedro@palves.net>

	* 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
This commit is contained in:
Simon Marchi 2020-07-06 15:53:28 -04:00 committed by Pedro Alves
parent 1192f124a3
commit b4b1a226df
8 changed files with 108 additions and 0 deletions

View file

@ -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)
{