[RFA,4/5] Darwin: fix thread ptid started by fork_inferior

  When debugging a program on Mac OS X Darwin, gdb stops with:

Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
Starting program: /private/tmp/helloworld
[New Thread 0xb03 of process 65066]
[New Thread 0xd03 of process 65066]
During startup program terminated with signal SIGTRAP, Trace/breakpoint trap.

When the inferior is started a thread with lwp=tid=0 is created
and has to be fixed later by darwin_init_thread_list(). Because
this is not done, GDB does not understand that the SIGTRAP is
coming from GDB and not the program itself.

gdb/ChangeLog:

        * darwin-nat.c (darwin_check_new_threads): Check if first
        silent thread case.
        (darwin_init_thread_list): Fixup thread ptid.
        (darwin_ptrace_him): Add call.
        (darwin_nat_target::attach): Add call.

Change-Id: Ie8ad7edfff167a1f95140907c75af45ceb3fae14
---
 gdb/darwin-nat.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)
  

On 2018-08-22 06:11, Xavier Roirand wrote:
> When debugging a program on Mac OS X Darwin, gdb stops with:
> 
> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
> Starting program: /private/tmp/helloworld
> [New Thread 0xb03 of process 65066]
> [New Thread 0xd03 of process 65066]
> During startup program terminated with signal SIGTRAP, Trace/breakpoint 
> trap.
> 
> When the inferior is started a thread with lwp=tid=0 is created
> and has to be fixed later by darwin_init_thread_list(). Because
> this is not done, GDB does not understand that the SIGTRAP is
> coming from GDB and not the program itself.

I think I have seen this error in my testing yesterday, though it seemed 
intermittent.  If I started a few times in a row, I would often get that 
SIGTRAP, but it would eventually work...  I can't really comment on the 
validity of the fix, so I will assume it's right (I am not at work right 
now so I can't test on the Mac).  But linux-nat does something similar 
(add a pid-only thread, then change it to its real ptid), so there is a 
precedent.

> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 96f70cf..9ad4a87 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
>  	  pti->gdb_port = new_id;
>  	  pti->msg_state = DARWIN_RUNNING;
> 
> -	  /* Add the new thread.  */
> -	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
> +	  if (old_nbr == 0 && new_ix == 0)
> +            {
> +	      /* A ptid is created when the inferior is started (see
> +                 fork-child.c) with lwp=tid=0.  This ptid will be 
> renamed
> +                 later by darwin_init_thread_list (), so find this 
> previous
> +                 thread silently added.  */
> +
> +              struct thread_info *tp = find_thread_ptid (ptid_t
> (inf->pid, 0, 0));
> +              tp->priv.reset (pti);

Put a gdb_assert (tp != nullptr), so that if for some reason the thread 
is not found (because of a GDB bug), we fail with a failed assertion 
rather than a segfault.

> +            }
> +          else
> +            {
> +              /* Add the new thread.  */
> +              add_thread_with_info (ptid_t (inf->pid, 0, new_id), 
> pti);
> +             }
> +
>  	  new_thread_vec.push_back (pti);
>  	  new_ix++;
>  	  continue;
> @@ -1733,6 +1747,8 @@ thread_info_from_private_thread_info
> (darwin_thread_info *pti)
>  static void
>  darwin_init_thread_list (struct inferior *inf)
>  {
> +  ptid_t new_ptid;
> +
>    darwin_check_new_threads (inf);
> 
>    darwin_inferior *priv = get_darwin_inferior (inf);
> @@ -1743,7 +1759,11 @@ darwin_init_thread_list (struct inferior *inf)
>    struct thread_info *first_thread
>      = thread_info_from_private_thread_info (first_pti);
> 
> -  inferior_ptid = first_thread->ptid;
> +  /* Note: fork_inferior automatically add a thread but it uses a 
> wrong ptid.
> +     Fix up.  */
> +  new_ptid = ptid_t (inf->pid, 0, first_pti->gdb_port);
> +  thread_change_ptid (inferior_ptid, new_ptid);
> +  inferior_ptid = new_ptid;

You can declare new_ptid when initializing it.

Simon
  

On 08/22/2018 11:11 AM, Xavier Roirand wrote:

> 
> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
> index 96f70cf..9ad4a87 100644
> --- a/gdb/darwin-nat.c
> +++ b/gdb/darwin-nat.c
> @@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
>  	  pti->gdb_port = new_id;
>  	  pti->msg_state = DARWIN_RUNNING;
>  
> -	  /* Add the new thread.  */
> -	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
> +	  if (old_nbr == 0 && new_ix == 0)
> +            {
> +	      /* A ptid is created when the inferior is started (see
> +                 fork-child.c) with lwp=tid=0.  

It looks like this patch was written against an older gdb,
because fork-child.c doesn't add a thread nowadays.  For GNU/Linux, it's
inf-ptrace.c that adds the initial thread, but only after
fork_inferior returns (inf_ptrace_target::create_inferior).

But were is that equivalent code in darwin-nat.c?

/me looks.

Answer: it's nowhere.  It does not exist.

So, when then shared fork-child.c was created a while ago,
the add_thread call was moved to darwin-nat.c's target_create_inferior
implementation.  But, later on, Simon removed that add_thread call with:

 commit db665f427ca781d631d9e29b1bb744fb11ffcbba
 Author:     Simon Marchi <simon.marchi@ericsson.com>
 AuthorDate: Tue Jun 27 10:55:36 2017 +0200
 Commit:     Simon Marchi <simon.marchi@ericsson.com>
 CommitDate: Tue Jun 27 10:56:53 2017 +0200

    darwin: Do not add a dummy thread

(Weird, I can't find that patch on the list's archives, even
though I received a local copy.)

It sounds to me like you need to reevaluate the patch from
scratch, because its premise is invalid.

> @@ -1933,6 +1956,8 @@ darwin_nat_target::attach (const char *args, int from_tty)
>    inferior_appeared (inf, pid);
>    inf->attach_flag = 1;
>  
> +  add_thread_silent (inferior_ptid);
> +
>    darwin_attach_pid (inf);
>  
>    darwin_suspend_inferior (inf);

This surely is not related to "run" and/or fork-inferior.c

Thanks,
Pedro Alves
  

On 2018-08-22 12:10, Pedro Alves wrote:
> On 08/22/2018 11:11 AM, Xavier Roirand wrote:
> 
>> 
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index 96f70cf..9ad4a87 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -344,8 +344,22 @@ darwin_check_new_threads (struct inferior *inf)
>>  	  pti->gdb_port = new_id;
>>  	  pti->msg_state = DARWIN_RUNNING;
>> 
>> -	  /* Add the new thread.  */
>> -	  add_thread_with_info (ptid_t (inf->pid, 0, new_id), pti);
>> +	  if (old_nbr == 0 && new_ix == 0)
>> +            {
>> +	      /* A ptid is created when the inferior is started (see
>> +                 fork-child.c) with lwp=tid=0.
> 
> It looks like this patch was written against an older gdb,
> because fork-child.c doesn't add a thread nowadays.  For GNU/Linux, 
> it's
> inf-ptrace.c that adds the initial thread, but only after
> fork_inferior returns (inf_ptrace_target::create_inferior).
> 
> But were is that equivalent code in darwin-nat.c?
> 
> /me looks.
> 
> Answer: it's nowhere.  It does not exist.
> 
> So, when then shared fork-child.c was created a while ago,
> the add_thread call was moved to darwin-nat.c's target_create_inferior
> implementation.  But, later on, Simon removed that add_thread call 
> with:
> 
>  commit db665f427ca781d631d9e29b1bb744fb11ffcbba
>  Author:     Simon Marchi <simon.marchi@ericsson.com>
>  AuthorDate: Tue Jun 27 10:55:36 2017 +0200
>  Commit:     Simon Marchi <simon.marchi@ericsson.com>
>  CommitDate: Tue Jun 27 10:56:53 2017 +0200
> 
>     darwin: Do not add a dummy thread
> 
> (Weird, I can't find that patch on the list's archives, even
> though I received a local copy.)
> 
> It sounds to me like you need to reevaluate the patch from
> scratch, because its premise is invalid.

Hmm I had not considered that, I did that a while ago and had completely 
forgotten about it.  I'll try to take a look tomorrow, when I have 
access to the Mac machine.  Maybe the spurious SIGTRAP I saw when 
running was caused by the bug fixed by patch 5/5.

Simon
  

>>>>> "Xavier" == Xavier Roirand <roirand@adacore.com> writes:

Xavier> When debugging a program on Mac OS X Darwin, gdb stops with:
Xavier> Temporary breakpoint 1 at 0x100000fb4: file /tmp/helloworld.c, line 1.
Xavier> Starting program: /private/tmp/helloworld
Xavier> [New Thread 0xb03 of process 65066]
Xavier> [New Thread 0xd03 of process 65066]
Xavier> During startup program terminated with signal SIGTRAP, Trace/breakpoint trap.

I applied patch #2 from this series locally, but not patch #4.  Then I
ran gdb on a simple test program.  This worked fine.  So at least here
this patch seems not to be needed now.

Tom