Re: [PATCH, cleanup] Standardize access to ptid

Joel Brobecker
2013-09-26T12:53:04Z

Hi Luis,

First of all, thank you for doing this!

> - 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!

> 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?

--
Joel