Re: [PATCH] Avoid get_ptrace_pid() usage on NetBSD in x86-bsd-nat.c

[Kamil Rytarowski <n54@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3464 bytes --]

On 19.03.2020 02:35, Simon Marchi wrote:
> On 2020-03-18 12:13 p.m., Kamil Rytarowski wrote:
>> Add gdb_ptrace() that wraps the ptrace(2) API and correctly passes
>> the pid,lwp pair to the calls on NetBSD; and the result of
>> get_ptrace_pid() on other BSD Operating Systems.
>>
>> gdb/ChangeLog:
>>
>> 	* x86-bsd-nat.c (gdb_ptrace): New.
>> 	* (x86bsd_dr_get, x86bsd_dr_set): Use gdb_ptrace.
>> ---
>>  gdb/ChangeLog     |  5 +++++
>>  gdb/x86-bsd-nat.c | 39 +++++++++++++++++++--------------------
>>  2 files changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>> index 84964dc00ac..1beb835df78 100644
>> --- a/gdb/ChangeLog
>> +++ b/gdb/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2020-03-18  Kamil Rytarowski  <n54@gmx.com>
>> +
>> +	* x86-bsd-nat.c (gdb_ptrace): New.
>> +	* (x86bsd_dr_get, x86bsd_dr_set): Use gdb_ptrace.
>> +
>>  2020-03-17  Kamil Rytarowski  <n54@gmx.com>
>>
>>  	* regformats/regdef.h: Put reg in gdb namespace.
>> diff --git a/gdb/x86-bsd-nat.c b/gdb/x86-bsd-nat.c
>> index 640a3c28110..37d0bfda37c 100644
>> --- a/gdb/x86-bsd-nat.c
>> +++ b/gdb/x86-bsd-nat.c
>> @@ -33,6 +33,19 @@
>>  #include "inf-ptrace.h"
>>  \f
>>
>> +static int
>> +gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr)
>> +{
>> +#ifdef __NetBSD__
>> +  /* Support for NetBSD threads: unlike other ptrace implementations in this
>> +     file, NetBSD requires that we pass both the pid and lwp.  */
>> +  return ptrace (request, ptid.pid (), addr, ptid.lwp ());
>> +#else
>> +  pid_t pid = get_ptrace_pid (ptid);
>> +  return ptrace (request, pid, addr, 0);
>> +#endif
>> +}
>> +
>>  #ifdef PT_GETXSTATE_INFO
>>  size_t x86bsd_xsave_len;
>>  #endif
>> @@ -56,14 +69,9 @@ static unsigned long
>>  x86bsd_dr_get (ptid_t ptid, int regnum)
>>  {
>>    struct dbreg dbregs;
>> -#ifdef __NetBSD__
>> -  int lwp = inferior_ptid.lwp ();
>> -#else
>> -  int lwp = 0;
>> -#endif
>>
>> -  if (ptrace (PT_GETDBREGS, get_ptrace_pid (inferior_ptid),
>> -	      (PTRACE_TYPE_ARG3) &dbregs, lwp) == -1)
>> +  if (gdb_ptrace (PT_GETDBREGS, inferior_ptid,
>> +		  (PTRACE_TYPE_ARG3) &dbregs) == -1)
>>      perror_with_name (_("Couldn't read debug registers"));
> 
> This function accepts a ptid parameter but does not use it.  Can you change
> it so it uses the parameter, and not inferior_ptid?  All the callers pass
> inferior_ptid, so it won't change the behavior.
> 

OK.

>>
>>    return DBREG_DRX ((&dbregs), regnum);
>> @@ -73,14 +81,9 @@ static void
>>  x86bsd_dr_set (int regnum, unsigned long value)
>>  {
>>    struct dbreg dbregs;
>> -#ifdef __NetBSD__
>> -  int lwp = inferior_ptid.lwp ();
>> -#else
>> -  int lwp = 0;
>> -#endif
>>
>> -  if (ptrace (PT_GETDBREGS, get_ptrace_pid (inferior_ptid),
>> -              (PTRACE_TYPE_ARG3) &dbregs, lwp) == -1)
>> +  if (gdb_ptrace (PT_GETDBREGS, inferior_ptid,
>> +		  (PTRACE_TYPE_ARG3) &dbregs) == -1)
>>      perror_with_name (_("Couldn't get debug registers"));
> 
> For symmetry, can you please change this function to accept a ptid, just like
> x86bsd_dr_get, and update the callers to pass inferior_ptid?
> 

This is a good idea.. however it is intrusive now and requires patching
code shared with windows, linux, darwin, ...

I prefer to leave it for refactoring in future. I don't have environment
to test other Operating Systems than NetBSD.

> Simon
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]