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

[Simon Marchi <simon.marchi@polymtl.ca>]

> 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.

Ok, that sounds good (although not trivial!).

>>  }
>> 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);

Done.

> 
>> +      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.

Right.  In my new versions, the loop has:

      /* Only threads that have a pending fork event.  */
      thread_info *child = target_thread_pending_child (thread);
      if (child == nullptr)
	continue;

In your series, you'll make target_thread_pending_child return the kind,
and you can add a check for the kind.

Simon