Re: [PATCH, cleanup] Standardize access to ptid

[Luis Machado <lgustavo@codesourcery.com>]

On 09/26/2013 09:52 AM, Joel Brobecker wrote:
> Hi Luis,
>
> First of all, thank you for doing this!
>

Thanks for taking the time to go through the huge patch.

>> -  int tid = TIDGET (ptid);
>> +  int tid = ptid_get_lwp (ptid);
>
> ARGH! I am *SO* glad that you've done this change, because I was
> very surprised by this change, and thought it was an oversight.
> It's only after I saw the same change a second time that I started
> looking at the actual definition of this macro and, surprise:
>
>          defs.h:#define TIDGET(PTID) (ptid_get_lwp (PTID))
>
> Double-argh!
>

Yeah. Quite confusing. :-(

>> I've been extra careful to convert from the ptid-building macros to
>> the ptid_build function since sol-thread.c and aix-thread.c had some
>> variations regarding the position of TID/LWP inside the ptid. Please
>> double check.
>
> I did as best as I could, given the length of patch, and it looks
> pretty good.
>
>> 2013-09-19  Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* aarch64-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	Replace GET_LWP with ptid_get_lwp.
>> 	* aix-thread.c (BUILD_THREAD, BUILD_LWP): Remove.
>> 	Replace BUILD_THREAD with ptid_build.
>> 	Replace BUILD_LWP with ptid_build.
>> 	Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* alphabsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* amd64-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* amd64bsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* arm-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	Replace GET_LWP with ptid_get_lwp.
>> 	* armnbsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* auxv.c: Likewise.
>> 	* breakpoint.c: Likewise.
>> 	* common/ptid.c (ptid_is_pid): Call ptid_is_invalid.
>> 	(ptid_is_lwp): New function.
>> 	(ptid_is_tid): New function.
>> 	(ptid_is_invalid): New function.
>> 	* common/ptid.h: Update comments for accessors.
>> 	(ptid_is_lwp): New prototype.
>> 	(ptid_is_tid): New prototype.
>> 	(ptid_is_invalid): New prototype.
>> 	* defs.h (PIDGET, TIDGET, MERGEPID): Do not define.
>> 	* gcore.c: Replace PIDGET with ptid_get_pid.
>> 	* gdbthread.h: Likewise.
>> 	* gnu-nat.c: Likewise.
>> 	* hppa-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* hppabsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* hppanbsd-nat.c: Likewise.
>> 	* i386-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* i386bsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* ia64-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* infcmd.c: Likewise.
>> 	* inferior.h: Likewise.
>> 	* inflow.c: Likewise.
>> 	* infrun.c: Likewise.
>> 	* linux-fork.c: Likewise.
>> 	* linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace GET_PID with ptid_get_pid.
>> 	Replace is_lwp with ptid_is_lwp.
>> 	Replace GET_LWP with ptid_get_lwp.
>> 	Replace BUILD_LWP with ptid_build.
>> 	* linux-nat.h (GET_LWP, GET_PID, is_lwp, BUILD_LWP): Remove.
>> 	* linux-thread-db.c: Replace GET_PID with ptid_get_pid.
>> 	Replace GET_LWP with ptid_get_lwp.
>> 	Replace BUILD_LWP with ptid_build.
>> 	* m32r-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* m68kbsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* m68klinux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* m88kbsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* mi/mi-interp.c: Likewise.
>> 	* mi/mi-main.c: Likewise.
>> 	* mips64obsd-nat.c: Likewise.
>> 	* mipsnbsd-nat.c: Likewise.
>> 	* nto-procfs.c: Likewise.
>> 	* ppc-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* ppcfbsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* ppcnbsd-nat.c: Likewise.
>> 	* ppcobsd-nat.c: Likewise.
>> 	* proc-service.c (BUILD_LWP): Remove.
>> 	Replace BUILD_LWP with ptid_build.
>> 	* procfs.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	Replace MERGEPID with ptid_build.
>> 	* python/py-inferior.c: Replace PIDGET with ptid_get_pid.
>> 	* python/py-infthread.c: Likewise.
>> 	* record.c: Likewise.
>> 	* rs6000-nat.c: Likewise.
>> 	* s390-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* shnbsd-nat.c: Replace PIDGET with ptid_get_pid.
>> 	* sol-thread.c (GET_PID, GET_LWP, GET_THREAD): Remove.
>> 	(is_lwp, is_thread, BUILD_LWP, BUILD_THREAD): Remove.
>> 	Replace GET_PID and PIDGET with ptid_get_pid.
>> 	Replace GET_LWP with ptid_get_lwp.
>> 	Replace GET_THREAD with ptid_get_tid.
>> 	Replace is_lwp with ptid_is_lwp.
>> 	Replace is_thread with ptid_is_tid.
>> 	Replace BUILD_LWP and BUILD_THREAD with ptid_build.
>> 	* solib-som.c: Replace PIDGET with ptid_get_pid.
>> 	* sparc-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>> 	* spu-linux-nat.c: Likewise.
>> 	* target.c: Replace PIDGET with ptid_get_pid.
>> 	* thread.c: Likewise.
>> 	* vax-nat.c: Likewise.
>> 	* vaxbsd-nat.c: Likewise.
>> 	* windows-nat.c: Likewise.
>> 	* xtensa-linux-nat.c: Replace PIDGET with ptid_get_pid.
>> 	Replace TIDGET with ptid_get_lwp.
>
> I only found a couple of issues. The patch looks good to me otherwise,
> and is OK to commit once the two trivial issues are fixed.
>
>> @@ -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.
>
>> @@ -4445,7 +4449,7 @@ procfs_init_inferior (struct target_ops *ops, int pid)
>>        this point, but it didn't have any lwp info yet.  Notify the core
>>        about it.  This changes inferior_ptid as well.  */
>>     thread_change_ptid (pid_to_ptid (pid),
>> -		      MERGEPID (pid, lwpid));
>> +		      ptid_build (pid, lwpid), 0);
>
> I think the first ')' is at the wrong place? Ie...
>
>     		      ptid_build (pid, lwpid, 0));
>
> ... instead?
>

Oops. I'll fix both of these. 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.

Thanks!
Luis