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

[Simon Marchi <simark@simark.ca>]

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.

> ---
>  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.

> 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".

Simon