[PATCH 0/2] remote.c: ptid.tid -> ptid.lwp

[PATCH 0/2] remote.c: ptid.tid -> ptid.lwp

[Pedro Alves]

See patch #1 for details.

I plan to push this in soon, assuming no barring comments.

Pedro Alves (2):
  remote.c: Use the ptid.lwp field to store remote thread ids rather
    than ptid.tid.
  common/ptid.h: Mention that process_stratum targets should prefer
    ptid.lwp.

 gdb/common/ptid.h |  7 ++++++-
 gdb/remote.c      | 34 +++++++++++++++++-----------------
 2 files changed, 23 insertions(+), 18 deletions(-)

-- 
1.7.11.7

[PATCH 1/2] remote.c: Use the ptid.lwp field to store remote thread ids rather than ptid.tid.

[Pedro Alves]

From GDB's perspective, independently of how the target really
implements threads, gdb/remote sees all threads as if kernel/system
threads.  A rationale along theses lines led to gdbserver storing
thread ids in ptid.lwp in all ports.

I believe that on the GDB side too, it's best that we standardize on
process_stratum targets using the ptid.lwp field to store thread ids.
The idea being leave the ptid.tid field free for any thread_stratum
target that might want to sit on top.

Because remote.c is currently using ptid.tid, we can't make gdbserver
and gdb share bits of remote-specific code that manipulates ptids.

This patch thus makes remote.c use ptid.lwp instead of ptid.tid.

Tested on x86_64 Fedora 17, w/ local gdbserver.

gdb/
2014-02-12  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_thread_alive, write_ptid, read_ptid)
	(read_ptid, remote_newthread_step, remote_threads_extra_info)
	(remote_get_ada_task_ptid, append_resumption, remote_stop_ns)
	(threadalive_test, remote_pid_to_str, _initialize_remote): Use the
	ptid.lwp field to store remote thread ids rather than ptid.tid.
---
 gdb/remote.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index eff4c44..02f2669 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1877,7 +1877,7 @@ remote_thread_alive (struct target_ops *ops, ptid_t ptid)
     /* The main thread is always alive.  */
     return 1;
 
-  if (ptid_get_pid (ptid) != 0 && ptid_get_tid (ptid) == 0)
+  if (ptid_get_pid (ptid) != 0 && ptid_get_lwp (ptid) == 0)
     /* The main thread is always alive.  This can happen after a
        vAttach, if the remote side doesn't support
        multi-threading.  */
@@ -2021,7 +2021,7 @@ write_ptid (char *buf, const char *endbuf, ptid_t ptid)
       else
 	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
     }
-  tid = ptid_get_tid (ptid);
+  tid = ptid_get_lwp (ptid);
   if (tid < 0)
     buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
   else
@@ -2051,7 +2051,7 @@ read_ptid (char *buf, char **obuf)
       pp = unpack_varlen_hex (p + 1, &tid);
       if (obuf)
 	*obuf = pp;
-      return ptid_build (pid, 0, tid);
+      return ptid_build (pid, tid, 0);
     }
 
   /* No multi-process.  Just a tid.  */
@@ -2068,7 +2068,7 @@ read_ptid (char *buf, char **obuf)
 
   if (obuf)
     *obuf = pp;
-  return ptid_build (pid, 0, tid);
+  return ptid_build (pid, tid, 0);
 }
 
 /* Encode 64 bits in 16 chars of hex.  */
@@ -2618,7 +2618,7 @@ static int
 remote_newthread_step (threadref *ref, void *context)
 {
   int pid = ptid_get_pid (inferior_ptid);
-  ptid_t ptid = ptid_build (pid, 0, threadref_to_int (ref));
+  ptid_t ptid = ptid_build (pid, threadref_to_int (ref), 0);
 
   if (!in_thread_list (ptid))
     add_thread (ptid);
@@ -2887,7 +2887,7 @@ remote_threads_extra_info (struct thread_info *tp)
 		    _("remote_threads_extra_info"));
 
   if (ptid_equal (tp->ptid, magic_null_ptid)
-      || (ptid_get_pid (tp->ptid) != 0 && ptid_get_tid (tp->ptid) == 0))
+      || (ptid_get_pid (tp->ptid) != 0 && ptid_get_lwp (tp->ptid) == 0))
     /* This is the main thread which was added by GDB.  The remote
        server doesn't know about it.  */
     return NULL;
@@ -2926,7 +2926,7 @@ remote_threads_extra_info (struct thread_info *tp)
   rs->use_threadextra_query = 0;
   set = TAG_THREADID | TAG_EXISTS | TAG_THREADNAME
     | TAG_MOREDISPLAY | TAG_DISPLAY;
-  int_to_threadref (&id, ptid_get_tid (tp->ptid));
+  int_to_threadref (&id, ptid_get_lwp (tp->ptid));
   if (remote_get_threadinfo (&id, set, &threadinfo))
     if (threadinfo.active)
       {
@@ -3035,7 +3035,7 @@ remote_static_tracepoint_markers_by_strid (const char *strid)
 static ptid_t
 remote_get_ada_task_ptid (long lwp, long thread)
 {
-  return ptid_build (ptid_get_pid (inferior_ptid), 0, lwp);
+  return ptid_build (ptid_get_pid (inferior_ptid), lwp, 0);
 }
 \f
 
@@ -4847,7 +4847,7 @@ append_resumption (char *p, char *endp,
       ptid_t nptid;
 
       /* All (-1) threads of process.  */
-      nptid = ptid_build (ptid_get_pid (ptid), 0, -1);
+      nptid = ptid_build (ptid_get_pid (ptid), -1, 0);
 
       p += xsnprintf (p, endp - p, ":");
       p = write_ptid (p, endp, nptid);
@@ -5162,7 +5162,7 @@ remote_stop_ns (ptid_t ptid)
 
       if (ptid_is_pid (ptid))
 	  /* All (-1) threads of process.  */
-	nptid = ptid_build (ptid_get_pid (ptid), 0, -1);
+	nptid = ptid_build (ptid_get_pid (ptid), -1, 0);
       else
 	{
 	  /* Small optimization: if we already have a stop reply for
@@ -9314,7 +9314,7 @@ threadalive_test (char *cmd, int tty)
 {
   int sample_thread = SAMPLE_THREAD;
   int pid = ptid_get_pid (inferior_ptid);
-  ptid_t ptid = ptid_build (pid, 0, sample_thread);
+  ptid_t ptid = ptid_build (pid, sample_thread, 0);
 
   if (remote_thread_alive (ptid))
     printf_filtered ("PASS: Thread alive test\n");
@@ -9462,10 +9462,10 @@ remote_pid_to_str (struct target_ops *ops, ptid_t ptid)
 	xsnprintf (buf, sizeof buf, "Thread <main>");
       else if (rs->extended && remote_multi_process_p (rs))
 	xsnprintf (buf, sizeof buf, "Thread %d.%ld",
-		   ptid_get_pid (ptid), ptid_get_tid (ptid));
+		   ptid_get_pid (ptid), ptid_get_lwp (ptid));
       else
 	xsnprintf (buf, sizeof buf, "Thread %ld",
-		   ptid_get_tid (ptid));
+		   ptid_get_lwp (ptid));
       return buf;
     }
 }
@@ -12318,11 +12318,11 @@ stepping is supported by the target.  The default is on."),
   /* Eventually initialize fileio.  See fileio.c */
   initialize_remote_fileio (remote_set_cmdlist, remote_show_cmdlist);
 
-  /* Take advantage of the fact that the LWP field is not used, to tag
+  /* Take advantage of the fact that the TID field is not used, to tag
      special ptids with it set to != 0.  */
-  magic_null_ptid = ptid_build (42000, 1, -1);
-  not_sent_ptid = ptid_build (42000, 1, -2);
-  any_thread_ptid = ptid_build (42000, 1, 0);
+  magic_null_ptid = ptid_build (42000, -1, 1);
+  not_sent_ptid = ptid_build (42000, -2, 1);
+  any_thread_ptid = ptid_build (42000, 0, 1);
 
   target_buf_size = 2048;
   target_buf = xmalloc (target_buf_size);
-- 
1.7.11.7

Re: [PATCH 1/2] remote.c: Use the ptid.lwp field to store remote thread ids rather than ptid.tid.

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I believe that on the GDB side too, it's best that we standardize on
Pedro> process_stratum targets using the ptid.lwp field to store thread ids.
Pedro> The idea being leave the ptid.tid field free for any thread_stratum
Pedro> target that might want to sit on top.

Thanks for doing this.

Pedro> Because remote.c is currently using ptid.tid, we can't make gdbserver
Pedro> and gdb share bits of remote-specific code that manipulates ptids.

Pedro reminded me that the functions in question are read_ptid and
write_ptid.

Tom

Re: [PATCH 1/2] remote.c: Use the ptid.lwp field to store remote thread ids rather than ptid.tid.

[Pedro Alves]

On 02/12/2014 04:56 PM, Tom Tromey wrote:
> Pedro reminded me that the functions in question are read_ptid and
> write_ptid.

I'll add this to the commit log.

-- 
Pedro Alves

Re: [PATCH 1/2] remote.c: Use the ptid.lwp field to store remote thread ids rather than ptid.tid.

[Doug Evans]

On Wed, Feb 12, 2014 at 6:06 AM, Pedro Alves <palves@redhat.com> wrote:
> From GDB's perspective, independently of how the target really
> implements threads, gdb/remote sees all threads as if kernel/system
> threads.  A rationale along theses lines led to gdbserver storing
> thread ids in ptid.lwp in all ports.
>
> I believe that on the GDB side too, it's best that we standardize on
> process_stratum targets using the ptid.lwp field to store thread ids.
> The idea being leave the ptid.tid field free for any thread_stratum
> target that might want to sit on top.
>
> Because remote.c is currently using ptid.tid, we can't make gdbserver
> and gdb share bits of remote-specific code that manipulates ptids.
>
> This patch thus makes remote.c use ptid.lwp instead of ptid.tid.
>
> Tested on x86_64 Fedora 17, w/ local gdbserver.
>
> gdb/
> 2014-02-12  Pedro Alves  <palves@redhat.com>
>
>         * remote.c (remote_thread_alive, write_ptid, read_ptid)
>         (read_ptid, remote_newthread_step, remote_threads_extra_info)
>         (remote_get_ada_task_ptid, append_resumption, remote_stop_ns)
>         (threadalive_test, remote_pid_to_str, _initialize_remote): Use the
>         ptid.lwp field to store remote thread ids rather than ptid.tid.

Awesome.  Thanks.

[PATCH 2/2] common/ptid.h: Mention that process_stratum targets should prefer ptid.lwp.

[Pedro Alves]

It's best that we standardize on process_stratum targets using the
ptid.lwp field to store thread ids.  The idea being leave the ptid.tid
field free for any thread_stratum target that might want to sit on
top.

gdb/
2014-02-12  Pedro Alves  <palves@redhat.com>

	* common/ptid.h (struct ptid): Mention that process_stratum
	targets should prefer ptid.lwp.
---
 gdb/common/ptid.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
index 362882d..cc1825e 100644
--- a/gdb/common/ptid.h
+++ b/gdb/common/ptid.h
@@ -25,7 +25,12 @@
    consists of the process id (pid), lightweight process id (lwp) and
    thread id (tid).  When manipulating ptids, the constructors,
    accessors, and predicates declared in this file should be used.  Do
-   NOT access the struct ptid members directly.  */
+   NOT access the struct ptid members directly.
+
+   process_stratum targets that handle threading themselves should
+   prefer using the ptid.lwp field, leaving the ptid.tid field for any
+   thread_stratum target that might want to sit on top.
+*/
 
 struct ptid
 {
-- 
1.7.11.7

Re: [PATCH 0/2] remote.c: ptid.tid -> ptid.lwp

[Pedro Alves]

On 02/12/2014 02:06 PM, Pedro Alves wrote:

I've pushed this in now.

-- 
Pedro Alves