Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Alan Hayward <Alan.Hayward@arm.com>]

Apologies, I’ve been away for the last week, only just saw this whole thread.
Thanks for fixing this without me.

Pedro: Some comments/nits below.  Given this is pushed, I’m happy if you don’t
raise a whole new patch for it.


> On 5 Apr 2019, at 17:39, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> 
> Pedro Franco de Carvalho wrote: 
> 
>> Note that there is a difference in the interface between linux_get_auxv
>> and linux_get_hwcap(2), the latter still return both the status and the
>> entry value, but I didn't want to change all their users.
> 
> Actually, I think this is fine -- for hwcap, 0 is a natural default
> value to use if the entry is not present.

Yes, all the targets in GDB treated an error condition the same as a hwcap
value of 0.  Which is sensible given it’s for checking features present in
the hardware/cpu.

> 
>> Also, contrary to the gdb client version (target_auxv_search
>> gdb/auxv.c), this one doesn't differentiate between an error and the
>> entry not being found.
> 
> That also seems reasonable.  If anybody requires more specific error
> handling, that can always be added later.


Originally I was going to keep gdbserver linux_get_auxv with the same returns
as the gdb version.  However, given it is a static function and the hwap
functions were not checking the error value, I removed it.

> 
>> gdb/gdbserver/ChangeLog:
>> 2019-04-DD  Pedro Franco de Carvalho  <pedromfc@linux.ibm.com>
>> 
>> 	* linux-low.c (linux_get_auxv): Remove static.  Return auxv entry
>> 	value in argument pointer, return 1 if the entry is found and 0
>> 	otherwise.  Move comment.

Returning true/false would have been better than 1/0. The build requires C++ support.

>> 	(linux_get_hwcap, linux_get_hwcap2): Use modified linux_get_auxv.
>> 	* linux-low.h (linux_get_auxv): Declare.
>> 	* linux-ppc-low.c (is_elfv2_inferior): Use linux_get_auxv.
> 


> index d825184835..d5d074efc5 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -435,6 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
> 
> extern int have_ptrace_getregset;
> 
> +/* Search for the value with type MATCH in the auxv vector with
> +   entries of length WORDSIZE bytes.  If found, store the value in
> +   *VALP and return 1.  If not found or if there is an error, return
> +   0.  */

Comment should state that VALP is not checked for nullptr.

> +
> +int linux_get_auxv (int wordsize, CORE_ADDR match,
> +		    CORE_ADDR *valp);
> +



> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 265043f97e..65919c3262 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7427,11 +7427,10 @@ linux_get_pc_64bit (struct regcache *regcache)
>   return pc;
> }
> 
> -/* Fetch the entry MATCH from the auxv vector, where entries are length
> -   WORDSIZE.  If no entry was found, return zero.  */
> +/* See linux-low.h.  */
> 
> -static CORE_ADDR
> -linux_get_auxv (int wordsize, CORE_ADDR match)
> +int
> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
> {
>   gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
>   int offset = 0;

Would be useful to set valp to zero here. (Not sure if the GDB version
does this either).

> @@ -7442,15 +7441,21 @@ linux_get_auxv (int wordsize, CORE_ADDR match)
>     {
>       if (wordsize == 4)
> 	{
> -	  uint32_t *data_p = (uint32_t *)data;
> +	  uint32_t *data_p = (uint32_t *) data;
> 	  if (data_p[0] == match)
> -	    return data_p[1];
> +	    {
> +	      *valp = data_p[1];
> +	      return 1;
> +	    }
> 	}
>       else
> 	{
> -	  uint64_t *data_p = (uint64_t *)data;
> +	  uint64_t *data_p = (uint64_t *) data;
> 	  if (data_p[0] == match)
> -	    return data_p[1];
> +	    {
> +	      *valp = data_p[1];
> +	      return 1;
> +	    }
> 	}



> diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
> index 8deb0ce068..f17f05a0a3 100644
> --- a/gdb/gdbserver/linux-ppc-low.c
> +++ b/gdb/gdbserver/linux-ppc-low.c
> @@ -1107,10 +1107,13 @@ is_elfv2_inferior (void)
> #else
>   const int def_res = 0;
> #endif
> -  unsigned long phdr;
> +  CORE_ADDR phdr;
>   Elf64_Ehdr ehdr;
> 
> -  if (!ppc_get_auxv (AT_PHDR, &phdr))
> +  const struct target_desc *tdesc = current_process ()->tdesc;
> +  int wordsize = register_size (tdesc, 0);
> +
> +  if (!linux_get_auxv (wordsize, AT_PHDR, &phdr))
>     return def_res;

I don’t think there is a requirement here for the error to be return separately
to the phdr. With the my version of linux_get_auxv, on error you would get 0
for phdr. Given that it is an address, 0 should never be a valid value.

With the code pre my patch and this patch, I’m not sure what will happen if the
PHDR value is 0 - will read_inferior_memory then the memcmp deal with that? (To
be fair, I suspect there are bigger issues to deal with if phdr is 0).

Therefore I’d suggest it’d be better to have:

CORE_ADDR phdr = linux_get_auxv (wordsize, AT_PHDR);
if (phdr == nullptr)
  return def_res;



Thanks,
Alan.