Re: [PATCH] Linux/ptrace: don't convert ptids when asking inf-ptrace layer to resume LWP

[Pedro Alves <palves@redhat.com>]

On 03/03/2015 02:39 PM, Mark Kettenis wrote:
>> From: Pedro Alves <palves@redhat.com>
>> Date: Tue, 3 Mar 2015 13:33:44 +0000
>>
>> Tested on x86-64 Fedora 20, -m32.
>>
>> gdb/ChangeLog:
>> 2015-03-03  Pedro Alves  <palves@redhat.com>
>>
>> 	* i386-linux-nat.c (i386_linux_resume): Get the ptrace PID out of
>> 	the lwp field of ptid.  Pass the full ptid to get_thread_regcache.
>> 	* inf-ptrace.c (get_ptrace_pid): New function.
>> 	(inf_ptrace_resume): Use it.
>> 	* linux-nat.c (linux_resume_one_lwp): Pass the LWP's ptid ummodified
>> 	to the lower layer.
>> ---
>>  gdb/ChangeLog        |  9 +++++++++
>>  gdb/i386-linux-nat.c |  5 ++---
>>  gdb/inf-ptrace.c     | 25 ++++++++++++++++++++++---
>>  gdb/linux-nat.c      |  6 +-----
>>  4 files changed, 34 insertions(+), 11 deletions(-)
> 
> I have some fear this is going to break non-Linux targets.
> 
>> --- a/gdb/inf-ptrace.c
>> +++ b/gdb/inf-ptrace.c
>> @@ -289,6 +289,22 @@ inf_ptrace_stop (struct target_ops *self, ptid_t ptid)
>>    kill (-inferior_process_group (), SIGINT);
>>  }
>>  
>> +/* Return which PID to pass to ptrace in order to observe/control the
>> +   tracee identified by PTID.  */
>> +
>> +static pid_t
>> +get_ptrace_pid (ptid_t ptid)
>> +{
>> +  pid_t pid;
>> +
>> +  /* If we have an LWPID to work with, use it.  Otherwise, we're
>> +     dealing with a non-threaded program/target.  */
>> +  pid = ptid_get_lwp (ptid);
>> +  if (pid == 0)
>> +    pid = ptid_get_pid (ptid);
>> +  return pid;
>> +}
>> +
>>  /* Resume execution of thread PTID, or all threads if PTID is -1.  If
>>     STEP is nonzero, single-step it.  If SIGNAL is nonzero, give it
>>     that signal.  */
>> @@ -297,13 +313,16 @@ static void
>>  inf_ptrace_resume (struct target_ops *ops,
>>  		   ptid_t ptid, int step, enum gdb_signal signal)
>>  {
>> -  pid_t pid = ptid_get_pid (ptid);
>> +  pid_t pid;
>> +
>>    int request;
> 
> Please don't introduce a blank line here.

Darn, LOL, I removed one, added another...:

 i386_linux_resume (struct target_ops *ops,
                   ptid_t ptid, int step, enum gdb_signal signal)
 {
-  int pid = ptid_get_pid (ptid);
-
+  int pid = ptid_get_lwp (ptid);
   int request;

I'll remove the one I added...

> 
>> -  if (pid == -1)
>> +  if (ptid_equal (minus_one_ptid, ptid))
>>      /* Resume all threads.  Traditionally ptrace() only supports
>>         single-threaded processes, so simply resume the inferior.  */
>> -    pid = ptid_get_pid (inferior_ptid);
>> +    pid = get_ptrace_pid (inferior_ptid);
> 
> This defenitely should remain ptid_get_pid(); you want to resume the
> entire process and not an individual thread.

My thinking is that it doesn't matter in practice.

Or is it that if there are multiple kernel threads in the
process, ptrace(PTRACE_CONT, PID) on bsd actually resumes
them all?  Then other things must be broken anyway.

I was assuming that on BSD targets that use this method,
there would only be one thread in the core thread list, and
it would either have LWPID==0, or have PID==LWPID, thus it didn't
matter if get_ptrace_pid returned the PID or the LWPID.

If there anything that actually creates other threads with
a different LWPID on these targets?

> 
>> +  else
>> +    pid = get_ptrace_pid (ptid);
> 
> This should work for OpenBSD, and probably works for FreeBSD.  It
> won't work for NetBSD, but they will probably need their own
> implementation of this function, so it's probably fine.
> 

Thanks,
Pedro Alves