Re: [PATCH] Don't explicitly set clone child ptrace options (was: Re: [PATCH v2 04/29] Step over clone syscall w/ breakpoint, TARGET_WAITKIND_THREAD_CLONED)

[Simon Marchi <simark@simark.ca>]

On 10/18/22 08:40, Pedro Alves wrote:
> On 2022-07-21 2:35 a.m., Simon Marchi wrote:
>>> +
>>> +  lwp_info *new_lp = add_lwp (child_ptid);
>>> +  new_lp->stopped = 1;
>>> +
>>> +  /* If the thread_db layer is active, let it record the user
>>> +     level thread id and status, and add the thread to GDB's
>>> +     list.  */
>>> +  if (!thread_db_notice_clone (inferior_ptid, new_lp->ptid))
>>> +    {
>>> +      /* The process is not using thread_db.  Add the LWP to
>>> +	 GDB's list.  */
>>> +      target_post_attach (new_lp->ptid.lwp ());
>> Can you explain why that target_post_attach call?  Oh, I just noticed it
>> was already there.  Still, can you explain?
>>
> 
> Looks like I added this in the initial native Linux non-stop support, back in
> 
>  commit 4c28f408dfc2ab71b7995f061cc725d3f217ec9c
>  Author:     Pedro Alves <palves@redhat.com>
>  AuthorDate: Thu Jul 10 22:58:37 2008 +0000
>  Commit:     Pedro Alves <palves@redhat.com>
>  CommitDate: Thu Jul 10 22:58:37 2008 +0000
> 
>              Non-stop linux native.
> 
> (back at CodeSourcery; the email is incorrect due to the cvs->git conversion)
> 
> I can't explain the target_post_attach call, it does look unnecessary.  Here's a patch
> removing it.
> 
> From f02d5043307a2c79b38477e7bcbd369fcf53c104 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Fri, 14 Oct 2022 20:17:36 +0100
> Subject: [PATCH] Don't explicitly set clone child ptrace options
> 
> linux_handle_extended_wait calls target_post_attach if we're handling
> a PTRACE_EVENT_CLONE, and libthread_db.so isn't active.
> target_post_attach just calls linux_init_ptrace_procfs to set the
> lwp's ptrace options.  However, this is completely unnecessary,
> because, as man ptrace [1] says, options are inherited:
> 
>   "Flags are inherited by new tracees created and "auto-attached" via
>    active PTRACE_O_TRACEFORK, PTRACE_O_TRACEVFORK, or PTRACE_O_TRACECLONE
>    options."
> 
> This removes the unnecessary call.
> 
> [1] - https://man7.org/linux/man-pages/man2/ptrace.2.html
> 
> Change-Id: I533eaa60b700f7e40760311fc0d344d0b3f19a78
> ---
>  gdb/linux-nat.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index d4639766088..2ef278a914f 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1915,7 +1915,6 @@ linux_handle_extended_wait (struct lwp_info *lp, int status)
>  	    {
>  	      /* The process is not using thread_db.  Add the LWP to
>  		 GDB's list.  */
> -	      target_post_attach (new_lp->ptid.lwp ());
>  	      add_thread (linux_target, new_lp->ptid);
>  	    }
>  
> 
> base-commit: 32e876a80f015281cd7fca46e8a589c6cf132d11
> -- 
> 2.36.0
> 

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon