public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/5] ARM function-calling / return fixes
@ 2023-10-20 19:15 Tom Tromey
  2023-10-20 19:15 ` [PATCH 1/5] Fix calls with small integers on ARM Tom Tromey
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Tom Tromey @ 2023-10-20 19:15 UTC (permalink / raw)
  To: gdb-patches

This series fixes a number of issues we found using the AdaCore
internal test suite on a big-endian ARM target.

I believe most of these patches have a corresponding test case in the
gdb test suite, but I'm not 100% certain -- I have never really tried
running the gdb test suite using the AdaCore toolchain in cross mode.
(If someone has a qemu-based target board, I guess I'd be willing to
give it a try...)

---
Tom Tromey (5):
      Fix calls with small integers on ARM
      Fix "finish" with range types on ARM
      Fix "finish" for vector types on ARM
      Fix range-type "return" command on ARM
      Fix fixed-point "return" on ARM

 gdb/arm-tdep.c | 53 +++++++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 26 deletions(-)
---
base-commit: 1fa80e4c8184d87d75ff30b552cc282f5811823a
change-id: 20231020-arm-params-db4852c3516f

Best regards,
-- 
Tom Tromey <tromey@adacore.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/5] Fix calls with small integers on ARM
  2023-10-20 19:15 [PATCH 0/5] ARM function-calling / return fixes Tom Tromey
@ 2023-10-20 19:15 ` Tom Tromey
  2023-10-23 13:14   ` Luis Machado
  2023-10-20 19:15 ` [PATCH 2/5] Fix "finish" with range types " Tom Tromey
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-20 19:15 UTC (permalink / raw)
  To: gdb-patches

On big-endian ARM, an inferior call with a small integer will pass the
wrong value.  This patch fixes the problem.  Because the code here
works using scalar values, and not just bytes, left-shifting is
unnecessary.
---
 gdb/arm-tdep.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index a0ad1fb7a82..97d7c5140d2 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -4824,9 +4824,6 @@ arm_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	    {
 	      /* The argument is being passed in a general purpose
 		 register.  */
-	      if (byte_order == BFD_ENDIAN_BIG)
-		regval <<= (ARM_INT_REGISTER_SIZE - partial_len) * 8;
-
 	      arm_debug_printf ("arg %d in %s = 0x%s", argnum,
 				gdbarch_register_name (gdbarch, argreg),
 				phex (regval, ARM_INT_REGISTER_SIZE));

-- 
2.40.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/5] Fix "finish" with range types on ARM
  2023-10-20 19:15 [PATCH 0/5] ARM function-calling / return fixes Tom Tromey
  2023-10-20 19:15 ` [PATCH 1/5] Fix calls with small integers on ARM Tom Tromey
@ 2023-10-20 19:15 ` Tom Tromey
  2023-10-23 13:14   ` Luis Machado
  2023-10-20 19:15 ` [PATCH 3/5] Fix "finish" for vector " Tom Tromey
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-20 19:15 UTC (permalink / raw)
  To: gdb-patches

On ARM (I tested big-endian but it may not matter), "finish" can
sometimes print the wrong result when the return type is a range type.
Range types should really be treated as their underlying type
(normally integer, but sometimes fixed-point).  This patch implements
this.
---
 gdb/arm-tdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 97d7c5140d2..ab0df0f16a8 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8915,6 +8915,9 @@ arm_extract_return_value (struct type *type, struct regcache *regs,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
 
+  while (type->code () == TYPE_CODE_RANGE)
+    type = check_typedef (type->target_type ());
+
   if (TYPE_CODE_FLT == type->code ())
     {
       switch (tdep->fp_model)

-- 
2.40.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/5] Fix "finish" for vector types on ARM
  2023-10-20 19:15 [PATCH 0/5] ARM function-calling / return fixes Tom Tromey
  2023-10-20 19:15 ` [PATCH 1/5] Fix calls with small integers on ARM Tom Tromey
  2023-10-20 19:15 ` [PATCH 2/5] Fix "finish" with range types " Tom Tromey
@ 2023-10-20 19:15 ` Tom Tromey
  2023-10-23 14:03   ` Luis Machado
  2023-10-20 19:15 ` [PATCH 4/5] Fix range-type "return" command " Tom Tromey
  2023-10-20 19:15 ` [PATCH 5/5] Fix fixed-point "return" " Tom Tromey
  4 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-20 19:15 UTC (permalink / raw)
  To: gdb-patches

On a big-endian ARM system, "finish" printed the wrong value when
finishing from a function that returned a vector type.  Similarly,
calls to a function also resulted in the wrong value being passed.  I
think both the read- and write-functions here should ignore the
endian-ness.

I tested this using the AdaCore internal test suite; the test case
that caught this is identical to gdb.base/gnu_vector.exp.
---
 gdb/arm-tdep.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index ab0df0f16a8..493e5b84758 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9777,29 +9777,22 @@ arm_neon_quad_read (struct gdbarch *gdbarch, readable_regcache *regcache,
 {
   char name_buf[4];
   gdb_byte reg_buf[8];
-  int offset, double_regnum;
+  int double_regnum;
   enum register_status status;
 
   xsnprintf (name_buf, sizeof (name_buf), "d%d", regnum << 1);
   double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
 					       strlen (name_buf));
 
-  /* d0 is always the least significant half of q0.  */
-  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
-    offset = 8;
-  else
-    offset = 0;
-
   status = regcache->raw_read (double_regnum, reg_buf);
   if (status != REG_VALID)
     return status;
-  memcpy (buf + offset, reg_buf, 8);
+  memcpy (buf, reg_buf, 8);
 
-  offset = 8 - offset;
   status = regcache->raw_read (double_regnum + 1, reg_buf);
   if (status != REG_VALID)
     return status;
-  memcpy (buf + offset, reg_buf, 8);
+  memcpy (buf + 8, reg_buf, 8);
 
   return REG_VALID;
 }
@@ -9874,21 +9867,14 @@ arm_neon_quad_write (struct gdbarch *gdbarch, struct regcache *regcache,
 		     int regnum, const gdb_byte *buf)
 {
   char name_buf[4];
-  int offset, double_regnum;
+  int double_regnum;
 
   xsnprintf (name_buf, sizeof (name_buf), "d%d", regnum << 1);
   double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
 					       strlen (name_buf));
 
-  /* d0 is always the least significant half of q0.  */
-  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
-    offset = 8;
-  else
-    offset = 0;
-
-  regcache->raw_write (double_regnum, buf + offset);
-  offset = 8 - offset;
-  regcache->raw_write (double_regnum + 1, buf + offset);
+  regcache->raw_write (double_regnum, buf);
+  regcache->raw_write (double_regnum + 1, buf + 8);
 }
 
 /* Store the contents of BUF to the MVE pseudo register REGNUM.  */

-- 
2.40.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 4/5] Fix range-type "return" command on ARM
  2023-10-20 19:15 [PATCH 0/5] ARM function-calling / return fixes Tom Tromey
                   ` (2 preceding siblings ...)
  2023-10-20 19:15 ` [PATCH 3/5] Fix "finish" for vector " Tom Tromey
@ 2023-10-20 19:15 ` Tom Tromey
  2023-10-23 13:15   ` Luis Machado
  2023-10-20 19:15 ` [PATCH 5/5] Fix fixed-point "return" " Tom Tromey
  4 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-20 19:15 UTC (permalink / raw)
  To: gdb-patches

On big-endian ARM, "return"ing from a function that returned a range
type did not work.  This patch strips the range type to treat the
function as though it were returning the underlying type instead.
---
 gdb/arm-tdep.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 493e5b84758..62412d92f85 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9132,6 +9132,9 @@ arm_store_return_value (struct type *type, struct regcache *regs,
   struct gdbarch *gdbarch = regs->arch ();
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
 
+  while (type->code () == TYPE_CODE_RANGE)
+    type = check_typedef (type->target_type ());
+
   if (type->code () == TYPE_CODE_FLT)
     {
       gdb_byte buf[ARM_FP_REGISTER_SIZE];

-- 
2.40.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 5/5] Fix fixed-point "return" on ARM
  2023-10-20 19:15 [PATCH 0/5] ARM function-calling / return fixes Tom Tromey
                   ` (3 preceding siblings ...)
  2023-10-20 19:15 ` [PATCH 4/5] Fix range-type "return" command " Tom Tromey
@ 2023-10-20 19:15 ` Tom Tromey
  2023-10-23 13:16   ` Luis Machado
  4 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-20 19:15 UTC (permalink / raw)
  To: gdb-patches

On a big-endian ARM machine, the "return" command resulted in the
wrong value being returned when the function had a fixed-point return
type.  This patch fixes the problem by unpacking and repacking the
fixed-point type appropriately.
---
 gdb/arm-tdep.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 62412d92f85..a9c43b27265 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -9170,16 +9170,28 @@ arm_store_return_value (struct type *type, struct regcache *regs,
 	   || type->code () == TYPE_CODE_BOOL
 	   || type->code () == TYPE_CODE_PTR
 	   || TYPE_IS_REFERENCE (type)
-	   || type->code () == TYPE_CODE_ENUM)
+	   || type->code () == TYPE_CODE_ENUM
+	   || is_fixed_point_type (type))
     {
       if (type->length () <= 4)
 	{
 	  /* Values of one word or less are zero/sign-extended and
 	     returned in r0.  */
 	  bfd_byte tmpbuf[ARM_INT_REGISTER_SIZE];
-	  LONGEST val = unpack_long (type, valbuf);
 
-	  store_signed_integer (tmpbuf, ARM_INT_REGISTER_SIZE, byte_order, val);
+	  if (is_fixed_point_type (type))
+	    {
+	      gdb_mpz unscaled;
+	      unscaled.read (gdb::make_array_view (valbuf, type->length ()),
+			     byte_order, type->is_unsigned ());
+	      unscaled.write (gdb::make_array_view (tmpbuf, sizeof (tmpbuf)),
+			      byte_order, type->is_unsigned ());
+	    }
+	  else
+	    {
+	      LONGEST val = unpack_long (type, valbuf);
+	      store_signed_integer (tmpbuf, ARM_INT_REGISTER_SIZE, byte_order, val);
+	    }
 	  regs->cooked_write (ARM_A1_REGNUM, tmpbuf);
 	}
       else

-- 
2.40.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] Fix calls with small integers on ARM
  2023-10-20 19:15 ` [PATCH 1/5] Fix calls with small integers on ARM Tom Tromey
@ 2023-10-23 13:14   ` Luis Machado
  2023-10-27 18:08     ` Tom Tromey
  0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-10-23 13:14 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 10/20/23 20:15, Tom Tromey wrote:
> On big-endian ARM, an inferior call with a small integer will pass the
> wrong value.  This patch fixes the problem.  Because the code here
> works using scalar values, and not just bytes, left-shifting is
> unnecessary.
> ---
>  gdb/arm-tdep.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index a0ad1fb7a82..97d7c5140d2 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -4824,9 +4824,6 @@ arm_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>  	    {
>  	      /* The argument is being passed in a general purpose
>  		 register.  */
> -	      if (byte_order == BFD_ENDIAN_BIG)
> -		regval <<= (ARM_INT_REGISTER_SIZE - partial_len) * 8;
> -
>  	      arm_debug_printf ("arg %d in %s = 0x%s", argnum,
>  				gdbarch_register_name (gdbarch, argreg),
>  				phex (regval, ARM_INT_REGISTER_SIZE));
> 

Do you have an example failure for the above? Is it a positve or negative integer?

The code is meant to adjust padding, as small integers need to be padded/sign-extended
when being put in the GPR's. This code has seen many changes over the years, and
the padding/extension could've worked then.

But looking at it now, it isn't really clear what the left-shifting is supposed to
accomplish, as regval should be padded/sign-extended from extract_unsigned_integer.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/5] Fix "finish" with range types on ARM
  2023-10-20 19:15 ` [PATCH 2/5] Fix "finish" with range types " Tom Tromey
@ 2023-10-23 13:14   ` Luis Machado
  0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-10-23 13:14 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 10/20/23 20:15, Tom Tromey wrote:
> On ARM (I tested big-endian but it may not matter), "finish" can
> sometimes print the wrong result when the return type is a range type.
> Range types should really be treated as their underlying type
> (normally integer, but sometimes fixed-point).  This patch implements
> this.
> ---
>  gdb/arm-tdep.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 97d7c5140d2..ab0df0f16a8 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -8915,6 +8915,9 @@ arm_extract_return_value (struct type *type, struct regcache *regs,
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    arm_gdbarch_tdep *tdep = gdbarch_tdep<arm_gdbarch_tdep> (gdbarch);
>  
> +  while (type->code () == TYPE_CODE_RANGE)
> +    type = check_typedef (type->target_type ());
> +
>    if (TYPE_CODE_FLT == type->code ())
>      {
>        switch (tdep->fp_model)
> 

Thanks for the patch. This is OK.

Approved-By: Luis Machado <luis.machado@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/5] Fix range-type "return" command on ARM
  2023-10-20 19:15 ` [PATCH 4/5] Fix range-type "return" command " Tom Tromey
@ 2023-10-23 13:15   ` Luis Machado
  0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-10-23 13:15 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 10/20/23 20:15, Tom Tromey wrote:
> On big-endian ARM, "return"ing from a function that returned a range
> type did not work.  This patch strips the range type to treat the
> function as though it were returning the underlying type instead.
> ---
>  gdb/arm-tdep.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 493e5b84758..62412d92f85 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -9132,6 +9132,9 @@ arm_store_return_value (struct type *type, struct regcache *regs,
>    struct gdbarch *gdbarch = regs->arch ();
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>  
> +  while (type->code () == TYPE_CODE_RANGE)
> +    type = check_typedef (type->target_type ());
> +
>    if (type->code () == TYPE_CODE_FLT)
>      {
>        gdb_byte buf[ARM_FP_REGISTER_SIZE];
> 

Thanks. This is OK.

Approved-By: Luis Machado <luis.machado@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 5/5] Fix fixed-point "return" on ARM
  2023-10-20 19:15 ` [PATCH 5/5] Fix fixed-point "return" " Tom Tromey
@ 2023-10-23 13:16   ` Luis Machado
  0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-10-23 13:16 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 10/20/23 20:15, Tom Tromey wrote:
> On a big-endian ARM machine, the "return" command resulted in the
> wrong value being returned when the function had a fixed-point return
> type.  This patch fixes the problem by unpacking and repacking the
> fixed-point type appropriately.
> ---
>  gdb/arm-tdep.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 62412d92f85..a9c43b27265 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -9170,16 +9170,28 @@ arm_store_return_value (struct type *type, struct regcache *regs,
>  	   || type->code () == TYPE_CODE_BOOL
>  	   || type->code () == TYPE_CODE_PTR
>  	   || TYPE_IS_REFERENCE (type)
> -	   || type->code () == TYPE_CODE_ENUM)
> +	   || type->code () == TYPE_CODE_ENUM
> +	   || is_fixed_point_type (type))
>      {
>        if (type->length () <= 4)
>  	{
>  	  /* Values of one word or less are zero/sign-extended and
>  	     returned in r0.  */
>  	  bfd_byte tmpbuf[ARM_INT_REGISTER_SIZE];
> -	  LONGEST val = unpack_long (type, valbuf);
>  
> -	  store_signed_integer (tmpbuf, ARM_INT_REGISTER_SIZE, byte_order, val);
> +	  if (is_fixed_point_type (type))
> +	    {
> +	      gdb_mpz unscaled;
> +	      unscaled.read (gdb::make_array_view (valbuf, type->length ()),
> +			     byte_order, type->is_unsigned ());
> +	      unscaled.write (gdb::make_array_view (tmpbuf, sizeof (tmpbuf)),
> +			      byte_order, type->is_unsigned ());
> +	    }
> +	  else
> +	    {
> +	      LONGEST val = unpack_long (type, valbuf);
> +	      store_signed_integer (tmpbuf, ARM_INT_REGISTER_SIZE, byte_order, val);
> +	    }
>  	  regs->cooked_write (ARM_A1_REGNUM, tmpbuf);
>  	}
>        else
> 

Thanks. This is OK.

Approved-By: Luis Machado <luis.machado@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] Fix "finish" for vector types on ARM
  2023-10-20 19:15 ` [PATCH 3/5] Fix "finish" for vector " Tom Tromey
@ 2023-10-23 14:03   ` Luis Machado
  2023-10-27 17:42     ` Tom Tromey
  0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-10-23 14:03 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 10/20/23 20:15, Tom Tromey wrote:
> On a big-endian ARM system, "finish" printed the wrong value when
> finishing from a function that returned a vector type.  Similarly,
> calls to a function also resulted in the wrong value being passed.  I
> think both the read- and write-functions here should ignore the
> endian-ness.
> 
> I tested this using the AdaCore internal test suite; the test case
> that caught this is identical to gdb.base/gnu_vector.exp.
> ---
>  gdb/arm-tdep.c | 26 ++++++--------------------
>  1 file changed, 6 insertions(+), 20 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index ab0df0f16a8..493e5b84758 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -9777,29 +9777,22 @@ arm_neon_quad_read (struct gdbarch *gdbarch, readable_regcache *regcache,
>  {
>    char name_buf[4];
>    gdb_byte reg_buf[8];
> -  int offset, double_regnum;
> +  int double_regnum;
>    enum register_status status;
>  
>    xsnprintf (name_buf, sizeof (name_buf), "d%d", regnum << 1);
>    double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
>  					       strlen (name_buf));
>  
> -  /* d0 is always the least significant half of q0.  */
> -  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> -    offset = 8;
> -  else
> -    offset = 0;
> -
>    status = regcache->raw_read (double_regnum, reg_buf);
>    if (status != REG_VALID)
>      return status;
> -  memcpy (buf + offset, reg_buf, 8);
> +  memcpy (buf, reg_buf, 8);
>  
> -  offset = 8 - offset;
>    status = regcache->raw_read (double_regnum + 1, reg_buf);
>    if (status != REG_VALID)
>      return status;
> -  memcpy (buf + offset, reg_buf, 8);
> +  memcpy (buf + 8, reg_buf, 8);
>  
>    return REG_VALID;
>  }
> @@ -9874,21 +9867,14 @@ arm_neon_quad_write (struct gdbarch *gdbarch, struct regcache *regcache,
>  		     int regnum, const gdb_byte *buf)
>  {
>    char name_buf[4];
> -  int offset, double_regnum;
> +  int double_regnum;
>  
>    xsnprintf (name_buf, sizeof (name_buf), "d%d", regnum << 1);
>    double_regnum = user_reg_map_name_to_regnum (gdbarch, name_buf,
>  					       strlen (name_buf));
>  
> -  /* d0 is always the least significant half of q0.  */
> -  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> -    offset = 8;
> -  else
> -    offset = 0;
> -
> -  regcache->raw_write (double_regnum, buf + offset);
> -  offset = 8 - offset;
> -  regcache->raw_write (double_regnum + 1, buf + offset);
> +  regcache->raw_write (double_regnum, buf);
> +  regcache->raw_write (double_regnum + 1, buf + 8);
>  }
>  
>  /* Store the contents of BUF to the MVE pseudo register REGNUM.  */
> 

Unfortunately I'm not sure passing vector types around is exercised by the testsuite, in
particular for ARM.

Do you have an example test where this is failing? I'd like to make sure things still
work for non-BE ARM.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] Fix "finish" for vector types on ARM
  2023-10-23 14:03   ` Luis Machado
@ 2023-10-27 17:42     ` Tom Tromey
  2023-10-30  9:36       ` Luis Machado
  0 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-27 17:42 UTC (permalink / raw)
  To: Luis Machado; +Cc: Tom Tromey, gdb-patches

>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:

>> I tested this using the AdaCore internal test suite; the test case
>> that caught this is identical to gdb.base/gnu_vector.exp.

Luis> Unfortunately I'm not sure passing vector types around is exercised by the testsuite, in
Luis> particular for ARM.

Luis> Do you have an example test where this is failing? I'd like to make sure things still
Luis> work for non-BE ARM.

The test code itself in the AdaCore test suite is pretty much identical
to gdb's gnu_vector.exp.  I don't know if that can run on ARM or not --
but if not, it should be fixed, because it runs fine internally.

Could you try that?

FWIW, AdaCore is also doing little-endian ARM testing.

Tom

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] Fix calls with small integers on ARM
  2023-10-23 13:14   ` Luis Machado
@ 2023-10-27 18:08     ` Tom Tromey
  2023-10-30  9:38       ` Luis Machado
  0 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-27 18:08 UTC (permalink / raw)
  To: Luis Machado; +Cc: Tom Tromey, gdb-patches

>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:

>> @@ -4824,9 +4824,6 @@ arm_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>> {
>> /* The argument is being passed in a general purpose
>> register.  */
>> -	      if (byte_order == BFD_ENDIAN_BIG)
>> -		regval <<= (ARM_INT_REGISTER_SIZE - partial_len) * 8;
>> -
>> arm_debug_printf ("arg %d in %s = 0x%s", argnum,
>> gdbarch_register_name (gdbarch, argreg),
>> phex (regval, ARM_INT_REGISTER_SIZE));
>> 

Luis> Do you have an example failure for the above? Is it a positve or
Luis> negative integer?

I backed this out and ran the internal tests, and it fails on one
Ada-specific test involving fixed-point types -- but also on the
internal equivalent of this test from gnu_vector.exp:

gdb_test "print add_singlevecs((char1) \{6\}, (int1) \{12\}, (double1) \{24\})" "= \\{42\\}" \
    "call add_singlevecs"

Luis> But looking at it now, it isn't really clear what the
Luis> left-shifting is supposed to accomplish, as regval should be
Luis> padded/sign-extended from extract_unsigned_integer.

Yeah.

Tom

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] Fix "finish" for vector types on ARM
  2023-10-27 17:42     ` Tom Tromey
@ 2023-10-30  9:36       ` Luis Machado
  2023-10-30 13:44         ` Tom Tromey
  0 siblings, 1 reply; 17+ messages in thread
From: Luis Machado @ 2023-10-30  9:36 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 10/27/23 18:42, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
> 
>>> I tested this using the AdaCore internal test suite; the test case
>>> that caught this is identical to gdb.base/gnu_vector.exp.
> 
> Luis> Unfortunately I'm not sure passing vector types around is exercised by the testsuite, in
> Luis> particular for ARM.
> 
> Luis> Do you have an example test where this is failing? I'd like to make sure things still
> Luis> work for non-BE ARM.
> 
> The test code itself in the AdaCore test suite is pretty much identical
> to gdb's gnu_vector.exp.  I don't know if that can run on ARM or not --
> but if not, it should be fixed, because it runs fine internally.
> 
> Could you try that?
> 

I don't see any change to the results of that test with or without your patch. This is likely
broken code for BE only, which is not well-exercised.

Do you have some stats on how well the BE testsuite runs for 32-bit ARM?

> FWIW, AdaCore is also doing little-endian ARM testing.
> 
> Tom

Approved-By: Luis Machado <luis.machado@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/5] Fix calls with small integers on ARM
  2023-10-27 18:08     ` Tom Tromey
@ 2023-10-30  9:38       ` Luis Machado
  0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-10-30  9:38 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 10/27/23 19:08, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:
> 
>>> @@ -4824,9 +4824,6 @@ arm_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>>> {
>>> /* The argument is being passed in a general purpose
>>> register.  */
>>> -	      if (byte_order == BFD_ENDIAN_BIG)
>>> -		regval <<= (ARM_INT_REGISTER_SIZE - partial_len) * 8;
>>> -
>>> arm_debug_printf ("arg %d in %s = 0x%s", argnum,
>>> gdbarch_register_name (gdbarch, argreg),
>>> phex (regval, ARM_INT_REGISTER_SIZE));
>>>
> 
> Luis> Do you have an example failure for the above? Is it a positve or
> Luis> negative integer?
> 
> I backed this out and ran the internal tests, and it fails on one
> Ada-specific test involving fixed-point types -- but also on the
> internal equivalent of this test from gnu_vector.exp:
> 
> gdb_test "print add_singlevecs((char1) \{6\}, (int1) \{12\}, (double1) \{24\})" "= \\{42\\}" \
>     "call add_singlevecs"
> 
> Luis> But looking at it now, it isn't really clear what the
> Luis> left-shifting is supposed to accomplish, as regval should be
> Luis> padded/sign-extended from extract_unsigned_integer.
> 
> Yeah.
> 

Probably untested BE breakage. This code is really old, and gdb has seen a lot of changes to
regcache functions.


> Tom

Thanks.


Approved-By: Luis Machado <luis.machado@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] Fix "finish" for vector types on ARM
  2023-10-30  9:36       ` Luis Machado
@ 2023-10-30 13:44         ` Tom Tromey
  2023-10-30 13:53           ` Luis Machado
  0 siblings, 1 reply; 17+ messages in thread
From: Tom Tromey @ 2023-10-30 13:44 UTC (permalink / raw)
  To: Luis Machado; +Cc: Tom Tromey, gdb-patches

Luis> Do you have some stats on how well the BE testsuite runs for 32-bit ARM?

No, I've never run gdb's own test suite on any sort of ARM -- only the
AdaCore internal test suite.  This test suite is more or less a subset
of gdb's test suite, though I think there are probably some Ada tests
that aren't upstream.

With this patch series, BE ARM passes pretty well -- I think as well as
LE ARM.

Tom

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/5] Fix "finish" for vector types on ARM
  2023-10-30 13:44         ` Tom Tromey
@ 2023-10-30 13:53           ` Luis Machado
  0 siblings, 0 replies; 17+ messages in thread
From: Luis Machado @ 2023-10-30 13:53 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On 10/30/23 13:44, Tom Tromey wrote:
> Luis> Do you have some stats on how well the BE testsuite runs for 32-bit ARM?
> 
> No, I've never run gdb's own test suite on any sort of ARM -- only the
> AdaCore internal test suite.  This test suite is more or less a subset
> of gdb's test suite, though I think there are probably some Ada tests
> that aren't upstream.
> 
> With this patch series, BE ARM passes pretty well -- I think as well as
> LE ARM.

Great. That's good to know. Thanks for the info.

> 
> Tom


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-10-30 13:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-20 19:15 [PATCH 0/5] ARM function-calling / return fixes Tom Tromey
2023-10-20 19:15 ` [PATCH 1/5] Fix calls with small integers on ARM Tom Tromey
2023-10-23 13:14   ` Luis Machado
2023-10-27 18:08     ` Tom Tromey
2023-10-30  9:38       ` Luis Machado
2023-10-20 19:15 ` [PATCH 2/5] Fix "finish" with range types " Tom Tromey
2023-10-23 13:14   ` Luis Machado
2023-10-20 19:15 ` [PATCH 3/5] Fix "finish" for vector " Tom Tromey
2023-10-23 14:03   ` Luis Machado
2023-10-27 17:42     ` Tom Tromey
2023-10-30  9:36       ` Luis Machado
2023-10-30 13:44         ` Tom Tromey
2023-10-30 13:53           ` Luis Machado
2023-10-20 19:15 ` [PATCH 4/5] Fix range-type "return" command " Tom Tromey
2023-10-23 13:15   ` Luis Machado
2023-10-20 19:15 ` [PATCH 5/5] Fix fixed-point "return" " Tom Tromey
2023-10-23 13:16   ` Luis Machado

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).