[PATCH v2 0/6] Fix inferior calls from breakpoint condition

[PATCH v2 0/6] Fix inferior calls from breakpoint condition

[Natalia Saiapova]

Hi,

This is the rebased on top of 

    51a948fdf0e gdb: Have allocate_target_description return a unique_ptr

version of
https://sourceware.org/pipermail/gdb-patches/2020-August/171592.html.

The patch set is tested on x86_64-pc-linux-gnu configuration, the default 
and gdbserver-native boards. 

Regards,
--Natalia

Natalia Saiapova:
Tankut Baris Aktemur:

  gdb: add in_cond_eval field to thread_control_state struct.
  gdb/infrun: in condition evaluation resume only current thread.
  gdb/infcall: in condition evaluation register target back after infcall.
  gdb/infrun: in condition evaluation wait only for the current inferior.
  gdb/infrun: in condition evaluation do not stop all threads.
  gdb/testsuite: add tests for inferior calls in breakpoint conditions.

 gdb/breakpoint.c                              |  3 +
 gdb/gdbthread.h                               |  3 +
 gdb/infcall.c                                 |  3 +
 gdb/infrun.c                                  | 35 +++++++--
 gdb/testsuite/gdb.threads/infcall-bp-cond.c   | 58 +++++++++++++++
 gdb/testsuite/gdb.threads/infcall-bp-cond.exp | 73 +++++++++++++++++++
 6 files changed, 169 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/infcall-bp-cond.c
 create mode 100644 gdb/testsuite/gdb.threads/infcall-bp-cond.exp

-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 1/6] gdb: add in_cond_eval field to thread_control_state struct.

[Natalia Saiapova]

This patch adds a new field to thread_control_state struct, which
is set to true when the thread is evaluating a breakpoint condition,
and false otherwise.

gdb/ChangeLog:
2020-08-27  Natalia Saiapova  <natalia.saiapova@intel.com>
	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

	* breakpoint.c (bpstat_check_breakpoint_conditions): Set/unset
	thread->control.in_cond_eval.
	* gdbthread.h (thread_control_state) <in_cond_eval>: New boolean field.

Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
---
 gdb/breakpoint.c | 3 +++
 gdb/gdbthread.h  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 296b08c0afc..77f927dc645 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5209,10 +5209,13 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread)
 	{
 	  try
 	    {
+	      thread->control.in_cond_eval = true;
 	      condition_result = breakpoint_cond_eval (cond);
+	      thread->control.in_cond_eval = false;
 	    }
 	  catch (const gdb_exception &ex)
 	    {
+	      thread->control.in_cond_eval = false;
 	      exception_fprintf (gdb_stderr, ex,
 				 "Error in testing breakpoint condition:\n");
 	    }
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index ab5771fdb47..9e93587f221 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -158,6 +158,9 @@ struct thread_control_state
      command.  This is used to decide whether "set scheduler-locking
      step" behaves like "on" or "off".  */
   int stepping_command = 0;
+
+  /* True if the thread is evaluating a BP condition.  */
+  bool in_cond_eval = false;
 };
 
 /* Inferior thread specific part of `struct infcall_suspend_state'.  */
-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 2/6] gdb/infrun: in condition evaluation resume only current thread.

[Natalia Saiapova]

When stopped at a conditional BP, GDB evaluates the condition for the
event thread in order to decide whether the thread should be stopped or
resumed.  Thus, if the condition includes an inferior call, during
the call not all threads have finalized state, some of them might
still be shown as running.  While executing the proceed for the inferior
call, GDB tries to resume all non-exited threads.  This leads to
the following assertion, when GDB tries to resume a running thread:

    (gdb) b 43 if foo()
    Breakpoint 1 at 0x119b: file main.c, line 43.
    (gdb) run
    Starting program: main
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    [New Thread 0x7ffff77c4700 (LWP 21421)]
    [New Thread 0x7ffff6fc3700 (LWP 21422)]
    gdb/nat/x86-linux-dregs.c:146: internal-error: void x86_linux_update_debug_registers(lwp_info*): Assertion `lwp_is_stopped (lwp)' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.

    This is a bug, please report it.  For instructions, see:
    <https://www.gnu.org/software/gdb/bugs/>.

This patch changes the behavior, so if the current thread is in the
middle of a condition evaluation, only the current thread is resumed.

gdb/ChangeLog:
2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

	* infrun.c (user_visible_resume_ptid): In BP condition
	evaluation, resume only the current thread

Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
---
 gdb/infrun.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d552fb3adb2..91271c2ddbe 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2156,6 +2156,14 @@ user_visible_resume_ptid (int step)
 	 mode.  */
       resume_ptid = inferior_ptid;
     }
+  else if (inferior_ptid != null_ptid
+	   && inferior_thread ()->control.in_cond_eval)
+    {
+      /* The inferior thread is evaluating a BP condition.  Other threads
+	 might be stopped or running and we do not want to change their
+	 state, thus, resume only the current thread.  */
+      resume_ptid = inferior_ptid;
+    }
   else if (!sched_multi && target_supports_multi_process ())
     {
       /* Resume all threads of the current process (and none of other
-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 3/6] gdb/infcall: in condition evaluation register target back after infcall.

[Natalia Saiapova]

After an inferior call is finished, the target is unregistered from
the event loop.  However, if the inferior call has happened during
the condition evaluation, we still want to get `stop` events from
other threads in `wait_one`.  So, register the target back.

gdb/ChangeLog:
2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

	* infcall.c (run_inferior_call): In condition evaluation,
	register the target back after the infcall.

Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
---
 gdb/infcall.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/infcall.c b/gdb/infcall.c
index 399b1724ea2..0b0226f8e82 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -667,6 +667,9 @@ run_inferior_call (struct call_thread_fsm *sm,
 
   call_thread->control.in_infcall = saved_in_infcall;
 
+  if (call_thread->control.in_cond_eval && target_can_async_p ())
+    target_async (true);
+
   return caught_error;
 }
 
-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the current inferior.

[Natalia Saiapova]

In the middle of a condition evaluation, to successfully finish the
inferior call we need to wait for events from the current thread.
Otherwise, a pending event from another thread might be taken and
the inferior call is abandoned.

gdb/ChangeLog:
2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

	* infrun.c (do_target_wait): Match an inferior PID with the
	expected PID.
	(fetch_inferior_event): In condition evaluation, wait for the
	event from the current inferior.

Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
---
 gdb/infrun.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 91271c2ddbe..d260eb6e3a7 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3565,10 +3565,11 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
      polling the rest of the inferior list starting from that one in a
      circular fashion until the whole list is polled once.  */
 
-  auto inferior_matches = [&wait_ptid] (inferior *inf)
+  ptid_t wait_pid_ptid {wait_ptid.pid ()};
+  auto inferior_matches = [&wait_pid_ptid] (inferior *inf)
     {
-      return (inf->process_target () != NULL
-	      && ptid_t (inf->pid).matches (wait_ptid));
+      return (inf->process_target () != nullptr
+	      && ptid_t (inf->pid).matches (wait_pid_ptid));
     };
 
   /* First see how many matching inferiors we have.  */
@@ -3906,7 +3907,15 @@ fetch_inferior_event ()
       = make_scoped_restore (&execution_direction,
 			     target_execution_direction ());
 
-    if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
+    ptid_t waiton_ptid = minus_one_ptid;
+
+    /* If the thread is in the middle of the condition evaluation,
+       wait for the event from the current inferior.  */
+    if (inferior_ptid != null_ptid
+	&& inferior_thread ()->control.in_cond_eval)
+      waiton_ptid = inferior_ptid;
+
+    if (!do_target_wait (waiton_ptid, ecs, TARGET_WNOHANG))
       return;
 
     gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 5/6] gdb/infrun: in condition evaluation do not stop all threads.

[Natalia Saiapova]

If a thread is evaluating a BP condition, do not stop other threads.
Some of them might be already resumed due to the condition being
evaluated as false.

gdb/ChangeLog:
2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

	* infrun.c (stop_waiting): In condition evaluation do not stop
	all threads.

Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
---
 gdb/infrun.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index d260eb6e3a7..286af5abbe1 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7764,9 +7764,15 @@ stop_waiting (struct execution_control_state *ecs)
   /* Let callers know we don't want to wait for the inferior anymore.  */
   ecs->wait_some_more = 0;
 
+  bool in_cond_eval = ecs->event_thread != nullptr
+    && ecs->event_thread->control.in_cond_eval;
+
   /* If all-stop, but there exists a non-stop target, stop all
-     threads now that we're presenting the stop to the user.  */
-  if (!non_stop && exists_non_stop_target ())
+     threads now that we're presenting the stop to the user.
+     Do not stop, if the event thread is evaluating a BP condition.
+     This will be handled elsewhere depending on the result of
+     the condition.  */
+  if (!non_stop && exists_non_stop_target () && !in_cond_eval)
     stop_all_threads ();
 }
 
-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v2 6/6] gdb/testsuite: add tests for inferior calls in breakpoint conditions.

[Natalia Saiapova]

Test three scenarios for breakpoints whose condition includes a function
call:
* only one thread matches the condition;
* both threads match the condition;
* no thread matches the condition.

gdb/testsuite/ChangeLog:
2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>

	* gdb.threads/infcall-bp-cond.c: New test.
	* gdb.threads/infcall-bp-cond.exp: New file.

Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
---
 gdb/testsuite/gdb.threads/infcall-bp-cond.c   | 58 +++++++++++++++
 gdb/testsuite/gdb.threads/infcall-bp-cond.exp | 73 +++++++++++++++++++
 2 files changed, 131 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/infcall-bp-cond.c
 create mode 100644 gdb/testsuite/gdb.threads/infcall-bp-cond.exp

diff --git a/gdb/testsuite/gdb.threads/infcall-bp-cond.c b/gdb/testsuite/gdb.threads/infcall-bp-cond.c
new file mode 100644
index 00000000000..dda2d3bb0e0
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/infcall-bp-cond.c
@@ -0,0 +1,58 @@
+/* Copyright 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+
+#define NUM_THREADS 2
+
+int
+is_one (int tid)
+{
+  return tid == 1;
+}
+
+int
+return_true ()
+{
+  return 1;
+}
+
+int
+return_false ()
+{
+  return 0;
+}
+
+void *
+doSmt (void *arg)
+{
+  int a = 42; /* breakpoint-here  */
+}
+
+int main(int argc, char *argv[])
+{
+  pthread_t threads[NUM_THREADS];
+  int args[NUM_THREADS];
+
+  for (int i = 0; i < NUM_THREADS; i++)
+    {
+      args[i] = i;
+      pthread_create (&threads[i], NULL, doSmt, &args[i]);
+    }
+
+  pthread_exit(NULL);
+}
diff --git a/gdb/testsuite/gdb.threads/infcall-bp-cond.exp b/gdb/testsuite/gdb.threads/infcall-bp-cond.exp
new file mode 100644
index 00000000000..c5abe2646f0
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/infcall-bp-cond.exp
@@ -0,0 +1,73 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Tests inferior calls executed from a breakpoint condition in
+# a multi-threaded program.
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${binfile} "${srcfile}" \
+	  {debug pthreads}] } {
+    return
+}
+
+set bp_line [gdb_get_line_number "breakpoint-here"]
+
+proc run_condition_test {message n_expected_hits condition} {
+    with_test_prefix $message {
+	global binfile gdb_prompt srcfile bp_line
+
+	clean_restart ${binfile}
+
+	if {![runto_main]} {
+	    fail "run to main"
+	    return
+	}
+
+	gdb_test "set \$n_cond_eval = 0"
+	gdb_breakpoint "$srcfile:$bp_line if (++\$n_cond_eval) $condition"
+
+	set n_hit_condition 0
+	set should_continue true
+	set iteration 0
+
+	while {$should_continue} {
+	    incr iteration
+	    with_test_prefix "continue iteration $iteration" {
+		gdb_test_multiple "continue" "continue" {
+		    -re "$srcfile:$bp_line.*$gdb_prompt" {
+			incr n_hit_condition
+		    }
+		    -re "$gdb_prompt" {
+			set should_continue false
+		    }
+		}
+	    }
+	}
+
+	set test_condition "expected number of hits"
+	if {$n_hit_condition == $n_expected_hits} {
+	    pass $test_condition
+	} else {
+	    fail $test_condition
+	}
+
+	gdb_test "print \$n_cond_eval" "= 2" "condition was evaluated twice"
+    }
+}
+
+run_condition_test "exactly one thread is hit" 1 "&& is_one (*(int*)arg)"
+run_condition_test "exactly two threads are hit" 2 "|| return_true ()"
+run_condition_test "no thread is hit" 0 "&& return_false ()"
-- 
2.17.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 0/6] Fix inferior calls from breakpoint condition

[Kevin Buettner]

Hi Natalia,

Thank you for rebasing and reposting your work.

I've successfully applied your v2 patches to recent GDB sources.  I've
built GDB with your patches applied and have done some a bit of testing.
All looks good so far.

I'll do a more thorough review in the coming week.

Thanks again,

Kevin

On Fri,  9 Oct 2020 13:27:12 +0200
Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> wrote:

> Hi,
> 
> This is the rebased on top of 
> 
>     51a948fdf0e gdb: Have allocate_target_description return a unique_ptr
> 
> version of
> https://sourceware.org/pipermail/gdb-patches/2020-August/171592.html.
> 
> The patch set is tested on x86_64-pc-linux-gnu configuration, the default 
> and gdbserver-native boards. 
> 
> Regards,
> --Natalia
> 
> Natalia Saiapova:
> Tankut Baris Aktemur:
> 
>   gdb: add in_cond_eval field to thread_control_state struct.
>   gdb/infrun: in condition evaluation resume only current thread.
>   gdb/infcall: in condition evaluation register target back after infcall.
>   gdb/infrun: in condition evaluation wait only for the current inferior.
>   gdb/infrun: in condition evaluation do not stop all threads.
>   gdb/testsuite: add tests for inferior calls in breakpoint conditions.
> 
>  gdb/breakpoint.c                              |  3 +
>  gdb/gdbthread.h                               |  3 +
>  gdb/infcall.c                                 |  3 +
>  gdb/infrun.c                                  | 35 +++++++--
>  gdb/testsuite/gdb.threads/infcall-bp-cond.c   | 58 +++++++++++++++
>  gdb/testsuite/gdb.threads/infcall-bp-cond.exp | 73 +++++++++++++++++++
>  6 files changed, 169 insertions(+), 6 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.threads/infcall-bp-cond.c
>  create mode 100644 gdb/testsuite/gdb.threads/infcall-bp-cond.exp
> 
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
> 

Re: [PATCH v2 3/6] gdb/infcall: in condition evaluation register target back after infcall.

[Kevin Buettner]

Hi Natalia,

Could you add your commit log comment (or something similar)
to your change to infcall.c?

Thanks,

Kevin

On Fri,  9 Oct 2020 13:27:15 +0200
Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> wrote:

> After an inferior call is finished, the target is unregistered from
> the event loop.  However, if the inferior call has happened during
> the condition evaluation, we still want to get `stop` events from
> other threads in `wait_one`.  So, register the target back.
> 
> gdb/ChangeLog:
> 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* infcall.c (run_inferior_call): In condition evaluation,
> 	register the target back after the infcall.
> 
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> ---
>  gdb/infcall.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 399b1724ea2..0b0226f8e82 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -667,6 +667,9 @@ run_inferior_call (struct call_thread_fsm *sm,
>  
>    call_thread->control.in_infcall = saved_in_infcall;
>  
> +  if (call_thread->control.in_cond_eval && target_can_async_p ())
> +    target_async (true);
> +
>    return caught_error;
>  }
>  
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
> 

Re: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the current inferior.

[Kevin Buettner]

Hi Natalia,

I have a question about this commit.  See below.

On Fri,  9 Oct 2020 13:27:16 +0200
Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> wrote:

> In the middle of a condition evaluation, to successfully finish the
> inferior call we need to wait for events from the current thread.
> Otherwise, a pending event from another thread might be taken and
> the inferior call is abandoned.
> 
> gdb/ChangeLog:
> 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* infrun.c (do_target_wait): Match an inferior PID with the
> 	expected PID.
> 	(fetch_inferior_event): In condition evaluation, wait for the
> 	event from the current inferior.
> 
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> ---
>  gdb/infrun.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 91271c2ddbe..d260eb6e3a7 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3565,10 +3565,11 @@ do_target_wait (ptid_t wait_ptid, execution_control_state *ecs,
>       polling the rest of the inferior list starting from that one in a
>       circular fashion until the whole list is polled once.  */
>  
> -  auto inferior_matches = [&wait_ptid] (inferior *inf)
> +  ptid_t wait_pid_ptid {wait_ptid.pid ()};
> +  auto inferior_matches = [&wait_pid_ptid] (inferior *inf)
>      {
> -      return (inf->process_target () != NULL
> -	      && ptid_t (inf->pid).matches (wait_ptid));
> +      return (inf->process_target () != nullptr
> +	      && ptid_t (inf->pid).matches (wait_pid_ptid));
>      };

I'm puzzled by the changes above.  Can you explain what this is
about?  (And perhaps add a comment...)

>  
>    /* First see how many matching inferiors we have.  */
> @@ -3906,7 +3907,15 @@ fetch_inferior_event ()
>        = make_scoped_restore (&execution_direction,
>  			     target_execution_direction ());
>  
> -    if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
> +    ptid_t waiton_ptid = minus_one_ptid;
> +
> +    /* If the thread is in the middle of the condition evaluation,
> +       wait for the event from the current inferior.  */
> +    if (inferior_ptid != null_ptid
> +	&& inferior_thread ()->control.in_cond_eval)
> +      waiton_ptid = inferior_ptid;
> +
> +    if (!do_target_wait (waiton_ptid, ecs, TARGET_WNOHANG))
>        return;
>  
>      gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
> 

RE: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the current inferior.

[Saiapova, Natalia]

Hi Kevin,

Thank you for looking into this! See my answer below.

Regards,
Natalia
> -----Original Message-----
> From: Kevin Buettner <kevinb@redhat.com>
> Sent: Monday, October 19, 2020 4:44 AM
> To: Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Saiapova, Natalia <natalia.saiapova@intel.com>
> Subject: Re: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the
> current inferior.
> 
> Hi Natalia,
> 
> I have a question about this commit.  See below.
> 
> On Fri,  9 Oct 2020 13:27:16 +0200
> Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> > In the middle of a condition evaluation, to successfully finish the
> > inferior call we need to wait for events from the current thread.
> > Otherwise, a pending event from another thread might be taken and
> > the inferior call is abandoned.
> >
> > gdb/ChangeLog:
> > 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> > 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> >
> > 	* infrun.c (do_target_wait): Match an inferior PID with the
> > 	expected PID.
> > 	(fetch_inferior_event): In condition evaluation, wait for the
> > 	event from the current inferior.
> >
> > Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> >
> > Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > ---
> >  gdb/infrun.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > index 91271c2ddbe..d260eb6e3a7 100644
> > --- a/gdb/infrun.c
> > +++ b/gdb/infrun.c
> > @@ -3565,10 +3565,11 @@ do_target_wait (ptid_t wait_ptid,
> execution_control_state *ecs,
> >       polling the rest of the inferior list starting from that one in a
> >       circular fashion until the whole list is polled once.  */
> >
> > -  auto inferior_matches = [&wait_ptid] (inferior *inf)
> > +  ptid_t wait_pid_ptid {wait_ptid.pid ()};
> > +  auto inferior_matches = [&wait_pid_ptid] (inferior *inf)
> >      {
> > -      return (inf->process_target () != NULL
> > -	      && ptid_t (inf->pid).matches (wait_ptid));
> > +      return (inf->process_target () != nullptr
> > +	      && ptid_t (inf->pid).matches (wait_pid_ptid));
> >      };
> 
> I'm puzzled by the changes above.  Can you explain what this is
> about?  (And perhaps add a comment...)

When we come here from the infcall inside the condition, the wait_ptid has a form {non-zero, non-zero, zero}, which corresponds to the thread started the infcall. So, when we construct a ptid via ptid_t (inf->pid) in inferior_matches function, it does not match the wait_ptid. That leads to  ecs->ws.kind = TARGET_WAITKIND_IGNORE and we are not getting the event. But we wait for an event from the thread which has started the infcall, so until we get one we are in an infinite loop. 

This code looks like the intention here is to find a matching inferior, so we are interested only in the PID part of the wait_ptid. Thus, I made the change. Does this make sense to you?

> 
> >
> >    /* First see how many matching inferiors we have.  */
> > @@ -3906,7 +3907,15 @@ fetch_inferior_event ()
> >        = make_scoped_restore (&execution_direction,
> >  			     target_execution_direction ());
> >
> > -    if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
> > +    ptid_t waiton_ptid = minus_one_ptid;
> > +
> > +    /* If the thread is in the middle of the condition evaluation,
> > +       wait for the event from the current inferior.  */
> > +    if (inferior_ptid != null_ptid
> > +	&& inferior_thread ()->control.in_cond_eval)
> > +      waiton_ptid = inferior_ptid;
> > +
> > +    if (!do_target_wait (waiton_ptid, ecs, TARGET_WNOHANG))
> >        return;
> >
> >      gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
> > --
> > 2.17.1
> >
> > Intel Deutschland GmbH
> > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> > Tel: +49 89 99 8853-0, www.intel.de
> > Managing Directors: Christin Eisenschmid, Gary Kershaw
> > Chairperson of the Supervisory Board: Nicole Lau
> > Registered Office: Munich
> > Commercial Register: Amtsgericht Muenchen HRB 186928
> >

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v2 1/6] gdb: add in_cond_eval field to thread_control_state struct.

[Andrew Burgess]

* Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> [2020-10-09 13:27:13 +0200]:

> This patch adds a new field to thread_control_state struct, which
> is set to true when the thread is evaluating a breakpoint condition,
> and false otherwise.
> 
> gdb/ChangeLog:
> 2020-08-27  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* breakpoint.c (bpstat_check_breakpoint_conditions): Set/unset
> 	thread->control.in_cond_eval.
> 	* gdbthread.h (thread_control_state) <in_cond_eval>: New boolean field.
> 
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> ---
>  gdb/breakpoint.c | 3 +++
>  gdb/gdbthread.h  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 296b08c0afc..77f927dc645 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5209,10 +5209,13 @@ bpstat_check_breakpoint_conditions (bpstat bs, thread_info *thread)
>  	{
>  	  try
>  	    {
> +	      thread->control.in_cond_eval = true;
>  	      condition_result = breakpoint_cond_eval (cond);
> +	      thread->control.in_cond_eval = false;
>  	    }
>  	  catch (const gdb_exception &ex)
>  	    {
> +	      thread->control.in_cond_eval = false;
>  	      exception_fprintf (gdb_stderr, ex,
>  				 "Error in testing breakpoint condition:\n");
>  	    }

Instead of these three writes, would you consider:

	  try
	    {
	      scoped_restore save_in_cond_eval
		= make_scoped_restore (&thread->control.in_cond_eval, true);

	      condition_result = breakpoint_cond_eval (cond);
	    }
	  catch (const gdb_exception &ex)
	    {
	      exception_fprintf (gdb_stderr, ex,
				 "Error in testing breakpoint condition:\n");
	    }

This should be identical in functionality, but seems more inline with
GDB style, using RAII to handle restoring.

Thanks,
Andrew



> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index ab5771fdb47..9e93587f221 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -158,6 +158,9 @@ struct thread_control_state
>       command.  This is used to decide whether "set scheduler-locking
>       step" behaves like "on" or "off".  */
>    int stepping_command = 0;
> +
> +  /* True if the thread is evaluating a BP condition.  */
> +  bool in_cond_eval = false;
>  };
>  
>  /* Inferior thread specific part of `struct infcall_suspend_state'.  */
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
> 

Re: [PATCH v2 2/6] gdb/infrun: in condition evaluation resume only current thread.

[Andrew Burgess]

* Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> [2020-10-09 13:27:14 +0200]:

> When stopped at a conditional BP, GDB evaluates the condition for the
> event thread in order to decide whether the thread should be stopped or
> resumed.  Thus, if the condition includes an inferior call, during
> the call not all threads have finalized state, some of them might
> still be shown as running.  While executing the proceed for the inferior
> call, GDB tries to resume all non-exited threads.  This leads to
> the following assertion, when GDB tries to resume a running thread:
> 
>     (gdb) b 43 if foo()
>     Breakpoint 1 at 0x119b: file main.c, line 43.
>     (gdb) run
>     Starting program: main
>     [Thread debugging using libthread_db enabled]
>     Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>     [New Thread 0x7ffff77c4700 (LWP 21421)]
>     [New Thread 0x7ffff6fc3700 (LWP 21422)]
>     gdb/nat/x86-linux-dregs.c:146: internal-error: void x86_linux_update_debug_registers(lwp_info*): Assertion `lwp_is_stopped (lwp)' failed.
>     A problem internal to GDB has been detected,
>     further debugging may prove unreliable.
> 
>     This is a bug, please report it.  For instructions, see:
>     <https://www.gnu.org/software/gdb/bugs/>.
> 
> This patch changes the behavior, so if the current thread is in the
> middle of a condition evaluation, only the current thread is resumed.
> 
> gdb/ChangeLog:
> 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* infrun.c (user_visible_resume_ptid): In BP condition
> 	evaluation, resume only the current thread

This patch is definitely going to need a test.

Maybe some of the tests you add in later patch(es) cover this case,
but ideally tests should be included in the patch that fixes the
issue.

The reason why bundling tests with the patch is good thing is that if
in the future I'm run into a problem with this patch then I can easily
(git blame) find this patch.  If the tests are included in the same
commit then I know that if I modify this code then these specific
tests must still work.

But, I hear you say, surely I run all tests when changing GDB?  Yes,
but as GDB changes it might be that the test you add now no longer
exercises this code, thus, removing, or changing, this code might no
longer cause the tests you add to fail.  At that point I'm stuck, this
code is basically untested.  If I have the tests included in this
commit then I can start to dig in to figure out why this code is no
longer needed for those tests to pass.

> 
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> ---
>  gdb/infrun.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index d552fb3adb2..91271c2ddbe 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2156,6 +2156,14 @@ user_visible_resume_ptid (int step)
>  	 mode.  */
>        resume_ptid = inferior_ptid;
>      }
> +  else if (inferior_ptid != null_ptid
> +	   && inferior_thread ()->control.in_cond_eval)
> +    {
> +      /* The inferior thread is evaluating a BP condition.  Other threads
> +	 might be stopped or running and we do not want to change their
> +	 state, thus, resume only the current thread.  */
> +      resume_ptid = inferior_ptid;
> +    }

I'm not sure about this.  We set threads going during an inferior call
so that any cross-thread interaction will not block.  At a minimum
this probably requires additional documentation.

I'd be interested to understand more about the different states that a
thread might be in here, and what conditions might put them into that
state.

If we assume a process with threads A, B, and C, then what happens is:

  1. Thread A bits conditional breakpoint, threads B & C are still
  running,

  2. GDB spots A has stopped and needs to execute the condition.

In this case we should only resume A as B & C are still running.  But,
we might also have:

  1. Thread A hits condition breakpoint, threads B & C are still
  running,

  2. Thread B hits condition breakpoint, thread C is still running,

  3. GDB spots A has stopped and needs to execute the condition.

In this case it's not clear what you would do... if you resume B to
evaluate A then you'll miss B's breakpoint hit.  But neither can you
evaluate B as that would require setting A running again.

Hmm, I think I've probably convinced myself that your approach might
be the only safe solution.

I think that I'd like to see the commit message expanded to give more
depth rather than just saying: "...some of them might still be shown
as running", I think you should say, what states a thread could be in
and why.  Also I think this probably is worth documenting in the
manual (that b/p conditions might be evaluated with some threads
stopped, even when running in all-stop mode).

Thanks,
Andrew

>    else if (!sched_multi && target_supports_multi_process ())
>      {
>        /* Resume all threads of the current process (and none of other
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
> 

Re: [PATCH v2 3/6] gdb/infcall: in condition evaluation register target back after infcall.

[Andrew Burgess]

* Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> [2020-10-09 13:27:15 +0200]:

> After an inferior call is finished, the target is unregistered from
> the event loop.  However, if the inferior call has happened during
> the condition evaluation, we still want to get `stop` events from
> other threads in `wait_one`.  So, register the target back.

My first question after reading this patch and the commit message is,
why don't we stop the unregistering for the case when we shouldn't
unregister.

There's probably a really good answer, and you probably have all the
knowledge in your head.... but the commit message is a little terse.

Could you explain why it's better to let GDB do the wrong thing and
then patch it up, instead of just doing to right thing all along?

Also, it feels like this change should have a test.

Thanks,
Andrew





> 
> gdb/ChangeLog:
> 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> 	* infcall.c (run_inferior_call): In condition evaluation,
> 	register the target back after the infcall.
> 
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> 
> Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> ---
>  gdb/infcall.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 399b1724ea2..0b0226f8e82 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -667,6 +667,9 @@ run_inferior_call (struct call_thread_fsm *sm,
>  
>    call_thread->control.in_infcall = saved_in_infcall;
>  
> +  if (call_thread->control.in_cond_eval && target_can_async_p ())
> +    target_async (true);
> +
>    return caught_error;
>  }
>  
> -- 
> 2.17.1
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
> 

Re: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the current inferior.

[Andrew Burgess]

* Saiapova, Natalia via Gdb-patches <gdb-patches@sourceware.org> [2020-10-26 12:11:06 +0000]:

> Hi Kevin,
> 
> Thank you for looking into this! See my answer below.
> 
> Regards,
> Natalia
> > -----Original Message-----
> > From: Kevin Buettner <kevinb@redhat.com>
> > Sent: Monday, October 19, 2020 4:44 AM
> > To: Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org>
> > Cc: Saiapova, Natalia <natalia.saiapova@intel.com>
> > Subject: Re: [PATCH v2 4/6] gdb/infrun: in condition evaluation wait only for the
> > current inferior.
> > 
> > Hi Natalia,
> > 
> > I have a question about this commit.  See below.
> > 
> > On Fri,  9 Oct 2020 13:27:16 +0200
> > Natalia Saiapova via Gdb-patches <gdb-patches@sourceware.org> wrote:
> > 
> > > In the middle of a condition evaluation, to successfully finish the
> > > inferior call we need to wait for events from the current thread.
> > > Otherwise, a pending event from another thread might be taken and
> > > the inferior call is abandoned.
> > >
> > > gdb/ChangeLog:
> > > 2020-08-26  Natalia Saiapova  <natalia.saiapova@intel.com>
> > > 	    Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> > >
> > > 	* infrun.c (do_target_wait): Match an inferior PID with the
> > > 	expected PID.
> > > 	(fetch_inferior_event): In condition evaluation, wait for the
> > > 	event from the current inferior.
> > >
> > > Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > > Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> > >
> > > Signed-off-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > > ---
> > >  gdb/infrun.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/gdb/infrun.c b/gdb/infrun.c
> > > index 91271c2ddbe..d260eb6e3a7 100644
> > > --- a/gdb/infrun.c
> > > +++ b/gdb/infrun.c
> > > @@ -3565,10 +3565,11 @@ do_target_wait (ptid_t wait_ptid,
> > execution_control_state *ecs,
> > >       polling the rest of the inferior list starting from that one in a
> > >       circular fashion until the whole list is polled once.  */
> > >
> > > -  auto inferior_matches = [&wait_ptid] (inferior *inf)
> > > +  ptid_t wait_pid_ptid {wait_ptid.pid ()};
> > > +  auto inferior_matches = [&wait_pid_ptid] (inferior *inf)
> > >      {
> > > -      return (inf->process_target () != NULL
> > > -	      && ptid_t (inf->pid).matches (wait_ptid));
> > > +      return (inf->process_target () != nullptr
> > > +	      && ptid_t (inf->pid).matches (wait_pid_ptid));
> > >      };
> > 
> > I'm puzzled by the changes above.  Can you explain what this is
> > about?  (And perhaps add a comment...)
> 
> When we come here from the infcall inside the condition, the
> wait_ptid has a form {non-zero, non-zero, zero}, which corresponds
> to the thread started the infcall. So, when we construct a ptid via
> ptid_t (inf->pid) in inferior_matches function, it does not match
> the wait_ptid. That leads to ecs->ws.kind = TARGET_WAITKIND_IGNORE
> and we are not getting the event. But we wait for an event from the
> thread which has started the infcall, so until we get one we are in
> an infinite loop.
> 
> This code looks like the intention here is to find a matching
> inferior, so we are interested only in the PID part of the
> wait_ptid. Thus, I made the change. Does this make sense to you?

This describes what, but not why.  You earlier change in
user_visible_resume_ptid, which I'm guessing relates to this change
returned the inferior_ptid.  But that's not the only path by which we
might return inferior_ptid.  So its not clear why this change runs
into problems, but other cases don't.

Also I agree with Kevin that this probably needs explaining with
comments in the code.

Thanks,
Andrew


> 
> > 
> > >
> > >    /* First see how many matching inferiors we have.  */
> > > @@ -3906,7 +3907,15 @@ fetch_inferior_event ()
> > >        = make_scoped_restore (&execution_direction,
> > >  			     target_execution_direction ());
> > >
> > > -    if (!do_target_wait (minus_one_ptid, ecs, TARGET_WNOHANG))
> > > +    ptid_t waiton_ptid = minus_one_ptid;
> > > +
> > > +    /* If the thread is in the middle of the condition evaluation,
> > > +       wait for the event from the current inferior.  */
> > > +    if (inferior_ptid != null_ptid
> > > +	&& inferior_thread ()->control.in_cond_eval)
> > > +      waiton_ptid = inferior_ptid;
> > > +
> > > +    if (!do_target_wait (waiton_ptid, ecs, TARGET_WNOHANG))
> > >        return;
> > >
> > >      gdb_assert (ecs->ws.kind != TARGET_WAITKIND_IGNORE);
> > > --
> > > 2.17.1
> > >
> > > Intel Deutschland GmbH
> > > Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> > > Tel: +49 89 99 8853-0, www.intel.de
> > > Managing Directors: Christin Eisenschmid, Gary Kershaw
> > > Chairperson of the Supervisory Board: Nicole Lau
> > > Registered Office: Munich
> > > Commercial Register: Amtsgericht Muenchen HRB 186928
> > >
> 
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Gary Kershaw
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
>