On 18.03.2020 23:21, Simon Marchi wrote:
> On 2020-03-18 5:55 p.m., Kamil Rytarowski wrote:
>> @@ -313,8 +325,12 @@ inf_ptrace_target::kill ()
>>    target_mourn_inferior (inferior_ptid);
>>  }
>>
>> +#ifndef __NetBSD__
>>  /* Return which PID to pass to ptrace in order to observe/control the
>> -   tracee identified by PTID.  */
>> +   tracee identified by PTID.
>> +
>> +   Unlike most other Operating Systems, NetBSD tracks both pid and lwp
>> +   and avoids this function.  */
> 
> To align this function with our current way of documenting functions, please move
> this comment to the declaration of get_ptrace_pid, in inf-ptrace.h, and put this here:
> 
>   /* See inf-ptrace.h.  */
> 

Done.

>>
>>  pid_t
>>  get_ptrace_pid (ptid_t ptid)
>> @@ -328,6 +344,7 @@ get_ptrace_pid (ptid_t ptid)
>>      pid = ptid.pid ();
>>    return pid;
>>  }
>> +#endif
>>
>>  /* 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
>> @@ -336,15 +353,12 @@ get_ptrace_pid (ptid_t ptid)
>>  void
>>  inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
>>  {
>> -  pid_t pid;
>>    int request;
>>
>>    if (minus_one_ptid == ptid)
>>      /* Resume all threads.  Traditionally ptrace() only supports
>>         single-threaded processes, so simply resume the inferior.  */
>> -    pid = inferior_ptid.pid ();
>> -  else
>> -    pid = get_ptrace_pid (ptid);
>> +    ptid = inferior_ptid;
> 
> I'm not certain about this part (which I suggested).  With the existing code,
> if inferior_ptid is {pid=10, lwp=20}, we'll call ptrace with pid=10.  With the
> patch, we'll call ptrace with pid=20.
> 
> I'm not entirely sure if/when resume can be called with ptid == minus_one_ptid.
> And if it's called with minus_one_ptid, is it possible that ptid.pid != ptid.lwp?
> 
> In any case, it's probably safer to do:
> 
>   ptid = ptid_t (inferior_ptid.pid ());
> 
> to retain the existing behavior.  WDYT?
> 

It's OK.

>>
>>    if (catch_syscall_enabled () > 0)
>>      request = PT_SYSCALL;
>> @@ -365,7 +379,7 @@ inf_ptrace_target::resume (ptid_t ptid, int step, enum gdb_signal signal)
>>       where it was.  If GDB wanted it to start some other way, we have
>>       already written a new program counter value to the child.  */
>>    errno = 0;
>> -  ptrace (request, pid, (PTRACE_TYPE_ARG3)1, gdb_signal_to_host (signal));
>> +  gdb_ptrace (request, ptid, (PTRACE_TYPE_ARG3)1, gdb_signal_to_host (signal));
>>    if (errno != 0)
>>      perror_with_name (("ptrace"));
>>  }
>> @@ -528,7 +542,7 @@ inf_ptrace_target::xfer_partial (enum target_object object,
>>  				 const gdb_byte *writebuf,
>>  				 ULONGEST offset, ULONGEST len, ULONGEST *xfered_len)
>>  {
>> -  pid_t pid = get_ptrace_pid (inferior_ptid);
>> +  ptid_t ptid = inferior_ptid;
>>
>>    switch (object)
>>      {
>> @@ -552,7 +566,7 @@ inf_ptrace_target::xfer_partial (enum target_object object,
>>  	piod.piod_len = len;
>>
>>  	errno = 0;
>> -	if (ptrace (PT_IO, pid, (caddr_t)&piod, 0) == 0)
>> +	if (gdb_ptrace (PT_IO, ptid, (caddr_t)&piod, 0) == 0)
>>  	  {
>>  	    /* Return the actual number of bytes read or written.  */
>>  	    *xfered_len = piod.piod_len;
>> @@ -565,7 +579,7 @@ inf_ptrace_target::xfer_partial (enum target_object object,
>>  	  return TARGET_XFER_EOF;
>>        }
>>  #endif
>> -      *xfered_len = inf_ptrace_peek_poke (pid, readbuf, writebuf,
>> +      *xfered_len = inf_ptrace_peek_poke (ptid.pid (), readbuf, writebuf,
>>  					  offset, len);
>>        return *xfered_len != 0 ? TARGET_XFER_OK : TARGET_XFER_EOF;
> 
> I'm pretty sure that this change is not right.  For non-NetBSD targets, this
> used to pass `inferior_ptid.lwp` (the result of get_ptrace_pid).  Now, we pass
> `inferior_ptid.pid`.
> 

OK.

> I think you'll need to pass the ptid to inf_ptrace_peek_poke and use gdb_ptrace
> inside that function too.
> 

Done.

> Simon
>