[PATCH, cleanup] Standardize access to ptid
Luis Machado
lgustavo@codesourcery.com
Sun Sep 29 17:17:00 GMT 2013
On 09/26/2013 12:10 PM, Pedro Alves wrote:
> First off, thanks a lot for doing this Luis. Should have
> been done many years ago, IMO.
>
> On 09/26/2013 01:57 PM, Luis Machado wrote:
>
>>>> @@ -387,7 +387,8 @@ linux_child_follow_fork (struct target_ops *ops, int follow_child,
>>>> parent_pid = ptid_get_lwp (inferior_ptid);
>>>> if (parent_pid == 0)
>>>> parent_pid = ptid_get_pid (inferior_ptid);
>>>> - child_pid = PIDGET (inferior_thread ()->pending_follow.value.related_pid);
>>>> + child_pid =
>>>> + ptid_get_pid (inferior_thread ()->pending_follow.value.related_pid);
>>>
>>> I think Pedro told me that the '=' binary operator should be on
>>> the second line.
>
> Yep, it's what the GNU coding standards say.
>
> "When you split an expression into multiple lines, split it
> before an operator, not after one."
>
Fixed. Also fixed another ocurrence of this in python/py-inferior.c.
>> I'll give the patch the rest of the week
>> in case someone decides to look into it some more and spot mistakes,
>> then i'll check it in.
>
> I had actually started out composing the below a couple days ago,
> but didn't manage to finish it, mainly because the patch does more
> than plain macro replacement, otherwise I'd have given it a
> quick OK... ;-)
>
Really, i should remember that before the next patch. :-)
>>
>> return (ptid_get_lwp (ptid) == 0 && ptid_get_tid (ptid) == 0);
>> }
>> +
>> +/* Returns true if PTID represents a lwp. */
>> +
>> +in
>> +ptid_is_lwp (ptid_t ptid)
>> +{
>> + if (ptid_is_invalid (ptid))
>> + return 0;
>> +
>> + return (ptid_get_lwp (ptid) != 0);
>> +}
>> +
>> +/* Returns true if PTID represents a tid. */
>> +
>> +int
>> +ptid_is_tid (ptid_t ptid)
>> +{
>> + if (ptid_is_invalid (ptid))
>> + return 0;
>> +
>> + return (ptid_get_tid (ptid) != 0);
>> +}
>> +
>> +/* Returns true if PTID is either MINUS_ONE_PTID or NULL_PTID. */
>> +
>> +int ptid_is_invalid (ptid_t ptid)
>
> Missing newline.
>
This function is gone now.
>> +{
>> + if (ptid_equal (minus_one_ptid, ptid)
>> + || ptid_equal (null_ptid, ptid))
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> +
>> diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
>> index fefe8b6..0fa85e3 100644
>> --- a/gdb/common/ptid.h
>> +++ b/gdb/common/ptid.h
>> @@ -33,6 +33,10 @@
>> ptid_get_lwp - Fetch the lwp component of a ptid.
>> ptid_get_tid - Fetch the tid component of a ptid.
>> ptid_equal - Test to see if two ptids are equal.
>> + ptid_is_pid - Test if a ptid's pid component is non-zero.
>
> No, that's not right:
>
> /* Returns true if PTID represents a process. */
>
> int
> ptid_is_pid (ptid_t ptid)
> {
> if (ptid_equal (minus_one_ptid, ptid))
> return 0;
> if (ptid_equal (null_ptid, ptid))
> return 0;
>
> return (ptid_get_lwp (ptid) == 0 && ptid_get_tid (ptid) == 0);
> }
>
> So this only returns true iff the ptid looks like (pid,0,0).
> (ptid_is_pid on (pid,lwp,0) returns false, for example.)
> This is considered a ptid that identifies the whole PID process (the
> whole thread group in Linux speak). Both the core and the targets
> use and agree on this.
I've changed the description to the following:
"Test if a ptid looks like (pid, 0, 0)."
Seems to clearly state what is being checked.
>
> It's more of a stretch to generalize the target's "is_lwp" predicate
> as a core "ptid_is_lwp" function (likewise is_thread->ptid_is_tid).
> The "is_lwp" and "is_thread" predicates really are currently target
> private. IMO, there's really nowhere in the core that should be using
> those. The Solaris port supports the M:N thread model, and uses
> (pid,lwp,0) to identify a kernel thread, and (pid,0,tid) to identify a
> userspace thread. The GNU/Linux port / linux-thread-db.c, having
> grown imitating Solaris, also supported that originally, likewise,
> and since the code was originally mostly copied, it ended up with
> the same macros. We only support the 1:1 model on GNU/Linux nowadays,
> and therefore we'll only ever see (pid,lwp,0) style ptids there.
>
> Now, there's nothing really stopping some target supporting a M:N
> thread model from identifying userspace threads as (pid,lwp,tid), and
> then ptid_is_lwp would return true to those too, while it's not clear
> it should. Again, a target side detail.
>
> I guess what I have is an issue with the "_is_" in ptid_is_lwp
> and ptid_is_tid. It's not the same as the "is" in ptid_is_pid.
>
> Now, we currently have the issue that the APIs don't really
> cleanly support OSs where processes/lwpid/thread numbered 0 is
> valid. We force the target to do some numeric transformation
> in/out of ptids. It's perhaps silly for a target to make 0
> a valid pid, but, they're out there.
>
> I've before imagined the ptid_t structure growing something like:
>
> struct ptid
> {
> /* Process id */
> int pid;
>
> /* Lightweight process id */
> long lwp;
>
> /* Thread id */
> long tid;
>
> + /* Flags to indicate the above fields have valid contents. */
> + int pid_p : 1;
> + int lwp_p : 1;
> + int tid_p : 1;
> };
>
> Very much like the frame_id structure. And just like
> the frame_id structure, we could play with the _p fields
> to build wildcard ptids, null ptids, and other special ptids
> as necessary.
All of this makes sense to me, but perhaps we should introduce such a
change later on? After the cleanup possibly, since this will require
changes in places of the code that deal with various subsystems of GDB.
>
> With that in mind, I think I'd prefer renaming these
> new "is" functions as:
>
> ptid_is_lwp -> ptid_lwp_p
> ptid_is_tid -> ptid_tid_p
>
> (or at least ptid_has_lwp, though the _p variant has
> precedent in the frame stuff, and it feels to me that frame_ids
> and ptids are at about the same conceptual level.)
I'm happy with ptid_lwp_p and ptid_tid_p.
>
> Folowing what I like to think as the "principle of least reversion
> (and easy bisect)", I'd drop the ptid_is_invalid checks, just like
> the existing code doesn't have them, at least from this patch...
>
>> + ptid_is_lwp - Test if a ptid's lwp component is non-zero.
>> + ptid_is_tid - Test if a ptid's tid component is non-zero.
>
>> + ptid_is_invalid - Test if ptid is minus_one_ptid or null_ptid.
>
> And I'm also don't really like the "ptid_is_invalid" function that much.
> minus_one_ptid or null_ptid aren't really always invalid. They have
> special meanings as either invalid, terminator, or as wildcard depending
> on context. See e.g, how frame_id_p returns true to wildcard frame ids,
> and the special outer_frame_id (although that one should die.
> But with the above suggestion, I don't think the function
> would end up with any use left, so it could just be dropped. But I suppose
> I'll just get used to it if it stays. ;-) But, if it stays, please,
> please, invert its logic, getting rid of the double
> negative ("if (!ptid_is_invalid ()").
I thought about ptid_special_p or ptid_is_special to check for both
null_ptid and minus_one_ptid, but this check would only be used (for now
at least) in the ptid.c file. Maybe not worth the effort, so i left it out.
Luis
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ptid.diff
Type: text/x-patch
Size: 151907 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20130929/8d91dc16/attachment.bin>
More information about the Gdb-patches
mailing list