Re: [PATCH v3 2/8] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

Pedro Alves
2023-02-06T19:54:40Z

On 2023-01-30 4:45 a.m., Thiago Jung Bauermann via Gdb-patches wrote:
> This patch doesn't change gdbserver behaviour,

That's not actually true -- before the patch, we use the lwpid
of the current thread, while after the patch we always use the pid of the
process.

Here:

> int
> -linux_process_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> - unsigned int len)
> +linux_process_target::read_auxv (int pid, CORE_ADDR offset,
> + unsigned char *myaddr, unsigned int len)
> {
> char filename[PATH_MAX];
> int fd, n;
> - int pid = lwpid_of (current_thread);
>
> xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
>

AFAICT by playing with debugging the gdb.threads/leader-exit.exp program,
you can't read /proc/PID/auxv if the leader thread has exited (is zombie).

Maybe that ends up not mattering in practice, not sure, but it is a
behavior change.

> 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.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> ---
> 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 | 3 ++-
> gdbserver/target.cc | 4 ++--
> gdbserver/target.h | 4 ++--
> 11 files changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 3c09e086afee..2ed6e95562c5 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -846,12 +846,13 @@ aarch64_target::low_arch_setup ()
> if (is_elf64)
> {
> struct aarch64_features features;
> + int pid = current_thread->id.pid ();

You can also write:

current_process ()->pid

which is a more to the point. Also, sometimes there's a current
process, but not a current thread, so when we don't actually need a thread,
current_process() is better.

Pedro Alves