Re: [PATCH] Remove lwp -> pid conversion in linux_nat_xfer_partial

[Pedro Alves <palves@redhat.com>]

On 03/21/2017 10:17 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> When inferior_ptid represents a lwp, linux_nat_xfer_partial converts it
> to a ptid with only the pid field set.  For example, if
> inferior_ptid is:
> 
>   { .pid = 1234, .lwp = 1235 }
> 
> it will change it to:
> 
>   { .pid = 1235, .lwp = 0 }
> 
> This is presumably because not all implementations of to_xfer_partial
> that might be called down the line know how to handle a ptid with a lwp.
> From what I found, it's been like this for a long long time, I traced
> the original implementation at least to this commit (1999)
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/lin-thread.c;h=2f255c0e54a0387f1b7994c0bf808f4320b9b054;hb=ed9a39e#l1241
> 
> It looks like a hack to me, I think it's simpler if we just make all
> implementations handle ptids correctly.  The only place I found that
> needed fixing is inf_ptrace_xfer_partial.

Yeah...  The stuffing of lwpids in the inferior_pid integer global was clearly
a hack.  But when later GDB grew the "ptid" structure, this shuffling
made some sense -- way back then, when we had LinuxThreads instead of NTPL, the
kernel didn't really have any concept of "threads" or "lwps".  Threads
were really each a heavy weight process.  So in that sense, in the abstract,
it made some sense to have inf-ptrace.c only ever think about processes.  But,
over the years we've been running into issues with that, and over time
the inf-ptrace.c layer as been adjusted to understand these ptids.  Commit 90ad5e1d4f34d0
("Linux/ptrace: don't convert ptids when asking inf-ptrace layer to resume LWP")
is one that comes to mind.  You've running into some left overs of a long
slow conversion...

> 
> There is also linux_proc_xfer_partial and linux_proc_xfer_spu, which
> both only use the pid field of inferior_ptid and ignore lwp.  However,
> since they use "/proc/<pid>", using the id of any thread in the process
> will give the same result (AFAIK).

It's generally better to use the lwp id:

- some files under /proc/<pid>/ may not work if the <pid> thread is
  running, just like ptrace requires a stopped thread.  The current
  thread's lwp id is more likely to be in the necessary state (stopped).

- if the leader exits, and goes zombie, then several files under
  "/proc/<pid>" won't work, though using "/proc/<pid>/task/<tid>" would.
  (try poking at leader-exit.exp a bit.)
  The latter path form is also generally better for being robust in
  the case TID exits and is reused in another process, much like
  tkill vs tgkill.

So if possible to switch those spots too, I'd recommend/prefer it.

> 
> The testsuite found no regression on native amd64 linux.
> 
> gdb/ChangeLog:
> 
> 	* inf-ptrace.c (inf_ptrace_xfer_partial): Get pid from ptid
> 	using get_ptrace_pid.
> 	* linux-nat.c (linux_nat_xfer_partial): Don't set/restore
> 	inferior_ptid.

Looks good to me.

Thanks,
Pedro Alves