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

[Luis Machado <luis.machado@arm.com>]

On 2/8/24 10:53, Pedro Alves wrote:
> 
> 
> 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.
>>

I can confirm the testsuite run against gdbserver (native/extended) looks much better with the above patch applied.