Re: [PATCH 3/3] gdb, gdbserver: detach fork child when detaching from fork parent

[Pedro Alves <pedro@palves.net>]

On 2021-11-24 20:04, Simon Marchi via Gdb-patches wrote:

> GDB-side, make the linux-nat and remote targets detach of fork children
> known because of pending fork events.  These pending fork events can be
> stored in thread_info::pending_waitstatus, if the core has consumed the
> event but then saved it for later.  They can also be stored, for the
> linux-nat target, in lwp_info::status and lwp_info::waitstatus.  

...

> For the
> remote target, I suppose that a pending fork event could maybe be in the
> stop reply queue, but I could generate a case where would could detach
> while having pending events there, so I didn't handle that case in this
> patch.

Yes, this can certainly happen.  I don't see a way to reliably stop the
program with an event held in the stop reply queue.  GDB constantly drains the
events asynchronously.  So for a testcase, I think you'd need to take
"constantly hammer" approach.  Something like:

 - enable non-stop / target non-stop 
 - make the test program have a few threads constantly fork and exit the child
   I say multiple threads so that the stop reply queue has a good chance of 
   holding an event unprocessed
 - make the test program call alarm() to set a timeout.  See why below.
 - make the testcase constantly attach / continue -a & / detach

Determining whether the fork child was detached correctly would be done by:

 - the parent/child sharing a pipe.  You'd have an array of pipes, one
   entry per thread.
 - the child writing to the pipe before exiting
 - the parent reading from the pipe after forking

If GDB fails to detach the child, then the parent thread hangs in
the pipe read, and eventually, the parent gets a SIGALRM.

To avoid the case of the test failing, then the parent dying with SIGALRM, and
GDB attaching to the wrong process (because the kernel reused the PID for
something else), you'd make the SIGALRM signal handler set a "timed_out"
global flag, and sleep for a while before exiting the process.  Enough time
so that GDB can always reattach before the process disappears due to SIGALRM.

When GDB reattaches, it first always consults that "timed_out" global, to
know whether the detach-from-children succeeded.  If it is set, the test
FAILs.

> There is a known limitation: we don't remove breakpoints from the
> children before detaching it.  So the children, could hit a trap
> instruction after being detached and crash.  I know this is wrong, and
> it should be fixed, but I would like to handle that later.  The current
> patch doesn't fix everything, but it's a step in the right direction.

*nod*

>  }
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index 7963f2faf267..2f3d5341758f 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1250,6 +1250,35 @@ handle_detach (char *own_buf)
>    /* We'll need this after PROCESS has been destroyed.  */
>    int pid = process->pid;
>  
> +  /* If this process has an unreported fork child, that child is not known to
> +     GDB, so detach it as well.
> +
> +     Here, we specifically don't want to use "safe iteration", as detaching
> +     another process might delete the next thread in the iteration, which is
> +     the one saved by the safe iterator.  We will never delete the currently
> +     iterated on thread, so standard iteration should be safe.  */
> +  for (thread_info *thread : all_threads)
> +    {
> +      /* Only threads that are of the process we are detaching.  */
> +      if (thread->id.pid () != pid)
> +	continue;
> +
> +      /* Only threads that have a pending fork event.  */
> +      if (thread->fork_child == nullptr)
> +	continue;
> +
> +      process_info *fork_child_process
> +	= find_process_pid (thread->fork_child->id.pid ());

This can be written as:

      process_info *fork_child_process
	= get_thread_process (thread->fork_child);

> +      gdb_assert (fork_child_process != nullptr);


Hmm, in my clone events work, I'm making clone events also set
the fork_parent/fork_child link, since clone events and fork/vfork
events are so similar.  In this spot however, we'll need to skip
detaching the child if the child is a clone child, not a fork child.
I wonder how best to check that.  See comments in the review of patch #1.

> +
> +      int fork_child_pid = fork_child_process->pid;
> +
> +      if (detach_inferior (fork_child_process) != 0)
> +	warning (_("Failed to detach fork child %s, child of %s"),
> +		 target_pid_to_str (ptid_t (fork_child_pid)).c_str (),
> +		 target_pid_to_str (thread->id).c_str ());
> +    }
> +
>    if (detach_inferior (process) != 0)
>      write_enn (own_buf);
>    else
>