diff mbox

[4/8] Enable SVE for GDB

Message ID 21329FE3-8072-44B4-A988-D2A7A15A0E1C@arm.com
State New
Headers show

Commit Message

Alan Hayward June 4, 2018, 11:19 a.m. UTC 
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:





Thanks!
Alan.
diff mbox

Patch

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