Commit Message
On 04/05/2017 08:44 PM, Simon Marchi wrote:
> 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?
Either of those sounds fine. Or maybe that's not a big deal, and
a ctor that takes a single "pid" would be fine. See below.
>>> 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.
Yeah, also, the compiler will probably be able to inline these
most of the times.
>
>>>
>>> /* 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.
Thanks. constexpr in C++11 forces you to write a single
return statement (C++14 gets rid of that requirement),
but it looks quite doable.
Also, note that it's not true that this type can't have a
constructor. It can, as long as the type remains POD.
I've pasted below a quick patch on top of yours to get
you going. Note the static_asserts.
>> 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 };
ISTR that in C++11 you'll need the double "{{" levels, like:
thread_resume default_action {{null_ptid}};
and that C++14 removed that requirement (brace elision).
But I haven't confirmed. Whatever works with -std=c++11.
From 63f3c4ed145a069df68991fabbf025ffaedf8cf6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 5 Apr 2017 22:21:58 +0100
Subject: [PATCH] POD
---
gdb/common/ptid.c | 8 +++----
gdb/common/ptid.h | 68 ++++++++++++++++++++++++++++---------------------------
2 files changed, 39 insertions(+), 37 deletions(-)
Comments
On 2017-04-05 17:31, Pedro Alves wrote:
>>> 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.
>
> Thanks. constexpr in C++11 forces you to write a single
> return statement (C++14 gets rid of that requirement),
> but it looks quite doable.
>
> Also, note that it's not true that this type can't have a
> constructor. It can, as long as the type remains POD.
Ah, so I was just missing the defaulted default constructor. Adding it
makes the type trivial, which then makes it POD.
>> Is the following ok?
>>
>> struct thread_resume default_action { null_ptid };
>
> ISTR that in C++11 you'll need the double "{{" levels, like:
>
> thread_resume default_action {{null_ptid}};
>
> and that C++14 removed that requirement (brace elision).
> But I haven't confirmed. Whatever works with -std=c++11.
It seems to work with a single pair of braces with c++11. I'll still
check that it does what we want at runtime, but I'd be surprised if it
didn't do the right thing.
> From 63f3c4ed145a069df68991fabbf025ffaedf8cf6 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 5 Apr 2017 22:21:58 +0100
> Subject: [PATCH] POD
>
> ---
> gdb/common/ptid.c | 8 +++----
> gdb/common/ptid.h | 68
> ++++++++++++++++++++++++++++---------------------------
> 2 files changed, 39 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
> index ca51a4e..dff0071 100644
> --- a/gdb/common/ptid.c
> +++ b/gdb/common/ptid.c
> @@ -22,8 +22,8 @@
>
> /* See ptid.h for these. */
>
> -ptid_t null_ptid = ptid_t::build (0, 0, 0);
> -ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
> +ptid_t null_ptid = ptid_t (0, 0, 0);
> +ptid_t minus_one_ptid = ptid_t (-1, 0, 0);
>
>
> /* See ptid.h. */
> @@ -31,7 +31,7 @@ ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
> ptid_t
> ptid_build (int pid, long lwp, long tid)
> {
> - return ptid_t::build (pid, lwp, tid);
> + return ptid_t (pid, lwp, tid);
> }
>
> /* See ptid.h. */
> @@ -39,7 +39,7 @@ ptid_build (int pid, long lwp, long tid)
> ptid_t
> pid_to_ptid (int pid)
> {
> - return ptid_t::build (pid);
> + return ptid_t (pid);
> }
>
> /* See ptid.h. */
> diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
> index c8649ae..1cb57e1 100644
> --- a/gdb/common/ptid.h
> +++ b/gdb/common/ptid.h
> @@ -20,6 +20,8 @@
> #ifndef PTID_H
> #define PTID_H
>
> +#include <type_traits>
> +
> class ptid_t;
>
> /* The null or zero ptid, often used to indicate no process. */
> @@ -44,69 +46,65 @@ extern ptid_t minus_one_ptid;
> class ptid_t
> {
> public:
> - static ptid_t build (int pid, long lwp = 0, long tid = 0)
> - {
> - ptid_t ptid;
> + /* Must have a trivial defaulted default constructor so that the
> + type remains POD. */
> + ptid_t () noexcept = default;
>
> - ptid.m_pid = pid;
> - ptid.m_lwp = lwp;
> - ptid.m_tid = tid;
> + constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
> + : m_pid (pid), m_lwp (lwp), m_tid (tid)
> + {}
>
> - return ptid;
> - }
> -
> - bool is_pid () const
> + constexpr bool is_pid () const
> {
> - if (is_any () || is_null())
> - return false;
> -
> - return m_lwp == 0 && m_tid == 0;
> + return (!is_any ()
> + && !is_null()
> + && m_lwp == 0
> + && m_tid == 0);
> }
>
> - bool is_null () const
> + constexpr bool is_null () const
> {
> return *this == null_ptid;
> }
>
> - bool is_any () const
> + constexpr bool is_any () const
> {
> return *this == minus_one_ptid;
> }
>
> - int pid () const
> + constexpr int pid () const
> { return m_pid; }
>
> - bool lwp_p () const
> + constexpr bool lwp_p () const
> { return m_lwp != 0; }
>
> - long lwp () const
> + constexpr long lwp () const
> { return m_lwp; }
>
> - bool tid_p () const
> + constexpr bool tid_p () const
> { return m_tid != 0; }
>
> - long tid () const
> + constexpr long tid () const
> { return m_tid; }
>
> - bool operator== (const ptid_t &other) const
> + constexpr 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
> + constexpr 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;
> + return (/* If filter represents any ptid, it's always a match. */
> + filter.is_any ()
> + /* 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);
> }
>
> private:
> @@ -120,6 +118,10 @@ private:
> long m_tid;
> };
>
> +static_assert (std::is_pod<ptid_t>::value, "");
> +
> +static_assert (ptid_t(1).pid () == 1, "");
Wow, nice. So all the tests are probably going to be static.
Just to be clear, do you suggest that we make a test that ensures ptid_t
is a POD, or you wrote that one just to show me it works? I We don't
really care if it is, it's just that the current situation (it being
used in another POD) requires it.
> +
> /* Make a ptid given the necessary PID, LWP, and TID components. */
> ptid_t ptid_build (int pid, long lwp, long tid);
Thanks,
Simon
On 2017-04-05 17:31, Pedro Alves wrote:
> - bool is_null () const
> + constexpr bool is_null () const
> {
> return *this == null_ptid;
> }
>
> - bool is_any () const
> + constexpr bool is_any () const
> {
> return *this == minus_one_ptid;
> }
Hmm I think there's a problem with these. null_ptid and minus_one_ptid
are not constant expressions, so these methods (and the ones that use
them) are not really constexpr (you notice when you actually try to use
them as constant expressions). Making null_ptid and minus_one_ptid
constexpr would be useful, I could use them in the static_assert tests
(e.g. to test "matches"). However, when trying to make them constexpr,
it creates a dependency loop: the class needs to be defined after the
null_ptid/minus_one_ptid definitions because it uses them (and you can't
forward declare a constexpr variable I suppose?), but the
null_ptid/minus_one_ptid definitions have to be be defined after the
class, when the type is complete.
An easy workaround would be to not refer to null_ptid/minus_one_ptid in
the class, but use manually crafted values:
constexpr bool is_null () const
{
return *this == ptid_t (0, 0, 0);
}
constexpr bool is_any () const
{
return *this == ptid_t (-1, 0, 0);
}
And to avoid duplication, use defines:
/* Work around the fact that we want to refer to null_ptid and
minus_one_ptid
in the definition of class ptid_t, but they have to be defined after.
*/
#define NULL_PTID ptid (0, 0, 0)
#define MINUS_ONE_PTID ptid (-1, 0, 0)
class ptid_t
{
...
constexpr bool is_null () const
{
return *this == NULL_PTID;
}
constexpr bool is_any () const
{
return *this == MINUS_ONE_PTID;
}
...
}
constexpr ptid_t null_ptid = NULL_PTID;
constexpr ptid_t minus_one_ptid = MINUS_ONE_PTID;
/* We don't want anybody using these macros, they are just temporary.
#undef NULL_PTID
#undef MINUS_ONE_PTID
What do you think?
Now, making null_ptid/minus_one_ptid constexpr brings its share of
fallouts, such as:
/home/simark/src/binutils-gdb/gdb/linux-nat.c: In function ‘void
linux_unstop_all_lwps()’:
/home/simark/src/binutils-gdb/gdb/linux-nat.c:2387:37: error: invalid
conversion from ‘const void*’ to ‘void*’ [-fpermissive]
resume_stopped_resumed_lwps, &minus_one_ptid);
^~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/linux-nat.c:980:1: note:
initializing argument 3 of ‘lwp_info* iterate_over_lwps(ptid_t, int
(*)(lwp_info*, void*), void*)’
iterate_over_lwps (ptid_t filter,
^~~~~~~~~~~~~~~~~
But it looks easy enough to fix by C++-ifying/modernizing
iterate_over_lwps.
On 04/06/2017 03:15 AM, Simon Marchi wrote:
> On 2017-04-05 17:31, Pedro Alves wrote:
>>>> 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.
>>
>> Thanks. constexpr in C++11 forces you to write a single
>> return statement (C++14 gets rid of that requirement),
>> but it looks quite doable.
>>
>> Also, note that it's not true that this type can't have a
>> constructor. It can, as long as the type remains POD.
>
> Ah, so I was just missing the defaulted default constructor. Adding it
> makes the type trivial, which then makes it POD.
Right, almost. There's a couple other requirements beyond trivial,
but they're fulfilled as well. See:
http://en.cppreference.com/w/cpp/concept/PODType
>
>>> Is the following ok?
>>>
>>> struct thread_resume default_action { null_ptid };
>>
>> ISTR that in C++11 you'll need the double "{{" levels, like:
>>
>> thread_resume default_action {{null_ptid}};
>>
>> and that C++14 removed that requirement (brace elision).
>> But I haven't confirmed. Whatever works with -std=c++11.
>
> It seems to work with a single pair of braces with c++11. I'll still
> check that it does what we want at runtime, but I'd be surprised if it
> didn't do the right thing.
Sorry, yes, it's not necessary -- I somehow confused myself into
thinking that the current double "{{" was because the ptid field was
inside a structure that itself is inside the thread_resume structure.
struct thread_resume
{
struct something_else
{
ptid_t thread;
but "something_else" doesn't really exist... If it existed, then
the double {{ would be necessary in C++11 to initialize the sub field, but
not in C++. See brace elision here:
http://en.cppreference.com/w/cpp/language/aggregate_initialization
But now that I try, I can't make g++ nor clang++ error in c++11 with
those constructs. Maybe other compilers would, though. In any
case, the workaround would be trivial if we needed it -- just
add the equals sign.
>>
>>
>> +static_assert (std::is_pod<ptid_t>::value, "");
>> +
>> +static_assert (ptid_t(1).pid () == 1, "");
>
> Wow, nice. So all the tests are probably going to be static.
>
> Just to be clear, do you suggest that we make a test that ensures ptid_t
> is a POD, or you wrote that one just to show me it works? I We don't
> really care if it is, it's just that the current situation (it being
> used in another POD) requires it.
Yes, I think we should put that in the unit test with a comment
so that if someone tries to add something that would make it
non-pod, gdb won't even compile. If/when we get to the point where
we can/want to make it non-pod, we can remove the assertion then.
Thanks,
Pedro Alves
On 04/06/2017 11:49 AM, Pedro Alves wrote:
> Yes, I think we should put that in the unit test with a comment
Or leave it in ptid.c, doesn't have to be in a separate file.
Putting it in the .c file instead of the .h has the advantage
that it doesn't expose type_traits.h to all of gdb, that's all.
(Though I'm not sure whether be able to avoid it as we grow more
C++ utilities.)
> so that if someone tries to add something that would make it
> non-pod, gdb won't even compile. If/when we get to the point where
> we can/want to make it non-pod, we can remove the assertion then.
Thanks,
Pedro Alves
On 2017-04-06 07:12, Pedro Alves wrote:
> Or leave it in ptid.c, doesn't have to be in a separate file.
> Putting it in the .c file instead of the .h has the advantage
> that it doesn't expose type_traits.h to all of gdb, that's all.
> (Though I'm not sure whether be able to avoid it as we grow more
> C++ utilities.)
My intention was to put the tests in unittests/ptid-selftests.c, any
objection to that?
On 04/06/2017 03:31 PM, Simon Marchi wrote:
> On 2017-04-06 07:12, Pedro Alves wrote:
>> Or leave it in ptid.c, doesn't have to be in a separate file.
>> Putting it in the .c file instead of the .h has the advantage
>> that it doesn't expose type_traits.h to all of gdb, that's all.
>> (Though I'm not sure whether be able to avoid it as we grow more
>> C++ utilities.)
>
> My intention was to put the tests in unittests/ptid-selftests.c, any
> objection to that?
No.
Thanks,
Pedro Alves
@@ -22,8 +22,8 @@
/* See ptid.h for these. */
-ptid_t null_ptid = ptid_t::build (0, 0, 0);
-ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
+ptid_t null_ptid = ptid_t (0, 0, 0);
+ptid_t minus_one_ptid = ptid_t (-1, 0, 0);
/* See ptid.h. */
@@ -31,7 +31,7 @@ ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
ptid_t
ptid_build (int pid, long lwp, long tid)
{
- return ptid_t::build (pid, lwp, tid);
+ return ptid_t (pid, lwp, tid);
}
/* See ptid.h. */
@@ -39,7 +39,7 @@ ptid_build (int pid, long lwp, long tid)
ptid_t
pid_to_ptid (int pid)
{
- return ptid_t::build (pid);
+ return ptid_t (pid);
}
/* See ptid.h. */
@@ -20,6 +20,8 @@
#ifndef PTID_H
#define PTID_H
+#include <type_traits>
+
class ptid_t;
/* The null or zero ptid, often used to indicate no process. */
@@ -44,69 +46,65 @@ extern ptid_t minus_one_ptid;
class ptid_t
{
public:
- static ptid_t build (int pid, long lwp = 0, long tid = 0)
- {
- ptid_t ptid;
+ /* Must have a trivial defaulted default constructor so that the
+ type remains POD. */
+ ptid_t () noexcept = default;
- ptid.m_pid = pid;
- ptid.m_lwp = lwp;
- ptid.m_tid = tid;
+ constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
+ : m_pid (pid), m_lwp (lwp), m_tid (tid)
+ {}
- return ptid;
- }
-
- bool is_pid () const
+ constexpr bool is_pid () const
{
- if (is_any () || is_null())
- return false;
-
- return m_lwp == 0 && m_tid == 0;
+ return (!is_any ()
+ && !is_null()
+ && m_lwp == 0
+ && m_tid == 0);
}
- bool is_null () const
+ constexpr bool is_null () const
{
return *this == null_ptid;
}
- bool is_any () const
+ constexpr bool is_any () const
{
return *this == minus_one_ptid;
}
- int pid () const
+ constexpr int pid () const
{ return m_pid; }
- bool lwp_p () const
+ constexpr bool lwp_p () const
{ return m_lwp != 0; }
- long lwp () const
+ constexpr long lwp () const
{ return m_lwp; }
- bool tid_p () const
+ constexpr bool tid_p () const
{ return m_tid != 0; }
- long tid () const
+ constexpr long tid () const
{ return m_tid; }
- bool operator== (const ptid_t &other) const
+ constexpr 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
+ constexpr 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;
+ return (/* If filter represents any ptid, it's always a match. */
+ filter.is_any ()
+ /* 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);
}
private:
@@ -120,6 +118,10 @@ private:
long m_tid;
};
+static_assert (std::is_pod<ptid_t>::value, "");
+
+static_assert (ptid_t(1).pid () == 1, "");
+
/* Make a ptid given the necessary PID, LWP, and TID components. */
ptid_t ptid_build (int pid, long lwp, long tid);