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

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

Pedro Alves <pedro@palves.net> writes:

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

Ah, indeed that is true. Thanks for pointing it out.

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

We are talking about this in the other branch of this email thread so I
won't respond here, but I'll leave the quote above because I reference
it below.

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

In the case of the spots changed by this patch, we know that
current_thread is always set because, as can be seen in the first code
hunk you quoted, before this patch was applied
linux_process_target::read_auxv dereferenced it.

But I agree that it makes more sense to use current_process (). I'll
post a new patch with the change.

-- 
Thiago