[2/2] gdbserver: Add linux_get_hwcap

  > On 25 Mar 2019, at 15:41, Simon Marchi <simark@simark.ca> wrote:
> 
> 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.

I’ve been flipping back and too in my head to change CORE_ADDR to uint64_t.

On a 32bit build, CORE_ADDR is 32bits. So it would be making a difference. Not sure if that really matters or not?

Left this change out of the update below for now.

> 
>>  /* 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

"gdbserver :2345 a.out” works on an AArch64 box if that’s what you're asking?
The code above is used to detect point auth.

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

Done. Agreed that given that this function is now static, there is really no need to
keep the unused error.

With this changed, I almost changed linux_get_hwcap and linux_get_hwcap2 into inline
functions in the header. But that means adding extra includes into the header (for
the AT_HWCAP defines) and reliving the static from linux_get_auxv.

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

Done.

> 
>> +{
>> +  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?
> 

Done.

> Simon

Updated patch below. Will push if you have no more comments.

Alan.

[2/2] gdbserver: Add linux_get_hwcap

Commit Message

Comments

Patch