Re: [PATCH 4/8] Enable SVE for GDB

[Alan Hayward <Alan dot Hayward at arm dot com>]

Push with minor changes as suggested below.

> On 31 May 2018, at 12:55, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> This patch enables SVE support for GDB by reading the VQ when
>> creating a target description.
>> 
>> It also ensures that SVE is taken into account when creating
>> the tdep structure, and stores the current VQ value directly
>> in tdep.
>> 
>> With this patch, gdb on an aarch64 system with SVE will now detect
>> SVE. The SVE registers will be displayed (but the contents will be
>> invalid). The following patches fill out the contents.
> 
> LGTM, with some nits.
> 
>> @@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>   /* Validate the descriptor provides the mandatory core R registers
>>      and allocate their numbers.  */
>>   for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
>> -    valid_p &=
>> -      tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
>> -			       aarch64_r_register_names[i]);
>> +    valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
>> +					AARCH64_X0_REGNUM + i,
>> +					aarch64_r_register_names[i]);
>> 
>>   num_regs = AARCH64_X0_REGNUM + i;
>> 
>> -  /* Look for the V registers.  */
>> -  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
>> -  if (feature)
>> +  /* Add the V registers.  */
>> +  if (feature_fpu != NULL)
>>     {
>> +      gdb_assert (feature_sve == NULL);
> 
> Again, if this situation can result from a bad input passed to GDB (a bad tdesc
> sent by the remote), we shouldn't gdb_assert on it, but show an error message
> and gracefully fail.

Ok. Updated to use _error. 

      if (feature_sve != NULL)
	error (_("Program contains both fpu and SVE features."));

> 
>> +
>>       /* Validate the descriptor provides the mandatory V registers
>> -         and allocate their numbers.  */
>> +	 and allocate their numbers.  */
>>       for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
>> -	valid_p &=
>> -	  tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
>> -				   aarch64_v_register_names[i]);
>> +	valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
>> +					    AARCH64_V0_REGNUM + i,
>> +					    aarch64_v_register_names[i]);
>> 
>>       num_regs = AARCH64_V0_REGNUM + i;
>> +    }
>> +
>> +  /* Add the SVE registers.  */
>> +  if (feature_sve != NULL)
>> +    {
>> +      gdb_assert (feature_fpu == NULL);
> 
> Same here.

I removed this one as it’s redundant - it’ll be caught by the error above.

> 
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index c9fd7b3578..046de6228f 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -73,6 +73,15 @@ struct gdbarch_tdep
>> 
>>   /* syscall record.  */
>>   int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
>> +
>> +  /* The VQ value for SVE targets, or zero if SVE is not supported.  */
>> +  long vq;
>> +
>> +  /* Returns true if the target supports SVE.  */
>> +  bool has_sve ()
>> +  {
>> +    return vq != 0;
>> +  }
> 
> This method can be const.
> 

Done.


> On 31 May 2018, at 15:59, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>   return tdesc;
>> }
>> 
>> +/* Return the VQ used when creating the target description TDESC.  */
>> +
>> +static long
>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
> 
> Is this use of "long" significant?  I mean, is it assuming 64-bit?
> I ask because longs are not 64-bit on x64 Windows, so it would
> do the wrong thing when cross debugging.
> 

Fixed by using both follow on patch "[PATCH] Use uint64_t for SVE VQ” and
obvious patch below:


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6674b7654e..0172e4ccd1 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2875,7 +2875,7 @@ aarch64_read_description (uint64_t vq)

 /* Return the VQ used when creating the target description TDESC.  */

-static long
+static uint64_t
 aarch64_get_tdesc_vq (const struct target_desc *tdesc)
 {
   const struct tdesc_feature *feature_sve;
@@ -2888,7 +2888,8 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
   if (feature_sve == nullptr)
     return 0;

-  long vl = tdesc_register_size (feature_sve, aarch64_sve_register_names[0]);
+  uint64_t vl = tdesc_register_size (feature_sve,
+                                    aarch64_sve_register_names[0]);
   return sve_vq_from_vl (vl);
 }

diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index b6b9b30e71..598a0aafa2 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -75,7 +75,7 @@ struct gdbarch_tdep
   int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);

   /* The VQ value for SVE targets, or zero if SVE is not supported.  */
-  long vq;
+  uint64_t vq;

   /* Returns true if the target supports SVE.  */
   bool has_sve () const



Thanks!
Alan.