[PATCH] [AArch64] Fix PR gdb/28681

[PATCH] [AArch64] Fix PR gdb/28681

[Luis Machado]

This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
fixing things for AArch64.

With the patch, gdb.cp/non-trivial-retval.exp has full passes on
AArch64-Linux Ubuntu 20.04/18.04.
---
 gdb/aarch64-tdep.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 70fb66954a4..802762f303c 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
 	  valbuf += X_REGISTER_SIZE;
 	}
     }
+  else if (!language_pass_by_reference (type).trivially_copyable)
+    {
+      /* If the object is a non-trivial C++ object, the result is passed as a
+	 pointer stored in X0.  */
+      CORE_ADDR addr;
+
+      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
+      read_memory (addr, valbuf, TYPE_LENGTH (type));
+    }
   else
     {
       /* For a structure or union the behaviour is as if the value had
-- 
2.25.1

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Simon Marchi]

On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:
> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
> fixing things for AArch64.
> 
> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
> AArch64-Linux Ubuntu 20.04/18.04.
> ---
>  gdb/aarch64-tdep.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 70fb66954a4..802762f303c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>  	  valbuf += X_REGISTER_SIZE;
>  	}
>      }
> +  else if (!language_pass_by_reference (type).trivially_copyable)
> +    {
> +      /* If the object is a non-trivial C++ object, the result is passed as a
> +	 pointer stored in X0.  */
> +      CORE_ADDR addr;
> +
> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
> +      read_memory (addr, valbuf, TYPE_LENGTH (type));
> +    }
>    else
>      {
>        /* For a structure or union the behaviour is as if the value had

I'll let somebody else review this (probably Andrew), but please change the
patch subject to something descriptive, not just the bug number.

Thanks,

Simon

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Luis Machado]

On 1/4/22 3:09 PM, Simon Marchi wrote:
> 
> 
> On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:
>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
>> fixing things for AArch64.
>>
>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
>> AArch64-Linux Ubuntu 20.04/18.04.
>> ---
>>   gdb/aarch64-tdep.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 70fb66954a4..802762f303c 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>>   	  valbuf += X_REGISTER_SIZE;
>>   	}
>>       }
>> +  else if (!language_pass_by_reference (type).trivially_copyable)
>> +    {
>> +      /* If the object is a non-trivial C++ object, the result is passed as a
>> +	 pointer stored in X0.  */
>> +      CORE_ADDR addr;
>> +
>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));
>> +    }
>>     else
>>       {
>>         /* For a structure or union the behaviour is as if the value had
> 
> I'll let somebody else review this (probably Andrew), but please change the
> patch subject to something descriptive, not just the bug number.

Did you want the entire bugzilla PR subject or something else? I can't 
really tell from your reply, sorry.

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Simon Marchi]

On 2022-01-04 13:44, Luis Machado wrote:
> On 1/4/22 3:09 PM, Simon Marchi wrote:
>>
>>
>> On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:
>>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
>>> fixing things for AArch64.
>>>
>>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
>>> AArch64-Linux Ubuntu 20.04/18.04.
>>> ---
>>>   gdb/aarch64-tdep.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>> index 70fb66954a4..802762f303c 100644
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>>>         valbuf += X_REGISTER_SIZE;
>>>       }
>>>       }
>>> +  else if (!language_pass_by_reference (type).trivially_copyable)
>>> +    {
>>> +      /* If the object is a non-trivial C++ object, the result is passed as a
>>> +     pointer stored in X0.  */
>>> +      CORE_ADDR addr;
>>> +
>>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
>>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));
>>> +    }
>>>     else
>>>       {
>>>         /* For a structure or union the behaviour is as if the value had
>>
>> I'll let somebody else review this (probably Andrew), but please change the
>> patch subject to something descriptive, not just the bug number.
> 
> Did you want the entire bugzilla PR subject or something else? I can't really tell from your reply, sorry.

No, not the bugzilla title (which is `Wrong pretty-printed unique_ptr
value when using "finish"`), since that is a distant symptom of the
problem you fix.  The subject should state what you are fixing, so
something about the handling of trivially copyable return values on
AArch64 (something like that, I didn't follow the resolution of the bug
close enough).

Simon

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Luis Machado]

On 1/4/22 3:47 PM, Simon Marchi wrote:
> 
> 
> On 2022-01-04 13:44, Luis Machado wrote:
>> On 1/4/22 3:09 PM, Simon Marchi wrote:
>>>
>>>
>>> On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:
>>>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
>>>> fixing things for AArch64.
>>>>
>>>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
>>>> AArch64-Linux Ubuntu 20.04/18.04.
>>>> ---
>>>>    gdb/aarch64-tdep.c | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>> index 70fb66954a4..802762f303c 100644
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>>>>          valbuf += X_REGISTER_SIZE;
>>>>        }
>>>>        }
>>>> +  else if (!language_pass_by_reference (type).trivially_copyable)
>>>> +    {
>>>> +      /* If the object is a non-trivial C++ object, the result is passed as a
>>>> +     pointer stored in X0.  */
>>>> +      CORE_ADDR addr;
>>>> +
>>>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
>>>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));
>>>> +    }
>>>>      else
>>>>        {
>>>>          /* For a structure or union the behaviour is as if the value had
>>>
>>> I'll let somebody else review this (probably Andrew), but please change the
>>> patch subject to something descriptive, not just the bug number.
>>
>> Did you want the entire bugzilla PR subject or something else? I can't really tell from your reply, sorry.
> 
> No, not the bugzilla title (which is `Wrong pretty-printed unique_ptr
> value when using "finish"`), since that is a distant symptom of the
> problem you fix.  The subject should state what you are fixing, so
> something about the handling of trivially copyable return values on
> AArch64 (something like that, I didn't follow the resolution of the bug
> close enough).

How about "gdb: on aarch64 non-trivial C++ objects are returned in memory"?

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Simon Marchi]

> How about "gdb: on aarch64 non-trivial C++ objects are returned in memory"?

Sounds good.  Although if I had to be picky, I'd say it's not perfect
because it just states a fact (as in "the sky is blue"), and doesn't
describe the code change the patch does.  I would go with something like
"gdb/aarch64: fix return value location of non-trivial C++ objects".
The commit message would then say that the architecture's ABI describes
that this kind of object must be returned in memory, but GDB currently
erroneously returns it in registers, and that the patch fixes that.

Simon

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Luis Machado]

cc-ing Alan back, since the mailing list software seems to have removed 
it, although I see it on my end.

On 1/4/22 3:56 PM, Simon Marchi wrote:
>> How about "gdb: on aarch64 non-trivial C++ objects are returned in memory"?
> 
> Sounds good.  Although if I had to be picky, I'd say it's not perfect
> because it just states a fact (as in "the sky is blue"), and doesn't
> describe the code change the patch does.  I would go with something like
> "gdb/aarch64: fix return value location of non-trivial C++ objects".
> The commit message would then say that the architecture's ABI describes
> that this kind of object must be returned in memory, but GDB currently
> erroneously returns it in registers, and that the patch fixes that.

Fair enough.

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Andrew Burgess]

* Luis Machado <luis.machado@linaro.org> [2022-01-04 14:22:54 -0300]:

> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
> fixing things for AArch64.
> 
> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
> AArch64-Linux Ubuntu 20.04/18.04.
> ---
>  gdb/aarch64-tdep.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 70fb66954a4..802762f303c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>  	  valbuf += X_REGISTER_SIZE;
>  	}
>      }
> +  else if (!language_pass_by_reference (type).trivially_copyable)
> +    {
> +      /* If the object is a non-trivial C++ object, the result is passed as a
> +	 pointer stored in X0.  */
> +      CORE_ADDR addr;
> +
> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
> +      read_memory (addr, valbuf, TYPE_LENGTH (type));
> +    }
>    else
>      {
>        /* For a structure or union the behaviour is as if the value had

The aarch64_extract_return_value function is called exclusively from
aarch64_return_value.

After calling this function the aarch64_return_value function returns
RETURN_VALUE_REGISTER_CONVENTION, which I don't think is correct in
the above case.

I think we should be returning one of either
RETURN_VALUE_STRUCT_CONVENTION, RETURN_VALUE_ABI_RETURNS_ADDRESS or
RETURN_VALUE_ABI_PRESERVES_ADDRESS.

I wonder if aarch64_return_in_memory should be doing more of the work
in this case?  But that's just a thought, I'm sure whatever you come
up with will be fine, so long as the return type is correct.

Thanks,
Andrew

Re: [PATCH] [AArch64] Fix PR gdb/28681

[Luis Machado]

On 1/5/22 9:54 AM, Andrew Burgess wrote:
> * Luis Machado <luis.machado@linaro.org> [2022-01-04 14:22:54 -0300]:
> 
>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
>> fixing things for AArch64.
>>
>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
>> AArch64-Linux Ubuntu 20.04/18.04.
>> ---
>>   gdb/aarch64-tdep.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 70fb66954a4..802762f303c 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,
>>   	  valbuf += X_REGISTER_SIZE;
>>   	}
>>       }
>> +  else if (!language_pass_by_reference (type).trivially_copyable)
>> +    {
>> +      /* If the object is a non-trivial C++ object, the result is passed as a
>> +	 pointer stored in X0.  */
>> +      CORE_ADDR addr;
>> +
>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));
>> +    }
>>     else
>>       {
>>         /* For a structure or union the behaviour is as if the value had
> 
> The aarch64_extract_return_value function is called exclusively from
> aarch64_return_value.
> 
> After calling this function the aarch64_return_value function returns
> RETURN_VALUE_REGISTER_CONVENTION, which I don't think is correct in
> the above case.
> 
> I think we should be returning one of either
> RETURN_VALUE_STRUCT_CONVENTION, RETURN_VALUE_ABI_RETURNS_ADDRESS or
> RETURN_VALUE_ABI_PRESERVES_ADDRESS.

Yes, it seems that way.

The original return convention does not cover this particular case. The 
current code may actually be incorrect, and 
RETURN_VALUE_ABI_RETURNS_ADDRESS seems to be a more correct option.

> 
> I wonder if aarch64_return_in_memory should be doing more of the work
> in this case?  But that's just a thought, I'm sure whatever you come
> up with will be fine, so long as the return type is correct.

Expanding aarch64_return_in_memory to check for non-trivially-copyable 
objects and filling in the readbuf with the right value should do it as 
well.

I have an updated patch for this.

Thanks,
Luis

[PATCH] [AArch64] Properly extract the reference to a return value from x8

[Luis Machado]

When running gdb.cp/non-trivial-retval.exp, the following shows up for
AArch64-Linux:

Breakpoint 3, f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
35        A a;
(gdb) finish
Run till exit from #0  f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
main () at /src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:163
163       B b = f2 (i1, i2);
Value returned is $6 = {a = -11952}
(gdb)

The return value should be {a = 123} instead. This happens because the AArch64
backend doesn't extract the return value from the correct location. GDB should
fetch a pointer to the memory location from X8.

With the patch, gdb.cp/non-trivial-retval.exp has full passes on
AArch64-Linux Ubuntu 20.04/18.04.

The problem only shows up with the "finish" command. The "call" command
works correctly and displays the correct return value.

This is also related to PR gdb/28681
(https://sourceware.org/bugzilla/show_bug.cgi?id=28681).
---
 gdb/aarch64-tdep.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 63d626f90ac..0efb3834584 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2362,7 +2362,8 @@ aarch64_return_in_memory (struct gdbarch *gdbarch, struct type *type)
       return 0;
     }
 
-  if (TYPE_LENGTH (type) > 16)
+  if (TYPE_LENGTH (type) > 16
+      || !language_pass_by_reference (type).trivially_copyable)
     {
       /* PCS B.6 Aggregates larger than 16 bytes are passed by
 	 invisible reference.  */
@@ -2474,8 +2475,24 @@ aarch64_return_value (struct gdbarch *gdbarch, struct value *func_value,
     {
       if (aarch64_return_in_memory (gdbarch, valtype))
 	{
+	  /* From the AAPCS64's Result Return section:
+
+	     "Otherwise, the caller shall reserve a block of memory of
+	      sufficient size and alignment to hold the result.  The address
+	      of the memory block shall be passed as an additional argument to
+	      the function in x8.  */
+
 	  aarch64_debug_printf ("return value in memory");
-	  return RETURN_VALUE_STRUCT_CONVENTION;
+
+	  if (readbuf)
+	    {
+	      CORE_ADDR addr;
+
+	      regcache->cooked_read (AARCH64_STRUCT_RETURN_REGNUM, &addr);
+	      read_memory (addr, readbuf, TYPE_LENGTH (valtype));
+	    }
+
+	  return RETURN_VALUE_ABI_RETURNS_ADDRESS;
 	}
     }
 
-- 
2.25.1

Re: [PATCH] [AArch64] Properly extract the reference to a return value from x8

[Andrew Burgess]

* Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2022-01-11 18:22:19 -0300]:

> When running gdb.cp/non-trivial-retval.exp, the following shows up for
> AArch64-Linux:
>
> Breakpoint 3, f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
> 35        A a;
> (gdb) finish
> Run till exit from #0  f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
> main () at /src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:163
> 163       B b = f2 (i1, i2);
> Value returned is $6 = {a = -11952}
> (gdb)
>
> The return value should be {a = 123} instead. This happens because the AArch64
> backend doesn't extract the return value from the correct location. GDB should
> fetch a pointer to the memory location from X8.
>
> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
> AArch64-Linux Ubuntu 20.04/18.04.
>
> The problem only shows up with the "finish" command. The "call" command
> works correctly and displays the correct return value.
>
> This is also related to PR gdb/28681
> (https://sourceware.org/bugzilla/show_bug.cgi?id=28681).
> ---
>  gdb/aarch64-tdep.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 63d626f90ac..0efb3834584 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2362,7 +2362,8 @@ aarch64_return_in_memory (struct gdbarch *gdbarch, struct type *type)
>        return 0;
>      }
>
> -  if (TYPE_LENGTH (type) > 16)
> +  if (TYPE_LENGTH (type) > 16
> +      || !language_pass_by_reference (type).trivially_copyable)
>      {
>        /* PCS B.6 Aggregates larger than 16 bytes are passed by
>  	 invisible reference.  */
> @@ -2474,8 +2475,24 @@ aarch64_return_value (struct gdbarch *gdbarch, struct value *func_value,
>      {
>        if (aarch64_return_in_memory (gdbarch, valtype))
>  	{
> +	  /* From the AAPCS64's Result Return section:
> +
> +	     "Otherwise, the caller shall reserve a block of memory of
> +	      sufficient size and alignment to hold the result.  The address
> +	      of the memory block shall be passed as an additional argument to
> +	      the function in x8.  */
> +
>  	  aarch64_debug_printf ("return value in memory");
> -	  return RETURN_VALUE_STRUCT_CONVENTION;
> +
> +	  if (readbuf)
> +	    {
> +	      CORE_ADDR addr;
> +
> +	      regcache->cooked_read (AARCH64_STRUCT_RETURN_REGNUM, &addr);
> +	      read_memory (addr, readbuf, TYPE_LENGTH (valtype));
> +	    }
> +
> +	  return RETURN_VALUE_ABI_RETURNS_ADDRESS;

So now, anything that should be returned in memory is of type
RETURN_VALUE_ABI_RETURNS_ADDRESS.  This is interesting because it
should have implications outside of gdb.cp/non-trivial-retval.exp.

In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for
most 64-bit targets, would normally be passed in registers, but which,
for aarch64 is required to be passed in memory.

After this change I would expect larger structs (> 16 bytes) to now
also work correctly in aarch64.  Did you see any additional tests
starting to pass after this commit?  For example, given this test
program:

  struct large_t
  {
    int array[32];
  };

  struct large_t
  func ()
  {
    int i;
    struct large_t obj;

    for (i = 0; i < 32; ++i)
      obj.array[i] = i;

    return obj;
  }

  int
  main ()
  {
    struct large_t obj = func ();
    return obj.array[0];
  }

On x86-64 this is what I see:

  $ gdb -q large-struct
  Reading symbols from large-struct...
  (gdb) set print elements 10
  (gdb) break func
  Breakpoint 1 at 0x401116: file large-struct.c, line 12.
  (gdb) r
  Starting program: /tmp/large-struct

  Breakpoint 1, func () at large-struct.c:12
  12	  for (i = 0; i < 32; ++i)
  (gdb) finish
  Run till exit from #0  func () at large-struct.c:12
  main () at large-struct.c:22
  22	  return obj.array[0];
  Value returned is $1 = {
    array = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9...}
  }
  (gdb)

I would expect on aarch64 that the finish didn't work correctly before
this patch, but now does.  Is this what you see?

If you did see other tests starting to pass then could you mention
them in the commit message please.  If not, could you add a test like
the above to the testsuite.

Otherwise, the change looks good to me.

Thanks,
Andrew

Re: [PATCH] [AArch64] Properly extract the reference to a return value from x8

[Luis Machado]

On 1/12/22 8:14 AM, Andrew Burgess wrote:
> * Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2022-01-11 18:22:19 -0300]:
> 
>> When running gdb.cp/non-trivial-retval.exp, the following shows up for
>> AArch64-Linux:
>>
>> Breakpoint 3, f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
>> 35        A a;
>> (gdb) finish
>> Run till exit from #0  f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
>> main () at /src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:163
>> 163       B b = f2 (i1, i2);
>> Value returned is $6 = {a = -11952}
>> (gdb)
>>
>> The return value should be {a = 123} instead. This happens because the AArch64
>> backend doesn't extract the return value from the correct location. GDB should
>> fetch a pointer to the memory location from X8.
>>
>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
>> AArch64-Linux Ubuntu 20.04/18.04.
>>
>> The problem only shows up with the "finish" command. The "call" command
>> works correctly and displays the correct return value.
>>
>> This is also related to PR gdb/28681
>> (https://sourceware.org/bugzilla/show_bug.cgi?id=28681).
>> ---
>>   gdb/aarch64-tdep.c | 21 +++++++++++++++++++--
>>   1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 63d626f90ac..0efb3834584 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2362,7 +2362,8 @@ aarch64_return_in_memory (struct gdbarch *gdbarch, struct type *type)
>>         return 0;
>>       }
>>
>> -  if (TYPE_LENGTH (type) > 16)
>> +  if (TYPE_LENGTH (type) > 16
>> +      || !language_pass_by_reference (type).trivially_copyable)
>>       {
>>         /* PCS B.6 Aggregates larger than 16 bytes are passed by
>>   	 invisible reference.  */
>> @@ -2474,8 +2475,24 @@ aarch64_return_value (struct gdbarch *gdbarch, struct value *func_value,
>>       {
>>         if (aarch64_return_in_memory (gdbarch, valtype))
>>   	{
>> +	  /* From the AAPCS64's Result Return section:
>> +
>> +	     "Otherwise, the caller shall reserve a block of memory of
>> +	      sufficient size and alignment to hold the result.  The address
>> +	      of the memory block shall be passed as an additional argument to
>> +	      the function in x8.  */
>> +
>>   	  aarch64_debug_printf ("return value in memory");
>> -	  return RETURN_VALUE_STRUCT_CONVENTION;
>> +
>> +	  if (readbuf)
>> +	    {
>> +	      CORE_ADDR addr;
>> +
>> +	      regcache->cooked_read (AARCH64_STRUCT_RETURN_REGNUM, &addr);
>> +	      read_memory (addr, readbuf, TYPE_LENGTH (valtype));
>> +	    }
>> +
>> +	  return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> 
> So now, anything that should be returned in memory is of type
> RETURN_VALUE_ABI_RETURNS_ADDRESS.  This is interesting because it
> should have implications outside of gdb.cp/non-trivial-retval.exp.

Right. That's what I thought as well.

> 
> In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for
> most 64-bit targets, would normally be passed in registers, but which,
> for aarch64 is required to be passed in memory.

For AArch64, the Generic C++ ABI states returning non-trivially-copyable 
objects is done via a reference in r8. Doesn't x86_64 have a similar rule?

> 
> After this change I would expect larger structs (> 16 bytes) to now
> also work correctly in aarch64.  Did you see any additional tests
> starting to pass after this commit?  For example, given this test
> program:

The curious thing is that I did not notice big changes in the testsuite 
results, only the FAIL->PASS transition from this particular test. I 
theorized this must be because, although we exercise function calling 
quite a bit, we do not exercise "finish" as much. Otherwise we would've 
seen this problem, given RETURN_VALUE_STRUCT_CONVENTION produces a 
nullptr result value.

> 
>    struct large_t
>    {
>      int array[32];
>    };
> 
>    struct large_t
>    func ()
>    {
>      int i;
>      struct large_t obj;
> 
>      for (i = 0; i < 32; ++i)
>        obj.array[i] = i;
> 
>      return obj;
>    }
> 
>    int
>    main ()
>    {
>      struct large_t obj = func ();
>      return obj.array[0];
>    }
> 
> On x86-64 this is what I see:
> 
>    $ gdb -q large-struct
>    Reading symbols from large-struct...
>    (gdb) set print elements 10
>    (gdb) break func
>    Breakpoint 1 at 0x401116: file large-struct.c, line 12.
>    (gdb) r
>    Starting program: /tmp/large-struct
> 
>    Breakpoint 1, func () at large-struct.c:12
>    12	  for (i = 0; i < 32; ++i)
>    (gdb) finish
>    Run till exit from #0  func () at large-struct.c:12
>    main () at large-struct.c:22
>    22	  return obj.array[0];
>    Value returned is $1 = {
>      array = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9...}
>    }
>    (gdb)
> 
> I would expect on aarch64 that the finish didn't work correctly before
> this patch, but now does.  Is this what you see?

Before the patch it doesn't return any value, as expected:

(gdb) finish
Run till exit from #0  func () at test.c:8
main () at test.c:22
22        return obj.array[0];
Value returned has type: struct large_t. Cannot determine contents
(gdb)

The patch fixes it and makes GDB produce the output you pasted for x86_64.

> 
> If you did see other tests starting to pass then could you mention
> them in the commit message please.  If not, could you add a test like
> the above to the testsuite.

I'll check the coverage for the "finish" command. Maybe exercise 
"finish" alongside manual function calls. Let me investigate that to see 
if I can come up with a useful testcase.

> 
> Otherwise, the change looks good to me.
> 
> Thanks,
> Andrew
> 

Re: [PATCH] [AArch64] Properly extract the reference to a return value from x8

[Andrew Burgess]

* Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2022-01-13 11:19:01 -0300]:

> On 1/12/22 8:14 AM, Andrew Burgess wrote:
> > * Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2022-01-11 18:22:19 -0300]:
> > 
> > > When running gdb.cp/non-trivial-retval.exp, the following shows up for
> > > AArch64-Linux:
> > > 
> > > Breakpoint 3, f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
> > > 35        A a;
> > > (gdb) finish
> > > Run till exit from #0  f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
> > > main () at /src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:163
> > > 163       B b = f2 (i1, i2);
> > > Value returned is $6 = {a = -11952}
> > > (gdb)
> > > 
> > > The return value should be {a = 123} instead. This happens because the AArch64
> > > backend doesn't extract the return value from the correct location. GDB should
> > > fetch a pointer to the memory location from X8.
> > > 
> > > With the patch, gdb.cp/non-trivial-retval.exp has full passes on
> > > AArch64-Linux Ubuntu 20.04/18.04.
> > > 
> > > The problem only shows up with the "finish" command. The "call" command
> > > works correctly and displays the correct return value.
> > > 
> > > This is also related to PR gdb/28681
> > > (https://sourceware.org/bugzilla/show_bug.cgi?id=28681).
> > > ---
> > >   gdb/aarch64-tdep.c | 21 +++++++++++++++++++--
> > >   1 file changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> > > index 63d626f90ac..0efb3834584 100644
> > > --- a/gdb/aarch64-tdep.c
> > > +++ b/gdb/aarch64-tdep.c
> > > @@ -2362,7 +2362,8 @@ aarch64_return_in_memory (struct gdbarch *gdbarch, struct type *type)
> > >         return 0;
> > >       }
> > > 
> > > -  if (TYPE_LENGTH (type) > 16)
> > > +  if (TYPE_LENGTH (type) > 16
> > > +      || !language_pass_by_reference (type).trivially_copyable)
> > >       {
> > >         /* PCS B.6 Aggregates larger than 16 bytes are passed by
> > >   	 invisible reference.  */
> > > @@ -2474,8 +2475,24 @@ aarch64_return_value (struct gdbarch *gdbarch, struct value *func_value,
> > >       {
> > >         if (aarch64_return_in_memory (gdbarch, valtype))
> > >   	{
> > > +	  /* From the AAPCS64's Result Return section:
> > > +
> > > +	     "Otherwise, the caller shall reserve a block of memory of
> > > +	      sufficient size and alignment to hold the result.  The address
> > > +	      of the memory block shall be passed as an additional argument to
> > > +	      the function in x8.  */
> > > +
> > >   	  aarch64_debug_printf ("return value in memory");
> > > -	  return RETURN_VALUE_STRUCT_CONVENTION;
> > > +
> > > +	  if (readbuf)
> > > +	    {
> > > +	      CORE_ADDR addr;
> > > +
> > > +	      regcache->cooked_read (AARCH64_STRUCT_RETURN_REGNUM, &addr);
> > > +	      read_memory (addr, readbuf, TYPE_LENGTH (valtype));
> > > +	    }
> > > +
> > > +	  return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> > 
> > So now, anything that should be returned in memory is of type
> > RETURN_VALUE_ABI_RETURNS_ADDRESS.  This is interesting because it
> > should have implications outside of gdb.cp/non-trivial-retval.exp.
> 
> Right. That's what I thought as well.
> 
> > 
> > In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for
> > most 64-bit targets, would normally be passed in registers, but which,
> > for aarch64 is required to be passed in memory.
> 
> For AArch64, the Generic C++ ABI states returning non-trivially-copyable
> objects is done via a reference in r8. Doesn't x86_64 have a similar rule?

Sorry, I realised what I wrote wasn't what I meant.  What I should
have said was:

  In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for
  most 64-bit targets, would normally be passed in registers **if the
  struct was trivially copyable**, but which, for aarch64 (and x86-64)
  is required to be passed in memory.

My point, which I think you've confirmed, is that the change you made,
correctly changes the behaviour for more than just small non-trivially
copyable structs; you also changed the behaviour for large structs.
This was clearly the right thing to do.

All I'm suggesting is that you should add a test case to cover the
returning a large, trivially copyable struct as part of this patch,
clearly this isn't something we otherwise test.

> 
> > 
> > After this change I would expect larger structs (> 16 bytes) to now
> > also work correctly in aarch64.  Did you see any additional tests
> > starting to pass after this commit?  For example, given this test
> > program:
> 
> The curious thing is that I did not notice big changes in the testsuite
> results, only the FAIL->PASS transition from this particular test. I
> theorized this must be because, although we exercise function calling quite
> a bit, we do not exercise "finish" as much. Otherwise we would've seen this
> problem, given RETURN_VALUE_STRUCT_CONVENTION produces a nullptr result
> value.
> 
> > 
> >    struct large_t
> >    {
> >      int array[32];
> >    };
> > 
> >    struct large_t
> >    func ()
> >    {
> >      int i;
> >      struct large_t obj;
> > 
> >      for (i = 0; i < 32; ++i)
> >        obj.array[i] = i;
> > 
> >      return obj;
> >    }
> > 
> >    int
> >    main ()
> >    {
> >      struct large_t obj = func ();
> >      return obj.array[0];
> >    }
> > 
> > On x86-64 this is what I see:
> > 
> >    $ gdb -q large-struct
> >    Reading symbols from large-struct...
> >    (gdb) set print elements 10
> >    (gdb) break func
> >    Breakpoint 1 at 0x401116: file large-struct.c, line 12.
> >    (gdb) r
> >    Starting program: /tmp/large-struct
> > 
> >    Breakpoint 1, func () at large-struct.c:12
> >    12	  for (i = 0; i < 32; ++i)
> >    (gdb) finish
> >    Run till exit from #0  func () at large-struct.c:12
> >    main () at large-struct.c:22
> >    22	  return obj.array[0];
> >    Value returned is $1 = {
> >      array = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9...}
> >    }
> >    (gdb)
> > 
> > I would expect on aarch64 that the finish didn't work correctly before
> > this patch, but now does.  Is this what you see?
> 
> Before the patch it doesn't return any value, as expected:
> 
> (gdb) finish
> Run till exit from #0  func () at test.c:8
> main () at test.c:22
> 22        return obj.array[0];
> Value returned has type: struct large_t. Cannot determine contents
> (gdb)
> 
> The patch fixes it and makes GDB produce the output you pasted for x86_64.
> 
> > 
> > If you did see other tests starting to pass then could you mention
> > them in the commit message please.  If not, could you add a test like
> > the above to the testsuite.
> 
> I'll check the coverage for the "finish" command. Maybe exercise "finish"
> alongside manual function calls. Let me investigate that to see if I can
> come up with a useful testcase.

I didn't mean to create too much work for you - was just requesting
one extra test :)

Thanks,
Andrew

Re: [PATCH] [AArch64] Properly extract the reference to a return value from x8

[Luis Machado]

On 1/13/22 12:18 PM, Andrew Burgess wrote:
> * Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2022-01-13 11:19:01 -0300]:
> 
>> On 1/12/22 8:14 AM, Andrew Burgess wrote:
>>> * Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2022-01-11 18:22:19 -0300]:
>>>
>>>> When running gdb.cp/non-trivial-retval.exp, the following shows up for
>>>> AArch64-Linux:
>>>>
>>>> Breakpoint 3, f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
>>>> 35        A a;
>>>> (gdb) finish
>>>> Run till exit from #0  f1 (i1=23, i2=100) at src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:35
>>>> main () at /src/gdb/testsuite/gdb.cp/non-trivial-retval.cc:163
>>>> 163       B b = f2 (i1, i2);
>>>> Value returned is $6 = {a = -11952}
>>>> (gdb)
>>>>
>>>> The return value should be {a = 123} instead. This happens because the AArch64
>>>> backend doesn't extract the return value from the correct location. GDB should
>>>> fetch a pointer to the memory location from X8.
>>>>
>>>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on
>>>> AArch64-Linux Ubuntu 20.04/18.04.
>>>>
>>>> The problem only shows up with the "finish" command. The "call" command
>>>> works correctly and displays the correct return value.
>>>>
>>>> This is also related to PR gdb/28681
>>>> (https://sourceware.org/bugzilla/show_bug.cgi?id=28681).
>>>> ---
>>>>    gdb/aarch64-tdep.c | 21 +++++++++++++++++++--
>>>>    1 file changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>>>> index 63d626f90ac..0efb3834584 100644
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -2362,7 +2362,8 @@ aarch64_return_in_memory (struct gdbarch *gdbarch, struct type *type)
>>>>          return 0;
>>>>        }
>>>>
>>>> -  if (TYPE_LENGTH (type) > 16)
>>>> +  if (TYPE_LENGTH (type) > 16
>>>> +      || !language_pass_by_reference (type).trivially_copyable)
>>>>        {
>>>>          /* PCS B.6 Aggregates larger than 16 bytes are passed by
>>>>    	 invisible reference.  */
>>>> @@ -2474,8 +2475,24 @@ aarch64_return_value (struct gdbarch *gdbarch, struct value *func_value,
>>>>        {
>>>>          if (aarch64_return_in_memory (gdbarch, valtype))
>>>>    	{
>>>> +	  /* From the AAPCS64's Result Return section:
>>>> +
>>>> +	     "Otherwise, the caller shall reserve a block of memory of
>>>> +	      sufficient size and alignment to hold the result.  The address
>>>> +	      of the memory block shall be passed as an additional argument to
>>>> +	      the function in x8.  */
>>>> +
>>>>    	  aarch64_debug_printf ("return value in memory");
>>>> -	  return RETURN_VALUE_STRUCT_CONVENTION;
>>>> +
>>>> +	  if (readbuf)
>>>> +	    {
>>>> +	      CORE_ADDR addr;
>>>> +
>>>> +	      regcache->cooked_read (AARCH64_STRUCT_RETURN_REGNUM, &addr);
>>>> +	      read_memory (addr, readbuf, TYPE_LENGTH (valtype));
>>>> +	    }
>>>> +
>>>> +	  return RETURN_VALUE_ABI_RETURNS_ADDRESS;
>>>
>>> So now, anything that should be returned in memory is of type
>>> RETURN_VALUE_ABI_RETURNS_ADDRESS.  This is interesting because it
>>> should have implications outside of gdb.cp/non-trivial-retval.exp.
>>
>> Right. That's what I thought as well.
>>
>>>
>>> In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for
>>> most 64-bit targets, would normally be passed in registers, but which,
>>> for aarch64 is required to be passed in memory.
>>
>> For AArch64, the Generic C++ ABI states returning non-trivially-copyable
>> objects is done via a reference in r8. Doesn't x86_64 have a similar rule?
> 
> Sorry, I realised what I wrote wasn't what I meant.  What I should
> have said was:
> 
>    In gdb.cp/non-trivial-retval.exp we pass a small struct, which, for
>    most 64-bit targets, would normally be passed in registers **if the
>    struct was trivially copyable**, but which, for aarch64 (and x86-64)
>    is required to be passed in memory.
> 
Got it. Thanks for confirming this.

> My point, which I think you've confirmed, is that the change you made,
> correctly changes the behaviour for more than just small non-trivially
> copyable structs; you also changed the behaviour for large structs.
> This was clearly the right thing to do.
> 
> All I'm suggesting is that you should add a test case to cover the
> returning a large, trivially copyable struct as part of this patch,
> clearly this isn't something we otherwise test.
> 
>>
>>>
>>> After this change I would expect larger structs (> 16 bytes) to now
>>> also work correctly in aarch64.  Did you see any additional tests
>>> starting to pass after this commit?  For example, given this test
>>> program:
>>
>> The curious thing is that I did not notice big changes in the testsuite
>> results, only the FAIL->PASS transition from this particular test. I
>> theorized this must be because, although we exercise function calling quite
>> a bit, we do not exercise "finish" as much. Otherwise we would've seen this
>> problem, given RETURN_VALUE_STRUCT_CONVENTION produces a nullptr result
>> value.
>>
>>>
>>>     struct large_t
>>>     {
>>>       int array[32];
>>>     };
>>>
>>>     struct large_t
>>>     func ()
>>>     {
>>>       int i;
>>>       struct large_t obj;
>>>
>>>       for (i = 0; i < 32; ++i)
>>>         obj.array[i] = i;
>>>
>>>       return obj;
>>>     }
>>>
>>>     int
>>>     main ()
>>>     {
>>>       struct large_t obj = func ();
>>>       return obj.array[0];
>>>     }
>>>
>>> On x86-64 this is what I see:
>>>
>>>     $ gdb -q large-struct
>>>     Reading symbols from large-struct...
>>>     (gdb) set print elements 10
>>>     (gdb) break func
>>>     Breakpoint 1 at 0x401116: file large-struct.c, line 12.
>>>     (gdb) r
>>>     Starting program: /tmp/large-struct
>>>
>>>     Breakpoint 1, func () at large-struct.c:12
>>>     12	  for (i = 0; i < 32; ++i)
>>>     (gdb) finish
>>>     Run till exit from #0  func () at large-struct.c:12
>>>     main () at large-struct.c:22
>>>     22	  return obj.array[0];
>>>     Value returned is $1 = {
>>>       array = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9...}
>>>     }
>>>     (gdb)
>>>
>>> I would expect on aarch64 that the finish didn't work correctly before
>>> this patch, but now does.  Is this what you see?
>>
>> Before the patch it doesn't return any value, as expected:
>>
>> (gdb) finish
>> Run till exit from #0  func () at test.c:8
>> main () at test.c:22
>> 22        return obj.array[0];
>> Value returned has type: struct large_t. Cannot determine contents
>> (gdb)
>>
>> The patch fixes it and makes GDB produce the output you pasted for x86_64.
>>
>>>
>>> If you did see other tests starting to pass then could you mention
>>> them in the commit message please.  If not, could you add a test like
>>> the above to the testsuite.
>>
>> I'll check the coverage for the "finish" command. Maybe exercise "finish"
>> alongside manual function calls. Let me investigate that to see if I can
>> come up with a useful testcase.
> 
> I didn't mean to create too much work for you - was just requesting
> one extra test :)

It is a tradition in GDB to have one or more bugs appear while fixing a 
single seemingly trivial bug. So no worries there. :-)

I wasn't aware, at the time, the return convention was incorrect for all 
memory-returned values. It is worth having more coverage for sure.