[v2] Class-ify ptid_t

  New in v2:

  - Constructor instead of "build" static method.

  - Keep the "minus_one" terminology instead of switching to "any".
    While writing the tests, it made me realize it didn't look very
    good: "some_ptid.is_any ()" sounds always true...  Plus, according
    to its comment, minus_one_ptid is also used to denote an error, not
    only the concept of "any" thread.

  - Removed is_null and is_any (which would have been renamed
    is_minus_one if it had not been removed).  Comparison to null_ptid
    and minus_one_ptid looks good enough:

    if (ptid != null_ptid)
    if (ptid != minus_one_ptid)

    If prefer if there's only one way to check whether a ptid is
    null/minus_one, so that we don't find two different styles in the
    code base.

  - constexpr everywhere, which allows...

  - Static tests.

  - Moved comments from existing ptid functions to ptid_t methods.

  - Added operator!=.

  - Other things I forgot.

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.  The fields are now
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 (pid, lwp, tid) -> ptid_build (pid, lwp, tid)
  ptid_t (pid) -> pid_to_ptid (pid)
  ptid.is_pid () -> ptid_is_pid (ptid)
  ptid == other -> ptid_equal (ptid, other)
  ptid != other -> !ptid_equal (ptid, other)
  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.
	<ptid_t>: New constructors.
	<pid, lwp_p, lwp, tid_p, tid, is_pid, operator==, operator!=,
	matches>: New methods.
	<make_null, make_minus_one>: New static methods.
	<pid>: Rename to...
	<m_pid>: ...this.
	<lwp>: Rename to...
	<m_lwp>: ...this.
	<tid>: Rename to...
	<m_tid>: ...this.
	(ptid_build, ptid_get_pid, ptid_get_lwp, ptid_get_tid, ptid_equal,
	ptid_is_pid, ptid_lwp_p, ptid_tid_p, ptid_match): Take ptid arguments
	as references, move comment to class ptid_t.
	* common/ptid.c (null_ptid, minus_one_ptid): Initialize with
	ptid_t static methods.
	(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 ptid arguments as references, implement using ptid_t methods.
	* unittests/ptid-selftests.c: New file.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/ptid-selftests.c.
	(SUBDIR_UNITTESTS_OBS): Add unittests/ptid-selftests.o.

gdb/gdbserver/ChangeLog:

	* server.c (handle_v_cont): Initialize thread_resume::thread
	with null_ptid.
---
 gdb/Makefile.in                |   6 +-
 gdb/common/ptid.c              |  61 ++++++--------
 gdb/common/ptid.h              | 175 ++++++++++++++++++++++++++++++++---------
 gdb/gdbserver/server.c         |   2 +-
 gdb/unittests/ptid-selftests.c | 148 ++++++++++++++++++++++++++++++++++
 5 files changed, 315 insertions(+), 77 deletions(-)
 create mode 100644 gdb/unittests/ptid-selftests.c
  

On 04/06/2017 08:03 PM, Simon Marchi wrote:

> -struct ptid
> +class ptid_t
>  {
> +public:
> +  /* Must have a trivial defaulted default constructor so that the
> +     type remains POD.  */
> +  ptid_t () noexcept = default;
> +
> +  /* Make a ptid given the necessary PID, LWP, and TID components.
> +
> +     A ptid with only a PID (LWP and TID equal to zero) is usually used to
> +     represent a whole process, including all its lwps/threads.  */
> +
> +  constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
> +    : m_pid (pid), m_lwp (lwp), m_tid (tid)
> +  {}

Hmm, I just realized that due to the default arguments, this results
in an implicit ctor from int, which doesn't sound like a good
idea to me.  I.e., this bug would compile:

 void foo (ptid_t ptid);

 void bar (int lwpid)
 {
   foo (lwpid); // automatically constructs a (pid,0,0) ptid.
 }

So I think we should make that ctor explicit, and add another assertion
to the unit tests:

  static_assert (!std::is_convertible<int, ptid_t>::value, "");

> +
> +  /* Returns true if the ptid matches FILTER.  FILTER can be the wild
> +     card MINUS_ONE_PTID (all ptid match it); can be a ptid representing

"all ptids"

> +     a process (ptid_is_pid returns true), in which case, all lwps and

"ptid.is_pid ()" ?

> +     threads of that given process match, lwps and threads of other
> +     processes do not; or, it can represent a specific thread, in which
> +     case, only that thread will match true.  The ptid must represent a
> +     specific LWP or THREAD, it can never be a wild card.  */
> +
> +  constexpr bool matches (const ptid_t &filter) const
> +  {
> +    return (/* If filter represents any ptid, it's always a match.  */
> +	    filter == make_minus_one ()
> +	    /* If filter is only a pid, any ptid with that pid
> +	       matches.  */
> +	    || (filter.is_pid () && m_pid == filter.pid ())
> +
> +	    /* Otherwise, this ptid only matches if it's exactly equal
> +	       to filter.  */
> +	    || *this == filter);
> +  }
> +
> +  /* Make a null ptid.  */
> +
> +  static constexpr ptid_t
> +  make_null ()
> +  { return {0, 0, 0}; }
> +
> +  /* Make a minus one ptid.  */
> +
> +  static constexpr ptid_t
> +  make_minus_one ()
> +  { return {-1, 0, 0}; }

I find it a bit odd to break the line after the return type in
these two, when we don't break it in non-static members.

> +#include "defs.h"
> +#include "common/ptid.h"
> +#include <type_traits>
> +
> +namespace selftests {
> +namespace ptid {
> +
> +/* Check that the ptid_t class is POD.
> +
> +   This isn't a strict requirement.  If we have a good reason to change it to
> +   a non-POD type, we can remove this check.  */

Hmm, I think this comment too lax.  There _is_ a reason this type
must remain POD for the time being.  So I think that's what we
should say here:

/* Check that the ptid_t class is POD.

   This is a requirement for a long as we have ptids embedded in
   structures allocated with malloc.  */

> +
> +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
> +

Otherwise looks good to me.  Please push.

Thanks,
Pedro Alves
  

On 2017-04-06 18:23, Pedro Alves wrote:
> On 04/06/2017 08:03 PM, Simon Marchi wrote:
> 
>> -struct ptid
>> +class ptid_t
>>  {
>> +public:
>> +  /* Must have a trivial defaulted default constructor so that the
>> +     type remains POD.  */
>> +  ptid_t () noexcept = default;
>> +
>> +  /* Make a ptid given the necessary PID, LWP, and TID components.
>> +
>> +     A ptid with only a PID (LWP and TID equal to zero) is usually 
>> used to
>> +     represent a whole process, including all its lwps/threads.  */
>> +
>> +  constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
>> +    : m_pid (pid), m_lwp (lwp), m_tid (tid)
>> +  {}
> 
> Hmm, I just realized that due to the default arguments, this results
> in an implicit ctor from int, which doesn't sound like a good
> idea to me.  I.e., this bug would compile:
> 
>  void foo (ptid_t ptid);
> 
>  void bar (int lwpid)
>  {
>    foo (lwpid); // automatically constructs a (pid,0,0) ptid.
>  }
> 
> So I think we should make that ctor explicit, and add another assertion
> to the unit tests:
> 
>   static_assert (!std::is_convertible<int, ptid_t>::value, "");

Definitely, good catch.

>> +
>> +  /* Returns true if the ptid matches FILTER.  FILTER can be the wild
>> +     card MINUS_ONE_PTID (all ptid match it); can be a ptid 
>> representing
> 
> "all ptids"

Thanks.

>> +     a process (ptid_is_pid returns true), in which case, all lwps 
>> and
> 
> "ptid.is_pid ()" ?

Thanks.

>> +     threads of that given process match, lwps and threads of other
>> +     processes do not; or, it can represent a specific thread, in 
>> which
>> +     case, only that thread will match true.  The ptid must represent 
>> a
>> +     specific LWP or THREAD, it can never be a wild card.  */
>> +
>> +  constexpr bool matches (const ptid_t &filter) const
>> +  {
>> +    return (/* If filter represents any ptid, it's always a match.  
>> */
>> +	    filter == make_minus_one ()
>> +	    /* If filter is only a pid, any ptid with that pid
>> +	       matches.  */
>> +	    || (filter.is_pid () && m_pid == filter.pid ())
>> +
>> +	    /* Otherwise, this ptid only matches if it's exactly equal
>> +	       to filter.  */
>> +	    || *this == filter);
>> +  }
>> +
>> +  /* Make a null ptid.  */
>> +
>> +  static constexpr ptid_t
>> +  make_null ()
>> +  { return {0, 0, 0}; }
>> +
>> +  /* Make a minus one ptid.  */
>> +
>> +  static constexpr ptid_t
>> +  make_minus_one ()
>> +  { return {-1, 0, 0}; }
> 
> I find it a bit odd to break the line after the return type in
> these two, when we don't break it in non-static members.

Indeed, it's a mistake.  I knew something looked odd with these, but 
couldn't put the finger on why.

>> +#include "defs.h"
>> +#include "common/ptid.h"
>> +#include <type_traits>
>> +
>> +namespace selftests {
>> +namespace ptid {
>> +
>> +/* Check that the ptid_t class is POD.
>> +
>> +   This isn't a strict requirement.  If we have a good reason to 
>> change it to
>> +   a non-POD type, we can remove this check.  */
> 
> Hmm, I think this comment too lax.  There _is_ a reason this type
> must remain POD for the time being.  So I think that's what we
> should say here:
> 
> /* Check that the ptid_t class is POD.
> 
>    This is a requirement for a long as we have ptids embedded in
>    structures allocated with malloc.  */

Ah, makes sense.  I was only thinking about the instances where ptid_t 
is embedded in structures allocated statically.  In those cases, 
compilation would fail anyway, which is why I didn't really see the 
point of that test.  But of course, it's important for malloc'ed 
structures as well, for which we get now error/warning.

>> +
>> +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
>> +
> 
> Otherwise looks good to me.  Please push.

I'll do that later tonight, thanks.

Simon
  

On 04/07/2017 01:04 AM, Simon Marchi wrote:

>> Hmm, I think this comment too lax.  There _is_ a reason this type
>> must remain POD for the time being.  So I think that's what we
>> should say here:
>>
>> /* Check that the ptid_t class is POD.
>>
>>    This is a requirement for a long as we have ptids embedded in
>>    structures allocated with malloc.  */
> 
> Ah, makes sense.  I was only thinking about the instances where ptid_t
> is embedded in structures allocated statically.  In those cases,
> compilation would fail anyway, which is why I didn't really see the
> point of that test.  

Actually, non-PODs in structures allocated statically is perfectly fine.
The compiler arranges for the objects to have their constructors
called.  That's why we can e.g., have global std::vector objects
or function local static std::string objects (both non-PODs), etc.

So we could e.g., have a default ptid_t ctor that initializes
the pid/lwpid/tid fields to zeros instead of leaving them undefined.
Except, if we did that, then the ctor would no longer be trivial, and
so the type would no longer be POD, meaning we couldn't create a ptid
with malloc and use it straight away.  We'd have to either call
the ctor manually with placement new after malloc to bring to
object to life, and call the dtor manually too (but that's off course
cumbersome), or we'd have to use normal new/delete instead, which is
the natural thing to do, but we're a bit far away from doing
that everywhere.

> But of course, it's important for malloc'ed
> structures as well, for which we get now error/warning.

Hmm, I'm not quite picturing what error/warning we now get?

Thanks,
Pedro Alves
  

On 2017-04-06 21:56, Pedro Alves wrote:
>> Ah, makes sense.  I was only thinking about the instances where ptid_t
>> is embedded in structures allocated statically.  In those cases,
>> compilation would fail anyway, which is why I didn't really see the
>> point of that test.
> 
> Actually, non-PODs in structures allocated statically is perfectly 
> fine.
> The compiler arranges for the objects to have their constructors
> called.  That's why we can e.g., have global std::vector objects
> or function local static std::string objects (both non-PODs), etc.
> 
> So we could e.g., have a default ptid_t ctor that initializes
> the pid/lwpid/tid fields to zeros instead of leaving them undefined.
> Except, if we did that, then the ctor would no longer be trivial, and
> so the type would no longer be POD, meaning we couldn't create a ptid
> with malloc and use it straight away.  We'd have to either call
> the ctor manually with placement new after malloc to bring to
> object to life, and call the dtor manually too (but that's off course
> cumbersome), or we'd have to use normal new/delete instead, which is
> the natural thing to do, but we're a bit far away from doing
> that everywhere.

Err sorry, I got confused.  The error I saw initially was about the 
target_waitstatus, where the ptid_t is in an union:

   /home/simark/src/binutils-gdb/gdb/target/waitstatus.h:104:8: note: 
‘target_waitstatus::target_waitstatus()’ is implicitly deleted because 
the default definition would be ill-formed:
  struct target_waitstatus
         ^~~~~~~~~~~~~~~~~
   /home/simark/src/binutils-gdb/gdb/target/waitstatus.h:104:8: error: 
use of deleted function ‘target_waitstatus::<anonymous 
union>::<constructor>()’

If you make a simple default constructor, like I tried in the beginning:

   ptid_t () {}

you'll see this error when building target.o.  I wrongfully attributed 
this error to the fact that target_waitstatus didn't have a constructor 
of its own, but I now see it doesn't make sense.  It must've been late 
:).


>> But of course, it's important for malloc'ed
>> structures as well, for which we get now error/warning.
> 
> Hmm, I'm not quite picturing what error/warning we now get?

Oops, s/now/no/.  What I meant is that if you use malloc for an object 
of a type you shouldn't (because it's not trivial), nothing will warn 
you.  The is_pod check protects us from that, since it will tell us if 
the class ever becomes unsafe to malloc.
  

Hi Simon,

I know I'm a little late, but there are some inconsistencies/ambiguities
in the error messages of the unittests bothering me.  For example

> +static_assert (!pid.matches (lwp), "pid matches lwp");
> +static_assert (lwp.matches (lwp), "lwp matches lwp");
[...]
> +static_assert (!ptid_t (2, 2, 0).matches (lwp), "other lwp doesn't
> match lwp");

where the 1st and 2nd asserts have the same error message different to
the 3rd one.  Although the 1st and 3rd assert the same (not matching)
while the 2nd asserts the opposite (matching). 

Or

> +static_assert (pid == ptid_t (1, 0, 0), "pid operator== is right")

where the only time you see the error message is when operator==
returns false.  But in that case operator== isn't right but wrong ;)

In this context I ask myself what the error message is supposed to
say (unfortunately you are the first one in GDB actually using it, all
others only use an empty string).  Is it what's expected ...

static_assert (!pid.matches (lwp), "pid mustn't match lwp");
static_assert (lwp.matches (lwp), "lwp must match lwp/itself");

static_assert (pid == ptid_t (1, 0, 0), "pid operator== must be true")


... or what went wrong?

static_assert (!pid.matches (lwp), "pid matches lwp");
static_assert (lwp.matches (lwp), "lwp doesn't match lwp/itself");

static_assert (pid == ptid_t (1, 0, 0), "pid operator== returned false")


I tend more to what is expected.

Otherwise your series is great. This will definitely help making the
code better readable.

Thanks
Philipp



On Thu, 6 Apr 2017 15:03:28 -0400
Simon Marchi <simon.marchi@ericsson.com> wrote:

> New in v2:
> 
>   - Constructor instead of "build" static method.
> 
>   - Keep the "minus_one" terminology instead of switching to "any".
>     While writing the tests, it made me realize it didn't look very
>     good: "some_ptid.is_any ()" sounds always true...  Plus, according
>     to its comment, minus_one_ptid is also used to denote an error,
> not only the concept of "any" thread.
> 
>   - Removed is_null and is_any (which would have been renamed
>     is_minus_one if it had not been removed).  Comparison to null_ptid
>     and minus_one_ptid looks good enough:
> 
>     if (ptid != null_ptid)
>     if (ptid != minus_one_ptid)
> 
>     If prefer if there's only one way to check whether a ptid is
>     null/minus_one, so that we don't find two different styles in the
>     code base.
> 
>   - constexpr everywhere, which allows...
> 
>   - Static tests.
> 
>   - Moved comments from existing ptid functions to ptid_t methods.
> 
>   - Added operator!=.
> 
>   - Other things I forgot.
> 
> 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.
> The fields are now 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 (pid, lwp, tid) -> ptid_build (pid, lwp, tid)
>   ptid_t (pid) -> pid_to_ptid (pid)
>   ptid.is_pid () -> ptid_is_pid (ptid)
>   ptid == other -> ptid_equal (ptid, other)
>   ptid != other -> !ptid_equal (ptid, other)
>   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.
> 	<ptid_t>: New constructors.
> 	<pid, lwp_p, lwp, tid_p, tid, is_pid, operator==, operator!=,
> 	matches>: New methods.
> 	<make_null, make_minus_one>: New static methods.
> 	<pid>: Rename to...
> 	<m_pid>: ...this.
> 	<lwp>: Rename to...
> 	<m_lwp>: ...this.
> 	<tid>: Rename to...
> 	<m_tid>: ...this.
> 	(ptid_build, ptid_get_pid, ptid_get_lwp, ptid_get_tid,
> ptid_equal, ptid_is_pid, ptid_lwp_p, ptid_tid_p, ptid_match): Take
> ptid arguments as references, move comment to class ptid_t.
> 	* common/ptid.c (null_ptid, minus_one_ptid): Initialize with
> 	ptid_t static methods.
> 	(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 ptid arguments as references, implement using ptid_t
> methods.
> 	* unittests/ptid-selftests.c: New file.
> 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
> 	unittests/ptid-selftests.c.
> 	(SUBDIR_UNITTESTS_OBS): Add unittests/ptid-selftests.o.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* server.c (handle_v_cont): Initialize thread_resume::thread
> 	with null_ptid.
> ---
>  gdb/Makefile.in                |   6 +-
>  gdb/common/ptid.c              |  61 ++++++--------
>  gdb/common/ptid.h              | 175
> ++++++++++++++++++++++++++++++++---------
> gdb/gdbserver/server.c         |   2 +-
> gdb/unittests/ptid-selftests.c | 148
> ++++++++++++++++++++++++++++++++++ 5 files changed, 315
> insertions(+), 77 deletions(-) create mode 100644
> gdb/unittests/ptid-selftests.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 479d27344b..23e4bed3aa 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -525,11 +525,13 @@ SUBDIR_PYTHON_CFLAGS =
> 
>  SUBDIR_UNITTESTS_SRCS = \
>  	unittests/function-view-selftests.c \
> -	unittests/offset-type-selftests.c
> +	unittests/offset-type-selftests.c \
> +	unittests/ptid-selftests.c
> 
>  SUBDIR_UNITTESTS_OBS = \
>  	function-view-selftests.o \
> -	offset-type-selftests.o
> +	offset-type-selftests.o \
> +	ptid-selftests.o
> 
>  # Opcodes currently live in one of two places.  Either they are in
> the # opcode library, typically ../opcodes, or they are in a header
> file diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
> index b56971b9af..81f16d047e 100644
> --- a/gdb/common/ptid.c
> +++ b/gdb/common/ptid.c
> @@ -22,20 +22,15 @@
> 
>  /* 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::make_null ();
> +ptid_t minus_one_ptid = ptid_t::make_minus_one ();
> 
>  /* 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 (pid, lwp, tid);
>  }
> 
>  /* See ptid.h.  */
> @@ -43,81 +38,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 (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);
>  }
> diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
> index 337bfb0899..a6b5ec7ea2 100644
> --- a/gdb/common/ptid.h
> +++ b/gdb/common/ptid.h
> @@ -32,65 +32,170 @@
>     thread_stratum target that might want to sit on top.
>  */
> 
> -struct ptid
> +class ptid_t
>  {
> +public:
> +  /* Must have a trivial defaulted default constructor so that the
> +     type remains POD.  */
> +  ptid_t () noexcept = default;
> +
> +  /* Make a ptid given the necessary PID, LWP, and TID components.
> +
> +     A ptid with only a PID (LWP and TID equal to zero) is usually
> used to
> +     represent a whole process, including all its lwps/threads.  */
> +
> +  constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
> +    : m_pid (pid), m_lwp (lwp), m_tid (tid)
> +  {}
> +
> +  /* Fetch the pid (process id) component from the ptid.  */
> +
> +  constexpr int pid () const
> +  { return m_pid; }
> +
> +  /* Return true if the ptid's lwp member is non-zero.  */
> +
> +  constexpr bool lwp_p () const
> +  { return m_lwp != 0; }
> +
> +  /* Fetch the lwp (lightweight process) component from the ptid.  */
> +
> +  constexpr long lwp () const
> +  { return m_lwp; }
> +
> +  /* Return true if the ptid's tid member is non-zero.  */
> +
> +  constexpr bool tid_p () const
> +  { return m_tid != 0; }
> +
> +  /* Fetch the tid (thread id) component from a ptid.  */
> +
> +  constexpr long tid () const
> +  { return m_tid; }
> +
> +  /* Return true if the ptid represents a whole process, including
> all its
> +     lwps/threads.  Such ptids have the form of (pid, 0, 0), with
> +     pid != -1.  */
> +
> +  constexpr bool is_pid () const
> +  {
> +    return (*this != make_null ()
> +	    && *this != make_minus_one ()
> +	    && m_lwp == 0
> +	    && m_tid == 0);
> +  }
> +
> +  /* Compare two ptids to see if they are equal.  */
> +
> +  constexpr bool operator== (const ptid_t &other) const
> +  {
> +    return (m_pid == other.m_pid
> +	    && m_lwp == other.m_lwp
> +	    && m_tid == other.m_tid);
> +  }
> +
> +  /* Compare two ptids to see if they are different.  */
> +
> +  constexpr bool operator!= (const ptid_t &other) const
> +  {
> +    return !(*this == other);
> +  }
> +
> +  /* Returns true if the ptid matches FILTER.  FILTER can be the wild
> +     card MINUS_ONE_PTID (all ptid match it); can be a ptid
> representing
> +     a process (ptid_is_pid returns true), in which case, all lwps
> and
> +     threads of that given process match, lwps and threads of other
> +     processes do not; or, it can represent a specific thread, in
> which
> +     case, only that thread will match true.  The ptid must
> represent a
> +     specific LWP or THREAD, it can never be a wild card.  */
> +
> +  constexpr bool matches (const ptid_t &filter) const
> +  {
> +    return (/* If filter represents any ptid, it's always a match.
> */
> +	    filter == make_minus_one ()
> +	    /* If filter is only a pid, any ptid with that pid
> +	       matches.  */
> +	    || (filter.is_pid () && m_pid == filter.pid ())
> +
> +	    /* Otherwise, this ptid only matches if it's exactly
> equal
> +	       to filter.  */
> +	    || *this == filter);
> +  }
> +
> +  /* Make a null ptid.  */
> +
> +  static constexpr ptid_t
> +  make_null ()
> +  { return {0, 0, 0}; }
> +
> +  /* Make a minus one ptid.  */
> +
> +  static constexpr ptid_t
> +  make_minus_one ()
> +  { return {-1, 0, 0}; }
> +
> +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);
> 
> -/* Make a new ptid from just a pid.  This ptid is usually used to
> -   represent a whole process, including all its lwps/threads.  */
> -ptid_t pid_to_ptid (int pid);
> +/* The following functions are kept for backwards compatibility.
> The use of
> +   the ptid_t methods is preferred.  */
> +
> +/* See ptid_t::ptid_t.  */
> +
> +extern ptid_t ptid_build (int pid, long lwp, long tid);
> +
> +/* See ptid_t::ptid_t.  */
> +
> +extern ptid_t pid_to_ptid (int pid);
> +
> +/* See ptid_t::pid.  */
> +
> +extern int ptid_get_pid (const ptid_t &ptid);
> +
> +/* See ptid_t::lwp.  */
> +
> +extern long ptid_get_lwp (const ptid_t &ptid);
> +
> +/* See ptid_t::tid.  */
> +
> +extern long ptid_get_tid (const ptid_t &ptid);
> +
> +/* See ptid_t::operator== and ptid_t::operator!=.  */
> 
> -/* Fetch the pid (process id) component from a ptid.  */
> -int ptid_get_pid (ptid_t ptid);
> +extern int ptid_equal (const ptid_t &ptid1, const ptid_t &ptid2);
> 
> -/* Fetch the lwp (lightweight process) component from a ptid.  */
> -long ptid_get_lwp (ptid_t ptid);
> +/* See ptid_t::is_pid.  */
> 
> -/* Fetch the tid (thread id) component from a ptid.  */
> -long ptid_get_tid (ptid_t ptid);
> +extern int ptid_is_pid (const ptid_t &ptid);
> 
> -/* Compare two ptids to see if they are equal.  */
> -int ptid_equal (ptid_t ptid1, ptid_t ptid2);
> +/* See ptid_t::lwp_p.  */
> 
> -/* 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);
> +extern int ptid_lwp_p (const ptid_t &ptid);
> 
> -/* Return true if PTID's lwp member is non-zero.  */
> -int ptid_lwp_p (ptid_t ptid);
> +/* See ptid_t::tid_p.  */
> 
> -/* Return true if PTID's tid member is non-zero.  */
> -int ptid_tid_p (ptid_t ptid);
> +extern 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
> -   a process (ptid_is_pid returns true), in which case, all lwps and
> -   threads of that given process match, lwps and threads of other
> -   processes do not; or, it can represent a specific thread, in which
> -   case, only that thread will match true.  PTID must represent a
> -   specific LWP or THREAD, it can never be a wild card.  */
> +/* See ptid_t::matches.  */
> 
> -extern int ptid_match (ptid_t ptid, ptid_t filter);
> +extern int ptid_match (const ptid_t &ptid, const ptid_t &filter);
> 
>  #endif
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 51f6a28419..9cc6145b05 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -2654,7 +2654,7 @@ 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 { null_ptid };
> 
>    /* Count the number of semicolons in the packet.  There should be
> one for every action.  */
> diff --git a/gdb/unittests/ptid-selftests.c
> b/gdb/unittests/ptid-selftests.c new file mode 100644
> index 0000000000..74cc3e0491
> --- /dev/null
> +++ b/gdb/unittests/ptid-selftests.c
> @@ -0,0 +1,148 @@
> +/* Self tests for ptid_t for GDB, the GNU debugger.
> +
> +   Copyright (C) 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see
> <http://www.gnu.org/licenses/>.  */ +
> +#include "defs.h"
> +#include "common/ptid.h"
> +#include <type_traits>
> +
> +namespace selftests {
> +namespace ptid {
> +
> +/* Check that the ptid_t class is POD.
> +
> +   This isn't a strict requirement.  If we have a good reason to
> change it to
> +   a non-POD type, we can remove this check.  */
> +
> +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
> +
> +/* Build some useful ptids.  */
> +
> +static constexpr ptid_t pid = ptid_t (1);
> +static constexpr ptid_t lwp = ptid_t (1, 2, 0);
> +static constexpr ptid_t tid = ptid_t (1, 0, 2);
> +static constexpr ptid_t both = ptid_t (1, 2, 2);
> +
> +/* Build some constexpr version of null_ptid and minus_one_ptid to
> use in
> +   static_assert.  Once the real ones are made constexpr, we can get
> rid of
> +   these.  */
> +
> +static constexpr ptid_t null = ptid_t::make_null ();
> +static constexpr ptid_t minus_one = ptid_t::make_minus_one ();
> +
> +/* Verify pid.  */
> +
> +static_assert (pid.pid () == 1, "pid's pid is right");
> +static_assert (lwp.pid () == 1, "lwp's pid is right");
> +static_assert (tid.pid () == 1, "tid's pid is right");
> +static_assert (both.pid () == 1, "both's pid is right");
> +
> +/* Verify lwp_p.  */
> +
> +static_assert (!pid.lwp_p (), "pid's lwp_p is right");
> +static_assert (lwp.lwp_p (), "lwp's lwp_p is right");
> +static_assert (!tid.lwp_p (), "tid's lwp_p is right");
> +static_assert (both.lwp_p (), "both's lwp_p is right");
> +
> +/* Verify lwp.  */
> +
> +static_assert (pid.lwp () == 0, "pid's lwp is right");
> +static_assert (lwp.lwp () == 2, "lwp's lwp is right");
> +static_assert (tid.lwp () == 0, "tid's lwp is right");
> +static_assert (both.lwp () == 2, "both's lwp is right");
> +
> +/* Verify tid_p.  */
> +
> +static_assert (!pid.tid_p (), "pid's tid_p is right");
> +static_assert (!lwp.tid_p (), "lwp's tid_p is right");
> +static_assert (tid.tid_p (), "tid's tid_p is right");
> +static_assert (both.tid_p (), "both's tid_p is right");
> +
> +/* Verify tid.  */
> +
> +static_assert (pid.tid () == 0, "pid's tid is right");
> +static_assert (lwp.tid () == 0, "lwp's tid is right");
> +static_assert (tid.tid () == 2, "tid's tid is right");
> +static_assert (both.tid () == 2, "both's tid is right");
> +
> +/* Verify is_pid.  */
> +
> +static_assert (pid.is_pid (), "pid is a pid");
> +static_assert (!lwp.is_pid (), "lwp isn't a pid");
> +static_assert (!tid.is_pid (), "tid isn't a pid");
> +static_assert (!both.is_pid (), "both isn't a pid");
> +static_assert (!null.is_pid (), "null ptid isn't a pid");
> +static_assert (!minus_one.is_pid (), "minus one ptid isn't a pid");
> +
> +/* Verify operator ==.  */
> +
> +static_assert (pid == ptid_t (1, 0, 0), "pid operator== is right");
> +static_assert (lwp == ptid_t (1, 2, 0), "lwp operator== is right");
> +static_assert (tid == ptid_t (1, 0, 2), "tid operator== is right");
> +static_assert (both == ptid_t (1, 2, 2), "both operator== is right");
> +
> +/* Verify operator !=.  */
> +
> +static_assert (pid != ptid_t (2, 0, 0), "pid isn't equal to a
> different pid"); +static_assert (pid != lwp, "pid isn't equal to one
> of its thread"); +static_assert (lwp != tid, "lwp isn't equal to
> tid"); +static_assert (both != lwp, "both isn't equal to lwp");
> +static_assert (both != tid, "both isn't equal to tid");
> +
> +/* Verify matches against minus_one.  */
> +
> +static_assert (pid.matches (minus_one), "pid matches minus one");
> +static_assert (lwp.matches (minus_one), "lwp matches minus one");
> +static_assert (tid.matches (minus_one), "tid matches minus one");
> +static_assert (both.matches (minus_one), "both matches minus one");
> +
> +/* Verify matches against pid.  */
> +
> +static_assert (pid.matches (pid), "pid matches pid");
> +static_assert (lwp.matches (pid), "lwp matches pid");
> +static_assert (tid.matches (pid), "tid matches pid");
> +static_assert (both.matches (pid), "both matches pid");
> +static_assert (!ptid_t (2, 0, 0).matches (pid), "other pid doesn't
> match pid"); +static_assert (!ptid_t (2, 2, 0).matches (pid), "other
> lwp doesn't match pid"); +static_assert (!ptid_t (2, 0, 2).matches
> (pid), "other tid doesn't match pid"); +static_assert (!ptid_t (2, 2,
> 2).matches (pid), "other both doesn't match pid"); +
> +/* Verify matches against exact matches.  */
> +
> +static_assert (!pid.matches (lwp), "pid matches lwp");
> +static_assert (lwp.matches (lwp), "lwp matches lwp");
> +static_assert (!tid.matches (lwp), "tid matches lwp");
> +static_assert (!both.matches (lwp), "both matches lwp");
> +static_assert (!ptid_t (2, 2, 0).matches (lwp), "other lwp doesn't
> match lwp"); +
> +static_assert (!pid.matches (tid), "pid matches tid");
> +static_assert (!lwp.matches (tid), "lwp matches tid");
> +static_assert (tid.matches (tid), "tid matches tid");
> +static_assert (!both.matches (tid), "both matches tid");
> +static_assert (!ptid_t (2, 0, 2).matches (tid), "other tid doesn't
> match tid"); +
> +static_assert (!pid.matches (both), "pid matches both");
> +static_assert (!lwp.matches (both), "lwp matches both");
> +static_assert (!tid.matches (both), "tid matches both");
> +static_assert (both.matches (both), "both matches both");
> +static_assert (!ptid_t (2, 2, 2).matches (both),
> +	       "other both doesn't match both");
> +
> +
> +} /* namespace ptid */
> +} /* namespace selftests */
  

On 04/07/2017 10:25 AM, Philipp Rudo wrote:

> In this context I ask myself what the error message is supposed to
> say (unfortunately you are the first one in GDB actually using it, all
> others only use an empty string).  Is it what's expected ...
> 

That's because the static assertion failure text includes both a
file:line indicating where's the failure, and modern gcc/clang
show you the line that failed, so it's not that big of a deal to
not include a message:

 src/gdb/thread.c:1589:1: error: static assertion failed: 
  static_assert (1 == 0, "");
  ^
 $

(and in C++17, static_assert got an overload with no second
parameter, even.)

> 
> static_assert (pid == ptid_t (1, 0, 0), "pid operator== returned false")
> 
> 
> I tend more to what is expected.

Definitely it should say what is expected, just like regular
testsuite tests.  Or say nothing when the expression is pretty clear
already, which works for me too.  :-)

Thanks,
Pedro Alves
  

On 17-04-07 05:25 AM, Philipp Rudo wrote:
> Hi Simon,
> 
> I know I'm a little late, but there are some inconsistencies/ambiguities
> in the error messages of the unittests bothering me.  For example

Hi Philipp, thanks for looking at this!

>> +static_assert (!pid.matches (lwp), "pid matches lwp");
>> +static_assert (lwp.matches (lwp), "lwp matches lwp");
> [...]
>> +static_assert (!ptid_t (2, 2, 0).matches (lwp), "other lwp doesn't
>> match lwp");
> 
> where the 1st and 2nd asserts have the same error message different to
> the 3rd one.  Although the 1st and 3rd assert the same (not matching)
> while the 2nd asserts the opposite (matching). 

Oops,

  static_assert (!pid.matches (lwp), "pid matches lwp");

is definitely a copy-paste leftover.  It should've said "pid doesn't match lwp".  There
are a few instances of that.

> Or
> 
>> +static_assert (pid == ptid_t (1, 0, 0), "pid operator== is right")
> 
> where the only time you see the error message is when operator==
> returns false.  But in that case operator== isn't right but wrong ;)
> 
> In this context I ask myself what the error message is supposed to
> say (unfortunately you are the first one in GDB actually using it, all
> others only use an empty string).  Is it what's expected ...
> 
> static_assert (!pid.matches (lwp), "pid mustn't match lwp");
> static_assert (lwp.matches (lwp), "lwp must match lwp/itself");
> 
> static_assert (pid == ptid_t (1, 0, 0), "pid operator== must be true")
> 
> 
> ... or what went wrong?
> 
> static_assert (!pid.matches (lwp), "pid matches lwp");
> static_assert (lwp.matches (lwp), "lwp doesn't match lwp/itself");
> 
> static_assert (pid == ptid_t (1, 0, 0), "pid operator== returned false")
> 
> 
> I tend more to what is expected.

I agree, that way it matches the condition right next to it.  But the verb tense is
probably not the best.  must/mustn't or should/shouldn't may be clearer than does/doesn't.

I'll at least fix what's obviously wrong right now, if you want to take a shot
at improving the messages, you are welcome :).

> Otherwise your series is great. This will definitely help making the
> code better readable.
> 
> Thanks
> Philipp

Thanks,

Simon
  

On 17-04-07 06:48 AM, Pedro Alves wrote:
> On 04/07/2017 10:25 AM, Philipp Rudo wrote:
> 
>> In this context I ask myself what the error message is supposed to
>> say (unfortunately you are the first one in GDB actually using it, all
>> others only use an empty string).  Is it what's expected ...
>>
> 
> That's because the static assertion failure text includes both a
> file:line indicating where's the failure, and modern gcc/clang
> show you the line that failed, so it's not that big of a deal to
> not include a message:
> 
>  src/gdb/thread.c:1589:1: error: static assertion failed: 
>   static_assert (1 == 0, "");
>   ^
>  $
> 
> (and in C++17, static_assert got an overload with no second
> parameter, even.)
> 
>>
>> static_assert (pid == ptid_t (1, 0, 0), "pid operator== returned false")
>>
>>
>> I tend more to what is expected.
> 
> Definitely it should say what is expected, just like regular
> testsuite tests.  Or say nothing when the expression is pretty clear
> already, which works for me too.  :-)

Heh, why did you not say that earlier, it would have saved me some time ;)

@Philipp: To come back to the verb tense thing.  Now that I think of it, when
I read the lines, I mentally insert a "that" in there.  So

  static_assert (!pid.matches (lwp), "pid doesn't match lwp");

becomes mentally

  assert that pid doesn't match lwp

I think it comes from using jUnit's assertThat:

https://github.com/junit-team/junit4/wiki/matchers-and-assertthat

Simon