This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] darwin: Do not add a dummy thread
- From: Sergio Durigan Junior <sergiodj at sergiodj dot net>
- To: Simon Marchi <simon dot marchi at ericsson dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Mon, 26 Jun 2017 17:27:36 -0400
- Subject: Re: [PATCH] darwin: Do not add a dummy thread
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=sergiodj.net
- Authentication-results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com EE3A980F8E
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EE3A980F8E
- References: <1498386916-9071-1-git-send-email-simon.marchi@ericsson.com>
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/