diff mbox series

[1/5] Fix calls with small integers on ARM

Message ID 20231020-arm-params-v1-1-19d4c89c11b6@adacore.com
State New
Headers
Series ARM function-calling / return fixes |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Tom Tromey Oct. 20, 2023, 7:15 p.m. UTC 
  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(-)

Comments

Luis Machado Oct. 23, 2023, 1:14 p.m. UTC | #1 
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.
Tom Tromey Oct. 27, 2023, 6:08 p.m. UTC | #2 
>>>>> "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
Luis Machado Oct. 30, 2023, 9:38 a.m. UTC | #3 
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>
diff mbox series

Patch

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));