[PATCH] gdb: iterate only on vfork parent threads in handle_vfork_child_exec_or_exit

[PATCH] gdb: iterate only on vfork parent threads in handle_vfork_child_exec_or_exit

[Simon Marchi]

I spotted what I think is a buglet in proceed_after_vfork_done.  After a
vfork child exits or execs, we resume all the threads of the parent.  To
do so, we iterate on all threads using iterate_over_threads with the
proceed_after_vfork_done callback.  Each thread is resumed if the
following condition is true:

    if (thread->ptid.pid () == pid
	&& thread->state == THREAD_RUNNING
	&& !thread->executing
	&& !thread->stop_requested
	&& thread->stop_signal () == GDB_SIGNAL_0)

where `pid` is the pid of the vfork parent.  This is not multi-target
aware: since it only filters on pid, if there is an inferior with the
same pid in another target, we could end up resuming a thread of that
other inferior.  The chances of the stars aligning for this to happen
are tiny, but still.

Fix that by iterating only on the vfork parent's threads, instead of on
all threads.  This is more efficient, as we iterate on just the required
threads (inferiors have their own thread list), and we can drop the pid
check.  The resulting code is also more straightforward in my opinion,
so it's a win-win.

Change-Id: I14647da72e2bf65592e82fbe6efb77a413a4be3a
---
 gdb/infrun.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 8870f82b9cc3..5ee650fa4645 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -855,17 +855,13 @@ follow_inferior_reset_breakpoints (void)
   insert_breakpoints ();
 }
 
-/* The child has exited or execed: resume threads of the parent the
-   user wanted to be executing.  */
+/* The child has exited or execed: resume THREAD, a thread of the parent,
+   if it was meant to be executing.  */
 
-static int
-proceed_after_vfork_done (struct thread_info *thread,
-			  void *arg)
+static void
+proceed_after_vfork_done (thread_info *thread)
 {
-  int pid = * (int *) arg;
-
-  if (thread->ptid.pid () == pid
-      && thread->state == THREAD_RUNNING
+  if (thread->state == THREAD_RUNNING
       && !thread->executing
       && !thread->stop_requested
       && thread->stop_signal () == GDB_SIGNAL_0)
@@ -877,8 +873,6 @@ proceed_after_vfork_done (struct thread_info *thread,
       clear_proceed_status (0);
       proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
     }
-
-  return 0;
 }
 
 /* Called whenever we notice an exec or exit event, to handle
@@ -891,7 +885,7 @@ handle_vfork_child_exec_or_exit (int exec)
 
   if (inf->vfork_parent)
     {
-      int resume_parent = -1;
+      inferior *resume_parent = nullptr;
 
       /* This exec or exit marks the end of the shared memory region
 	 between the parent and the child.  Break the bonds.  */
@@ -969,7 +963,7 @@ handle_vfork_child_exec_or_exit (int exec)
 	  inf->removable = 1;
 	  set_current_program_space (inf->pspace);
 
-	  resume_parent = vfork_parent->pid;
+	  resume_parent = vfork_parent;
 	}
       else
 	{
@@ -995,21 +989,22 @@ handle_vfork_child_exec_or_exit (int exec)
 	  inf->symfile_flags = SYMFILE_NO_READ;
 	  clone_program_space (inf->pspace, vfork_parent->pspace);
 
-	  resume_parent = vfork_parent->pid;
+	  resume_parent = vfork_parent;
 	}
 
       gdb_assert (current_program_space == inf->pspace);
 
-      if (non_stop && resume_parent != -1)
+      if (non_stop && resume_parent != nullptr)
 	{
 	  /* If the user wanted the parent to be running, let it go
 	     free now.  */
 	  scoped_restore_current_thread restore_thread;
 
 	  infrun_debug_printf ("resuming vfork parent process %d",
-			       resume_parent);
+			       resume_parent->pid);
 
-	  iterate_over_threads (proceed_after_vfork_done, &resume_parent);
+	  for (thread_info *thread : resume_parent->threads ())
+	    proceed_after_vfork_done (thread);
 	}
     }
 }
-- 
2.32.0

Re: [PATCH] gdb: iterate only on vfork parent threads in handle_vfork_child_exec_or_exit

[John Baldwin]

On 8/5/21 9:21 AM, Simon Marchi via Gdb-patches wrote:
> I spotted what I think is a buglet in proceed_after_vfork_done.  After a
> vfork child exits or execs, we resume all the threads of the parent.  To
> do so, we iterate on all threads using iterate_over_threads with the
> proceed_after_vfork_done callback.  Each thread is resumed if the
> following condition is true:
> 
>      if (thread->ptid.pid () == pid
> 	&& thread->state == THREAD_RUNNING
> 	&& !thread->executing
> 	&& !thread->stop_requested
> 	&& thread->stop_signal () == GDB_SIGNAL_0)
> 
> where `pid` is the pid of the vfork parent.  This is not multi-target
> aware: since it only filters on pid, if there is an inferior with the
> same pid in another target, we could end up resuming a thread of that
> other inferior.  The chances of the stars aligning for this to happen
> are tiny, but still.
> 
> Fix that by iterating only on the vfork parent's threads, instead of on
> all threads.  This is more efficient, as we iterate on just the required
> threads (inferiors have their own thread list), and we can drop the pid
> check.  The resulting code is also more straightforward in my opinion,
> so it's a win-win.

This looks sensible to me.

-- 
John Baldwin

Re: [PATCH] gdb: iterate only on vfork parent threads in handle_vfork_child_exec_or_exit

[Pedro Alves]

On 2021-08-05 5:21 p.m., Simon Marchi via Gdb-patches wrote:
> I spotted what I think is a buglet in proceed_after_vfork_done.  After a
> vfork child exits or execs, we resume all the threads of the parent.  To
> do so, we iterate on all threads using iterate_over_threads with the
> proceed_after_vfork_done callback.  Each thread is resumed if the
> following condition is true:
> 
>     if (thread->ptid.pid () == pid
> 	&& thread->state == THREAD_RUNNING
> 	&& !thread->executing
> 	&& !thread->stop_requested
> 	&& thread->stop_signal () == GDB_SIGNAL_0)
> 
> where `pid` is the pid of the vfork parent.  This is not multi-target
> aware: since it only filters on pid, if there is an inferior with the
> same pid in another target, we could end up resuming a thread of that
> other inferior.  The chances of the stars aligning for this to happen
> are tiny, but still.
> 
> Fix that by iterating only on the vfork parent's threads, instead of on
> all threads.  This is more efficient, as we iterate on just the required
> threads (inferiors have their own thread list), and we can drop the pid
> check.  The resulting code is also more straightforward in my opinion,
> so it's a win-win.
> 
> Change-Id: I14647da72e2bf65592e82fbe6efb77a413a4be3a

LGTM.

Re: [PATCH] gdb: iterate only on vfork parent threads in handle_vfork_child_exec_or_exit

[Simon Marchi]

On 2021-08-05 1:08 p.m., John Baldwin wrote:
> On 8/5/21 9:21 AM, Simon Marchi via Gdb-patches wrote:
>> I spotted what I think is a buglet in proceed_after_vfork_done.  After a
>> vfork child exits or execs, we resume all the threads of the parent.  To
>> do so, we iterate on all threads using iterate_over_threads with the
>> proceed_after_vfork_done callback.  Each thread is resumed if the
>> following condition is true:
>>
>>      if (thread->ptid.pid () == pid
>>     && thread->state == THREAD_RUNNING
>>     && !thread->executing
>>     && !thread->stop_requested
>>     && thread->stop_signal () == GDB_SIGNAL_0)
>>
>> where `pid` is the pid of the vfork parent.  This is not multi-target
>> aware: since it only filters on pid, if there is an inferior with the
>> same pid in another target, we could end up resuming a thread of that
>> other inferior.  The chances of the stars aligning for this to happen
>> are tiny, but still.
>>
>> Fix that by iterating only on the vfork parent's threads, instead of on
>> all threads.  This is more efficient, as we iterate on just the required
>> threads (inferiors have their own thread list), and we can drop the pid
>> check.  The resulting code is also more straightforward in my opinion,
>> so it's a win-win.
> 
> This looks sensible to me.

Thanks to you and Pedro, pushed.

Simon