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