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

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

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.

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

> 
> From 0c308ac13c4537c885491305cee7215fbfdf04c0 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 7 Feb 2024 18:48:16 +0000
> Subject: [PATCH] Fix crash in aarch64-linux gdbserver
> 
> Since commit 393a6b5947d0 ("Thread options & clone events (Linux
> GDBserver)"), aarch64-linux gdbserver crashes when the inferior
> vforks.  This happens in aarch64_get_debug_reg_state:
> 
>   struct process_info *proc = find_process_pid (pid);
> 
>   return &proc->priv->arch_private->debug_reg_state;
> 
> Here, find_process_pid returns nullptr -- the new inferior hasn't yet
> been created in linux_process_target::handle_extended_wait.
> 
> This patch fixes the problem by having
> linux_process_target::handle_extended_wait create the child process
> earlier, before the child LWP is created.  This is what the function
> did before it was reorganized by the commit referred above.
> 
> Change-Id: Ib8b3a2e6048c3ad2b91a92ea4430da507db03c50
> Co-Authored-By: Tom Tromey <tromey@adacore.com>
> ---
>  gdbserver/linux-low.cc | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 444eebc6bbe..9d5a6242803 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -555,6 +555,16 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>  			   ? ptid_t (new_pid, new_pid)
>  			   : ptid_t (ptid_of (event_thr).pid (), new_pid));
>  
> +      process_info *child_proc = nullptr;
> +
> +      if (event != PTRACE_EVENT_CLONE)
> +	{
> +	  /* Add the new process to the tables before we add the LWP.
> +	     We need to do this even if the new process will be
> +	     detached.  See breakpoint cloning code further below.  */
> +	  child_proc = add_linux_process (new_pid, 0);
> +	}
> +
>        lwp_info *child_lwp = add_lwp (child_ptid);
>        gdb_assert (child_lwp != NULL);
>        child_lwp->stopped = 1;
> @@ -588,12 +598,11 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>  
>        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);
> +	  /* 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.  */
>  	  gdb_assert (child_proc != NULL);
>  
>  	  process_info *parent_proc = get_thread_process (event_thr);
> 
> base-commit: 6fb99666f4bbc79708acb8efb2d80e57de67b80b