Re: [PATCH 1/2] Port gdbserver to GNU/Hurd

[Pedro Alves <palves at redhat dot com>]

On 09/09/2013 10:58 AM, Thomas Schwinge wrote:
> Hi!
> 
> On Sun, 8 Sep 2013 21:35:05 +0800, Yue Lu <hacklu.newborn@gmail.com> wrote:
>> On Fri, Sep 6, 2013 at 5:37 AM, Thomas Schwinge <thomas@codesourcery.com> wrote:
>>>> (correct me if
>>>> I'm wrong here), the Hurd's threads are kernel threads
>>>
>>> Correct.
>>>
>>>> so it'd
>>>> be better to just make the GDB side use the lwp field too.
>>>> It's really a simple and mechanic change.  Nothing in GDB core
>>>> actually cares which field is used.  So in this case, it'd be
> 
> In GDB's parlance, a lightweight process (identified by a LWP) is a
> thread that always has a corresponding kernel thread, and in contrast a
> "generic" thread (identified by a TID) is not required to always have a
> corresponding kernel thread, for example, when managed by a run-time
> library?  Then, yes, conceptually the native Hurd port should be switched
> to using LWPs instead of TIDs.
> 
>>>> better if you send a preparatory patch
>>>
>>> Based on the current upstream master branch.
>>
>> Should I change the gdb use lwp filed instead of tid field? There are
>> too many functions use tid. Like
>> make_proc(),inf_tid_to_thread(),ptid_build(), and there is a field
>> named tid in the structure proc also.
> 
> As you have found, there is a lot of TID usage in gnu-nat.c.  TIDs are
> assigned based on the next_thread_id variable:
> 
>     /* A variable from which to assign new TIDs.  */
>     static int next_thread_id = 1;
>     [...]
>               /* THREADS[I] is a thread we don't know about yet!  */
>               {
>                 ptid_t ptid;
>     
>                 thread = make_proc (inf, threads[i], next_thread_id++);
> 
> Five years ago, we've already concluded this is due for some cleanup,
> <http://www.gnu.org/software/hurd/open_issues/gdb_thread_ids.html>.  But
> I don't want to require this cleanup to happen before/in context of the
> Google Summer of Code project's code submission discussed here.

That's not what I'm suggesting at all.  I'm just talking about storing
the thread id in the lwpid field.  It's always the target that
stores and extracts the fields of a ptid -- the ptid_t struct is mostly
opaque to the core.  It should really be a small change.

(while looking at this, I noticed a bug, and fixed it:
http://sourceware.org/ml/gdb-patches/2013-09/msg00579.html)

/me gives it a try.

I grepped for ptid_build and ptid_get_tid in *gnu* files, and
adjusted all I found.

-----------
Subject: [PATCH] [Hurd/gnu-nat.c] Use ptid_t.lwpid to store thread ids
 instead of ptid_t.tid.

In preparation for reusing gnu-nat.c in gdbserver, switch to storing
thread ids in the lwpid field of ptid_t rather than in the tid
field.  The Hurd's thread model is 1:1, so it doesn't feel wrong
anyway.

gdb/
2013-09-18  Pedro Alves  <palves@redhat.com>

	* gnu-nat.c (inf_validate_procs, gnu_wait, gnu_resume)
	(gnu_create_inferior)
	(gnu_attach, gnu_thread_alive, gnu_pid_to_str, cur_thread)
	(set_sig_thread_cmd): Use the lwpid field of ptids to store the
	thread id instead of the tid field.
---
 gdb/gnu-nat.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index fa55b10..b4f99f8 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -1083,7 +1083,7 @@ inf_validate_procs (struct inf *inf)
 	    last = thread;
 	    proc_debug (thread, "new thread: %d", threads[i]);
 
-	    ptid = ptid_build (inf->pid, 0, thread->tid);
+	    ptid = ptid_build (inf->pid, thread->tid, 0);
 
 	    /* Tell GDB's generic thread code.  */
 
@@ -1613,17 +1613,17 @@ rewait:
 
   thread = inf->wait.thread;
   if (thread)
-    ptid = ptid_build (inf->pid, 0, thread->tid);
+    ptid = ptid_build (inf->pid, thread->tid, 0);
   else if (ptid_equal (ptid, minus_one_ptid))
     thread = inf_tid_to_thread (inf, -1);
   else
-    thread = inf_tid_to_thread (inf, ptid_get_tid (ptid));
+    thread = inf_tid_to_thread (inf, ptid_get_lwp (ptid));
 
   if (!thread || thread->port == MACH_PORT_NULL)
     {
       /* TID is dead; try and find a new thread.  */
       if (inf_update_procs (inf) && inf->threads)
-	ptid = ptid_build (inf->pid, 0, inf->threads->tid); /* The first
+	ptid = ptid_build (inf->pid, inf->threads->tid, 0); /* The first
 							       available
 							       thread.  */
       else
@@ -2022,7 +2022,7 @@ gnu_resume (struct target_ops *ops,
   else
     /* Just allow a single thread to run.  */
     {
-      struct proc *thread = inf_tid_to_thread (inf, ptid_get_tid (ptid));
+      struct proc *thread = inf_tid_to_thread (inf, ptid_get_lwp (ptid));
 
       if (!thread)
 	error (_("Can't run single thread id %s: no such thread!"),
@@ -2033,7 +2033,7 @@ gnu_resume (struct target_ops *ops,
 
   if (step)
     {
-      step_thread = inf_tid_to_thread (inf, ptid_get_tid (ptid));
+      step_thread = inf_tid_to_thread (inf, ptid_get_lwp (ptid));
       if (!step_thread)
 	warning (_("Can't step thread id %s: no such thread."),
 		 target_pid_to_str (ptid));
@@ -2133,7 +2133,7 @@ gnu_create_inferior (struct target_ops *ops,
 
   /* We now have thread info.  */
   thread_change_ptid (inferior_ptid,
-		      ptid_build (inf->pid, 0, inf_pick_first_thread ()));
+		      ptid_build (inf->pid, inf_pick_first_thread (), 0));
 
   startup_inferior (inf->pending_execs);
 
@@ -2190,7 +2190,7 @@ gnu_attach (struct target_ops *ops, char *args, int from_tty)
 
   inf_update_procs (inf);
 
-  inferior_ptid = ptid_build (pid, 0, inf_pick_first_thread ());
+  inferior_ptid = ptid_build (pid, inf_pick_first_thread (), 0);
 
   /* We have to initialize the terminal settings now, since the code
      below might try to restore them.  */
@@ -2261,7 +2261,7 @@ gnu_thread_alive (struct target_ops *ops, ptid_t ptid)
 {
   inf_update_procs (gnu_current_inf);
   return !!inf_tid_to_thread (gnu_current_inf,
-			      ptid_get_tid (ptid));
+			      ptid_get_lwp (ptid));
 }
 
 
@@ -2596,7 +2596,7 @@ static char *
 gnu_pid_to_str (struct target_ops *ops, ptid_t ptid)
 {
   struct inf *inf = gnu_current_inf;
-  int tid = ptid_get_tid (ptid);
+  int tid = ptid_get_lwp (ptid);
   struct proc *thread = inf_tid_to_thread (inf, tid);
 
   if (thread)
@@ -2729,7 +2729,7 @@ cur_thread (void)
 {
   struct inf *inf = cur_inf ();
   struct proc *thread = inf_tid_to_thread (inf,
-					   ptid_get_tid (inferior_ptid));
+					   ptid_get_lwp (inferior_ptid));
   if (!thread)
     error (_("No current thread."));
   return thread;
@@ -2928,7 +2928,7 @@ set_sig_thread_cmd (char *args, int from_tty)
 	error (_("Thread ID %s not known.  "
 		 "Use the \"info threads\" command to\n"
 	       "see the IDs of currently known threads."), args);
-      inf->signal_thread = inf_tid_to_thread (inf, ptid_get_tid (ptid));
+      inf->signal_thread = inf_tid_to_thread (inf, ptid_get_lwp (ptid));
     }
 }
 
-- 
1.7.11.7