Re: [PATCH, cleanup] Standardize access to ptid

[Pedro Alves <palves at redhat dot com>]

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."

> 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...  ;-)

>
>    return (ptid_get_lwp (ptid) == 0 && ptid_get_tid (ptid) == 0);
>  }
> +
> +/* Returns true if PTID represents a lwp.  */
> +
> +int
> +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.

> +{
> +  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.

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.

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.)

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 ()").

Thanks,
-- 
Pedro Alves