From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10607 invoked by alias); 26 Sep 2013 15:10:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 10594 invoked by uid 89); 26 Sep 2013 15:10:40 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 26 Sep 2013 15:10:40 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPAM_SUBJECT autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8QFAaPq012385 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 26 Sep 2013 11:10:36 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r8QFAYWp019700; Thu, 26 Sep 2013 11:10:35 -0400 Message-ID: <52444E6A.2060104@redhat.com> Date: Thu, 26 Sep 2013 15:10:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: lgustavo@codesourcery.com CC: Joel Brobecker , "'gdb-patches@sourceware.org'" Subject: Re: [PATCH, cleanup] Standardize access to ptid References: <523B7A79.1060901@codesourcery.com> <20130926125258.GA3198@adacore.com> <52442F4E.8000808@codesourcery.com> In-Reply-To: <52442F4E.8000808@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-09/txt/msg00925.txt.bz2 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