Re: [FYI/pushed v4 08/25] Thread options & clone events (Linux GDBserver)

[Pedro Alves <pedro@palves.net>]

On 2024-02-08 08:57, Luis Machado wrote:
> Hi!
> 
> On 2/7/24 20:11, Pedro Alves wrote:
>> On 2024-02-07 18:56, Pedro Alves wrote:
>>> Hi!
>>>
>>> On 2024-02-07 18:18, Tom Tromey wrote:
>>>>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
>>>>
>>>> Luis> I see. Is this logic expected? Naturally I'd expect a process to
>>>> Luis> exist before a thread can exist.
>>>>
>>>> Me too but you can see it in
>>>> linux-low.cc:linux_process_target::handle_extended_wait.
>>>>
>>>>       lwp_info *child_lwp = add_lwp (child_ptid);
>>>> [...]
>>>>       if (event != PTRACE_EVENT_CLONE)
>>>> 	{
>>>> 	  /* Add the new process to the tables and clone the breakpoint
>>>> 	     lists of the parent.  We need to do this even if the new process
>>>> 	     will be detached, since we will need the process object and the
>>>> 	     breakpoints to remove any breakpoints from memory when we
>>>> 	     detach, and the client side will access registers.  */
>>>> 	  process_info *child_proc = add_linux_process (new_pid, 0);
>>>> [...]
>>>>
>>>
>>> I don't recall off hand a reason that prevents us from tweaking this code a little to
>>> create the child process before the child lwp is created.  I think that was how it was
>>> done before my changes, and I just reordered code to make it end up with fewer lines.
>>> I think we can create the child process earlier.
>>>
>>> I'll send a patch in a sec, once I test it.
>>
>> Like so?  Does it fix the crash?
> 
> It does, thanks for the quick patch.
> 
> Maybe before this series we were relying on some other path eventually creating a process first, and
> the new code somehow caused a (indirect?) change.

Right.  It was really a direct change in commit 393a6b5947d0 ("Thread options & clone events (Linux GDBserver)").
Before that change, we had, early in handle_extended_wait:

int
linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
                                            int wstat)
{
...
     if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
        {
...
          /* Add the new process to the tables and clone the breakpoint
             lists of the parent.  We need to do this even if the new process
             will be detached, since we will need the process object and the
             breakpoints to remove any breakpoints from memory when we
             detach, and the client side will access registers.  */
          child_proc = add_linux_process (new_pid, 0);
          gdb_assert (child_proc != NULL);
          child_lwp = add_lwp (ptid);
          gdb_assert (child_lwp != NULL);


So we used to add the process before the LWP.  393a6b5947d0 reordered things, as mentioned in the commit log:

    ...
    This shuffles code in linux_process_target::handle_extended_wait
    around to a more logical order when we now have to handle and
    potentially report all of fork/vfork/clone.
    ...

That shuffling made us create the process _after_ creating the LWP.  I had missed that this could
have a consequence, back then.

> 
> I'm putting this through the gdbserver testsuite on my end. I'll let you know what comes out of it.
> 

Thanks!

Pedro Alves