Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Simon Marchi <simark@simark.ca> writes:

> On 11/25/22 21:04, Thiago Jung Bauermann via Gdb-patches wrote:
>> This patch doesn't change gdbserver behaviour, but after later changes are
>> made it avoids a null pointer dereference when HWCAP needs to be obtained
>> for a specific process while current_thread is nullptr.
>> 
>> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
>> parameter seems more correct than setting current_thread in one particular
>> code path.
>> 
>> Changes are propagated to allow passing the new parameter through the call
>> chain.
>
> Thanks, this is ok, with the following nits fixed:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> ... unless Luis is strongly against it.

Thanks!

v3 will have the nits fixed and also change uses of “pid_of (thread)” to
“thread->id.pid ()”.

>> ---
>>  gdbserver/linux-aarch64-low.cc |  7 ++++---
>>  gdbserver/linux-arm-low.cc     |  2 +-
>>  gdbserver/linux-low.cc         | 18 +++++++++---------
>>  gdbserver/linux-low.h          |  9 ++++-----
>>  gdbserver/linux-ppc-low.cc     |  6 +++---
>>  gdbserver/linux-s390-low.cc    |  2 +-
>>  gdbserver/netbsd-low.cc        |  4 +---
>>  gdbserver/netbsd-low.h         |  2 +-
>>  gdbserver/server.cc            |  2 +-
>>  gdbserver/target.cc            |  4 ++--
>>  gdbserver/target.h             |  2 +-
>>  11 files changed, 28 insertions(+), 30 deletions(-)
>> 
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index db5086962612..a6ed68f93029 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
>>    if (is_elf64)
>>      {
>>        struct aarch64_features features;
>> +      int pid = pid_of (current_thread);
>>  
>>        features.vq = aarch64_sve_get_vq (tid);
>>        /* A-profile PAC is 64-bit only.  */
>> -      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
>> +      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
>
> Avoid adding the outer parenthesis.

Ah, that's a remnant from an older version of the code where this line
was longer and I added the parentheses to fix indentation. Fixed for v3.

>> diff --git a/gdbserver/target.h b/gdbserver/target.h
>> index 18ab969dda70..fe7a80b645bc 100644
>> --- a/gdbserver/target.h
>> +++ b/gdbserver/target.h
>> @@ -175,7 +175,7 @@ class process_stratum_target
>>    /* Read auxiliary vector data from the inferior process.
>>  
>>       Read LEN bytes at OFFSET into a buffer at MYADDR.  */
>> -  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
>> +  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>
> Please update the doc of the method to replace "from the inferior
> process" to from "process with pid PID".

Indeed, thanks for spotting it. Fixed for v3.

-- 
Thiago