Re: [PATCH, cleanup] Standardize access to ptid

Pedro Alves
2013-09-26T15:10:41Z

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