This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]