gdb: fix b/p conditions with infcalls in multi-threaded inferiors

This commit fixes bug PR 28942, that is, creating a conditional
breakpoint in a multi-threaded inferior, where the breakpoint
condition includes an inferior function call.

Currently, when a user tries to create such a breakpoint, then GDB
will fail with:

  (gdb) break infcall-from-bp-cond-single.c:61 if (return_true ())
  Breakpoint 2 at 0x4011fa: file /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/infcall-from-bp-cond-single.c, line 61.
  (gdb) continue
  Continuing.
  [New Thread 0x7ffff7c5d700 (LWP 2460150)]
  [New Thread 0x7ffff745c700 (LWP 2460151)]
  [New Thread 0x7ffff6c5b700 (LWP 2460152)]
  [New Thread 0x7ffff645a700 (LWP 2460153)]
  [New Thread 0x7ffff5c59700 (LWP 2460154)]
  Error in testing breakpoint condition:
  Couldn't get registers: No such process.
  An error occurred while in a function called from GDB.
  Evaluation of the expression containing the function
  (return_true) will be abandoned.
  When the function is done executing, GDB will silently stop.
  Selected thread is running.
  (gdb)

Or, in some cases, like this:

  (gdb) break infcall-from-bp-cond-simple.c:56 if (is_matching_tid (arg, 1))
  Breakpoint 2 at 0x401194: file /tmp/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/infcall-from-bp-cond-simple.c, line 56.
  (gdb) continue
  Continuing.
  [New Thread 0x7ffff7c5d700 (LWP 2461106)]
  [New Thread 0x7ffff745c700 (LWP 2461107)]
  ../../src.release/gdb/nat/x86-linux-dregs.c:146: internal-error: x86_linux_update_debug_registers: Assertion `lwp_is_stopped (lwp)' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.

The precise error depends on the exact thread state; so there's race
conditions depending on which threads have fully started, and which
have not.  But the underlying problem is always the same; when GDB
tries to execute the inferior function call from within the breakpoint
condition, GDB will, incorrectly, try to resume threads that are
already running - GDB doesn't realise that some threads might already
be running.

The solution proposed in this patch requires an additional member
variable thread_info::in_cond_eval.  This flag is set to true (in
breakpoint.c) when GDB is evaluating a breakpoint condition.

In user_visible_resume_ptid (infrun.c), when the in_cond_eval flag is
true, then GDB will only try to resume the current thread, that is,
the thread for which the breakpoint condition is being evaluated.
This solves the problem of GDB trying to resume threads that are
already running.

The next problem is that inferior function calls are assumed to be
synchronous, that is, GDB doesn't expect to start an inferior function
call in thread #1, then receive a stop from thread #2 for some other,
unrelated reason.  To prevent GDB responding to an event from another
thread, we update fetch_inferior_event and do_target_wait in infrun.c,
so that, when an inferior function call (on behalf of a breakpoint
condition) is in progress, we only wait for events from the current
thread (the one evaluating the condition).

In do_target_wait I had to change the inferior_matches lambda
function, which is used to select which inferior to wait on.
Previously the logic was this:

   auto inferior_matches = [&wait_ptid] (inferior *inf)
     {
       return (inf->process_target () != nullptr
               && ptid_t (inf->pid).matches (wait_ptid));
     };

This compares the pid of the inferior against the complete ptid we
want to wait on.  Before this commit wait_ptid was only ever
minus_one_ptid (which is special, and means any process), and so every
inferior would match.

After this commit though wait_ptid might represent a specific thread
in a specific inferior.  If we compare the pid of the inferior to a
specific ptid then these will not match.  The fix is to compare
against the pid extracted from the wait_ptid, not against the complete
wait_ptid itself.

In fetch_inferior_event, after receiving the event, we only want to
stop all the other threads, and call inferior_event_handler with
INF_EXEC_COMPLETE, if we are not evaluating a conditional breakpoint.
If we are, then all the other threads should be left doing whatever
they were before.  The inferior_event_handler call will be performed
once the breakpoint condition has finished being evaluated, and GDB
decides to stop or not.

The final problem that needs solving relates to GDB's commit-resume
mechanism, which allows GDB to collect resume requests into a single
packet in order to reduce traffic to a remote target.

The problem is that the commit-resume mechanism will not send any
resume requests for an inferior if there are already events pending on
the GDB side.

Imagine an inferior with two threads.  Both threads hit a breakpoint,
maybe the same conditional breakpoint.  At this point there are two
pending events, one for each thread.

GDB selects one of the events and spots that this is a conditional
breakpoint, GDB evaluates the condition.

The condition includes an inferior function call, so GDB sets up for
the call and resumes the one thread, the resume request is added to
the commit-resume queue.

When the commit-resume queue is committed GDB sees that there is a
pending event from another thread, and so doesn't send any resume
requests to the actual target, GDB is assuming that when we wait we
will select the event from the other thread.

However, as this is an inferior function call for a condition
evaluation, we will not select the event from the other thread, we
only care about events from the thread that is evaluating the
condition - and the resume for this thread was never sent to the
target.

And so, GDB hangs, waiting for an event from a thread that was never
fully resumed.

To fix this issue I have added the concept of "forcing" the
commit-resume queue.  When enabling commit resume, if the force flag
is true, then any resumes will be committed to the target, even if
there are other threads with pending events.

A note on authorship: this patch was based on some work done by
Natalia Saiapova and Tankut Baris Aktemur from Intel[1].  I have made
some changes to their work in this version.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28942

[1] https://sourceware.org/pipermail/gdb-patches/2020-October/172454.html

Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
Tested-By: Luis Machado <luis.machado@arm.com>
Tested-By: Keith Seitz <keiths@redhat.com>

1 files changed, 2 insertions, 1 deletions

diff --git a/gdb/infrun.h b/gdb/infrun.h
index 6339fd9..5f83ca2 100644
--- a/gdb/infrun.h
+++ b/gdb/infrun.h
@@ -406,7 +406,8 @@ extern void maybe_call_commit_resumed_all_targets ();
 
 struct scoped_enable_commit_resumed
 {
-  explicit scoped_enable_commit_resumed (const char *reason);
+  explicit scoped_enable_commit_resumed (const char *reason,
+					 bool force_p = false);
   ~scoped_enable_commit_resumed ();
 
   DISABLE_COPY_AND_ASSIGN (scoped_enable_commit_resumed);