This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] darwin: Do not add a dummy thread


On Sunday, June 25 2017, Simon Marchi wrote:

> Starting a process on macOS/Darwin currently leads to this error:
>
> /Users/simark/src/binutils-gdb/gdb/darwin-nat.c:383: internal-error: void darwin_check_new_threads(struct inferior *): Assertion `tp' failed.
>
> with the corresponding partial backtrace (sorry, taken with lldb,
> because well, gdb is broken :)):
>
>     frame #9: 0x000000010004605a gdb`darwin_check_new_threads(inf=0x0000000100edf670) at darwin-nat.c:383
>     frame #10: 0x0000000100045848 gdb`darwin_init_thread_list(inf=0x0000000100edf670) at darwin-nat.c:1710
>     frame #11: 0x00000001000452f8 gdb`darwin_ptrace_him(pid=8375) at darwin-nat.c:1792
>     frame #12: 0x0000000100041d95 gdb`fork_inferior(...) at fork-inferior.c:440
>     frame #13: 0x0000000100043f82 gdb`darwin_create_inferior(...) at darwin-nat.c:1841
>     frame #14: 0x000000010034ac32 gdb`run_command_1(args=0x0000000000000000, from_tty=1, tbreak_at_main=1) at infcmd.c:611
>
> The issue was introduced by commit
>
>   "Share fork_inferior et al with gdbserver"
>
> because it changed the place where the dummy thread (pid, 0, 0) is added,
> relative to the call to the init_trace_fun callback.  In this callback, darwin
> checks for new threads in the program (there should be exactly one) in order to
> update this dummy thread with the right tid.  Previously, things happened in
> this order:
>
>  - fork_inferior calls fork()
>  - fork_inferior adds dummy thread
>  - fork_inferior calls init_trace_fun callback, which updates the dummy
>    thread info
>
> Following the commit mentioned above, the new thread is added in the
> darwin-nat code, after having called fork_inferior (in
> darwin_create_inferior).  So gdb tries to do things in this order:
>
>  - fork_inferior calls fork()
>  - fork_inferior calls init_trace_fun callback, which tries to update
>    the dummy thread info
>  - darwin_create_inferior adds the dummy thread
>
> The error happens while trying to update the dummy thread that has not
> been added yet.
>
> I don't think this dummy thread is necessary for darwin.  Previously, it
> was fork_inferior that was adding this thread, for all targets, so
> darwin had to deal with it.  Now that it's done by targets themselves,
> we can just skip that on darwin.  darwin_check_new_threads called
> indirectly by init_trace_fun/darwin_ptrace_him will simply notice the
> new thread and add it with the right information.
>
> My level of testing was: try to start a process and try to attach to a
> process, and it seems to work somewhat like it did before.  I tried to
> run the testsuite, but it leaves a huge amount of zombie processes that
> launchd doesn't seem to reap, leading to exhaustion of system resources
> (number of processes).

Hey Simon,

Thank you very much for investigating this.  Indeed, fork_inferior had a
few changes and one of the most important was the fact that it doesn't
add a thread anymore; this is left to the caller.

Your patch looks good to me, and I like the idea of getting rid of the
dummy thread while you're at it.  I just have nits to point, but
otherwise I think it can go in.

OOC, re. the fact that running the testsuite leaves a lot of zombie
processes behind, I'm assuming that this behaviour already existed
before the fork_inferior rework, right?

> gdb/ChangeLog:
>
> 	* darwin-nat.c (darwin_check_new_threads): Don't handle dummy
> 	thread.
> 	(darwin_init_thread_list): Don't update dummy thread.
> 	(darwin_create_inferior, darwin_attach): Don't add a dummy thread.
> ---
>  gdb/darwin-nat.c | 74 +++++++++++++++++++++-----------------------------------
>  1 file changed, 28 insertions(+), 46 deletions(-)
>
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index cd67249..bb52d9f 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -367,29 +367,14 @@ darwin_check_new_threads (struct inferior *inf)
>        if (new_ix < new_nbr && (old_ix == old_nbr || new_id < old_id))
>  	{
>  	  /* A thread was created.  */
> -	  struct thread_info *tp;
>  	  struct private_thread_info *pti;
>  
>  	  pti = XCNEW (struct private_thread_info);
>  	  pti->gdb_port = new_id;
>  	  pti->msg_state = DARWIN_RUNNING;
>  
> -	  if (old_nbr == 0 && new_ix == 0)
> -	    {
> -	      /* A ptid is create when the inferior is started (see
> -		 fork-child.c) with lwp=tid=0.  This ptid will be renamed
> -		 later by darwin_init_thread_list ().  */
> -	      tp = find_thread_ptid (ptid_build (inf->pid, 0, 0));
> -	      gdb_assert (tp);
> -	      gdb_assert (tp->priv == NULL);
> -	      tp->priv = pti;
> -	    }
> -	  else
> -	    {
> -	      /* Add the new thread.  */
> -	      tp = add_thread_with_info
> -		(ptid_build (inf->pid, 0, new_id), pti);
> -	    }
> +	  /* Add the new thread.  */
> +	  add_thread_with_info (ptid_build (inf->pid, 0, new_id), pti);
>  	  VEC_safe_push (darwin_thread_t, thread_vec, pti);
>  	  new_ix++;
>  	  continue;
> @@ -1701,23 +1686,36 @@ darwin_attach_pid (struct inferior *inf)
>      push_target (darwin_ops);
>  }
>  
> +static struct thread_info *
> +thread_info_from_private_thread_info (private_thread_info *pti)

Missing comment for the function.

> +{
> +  struct thread_info *it;
> +
> +  ALL_THREADS (it)
> +    {
> +      if (it->priv->gdb_port == pti->gdb_port)
> +	break;
> +    }
> +
> +  gdb_assert (it != NULL);
> +
> +  return it;
> +}
> +
>  static void
>  darwin_init_thread_list (struct inferior *inf)
>  {
> -  darwin_thread_t *thread;
> -  ptid_t new_ptid;
> -
>    darwin_check_new_threads (inf);
>  
> -  gdb_assert (inf->priv->threads
> +  gdb_assert (inf->priv->threads != NULL
>  	      && VEC_length (darwin_thread_t, inf->priv->threads) > 0);

It's just a matter of personal preference, but I don't like to use &&
and || on gdb_assert.  It makes it harder to identify what went wrong if
the assert triggers.  In this case, I like to split the condition into
two assertions.  But as I said, personal preference.

> -  thread = VEC_index (darwin_thread_t, inf->priv->threads, 0);
>  
> -  /* Note: fork_inferior automatically add a thead but it uses a wrong ptid.
> -     Fix up.  */
> -  new_ptid = ptid_build (inf->pid, 0, thread->gdb_port);
> -  thread_change_ptid (inferior_ptid, new_ptid);
> -  inferior_ptid = new_ptid;
> +  private_thread_info *first_pti
> +    = VEC_index (darwin_thread_t, inf->priv->threads, 0);
> +  struct thread_info *first_thread
> +    = thread_info_from_private_thread_info (first_pti);
> +
> +  inferior_ptid = first_thread->ptid;
>  }
>  
>  /* The child must synchronize with gdb: gdb must set the exception port
> @@ -1834,23 +1832,10 @@ darwin_create_inferior (struct target_ops *ops,
>  			const std::string &allargs,
>  			char **env, int from_tty)
>  {
> -  pid_t pid;
> -  ptid_t ptid;
> -
>    /* Do the hard work.  */
> -  pid = fork_inferior (exec_file, allargs, env, darwin_ptrace_me,
> -		       darwin_ptrace_him, darwin_pre_ptrace, NULL,
> -		       darwin_execvp);
> -
> -  ptid = pid_to_ptid (pid);
> -  /* Return now in case of error.  */
> -  if (ptid_equal (inferior_ptid, null_ptid))
> -    return;
> -
> -  /* We have something that executes now.  We'll be running through
> -     the shell at this point (if startup-with-shell is true), but the
> -     pid shouldn't change.  */
> -  add_thread_silent (ptid);
> +  fork_inferior (exec_file, allargs, env, darwin_ptrace_me,
> +		 darwin_ptrace_him, darwin_pre_ptrace, NULL,
> +		 darwin_execvp);

I like the simplification :-).

>  }
>  
>  
> @@ -1920,9 +1905,6 @@ darwin_attach (struct target_ops *ops, const char *args, int from_tty)
>    inferior_appeared (inf, pid);
>    inf->attach_flag = 1;
>  
> -  /* Always add a main thread.  */
> -  add_thread_silent (inferior_ptid);
> -
>    darwin_attach_pid (inf);
>  
>    darwin_suspend_inferior (inf);
> -- 
> 2.7.4

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]