Commit Message
I grew a bit tired of using ptid_get_{lwp,pid,tid} and friends, so I
decided to make it a bit easier to use by making it a proper class.
Because ptid_t is used in things that aren't constructed, it is not
possible to have a constructor. Instead I added a "build" static
method, which maps well to the current ptid_build anyway, and ptid_t is
basically just a plain old data type with read-only methods. The
difference with before is that the fields are private, so it's not
possible to change a ptid_t field by mistake.
The new methods of ptid_t map to existing functions/practice like this:
ptid_t::build (pid, lwp, tid) -> ptid_build (pid, lwp, tid)
ptid_t::build (pid) -> pid_to_ptid (pid)
ptid.is_pid () -> ptid_is_pid (ptid)
ptid == other -> ptid_equal (ptid, other)
ptid.is_null () -> ptid_equal (ptid, null_ptid)
ptid.is_any () -> ptid_equal (ptid, minus_one_ptid)
ptid.pid () -> ptid_get_pid (ptid)
ptid.lwp_p () -> ptid_lwp_p (ptid)
ptid.lwp () -> ptid_get_lwp (ptid)
ptid.tid_p () -> ptid_tid_p (ptid)
ptid.tid () -> ptid_get_tid (ptid)
ptid.matches (filter) -> ptid_match (ptid, filter)
I've replaced the implementation of the existing functions with calls to
the new methods. People are encouraged to gradually switch to using the
ptid_t methods instead of the functions (or we can change them all in
one pass eventually).
Also, I'm not sure if it's worth it (because of ptid_t's relatively
small size), but I have made the functions and methods take ptid_t
arguments by const reference instead of by value.
gdb/ChangeLog:
* common/ptid.h (struct ptid): Change to...
(class ptid_t): ... this.
<build>: New static method.
<is_pid, is_null, is_any, pid, lwp_p, lwp, tid_p, tid,
operator==, matches>: New methods.
<pid>: Rename to...
<m_pid>: ...this.
<lwp>: Rename to...
<m_lwp>: ...this.
<tid>: Rename to...
<m_tid>: ...this.
(ptid_get_pid, ptid_get_lwp, ptid_get_tid, ptid_equal,
ptid_is_pid, ptid_lwp_p, ptid_tid_p, ptid_match): Take arguments
as references.
* common/ptid.c (null_ptid, minus_one_ptid): Initialize with
ptid_t::build.
(ptid_build, pid_to_ptid, ptid_get_pid, ptid_get_tid,
ptid_equal, ptid_is_pid, ptid_lwp_p, ptid_tid_p, ptid_match):
Take arguments as references, implement using ptid_t methods.
gdb/gdbserver/ChangeLog:
* server.c (handle_v_cont): Initialize thread_resume::thread
with null_ptid.
---
gdb/common/ptid.c | 62 +++++++++++-----------------
gdb/common/ptid.h | 109 +++++++++++++++++++++++++++++++++++++++----------
gdb/gdbserver/server.c | 4 +-
3 files changed, 114 insertions(+), 61 deletions(-)
Comments
Hi Simon,
Hmm, "unit tests or it didn't happen" ? :-)
On 04/04/2017 07:32 PM, Simon Marchi wrote:
> I grew a bit tired of using ptid_get_{lwp,pid,tid} and friends, so I
> decided to make it a bit easier to use by making it a proper class.
>
> Because ptid_t is used in things that aren't constructed, it is not
> possible to have a constructor. Instead I added a "build" static
> method, which maps well to the current ptid_build anyway, and ptid_t is
> basically just a plain old data type with read-only methods. The
> difference with before is that the fields are private, so it's not
> possible to change a ptid_t field by mistake.
>
> The new methods of ptid_t map to existing functions/practice like this:
>
> ptid_t::build (pid, lwp, tid) -> ptid_build (pid, lwp, tid)
> ptid_t::build (pid) -> pid_to_ptid (pid)
Not sure these two are an improvement. pid_to_ptid is the
counterpart of ptid_is_pid, and that is lost with the
overloading of ptid_t::build.
> ptid.is_pid () -> ptid_is_pid (ptid)
> ptid == other -> ptid_equal (ptid, other)
> ptid.is_null () -> ptid_equal (ptid, null_ptid)
> ptid.is_any () -> ptid_equal (ptid, minus_one_ptid)
> ptid.pid () -> ptid_get_pid (ptid)
> ptid.lwp_p () -> ptid_lwp_p (ptid)
> ptid.lwp () -> ptid_get_lwp (ptid)
> ptid.tid_p () -> ptid_tid_p (ptid)
> ptid.tid () -> ptid_get_tid (ptid)
> ptid.matches (filter) -> ptid_match (ptid, filter)
>
> I've replaced the implementation of the existing functions with calls to
> the new methods. People are encouraged to gradually switch to using the
> ptid_t methods instead of the functions (or we can change them all in
> one pass eventually).
>
> Also, I'm not sure if it's worth it (because of ptid_t's relatively
> small size), but I have made the functions and methods take ptid_t
> arguments by const reference instead of by value.
I'd guess that the structure is still sufficiently small that passing
by value would be a benefit (plus, it avoids inefficiency caused
by the compiler having to assume that the references can alias),
but OTOH, this structure is likely to grow with the multi-target
work. Fine with me to go with what you have.
>
> /* See ptid.h for these. */
>
> -ptid_t null_ptid = { 0, 0, 0 };
> -ptid_t minus_one_ptid = { -1, 0, 0 };
> +ptid_t null_ptid = ptid_t::build (0, 0, 0);
> +ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
It's probably going to be worth it to sprinkle "constexpr"
all over the new API. Helps with static_asserts in
unit testing too. *cough* :-)
> -struct ptid
> +class ptid_t
> {
> +public:
> + static ptid_t build (int pid, long lwp = 0, long tid = 0)
> + {
> + ptid_t ptid;
> +
> + ptid.m_pid = pid;
> + ptid.m_lwp = lwp;
> + ptid.m_tid = tid;
> +
> + return ptid;
> + }
> +
> + bool is_pid () const
> + {
> + if (is_any () || is_null())
Missing space after "null".
Wonder about migrating/copying the comments API comments to
the methods, if these are the entry points that people should
be looking at going forward.
> + return false;
> +
> + return m_lwp == 0 && m_tid == 0;
> + }
> +
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 4bc7f71b00..1287114cc1 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -2654,7 +2654,9 @@ handle_v_cont (char *own_buf)
> char *p, *q;
> int n = 0, i = 0;
> struct thread_resume *resume_info;
> - struct thread_resume default_action = {{0}};
> + struct thread_resume default_action = {
> + .thread = null_ptid,
> + };
Note that C99 designated initializers are not valid C++11.
Not sure whether any compiler _doesn't_ support them though.
Thanks,
Pedro Alves
On 2017-04-05 11:47, Pedro Alves wrote:
> Hi Simon,
>
> Hmm, "unit tests or it didn't happen" ? :-)
Right, I don't have the unit test in GDB mindset yet. But of course,
it's a good idea, I'll do it.
> On 04/04/2017 07:32 PM, Simon Marchi wrote:
>> I grew a bit tired of using ptid_get_{lwp,pid,tid} and friends, so I
>> decided to make it a bit easier to use by making it a proper class.
>>
>> Because ptid_t is used in things that aren't constructed, it is not
>> possible to have a constructor. Instead I added a "build" static
>> method, which maps well to the current ptid_build anyway, and ptid_t
>> is
>> basically just a plain old data type with read-only methods. The
>> difference with before is that the fields are private, so it's not
>> possible to change a ptid_t field by mistake.
>>
>> The new methods of ptid_t map to existing functions/practice like
>> this:
>>
>> ptid_t::build (pid, lwp, tid) -> ptid_build (pid, lwp, tid)
>> ptid_t::build (pid) -> pid_to_ptid (pid)
>
> Not sure these two are an improvement. pid_to_ptid is the
> counterpart of ptid_is_pid, and that is lost with the
> overloading of ptid_t::build.
Would you prefer having a ptid_t::from_pid method instead? It would be
the counter part of ptid_t::is_pid. Or do you prefer if we keep the
current function?
>> ptid.is_pid () -> ptid_is_pid (ptid)
>> ptid == other -> ptid_equal (ptid, other)
>> ptid.is_null () -> ptid_equal (ptid, null_ptid)
>> ptid.is_any () -> ptid_equal (ptid, minus_one_ptid)
>> ptid.pid () -> ptid_get_pid (ptid)
>> ptid.lwp_p () -> ptid_lwp_p (ptid)
>> ptid.lwp () -> ptid_get_lwp (ptid)
>> ptid.tid_p () -> ptid_tid_p (ptid)
>> ptid.tid () -> ptid_get_tid (ptid)
>> ptid.matches (filter) -> ptid_match (ptid, filter)
>>
>> I've replaced the implementation of the existing functions with calls
>> to
>> the new methods. People are encouraged to gradually switch to using
>> the
>> ptid_t methods instead of the functions (or we can change them all in
>> one pass eventually).
>>
>> Also, I'm not sure if it's worth it (because of ptid_t's relatively
>> small size), but I have made the functions and methods take ptid_t
>> arguments by const reference instead of by value.
>
> I'd guess that the structure is still sufficiently small that passing
> by value would be a benefit (plus, it avoids inefficiency caused
> by the compiler having to assume that the references can alias),
> but OTOH, this structure is likely to grow with the multi-target
> work. Fine with me to go with what you have.
Ok.
>>
>> /* See ptid.h for these. */
>>
>> -ptid_t null_ptid = { 0, 0, 0 };
>> -ptid_t minus_one_ptid = { -1, 0, 0 };
>> +ptid_t null_ptid = ptid_t::build (0, 0, 0);
>> +ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
>
> It's probably going to be worth it to sprinkle "constexpr"
> all over the new API. Helps with static_asserts in
> unit testing too. *cough* :-)
Ok, will look into it.
>> -struct ptid
>> +class ptid_t
>> {
>> +public:
>> + static ptid_t build (int pid, long lwp = 0, long tid = 0)
>> + {
>> + ptid_t ptid;
>> +
>> + ptid.m_pid = pid;
>> + ptid.m_lwp = lwp;
>> + ptid.m_tid = tid;
>> +
>> + return ptid;
>> + }
>> +
>> + bool is_pid () const
>> + {
>> + if (is_any () || is_null())
>
> Missing space after "null".
Thanks, fixed
> Wonder about migrating/copying the comments API comments to
> the methods, if these are the entry points that people should
> be looking at going forward.
Right, it would make sense.
>> + return false;
>> +
>> + return m_lwp == 0 && m_tid == 0;
>> + }
>> +
>
>> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
>> index 4bc7f71b00..1287114cc1 100644
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -2654,7 +2654,9 @@ handle_v_cont (char *own_buf)
>> char *p, *q;
>> int n = 0, i = 0;
>> struct thread_resume *resume_info;
>> - struct thread_resume default_action = {{0}};
>> + struct thread_resume default_action = {
>> + .thread = null_ptid,
>> + };
>
> Note that C99 designated initializers are not valid C++11.
> Not sure whether any compiler _doesn't_ support them though.
Ok. But anyway C++11-style initialization is probably better anyway.
Is the following ok?
struct thread_resume default_action { null_ptid };
Thanks,
Simon
@@ -22,20 +22,16 @@
/* See ptid.h for these. */
-ptid_t null_ptid = { 0, 0, 0 };
-ptid_t minus_one_ptid = { -1, 0, 0 };
+ptid_t null_ptid = ptid_t::build (0, 0, 0);
+ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
+
/* See ptid.h. */
ptid_t
ptid_build (int pid, long lwp, long tid)
{
- ptid_t ptid;
-
- ptid.pid = pid;
- ptid.lwp = lwp;
- ptid.tid = tid;
- return ptid;
+ return ptid_t::build (pid, lwp, tid);
}
/* See ptid.h. */
@@ -43,81 +39,69 @@ ptid_build (int pid, long lwp, long tid)
ptid_t
pid_to_ptid (int pid)
{
- return ptid_build (pid, 0, 0);
+ return ptid_t::build (pid);
}
/* See ptid.h. */
int
-ptid_get_pid (ptid_t ptid)
+ptid_get_pid (const ptid_t &ptid)
{
- return ptid.pid;
+ return ptid.pid ();
}
/* See ptid.h. */
long
-ptid_get_lwp (ptid_t ptid)
+ptid_get_lwp (const ptid_t &ptid)
{
- return ptid.lwp;
+ return ptid.lwp ();
}
/* See ptid.h. */
long
-ptid_get_tid (ptid_t ptid)
+ptid_get_tid (const ptid_t &ptid)
{
- return ptid.tid;
+ return ptid.tid ();
}
/* See ptid.h. */
int
-ptid_equal (ptid_t ptid1, ptid_t ptid2)
+ptid_equal (const ptid_t &ptid1, const ptid_t &ptid2)
{
- return (ptid1.pid == ptid2.pid
- && ptid1.lwp == ptid2.lwp
- && ptid1.tid == ptid2.tid);
+ return ptid1 == ptid2;
}
/* See ptid.h. */
int
-ptid_is_pid (ptid_t ptid)
+ptid_is_pid (const ptid_t &ptid)
{
- if (ptid_equal (minus_one_ptid, ptid)
- || ptid_equal (null_ptid, ptid))
- return 0;
-
- return (ptid_get_lwp (ptid) == 0 && ptid_get_tid (ptid) == 0);
+ return ptid.is_pid ();
}
/* See ptid.h. */
int
-ptid_lwp_p (ptid_t ptid)
+ptid_lwp_p (const ptid_t &ptid)
{
- return (ptid_get_lwp (ptid) != 0);
+ return ptid.lwp_p ();
}
/* See ptid.h. */
int
-ptid_tid_p (ptid_t ptid)
+ptid_tid_p (const ptid_t &ptid)
{
- return (ptid_get_tid (ptid) != 0);
+ return ptid.tid_p ();
}
+/* See ptid.h. */
+
int
-ptid_match (ptid_t ptid, ptid_t filter)
+ptid_match (const ptid_t &ptid, const ptid_t &filter)
{
- if (ptid_equal (filter, minus_one_ptid))
- return 1;
- if (ptid_is_pid (filter)
- && ptid_get_pid (ptid) == ptid_get_pid (filter))
- return 1;
- else if (ptid_equal (ptid, filter))
- return 1;
-
- return 0;
+ return ptid.matches (filter);
}
@@ -20,6 +20,15 @@
#ifndef PTID_H
#define PTID_H
+class ptid_t;
+
+/* The null or zero ptid, often used to indicate no process. */
+extern ptid_t null_ptid;
+
+/* The (-1,0,0) ptid, often used to indicate either an error condition
+ or a "don't care" condition, i.e, "run all threads." */
+extern ptid_t minus_one_ptid;
+
/* The ptid struct is a collection of the various "ids" necessary for
identifying the inferior process/thread being debugged. This
consists of the process id (pid), lightweight process id (lwp) and
@@ -32,27 +41,85 @@
thread_stratum target that might want to sit on top.
*/
-struct ptid
+class ptid_t
{
+public:
+ static ptid_t build (int pid, long lwp = 0, long tid = 0)
+ {
+ ptid_t ptid;
+
+ ptid.m_pid = pid;
+ ptid.m_lwp = lwp;
+ ptid.m_tid = tid;
+
+ return ptid;
+ }
+
+ bool is_pid () const
+ {
+ if (is_any () || is_null())
+ return false;
+
+ return m_lwp == 0 && m_tid == 0;
+ }
+
+ bool is_null () const
+ {
+ return *this == null_ptid;
+ }
+
+ bool is_any () const
+ {
+ return *this == minus_one_ptid;
+ }
+
+ int pid () const
+ { return m_pid; }
+
+ bool lwp_p () const
+ { return m_lwp != 0; }
+
+ long lwp () const
+ { return m_lwp; }
+
+ bool tid_p () const
+ { return m_tid != 0; }
+
+ long tid () const
+ { return m_tid; }
+
+ bool operator== (const ptid_t &other) const
+ {
+ return (m_pid == other.m_pid
+ && m_lwp == other.m_lwp
+ && m_tid == other.m_tid);
+ }
+
+ bool matches (const ptid_t &filter) const
+ {
+ /* If filter represents any ptid, it's always a match. */
+ if (filter.is_any ())
+ return true;
+
+ /* If filter is only a pid, any ptid with that pid matches. */
+ if (filter.is_pid () && m_pid == filter.pid ())
+ return true;
+
+ /* Otherwise, this ptid only matches if it's exactly equal to filter. */
+ return *this == filter;
+ }
+
+private:
/* Process id. */
- int pid;
+ int m_pid;
/* Lightweight process id. */
- long lwp;
+ long m_lwp;
/* Thread id. */
- long tid;
+ long m_tid;
};
-typedef struct ptid ptid_t;
-
-/* The null or zero ptid, often used to indicate no process. */
-extern ptid_t null_ptid;
-
-/* The (-1,0,0) ptid, often used to indicate either an error condition
- or a "don't care" condition, i.e, "run all threads." */
-extern ptid_t minus_one_ptid;
-
/* Make a ptid given the necessary PID, LWP, and TID components. */
ptid_t ptid_build (int pid, long lwp, long tid);
@@ -61,27 +128,27 @@ ptid_t ptid_build (int pid, long lwp, long tid);
ptid_t pid_to_ptid (int pid);
/* Fetch the pid (process id) component from a ptid. */
-int ptid_get_pid (ptid_t ptid);
+int ptid_get_pid (const ptid_t &ptid);
/* Fetch the lwp (lightweight process) component from a ptid. */
-long ptid_get_lwp (ptid_t ptid);
+long ptid_get_lwp (const ptid_t &ptid);
/* Fetch the tid (thread id) component from a ptid. */
-long ptid_get_tid (ptid_t ptid);
+long ptid_get_tid (const ptid_t &ptid);
/* Compare two ptids to see if they are equal. */
-int ptid_equal (ptid_t ptid1, ptid_t ptid2);
+int ptid_equal (const ptid_t &ptid1, const ptid_t &ptid2);
/* Returns true if PTID represents a whole process, including all its
lwps/threads. Such ptids have the form of (pid,0,0), with pid !=
-1. */
-int ptid_is_pid (ptid_t ptid);
+int ptid_is_pid (const ptid_t &ptid);
/* Return true if PTID's lwp member is non-zero. */
-int ptid_lwp_p (ptid_t ptid);
+int ptid_lwp_p (const ptid_t &ptid);
/* Return true if PTID's tid member is non-zero. */
-int ptid_tid_p (ptid_t ptid);
+int ptid_tid_p (const ptid_t &ptid);
/* Returns true if PTID matches filter FILTER. FILTER can be the wild
card MINUS_ONE_PTID (all ptid match it); can be a ptid representing
@@ -91,6 +158,6 @@ int ptid_tid_p (ptid_t ptid);
case, only that thread will match true. PTID must represent a
specific LWP or THREAD, it can never be a wild card. */
-extern int ptid_match (ptid_t ptid, ptid_t filter);
+extern int ptid_match (const ptid_t &ptid, const ptid_t &filter);
#endif
@@ -2654,7 +2654,9 @@ handle_v_cont (char *own_buf)
char *p, *q;
int n = 0, i = 0;
struct thread_resume *resume_info;
- struct thread_resume default_action = {{0}};
+ struct thread_resume default_action = {
+ .thread = null_ptid,
+ };
/* Count the number of semicolons in the packet. There should be one
for every action. */