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

  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(-)
  

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.
  

>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:

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

I didn't try this, but I do have a fix for the fork bug.
I'll send it in a few days, I hope.

Tom
  

On 2/6/24 14:57, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
> 
> Luis> Tromey had pointed out, on IRC, gdbserver was crashing when
> Luis> stepping over a fork on aarch64. I went to investigate it and
> Luis> noticed the testsuite run for --target_board=native-gdbserver was
> Luis> really bad in terms of FAIL's (over 700). This is Ubuntu 20.04.
> 
> I didn't try this, but I do have a fix for the fork bug.
> I'll send it in a few days, I hope.
> 
> Tom

I'll keep an eye out for it, so I can do some testing on our end.
  

On 2/6/24 14:57, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
> 
> Luis> Tromey had pointed out, on IRC, gdbserver was crashing when
> Luis> stepping over a fork on aarch64. I went to investigate it and
> Luis> noticed the testsuite run for --target_board=native-gdbserver was
> Luis> really bad in terms of FAIL's (over 700). This is Ubuntu 20.04.
> 
> I didn't try this, but I do have a fix for the fork bug.
> I'll send it in a few days, I hope.
> 
> Tom

I was checking this today. Turns out we're trying to locate the process PID of the
process in this function, line 405:

402     struct aarch64_debug_reg_state *
403     aarch64_get_debug_reg_state (pid_t pid)
404     {
405       struct process_info *proc = find_process_pid (pid);
406
407       return &proc->priv->arch_private->debug_reg_state;
408     }

But find_process_pid returns nullptr. I wonder if it is one of those cases
where we have to deal with the tid rather than the pid.

Does this look like the same case you were chasing?
  

>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:

Luis> But find_process_pid returns nullptr. I wonder if it is one of those cases
Luis> where we have to deal with the tid rather than the pid.

Luis> Does this look like the same case you were chasing?

Yes.  The issue is that the new inferior isn't created until after the
new thread -- but the order can't really be reversed in the caller.

I've appended the patch.  I put off sending it because for internal
reasons it hasn't been through the AdaCore automated testing yet.
However, I did test it (using the AdaCore test suite -- not gdb's)
myself.

Let me know what you think.

Tom

commit 5464152cb1145bc1df108eb6904a642d8bc73b8c
Author: Tom Tromey <tromey@adacore.com>
Date:   Mon Feb 5 13:18:51 2024 -0700

    Fix crash in aarch64-linux gdbserver
    
    We noticed that aarch64-linux gdbserver will crash 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 aarch64_get_debug_reg_state
    return nullptr in this case, and then updating
    aarch64_linux_new_thread to check for this.

diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
index 5ebbc9b81f8..894de8aa3eb 100644
--- a/gdb/nat/aarch64-linux.c
+++ b/gdb/nat/aarch64-linux.c
@@ -81,9 +81,9 @@ aarch64_linux_new_thread (struct lwp_info *lwp)
   /* If there are hardware breakpoints/watchpoints in the process then mark that
      all the hardware breakpoint/watchpoint register pairs for this thread need
      to be initialized (with data from aarch_process_info.debug_reg_state).  */
-  if (aarch64_any_set_debug_regs_state (state, false))
+  if (state == nullptr || aarch64_any_set_debug_regs_state (state, false))
     DR_MARK_ALL_CHANGED (info->dr_changed_bp, aarch64_num_bp_regs);
-  if (aarch64_any_set_debug_regs_state (state, true))
+  if (state == nullptr || aarch64_any_set_debug_regs_state (state, true))
     DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs);
 
   lwp_set_arch_private_info (lwp, info);
diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 28d75d035dc..2a4f01a54da 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -403,7 +403,8 @@ struct aarch64_debug_reg_state *
 aarch64_get_debug_reg_state (pid_t pid)
 {
   struct process_info *proc = find_process_pid (pid);
-
+  if (proc == nullptr)
+    return nullptr;
   return &proc->priv->arch_private->debug_reg_state;
 }
  

On 2/7/24 10:43, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
> 
> Luis> But find_process_pid returns nullptr. I wonder if it is one of those cases
> Luis> where we have to deal with the tid rather than the pid.
> 
> Luis> Does this look like the same case you were chasing?
> 
> Yes.  The issue is that the new inferior isn't created until after the
> new thread -- but the order can't really be reversed in the caller.
> 
> I've appended the patch.  I put off sending it because for internal
> reasons it hasn't been through the AdaCore automated testing yet.
> However, I did test it (using the AdaCore test suite -- not gdb's)
> myself.
> 
> Let me know what you think.
> 
> Tom
> 
> commit 5464152cb1145bc1df108eb6904a642d8bc73b8c
> Author: Tom Tromey <tromey@adacore.com>
> Date:   Mon Feb 5 13:18:51 2024 -0700
> 
>     Fix crash in aarch64-linux gdbserver
>     
>     We noticed that aarch64-linux gdbserver will crash 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 aarch64_get_debug_reg_state
>     return nullptr in this case, and then updating
>     aarch64_linux_new_thread to check for this.
> 
> diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
> index 5ebbc9b81f8..894de8aa3eb 100644
> --- a/gdb/nat/aarch64-linux.c
> +++ b/gdb/nat/aarch64-linux.c
> @@ -81,9 +81,9 @@ aarch64_linux_new_thread (struct lwp_info *lwp)
>    /* If there are hardware breakpoints/watchpoints in the process then mark that
>       all the hardware breakpoint/watchpoint register pairs for this thread need
>       to be initialized (with data from aarch_process_info.debug_reg_state).  */
> -  if (aarch64_any_set_debug_regs_state (state, false))
> +  if (state == nullptr || aarch64_any_set_debug_regs_state (state, false))
>      DR_MARK_ALL_CHANGED (info->dr_changed_bp, aarch64_num_bp_regs);
> -  if (aarch64_any_set_debug_regs_state (state, true))
> +  if (state == nullptr || aarch64_any_set_debug_regs_state (state, true))
>      DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs);

I don't really understand all of this, but I'm wondering if the
condition should be:

  if (state != nullptr && aarch64_any_set_debug_regs_state (state, ...))

If we have no existing aarch64_debug_reg_state, do we really need to
mark the breakpoints as needing to be updated?

>    lwp_set_arch_private_info (lwp, info);
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 28d75d035dc..2a4f01a54da 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -403,7 +403,8 @@ struct aarch64_debug_reg_state *
>  aarch64_get_debug_reg_state (pid_t pid)
>  {
>    struct process_info *proc = find_process_pid (pid);
> -
> +  if (proc == nullptr)
> +    return nullptr;
>    return &proc->priv->arch_private->debug_reg_state;
>  }

I was wondering if the GDB version of this function needed to get
updated too.  It works differently:

    /* See aarch64-nat.h.  */

    struct aarch64_debug_reg_state *
    aarch64_get_debug_reg_state (pid_t pid)
    {
      return &aarch64_debug_process_state[pid];
    }

Here, aarch64_debug_process_state is an unordered_map<pid_t,
aarch64_debug_reg_state>, meaning that if pid isn't currently in the
map, a default aarch64_debug_reg_state will be constructed (is it going
to be initialized properly?).

So we end up with two different semantics for the two versions of the
function, which might become a source of confusion later.

Simon
  

Replying to both Tom's and Simon's comments.

On 2/7/24 17:10, Simon Marchi wrote:
> On 2/7/24 10:43, Tom Tromey wrote:
>>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
>>
>> Luis> But find_process_pid returns nullptr. I wonder if it is one of those cases
>> Luis> where we have to deal with the tid rather than the pid.
>>
>> Luis> Does this look like the same case you were chasing?
>>
>> Yes.  The issue is that the new inferior isn't created until after the
>> new thread -- but the order can't really be reversed in the caller.
>>

I see. Is this logic expected? Naturally I'd expect a process to exist before a thread can exist.

I haven't followed the patch series closely though, so there may be a reason for it.

>> I've appended the patch.  I put off sending it because for internal
>> reasons it hasn't been through the AdaCore automated testing yet.
>> However, I did test it (using the AdaCore test suite -- not gdb's)
>> myself.
>>
>> Let me know what you think.

It does fix the regressions I was seeing, but Simon made some good points as well.

>>
>> Tom
>>
>> commit 5464152cb1145bc1df108eb6904a642d8bc73b8c
>> Author: Tom Tromey <tromey@adacore.com>
>> Date:   Mon Feb 5 13:18:51 2024 -0700
>>
>>     Fix crash in aarch64-linux gdbserver
>>     
>>     We noticed that aarch64-linux gdbserver will crash 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 aarch64_get_debug_reg_state
>>     return nullptr in this case, and then updating
>>     aarch64_linux_new_thread to check for this.
>>
>> diff --git a/gdb/nat/aarch64-linux.c b/gdb/nat/aarch64-linux.c
>> index 5ebbc9b81f8..894de8aa3eb 100644
>> --- a/gdb/nat/aarch64-linux.c
>> +++ b/gdb/nat/aarch64-linux.c
>> @@ -81,9 +81,9 @@ aarch64_linux_new_thread (struct lwp_info *lwp)
>>    /* If there are hardware breakpoints/watchpoints in the process then mark that
>>       all the hardware breakpoint/watchpoint register pairs for this thread need
>>       to be initialized (with data from aarch_process_info.debug_reg_state).  */
>> -  if (aarch64_any_set_debug_regs_state (state, false))
>> +  if (state == nullptr || aarch64_any_set_debug_regs_state (state, false))
>>      DR_MARK_ALL_CHANGED (info->dr_changed_bp, aarch64_num_bp_regs);
>> -  if (aarch64_any_set_debug_regs_state (state, true))
>> +  if (state == nullptr || aarch64_any_set_debug_regs_state (state, true))
>>      DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs);
> 
> I don't really understand all of this, but I'm wondering if the
> condition should be:
> 
>   if (state != nullptr && aarch64_any_set_debug_regs_state (state, ...))
> 
> If we have no existing aarch64_debug_reg_state, do we really need to
> mark the breakpoints as needing to be updated?
> 

I think as long as we have a thread, we should always have the state for the debug registers,
so changing the approach to always initialize the state if there isn't one seems reasonable.

See below.

>>    lwp_set_arch_private_info (lwp, info);
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index 28d75d035dc..2a4f01a54da 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -403,7 +403,8 @@ struct aarch64_debug_reg_state *
>>  aarch64_get_debug_reg_state (pid_t pid)
>>  {
>>    struct process_info *proc = find_process_pid (pid);
>> -
>> +  if (proc == nullptr)
>> +    return nullptr;
>>    return &proc->priv->arch_private->debug_reg_state;
>>  }
> 
> I was wondering if the GDB version of this function needed to get
> updated too.  It works differently:
> 
>     /* See aarch64-nat.h.  */
> 
>     struct aarch64_debug_reg_state *
>     aarch64_get_debug_reg_state (pid_t pid)
>     {
>       return &aarch64_debug_process_state[pid];
>     }
> 
> Here, aarch64_debug_process_state is an unordered_map<pid_t,
> aarch64_debug_reg_state>, meaning that if pid isn't currently in the
> map, a default aarch64_debug_reg_state will be constructed (is it going
> to be initialized properly?).
> 
> So we end up with two different semantics for the two versions of the
> function, which might become a source of confusion later.

And it would sync the behavior from gdb nat and gdbserver nat layers.

I can put together a patch to do that. I wasn't aware there was this discrepancy
between gdb and gdbserver.

> 
> Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> -  if (aarch64_any_set_debug_regs_state (state, true))
>> +  if (state == nullptr || aarch64_any_set_debug_regs_state (state, true))
>> DR_MARK_ALL_CHANGED (info->dr_changed_wp, aarch64_num_wp_regs);

Simon> I don't really understand all of this, but I'm wondering if the
Simon> condition should be:

Simon>   if (state != nullptr && aarch64_any_set_debug_regs_state (state, ...))

Simon> If we have no existing aarch64_debug_reg_state, do we really need to
Simon> mark the breakpoints as needing to be updated?

I wasn't sure but I followed what I understood x86 to do, see
nat/x86-linux.c:lwp_set_debug_registers_changed.

Simon> Here, aarch64_debug_process_state is an unordered_map<pid_t,
Simon> aarch64_debug_reg_state> , meaning that if pid isn't currently in the
Simon> map, a default aarch64_debug_reg_state will be constructed (is it going
Simon> to be initialized properly?).

Simon> So we end up with two different semantics for the two versions of the
Simon> function, which might become a source of confusion later.

Yeah, I don't know the answer here.  I personally don't find it super
confusing, or at least not any more than the way that gdb and gdbserver
randomly do things differently already.

Tom
  

>>>>> "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);
[...]

Tom
  

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.

Pedro Alves
  

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?

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
  

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
  

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
  

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.
  

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Like so?  Does it fix the crash?

Thanks for doing this.

Tom