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

[Simon Marchi <simark@simark.ca>]

On 2019-03-25 8:05 a.m., Alan Hayward wrote:
> In gdbserver, Tidy up calls to read HWCAP (and HWCAP2) by adding common
> functions, removing the Arm, AArch64, PPC and S390 specific versions.
> 
> No functionality differences.
> 
> [ I wasn't sure in gdbserver when to use CORE_ADDR and when to use int/long.
>    I'm assuming CORE_ADDR is fairly recent to gdbserver? ]

I don't know if CORE_ADDR is a recent addition to gdbserver.  But I 
suppose CORE_ADDR was chosen as the return type for functions reading 
arbitrary AUXV values, since some of them may be pointers.  With 
CORE_ADDR, we know those values will fit in the data type.  When we 
return the HWCAP value, we know it won't be a pointer though, so 
returning a CORE_ADDR is a bit confusing, IMO.  Those functions 
returning the HWCAP value could return something else, an uint64_t 
maybe.  But then I would change it in the gdb version as well to match.

>   /* Implementation of linux_target_ops method "arch_setup".  */
>   
>   static void
> @@ -545,8 +521,8 @@ aarch64_arch_setup (void)
>     if (is_elf64)
>       {
>         uint64_t vq = aarch64_sve_get_vq (tid);
> -      unsigned long hwcap = 0;
> -      bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA);
> +      unsigned long hwcap = linux_get_hwcap (8);
> +      bool pauth_p = hwcap & AARCH64_HWCAP_PACA;

Just wondering, can the linux-aarch64-low.c code be used to debug a process

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 6f703f589f..481919c205 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7423,6 +7423,64 @@ linux_get_pc_64bit (struct regcache *regcache)
>     return pc;
>   }
>   
> +/* Extract the auxiliary vector entry with a_type matching MATCH, storing the
> +   value in VALP and returning true.  If no entry was found, return false.  */
> +
> +static bool
> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)

I think this function could return the result (CORE_ADDR) directly,
returning 0 on failure.

If 4 and 8 are the only supported wordsize values, I would suggest 
adding an assert to verify it.

> +{
> +  gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
> +  int offset = 0;
> +
> +  while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize)
> +    {
> +      if (wordsize == 4)
> +	{
> +	  unsigned int *data_p = (unsigned int *)data;
> +	  if (data_p[0] == match)
> +	    {
> +	      *valp = data_p[1];
> +	      return true;
> +	    }
> +	}
> +      else
> +	{
> +	  unsigned long *data_p = (unsigned long *)data;
> +	  if (data_p[0] == match)
> +	    {
> +	      *valp = data_p[1];
> +	      return true;
> +	    }
> +	}

I am a bit worried about relying on the size of the "int" and "long" 
types in architecture-independent code.  Could we use uint32_t and 
uint64_t instead?

Simon