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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-06-26 23:27, Sergio Durigan Junior wrote:
> 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?

Indeed, I tried to run the testsuite on a commit prior to your patchset, 
and it was disastrous.

Interesting fact: when you type "start", you stop at main with thread 2, 
and there's no thread 1.  What I think happen is that we first check for 
new threads after gdb has forked, but before it has exec'ed, that gives 
us a thread with some tid (or whatever we use as a tid).  After the 
exec, once we stop at main, we check again for new threads.  The only 
thread has a different tid than before, so gdb assumes that thread 1 has 
exited and thread 2 appeared.  It doesn't really matter, it's just 
strange.

> > 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.

Oops, thanks.

> > +{
> > +  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.

Makes sense, I'll update that.

> > -  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,

Thanks.!

Simon