[RFA,4/5] Darwin: fix thread ptid started by fork_inferior
Commit Message
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(-)
Comments
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
@@ -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);
+ }
+ 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;
}
/* The child must synchronize with gdb: gdb must set the exception port
@@ -1806,6 +1826,9 @@ darwin_ptrace_him (int pid)
int traps_expected;
struct inferior *inf = current_inferior ();
+ inferior_ptid = ptid_t (pid);
+ add_thread_silent (inferior_ptid);
+
darwin_attach_pid (inf);
/* Let's the child run. */
@@ -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);