Patchwork [4/8] Enable SVE for GDB

login
register
mail settings
Submitter Alan Hayward
Date May 11, 2018, 10:52 a.m.
Message ID <20180511105256.27388-5-alan.hayward@arm.com>
Download mbox | patch
Permalink /patch/27235/
State New
Headers show

Comments

Alan Hayward - May 11, 2018, 10:52 a.m.
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.

2018-05-101 Alan Hayward  <alan.hayward@arm.com>

	* aarch64-linux-nat.c (aarch64_linux_read_description): Support SVE.
	* aarch64-tdep.c (aarch64_get_tdesc_vq): New function.
	(aarch64_gdbarch_init): Check for SVE.
	* aarch64-tdep.h (gdbarch_tdep::has_sve): New function.
---
 gdb/aarch64-linux-nat.c |  4 +--
 gdb/aarch64-tdep.c      | 73 +++++++++++++++++++++++++++++++++++++++----------
 gdb/aarch64-tdep.h      |  9 ++++++
 3 files changed, 69 insertions(+), 17 deletions(-)
Simon Marchi - May 31, 2018, 11:55 a.m.
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.

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

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

Simon
Pedro Alves - May 31, 2018, 2:57 p.m.
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> +  if (feature_sve != NULL)
> +    {
> +      gdb_assert (feature_fpu == NULL);
> +
> +      /* Validate the descriptor provides the mandatory sve registers

"description".  Please "grep -i" your patches for "descriptor", since
this appears in other spots.

Uppercase "SVE", I'd assume.  I noticed that at least patch #8 uses
lowercase "sve" in an error message, which should probably be fixed.
Suggest grepping the patches for that too.

> +	 and allocate their numbers.  */
> +      for (i = 0; i < ARRAY_SIZE (aarch64_sve_register_names); i++)
> +	valid_p &= tdesc_numbered_register (feature_sve, tdesc_data,

Thanks,
Pedro Alves
Pedro Alves - May 31, 2018, 2:59 p.m.
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.

> +{
> +  const struct tdesc_feature *feature_sve;
> +
> +  if (!tdesc_has_registers (tdesc))
> +    return 0;
> +

Thanks,
Pedro Alves
Alan Hayward - May 31, 2018, 4:13 p.m.
> 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.


In the kernel structure it’s a 16bit value.

However, the VG "register" in the regcache is a 64bit value. (It’s not
a real system register, but helps a great deal to see it as one in gdb,
and we need it as a dwarf register.) I made it 64bits to match the
minimum size of all the other aarch64 registers.
I did have it as 16bits at one point, but it went wrong when using it
with dwarf as it expects all the dwarf registers to be the minimum
register.

I chose to do the conversion from 16bits to 64bits at the point vg is
read from the kernel. This makes the code easy as from then on as it’s
always 64bits throughout.

Alternatively, I could have kept as 16bits up to the point it hits
the regcache. But that leaves two different types around the code.


Alan.
Pedro Alves - May 31, 2018, 4:18 p.m.
On 05/31/2018 05:13 PM, Alan Hayward wrote:
> 
> 
>> 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.
> 
> In the kernel structure it’s a 16bit value.
> 
> However, the VG "register" in the regcache is a 64bit value. (It’s not
> a real system register, but helps a great deal to see it as one in gdb,
> and we need it as a dwarf register.) I made it 64bits to match the
> minimum size of all the other aarch64 registers.

OK, fine to make it 64-bit, but then the "do the wrong thing when
cross debugging" point applies.  "long" is 32-bit on some hosts.
I don't really know what values VG can take -- can they be
negative?  I.e., is sign extension expected?  You did not seem to address
this in your reply.  Why not use host-independent ULONGEST, for example?

> I did have it as 16bits at one point, but it went wrong when using it
> with dwarf as it expects all the dwarf registers to be the minimum
> register.
> 
> I chose to do the conversion from 16bits to 64bits at the point vg is
> read from the kernel. This makes the code easy as from then on as it’s
> always 64bits throughout.
> 
> Alternatively, I could have kept as 16bits up to the point it hits
> the regcache. But that leaves two different types around the code.

Thanks,
Pedro Alves
Alan Hayward - May 31, 2018, 4:26 p.m.
> On 31 May 2018, at 17:18, Pedro Alves <palves@redhat.com> wrote:

> 

> On 05/31/2018 05:13 PM, Alan Hayward wrote:

>> 

>> 

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

>> 

>> In the kernel structure it’s a 16bit value.

>> 

>> However, the VG "register" in the regcache is a 64bit value. (It’s not

>> a real system register, but helps a great deal to see it as one in gdb,

>> and we need it as a dwarf register.) I made it 64bits to match the

>> minimum size of all the other aarch64 registers.

> 

> OK, fine to make it 64-bit, but then the "do the wrong thing when

> cross debugging" point applies.  "long" is 32-bit on some hosts.

> I don't really know what values VG can take -- can they be

> negative?  I.e., is sign extension expected?  You did not seem to address

> this in your reply.  Why not use host-independent ULONGEST, for example?


Right, I didn’t spot that. It can’t be negative, only 0 or a value
divisible by 2.
To match the register, I’m thinking uint64_t would be right then.


Alan.

Patch

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 9a6ee91d2d..1270b7e0c0 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -32,6 +32,7 @@ 
 #include "aarch32-linux-nat.h"
 #include "nat/aarch64-linux.h"
 #include "nat/aarch64-linux-hw-point.h"
+#include "nat/aarch64-sve-linux-ptrace.h"
 
 #include "elf/external.h"
 #include "elf/common.h"
@@ -541,8 +542,7 @@  aarch64_linux_nat_target::read_description ()
   if (ret == 0)
     return tdesc_arm_with_neon;
   else
-    /* SVE not yet supported.  */
-    return aarch64_read_description (0);
+    return aarch64_read_description (aarch64_sve_get_vq (tid));
 }
 
 /* Convert a native/host siginfo object, into/from the siginfo in the
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 1dc31a43bd..7893579d58 100644
--- 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)
+{
+  const struct tdesc_feature *feature_sve;
+
+  if (!tdesc_has_registers (tdesc))
+    return 0;
+
+  feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
+
+  if (feature_sve == nullptr)
+    return 0;
+
+  long vl = tdesc_register_size (feature_sve, aarch64_sve_register_names[0]);
+  return sve_vq_from_vl (vl);
+}
+
+
 /* Initialize the current architecture based on INFO.  If possible,
    re-use an architecture from ARCHES, which is a list of
    architectures already created during this debugging session.
@@ -2890,20 +2910,22 @@  aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   const struct target_desc *tdesc = info.target_desc;
   int i;
   int valid_p = 1;
-  const struct tdesc_feature *feature;
+  const struct tdesc_feature *feature_core;
+  const struct tdesc_feature *feature_fpu;
+  const struct tdesc_feature *feature_sve;
   int num_regs = 0;
   int num_pseudo_regs = 0;
 
   /* Ensure we always have a target descriptor.  */
   if (!tdesc_has_registers (tdesc))
-    /* SVE is not yet supported.  */
     tdesc = aarch64_read_description (0);
-
   gdb_assert (tdesc);
 
-  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.core");
+  feature_core = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.core");
+  feature_fpu = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
+  feature_sve = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.sve");
 
-  if (feature == NULL)
+  if (feature_core == NULL)
     return NULL;
 
   tdesc_data = tdesc_data_alloc ();
@@ -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);
+
       /* 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);
+
+      /* Validate the descriptor provides the mandatory sve registers
+	 and allocate their numbers.  */
+      for (i = 0; i < ARRAY_SIZE (aarch64_sve_register_names); i++)
+	valid_p &= tdesc_numbered_register (feature_sve, tdesc_data,
+					    AARCH64_SVE_Z0_REGNUM + i,
+					    aarch64_sve_register_names[i]);
 
+      num_regs = AARCH64_SVE_Z0_REGNUM + i;
+      num_pseudo_regs += 32;	/* add the Vn register pseudos.  */
+    }
+
+  if (feature_fpu != NULL || feature_sve != NULL)
+    {
       num_pseudo_regs += 32;	/* add the Qn scalar register pseudos */
       num_pseudo_regs += 32;	/* add the Dn scalar register pseudos */
       num_pseudo_regs += 32;	/* add the Sn scalar register pseudos */
@@ -2969,6 +3011,7 @@  aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   tdep->lowest_pc = 0x20;
   tdep->jb_pc = -1;		/* Longjump support not enabled by default.  */
   tdep->jb_elt_size = 8;
+  tdep->vq = aarch64_get_tdesc_vq (tdesc);
 
   set_gdbarch_push_dummy_call (gdbarch, aarch64_push_dummy_call);
   set_gdbarch_frame_align (gdbarch, aarch64_frame_align);
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;
+  }
 };
 
 const target_desc *aarch64_read_description (long vq);