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

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

Hi,

On 11/13/23 15:04, Pedro Alves wrote:
> This patch teaches the Linux GDBserver backend to report clone events
> to GDB, when GDB has requested them with the GDB_THREAD_OPTION_CLONE
> thread option, via the new QThreadOptions packet.
> 
> 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.
> 
> Raname lwp_info::fork_relative -> lwp_info::relative as the field is
> no longer only about (v)fork.
> 
> With this, gdb.threads/stepi-over-clone.exp now cleanly passes against
> GDBserver, so remove the native-target-only requirement from that
> testcase.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=19675
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27830
> Reviewed-By: Andrew Burgess <aburgess@redhat.com>
> Change-Id: I3a19bc98801ec31e5c6fdbe1ebe17df855142bb2
> ---
>  .../gdb.threads/stepi-over-clone.exp          |   6 -
>  gdbserver/linux-low.cc                        | 253 ++++++++++--------
>  gdbserver/linux-low.h                         |  47 ++--
>  3 files changed, 160 insertions(+), 146 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.threads/stepi-over-clone.exp b/gdb/testsuite/gdb.threads/stepi-over-clone.exp
> index e580f2248ac..4c496429632 100644
> --- a/gdb/testsuite/gdb.threads/stepi-over-clone.exp
> +++ b/gdb/testsuite/gdb.threads/stepi-over-clone.exp
> @@ -19,12 +19,6 @@
>  # disassembly output.  For now this is only implemented for x86-64.
>  require {istarget x86_64-*-*}
>  
> -# Test only on native targets, for now.
> -proc is_native_target {} {
> -    return [expr {[target_info gdb_protocol] == ""}]
> -}
> -require is_native_target
> -
>  standard_testfile
>  
>  if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 40b6a907ad9..136a8b6c9a1 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -491,7 +491,6 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>    struct lwp_info *event_lwp = *orig_event_lwp;
>    int event = linux_ptrace_get_extended_event (wstat);
>    struct thread_info *event_thr = get_lwp_thread (event_lwp);
> -  struct lwp_info *new_lwp;
>  
>    gdb_assert (event_lwp->waitstatus.kind () == TARGET_WAITKIND_IGNORE);
>  
> @@ -503,7 +502,6 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>    if ((event == PTRACE_EVENT_FORK) || (event == PTRACE_EVENT_VFORK)
>        || (event == PTRACE_EVENT_CLONE))
>      {
> -      ptid_t ptid;
>        unsigned long new_pid;
>        int ret, status;
>  
> @@ -527,61 +525,65 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>  	    warning ("wait returned unexpected status 0x%x", status);
>  	}
>  
> -      if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
> +      if (debug_threads)
>  	{
> -	  struct process_info *parent_proc;
> -	  struct process_info *child_proc;
> -	  struct lwp_info *child_lwp;
> -	  struct thread_info *child_thr;
> +	  debug_printf ("HEW: Got %s event from LWP %ld, new child is %ld\n",
> +			(event == PTRACE_EVENT_FORK ? "fork"
> +			 : event == PTRACE_EVENT_VFORK ? "vfork"
> +			 : event == PTRACE_EVENT_CLONE ? "clone"
> +			 : "???"),
> +			ptid_of (event_thr).lwp (),
> +			new_pid);
> +	}
> +
> +      ptid_t child_ptid = (event != PTRACE_EVENT_CLONE
> +			   ? ptid_t (new_pid, new_pid)
> +			   : ptid_t (ptid_of (event_thr).pid (), new_pid));
>  
> -	  ptid = ptid_t (new_pid, new_pid);
> +      lwp_info *child_lwp = add_lwp (child_ptid);
> +      gdb_assert (child_lwp != NULL);
> +      child_lwp->stopped = 1;
> +      if (event != PTRACE_EVENT_CLONE)
> +	child_lwp->must_set_ptrace_flags = 1;
> +      child_lwp->status_pending_p = 0;
>  
> -	  threads_debug_printf ("Got fork event from LWP %ld, "
> -				"new child is %d",
> -				ptid_of (event_thr).lwp (),
> -				ptid.pid ());
> +      thread_info *child_thr = get_lwp_thread (child_lwp);
>  
> +      /* If we're suspending all threads, leave this one suspended
> +	 too.  If the fork/clone parent is stepping over a breakpoint,
> +	 all other threads have been suspended already.  Leave the
> +	 child suspended too.  */
> +      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> +	  || event_lwp->bp_reinsert != 0)
> +	{
> +	  threads_debug_printf ("leaving child suspended");
> +	  child_lwp->suspended = 1;
> +	}
> +
> +      if (event_lwp->bp_reinsert != 0
> +	  && supports_software_single_step ()
> +	  && event == PTRACE_EVENT_VFORK)
> +	{
> +	  /* If we leave single-step breakpoints there, child will
> +	     hit it, so uninsert single-step breakpoints from parent
> +	     (and child).  Once vfork child is done, reinsert
> +	     them back to parent.  */
> +	  uninsert_single_step_breakpoints (event_thr);
> +	}
> +
> +      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.  */
> -	  child_proc = add_linux_process (new_pid, 0);
> +	  process_info *child_proc = add_linux_process (new_pid, 0);
>  	  gdb_assert (child_proc != NULL);
> -	  child_lwp = add_lwp (ptid);
> -	  gdb_assert (child_lwp != NULL);
> -	  child_lwp->stopped = 1;
> -	  child_lwp->must_set_ptrace_flags = 1;
> -	  child_lwp->status_pending_p = 0;
> -	  child_thr = get_lwp_thread (child_lwp);
> -	  child_thr->last_resume_kind = resume_stop;
> -	  child_thr->last_status.set_stopped (GDB_SIGNAL_0);
> -
> -	  /* If we're suspending all threads, leave this one suspended
> -	     too.  If the fork/clone parent is stepping over a breakpoint,
> -	     all other threads have been suspended already.  Leave the
> -	     child suspended too.  */
> -	  if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> -	      || event_lwp->bp_reinsert != 0)
> -	    {
> -	      threads_debug_printf ("leaving child suspended");
> -	      child_lwp->suspended = 1;
> -	    }
>  
> -	  parent_proc = get_thread_process (event_thr);
> +	  process_info *parent_proc = get_thread_process (event_thr);
>  	  child_proc->attached = parent_proc->attached;
>  
> -	  if (event_lwp->bp_reinsert != 0
> -	      && supports_software_single_step ()
> -	      && event == PTRACE_EVENT_VFORK)
> -	    {
> -	      /* If we leave single-step breakpoints there, child will
> -		 hit it, so uninsert single-step breakpoints from parent
> -		 (and child).  Once vfork child is done, reinsert
> -		 them back to parent.  */
> -	      uninsert_single_step_breakpoints (event_thr);
> -	    }
> -
>  	  clone_all_breakpoints (child_thr, event_thr);
>  
>  	  target_desc_up tdesc = allocate_target_description ();
> @@ -590,88 +592,97 @@ linux_process_target::handle_extended_wait (lwp_info **orig_event_lwp,
>  
>  	  /* Clone arch-specific process data.  */
>  	  low_new_fork (parent_proc, child_proc);
> +	}
>  
> -	  /* Save fork info in the parent thread.  */
> -	  if (event == PTRACE_EVENT_FORK)
> -	    event_lwp->waitstatus.set_forked (ptid);
> -	  else if (event == PTRACE_EVENT_VFORK)
> -	    event_lwp->waitstatus.set_vforked (ptid);
> -
> +      /* Save fork/clone info in the parent thread.  */
> +      if (event == PTRACE_EVENT_FORK)
> +	event_lwp->waitstatus.set_forked (child_ptid);
> +      else if (event == PTRACE_EVENT_VFORK)
> +	event_lwp->waitstatus.set_vforked (child_ptid);
> +      else if (event == PTRACE_EVENT_CLONE
> +	       && (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) != 0)
> +	event_lwp->waitstatus.set_thread_cloned (child_ptid);
> +
> +      if (event != PTRACE_EVENT_CLONE
> +	  || (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) != 0)
> +	{
>  	  /* The status_pending field contains bits denoting the
> -	     extended event, so when the pending event is handled,
> -	     the handler will look at lwp->waitstatus.  */
> +	     extended event, so when the pending event is handled, the
> +	     handler will look at lwp->waitstatus.  */
>  	  event_lwp->status_pending_p = 1;
>  	  event_lwp->status_pending = wstat;
>  
> -	  /* Link the threads until the parent event is passed on to
> -	     higher layers.  */
> -	  event_lwp->fork_relative = child_lwp;
> -	  child_lwp->fork_relative = event_lwp;
> -
> -	  /* If the parent thread is doing step-over with single-step
> -	     breakpoints, the list of single-step breakpoints are cloned
> -	     from the parent's.  Remove them from the child process.
> -	     In case of vfork, we'll reinsert them back once vforked
> -	     child is done.  */
> -	  if (event_lwp->bp_reinsert != 0
> -	      && supports_software_single_step ())
> -	    {
> -	      /* The child process is forked and stopped, so it is safe
> -		 to access its memory without stopping all other threads
> -		 from other processes.  */
> -	      delete_single_step_breakpoints (child_thr);
> -
> -	      gdb_assert (has_single_step_breakpoints (event_thr));
> -	      gdb_assert (!has_single_step_breakpoints (child_thr));
> -	    }
> -
> -	  /* Report the event.  */
> -	  return 0;
> +	  /* Link the threads until the parent's event is passed on to
> +	     GDB.  */
> +	  event_lwp->relative = child_lwp;
> +	  child_lwp->relative = event_lwp;
>  	}
>  
> -      threads_debug_printf
> -	("Got clone event from LWP %ld, new child is LWP %ld",
> -	 lwpid_of (event_thr), new_pid);
> -
> -      ptid = ptid_t (pid_of (event_thr), new_pid);
> -      new_lwp = add_lwp (ptid);
> -
> -      /* Either we're going to immediately resume the new thread
> -	 or leave it stopped.  resume_one_lwp is a nop if it
> -	 thinks the thread is currently running, so set this first
> -	 before calling resume_one_lwp.  */
> -      new_lwp->stopped = 1;
> +      /* If the parent thread is doing step-over with single-step
> +	 breakpoints, the list of single-step breakpoints are cloned
> +	 from the parent's.  Remove them from the child process.
> +	 In case of vfork, we'll reinsert them back once vforked
> +	 child is done.  */
> +      if (event_lwp->bp_reinsert != 0
> +	  && supports_software_single_step ())
> +	{
> +	  /* The child process is forked and stopped, so it is safe
> +	     to access its memory without stopping all other threads
> +	     from other processes.  */
> +	  delete_single_step_breakpoints (child_thr);
>  
> -      /* If we're suspending all threads, leave this one suspended
> -	 too.  If the fork/clone parent is stepping over a breakpoint,
> -	 all other threads have been suspended already.  Leave the
> -	 child suspended too.  */
> -      if (stopping_threads == STOPPING_AND_SUSPENDING_THREADS
> -	  || event_lwp->bp_reinsert != 0)
> -	new_lwp->suspended = 1;
> +	  gdb_assert (has_single_step_breakpoints (event_thr));
> +	  gdb_assert (!has_single_step_breakpoints (child_thr));
> +	}
>  
>        /* Normally we will get the pending SIGSTOP.  But in some cases
>  	 we might get another signal delivered to the group first.
>  	 If we do get another signal, be sure not to lose it.  */
>        if (WSTOPSIG (status) != SIGSTOP)
>  	{
> -	  new_lwp->stop_expected = 1;
> -	  new_lwp->status_pending_p = 1;
> -	  new_lwp->status_pending = status;
> +	  child_lwp->stop_expected = 1;
> +	  child_lwp->status_pending_p = 1;
> +	  child_lwp->status_pending = status;
>  	}
> -      else if (cs.report_thread_events)
> +      else if (event == PTRACE_EVENT_CLONE && cs.report_thread_events)
>  	{
> -	  new_lwp->waitstatus.set_thread_created ();
> -	  new_lwp->status_pending_p = 1;
> -	  new_lwp->status_pending = status;
> +	  child_lwp->waitstatus.set_thread_created ();
> +	  child_lwp->status_pending_p = 1;
> +	  child_lwp->status_pending = status;
>  	}
>  
> +      if (event == PTRACE_EVENT_CLONE)
> +	{
>  #ifdef USE_THREAD_DB
> -      thread_db_notice_clone (event_thr, ptid);
> +	  thread_db_notice_clone (event_thr, child_ptid);
>  #endif
> +	}
>  
> -      /* Don't report the event.  */
> -      return 1;
> +      if (event == PTRACE_EVENT_CLONE
> +	  && (event_thr->thread_options & GDB_THREAD_OPTION_CLONE) == 0)
> +	{
> +	  threads_debug_printf
> +	    ("not reporting clone event from LWP %ld, new child is %ld\n",
> +	     ptid_of (event_thr).lwp (),
> +	     new_pid);
> +	  return 1;
> +	}
> +
> +      /* Leave the child stopped until GDB processes the parent
> +	 event.  */
> +      child_thr->last_resume_kind = resume_stop;
> +      child_thr->last_status.set_stopped (GDB_SIGNAL_0);
> +
> +      /* Report the event.  */
> +      threads_debug_printf
> +	("reporting %s event from LWP %ld, new child is %ld\n",
> +	 (event == PTRACE_EVENT_FORK ? "fork"
> +	  : event == PTRACE_EVENT_VFORK ? "vfork"
> +	  : event == PTRACE_EVENT_CLONE ? "clone"
> +	  : "???"),
> +	 ptid_of (event_thr).lwp (),
> +	 new_pid);
> +      return 0;
>      }
>    else if (event == PTRACE_EVENT_VFORK_DONE)
>      {
> @@ -3531,15 +3542,14 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
>  
>    if (event_child->waitstatus.kind () != TARGET_WAITKIND_IGNORE)
>      {
> -      /* If the reported event is an exit, fork, vfork or exec, let
> -	 GDB know.  */
> +      /* If the reported event is an exit, fork, vfork, clone or exec,
> +	 let GDB know.  */
>  
> -      /* Break the unreported fork relationship chain.  */
> -      if (event_child->waitstatus.kind () == TARGET_WAITKIND_FORKED
> -	  || event_child->waitstatus.kind () == TARGET_WAITKIND_VFORKED)
> +      /* Break the unreported fork/vfork/clone relationship chain.  */
> +      if (is_new_child_status (event_child->waitstatus.kind ()))
>  	{
> -	  event_child->fork_relative->fork_relative = NULL;
> -	  event_child->fork_relative = NULL;
> +	  event_child->relative->relative = NULL;
> +	  event_child->relative = NULL;
>  	}
>  
>        *ourstatus = event_child->waitstatus;
> @@ -4272,15 +4282,14 @@ linux_set_resume_request (thread_info *thread, thread_resume *resume, size_t n)
>  	      continue;
>  	    }
>  
> -	  /* Don't let wildcard resumes resume fork children that GDB
> -	     does not yet know are new fork children.  */
> -	  if (lwp->fork_relative != NULL)
> +	  /* Don't let wildcard resumes resume fork/vfork/clone
> +	     children that GDB does not yet know are new children.  */
> +	  if (lwp->relative != NULL)
>  	    {
> -	      struct lwp_info *rel = lwp->fork_relative;
> +	      struct lwp_info *rel = lwp->relative;
>  
>  	      if (rel->status_pending_p
> -		  && (rel->waitstatus.kind () == TARGET_WAITKIND_FORKED
> -		      || rel->waitstatus.kind () == TARGET_WAITKIND_VFORKED))
> +		  && is_new_child_status (rel->waitstatus.kind ()))
>  		{
>  		  threads_debug_printf
>  		    ("not resuming LWP %ld: has queued stop reply",
> @@ -5907,6 +5916,14 @@ linux_process_target::supports_vfork_events ()
>    return true;
>  }
>  
> +/* Return the set of supported thread options.  */
> +
> +gdb_thread_options
> +linux_process_target::supported_thread_options ()
> +{
> +  return GDB_THREAD_OPTION_CLONE;
> +}
> +
>  /* Check if exec events are supported.  */
>  
>  bool
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index f7cedf6706b..94093dd4ed8 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -234,6 +234,8 @@ class linux_process_target : public process_stratum_target
>  
>    bool supports_vfork_events () override;
>  
> +  gdb_thread_options supported_thread_options () override;
> +
>    bool supports_exec_events () override;
>  
>    void handle_new_gdb_connection () override;
> @@ -732,48 +734,47 @@ struct pending_signal
>  
>  struct lwp_info
>  {
> -  /* If this LWP is a fork child that wasn't reported to GDB yet, return
> -     its parent, else nullptr.  */
> +  /* If this LWP is a fork/vfork/clone child that wasn't reported to
> +     GDB yet, return its parent, else nullptr.  */
>    lwp_info *pending_parent () const
>    {
> -    if (this->fork_relative == nullptr)
> +    if (this->relative == nullptr)
>        return nullptr;
>  
> -    gdb_assert (this->fork_relative->fork_relative == this);
> +    gdb_assert (this->relative->relative == this);
>  
> -    /* In a fork parent/child relationship, the parent has a status pending and
> +    /* In a parent/child relationship, the parent has a status pending and
>         the child does not, and a thread can only be in one such relationship
>         at most.  So we can recognize who is the parent based on which one has
>         a pending status.  */
>      gdb_assert (!!this->status_pending_p
> -		!= !!this->fork_relative->status_pending_p);
> +		!= !!this->relative->status_pending_p);
>  
> -    if (!this->fork_relative->status_pending_p)
> +    if (!this->relative->status_pending_p)
>        return nullptr;
>  
>      const target_waitstatus &ws
> -      = this->fork_relative->waitstatus;
> +      = this->relative->waitstatus;
>      gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED
>  		|| ws.kind () == TARGET_WAITKIND_VFORKED);
>  
> -    return this->fork_relative;
> -  }
> +    return this->relative; }
>  
> -  /* If this LWP is the parent of a fork child we haven't reported to GDB yet,
> -     return that child, else nullptr.  */
> +  /* If this LWP is the parent of a fork/vfork/clone child we haven't
> +     reported to GDB yet, return that child, else nullptr.  */
>    lwp_info *pending_child () const
>    {
> -    if (this->fork_relative == nullptr)
> +    if (this->relative == nullptr)
>        return nullptr;
>  
> -    gdb_assert (this->fork_relative->fork_relative == this);
> +    gdb_assert (this->relative->relative == this);
>  
> -    /* In a fork parent/child relationship, the parent has a status pending and
> +    /* In a parent/child relationship, the parent has a status pending and
>         the child does not, and a thread can only be in one such relationship
>         at most.  So we can recognize who is the parent based on which one has
>         a pending status.  */
>      gdb_assert (!!this->status_pending_p
> -		!= !!this->fork_relative->status_pending_p);
> +		!= !!this->relative->status_pending_p);
>  
>      if (!this->status_pending_p)
>        return nullptr;
> @@ -782,7 +783,7 @@ struct lwp_info
>      gdb_assert (ws.kind () == TARGET_WAITKIND_FORKED
>  		|| ws.kind () == TARGET_WAITKIND_VFORKED);
>  
> -    return this->fork_relative;
> +    return this->relative;
>    }
>  
>    /* Backlink to the parent object.  */
> @@ -820,11 +821,13 @@ struct lwp_info
>       information or exit status until it can be reported to GDB.  */
>    struct target_waitstatus waitstatus;
>  
> -  /* A pointer to the fork child/parent relative.  Valid only while
> -     the parent fork event is not reported to higher layers.  Used to
> -     avoid wildcard vCont actions resuming a fork child before GDB is
> -     notified about the parent's fork event.  */
> -  struct lwp_info *fork_relative = nullptr;
> +  /* A pointer to the fork/vfork/clone child/parent relative (like
> +     people, LWPs have relatives).  Valid only while the parent
> +     fork/vfork/clone event is not reported to higher layers.  Used to
> +     avoid wildcard vCont actions resuming a fork/vfork/clone child
> +     before GDB is notified about the parent's fork/vfork/clone
> +     event.  */
> +  struct lwp_info *relative = nullptr;
>  
>    /* When stopped is set, this is where the lwp last stopped, with
>       decr_pc_after_break already accounted for.  If the LWP is

Tromey had pointed out, on IRC, gdbserver was crashing when stepping over a fork on aarch64. I went to investigate it and noticed the testsuite
run for --target_board=native-gdbserver was really bad in terms of FAIL's (over 700). This is Ubuntu 20.04.

I bisected the FAIL's for at least one testcase (gdb.threads/next-fork-other-thread.exp) to this particular commit. But the series is large, so it could
potentially be something else in the series.

I haven't fully investigated the crashes yet, but thought I'd mention it for the record and to see if any bells rang.