[PATCH] darwin: Do not add a dummy thread

[Simon Marchi]

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