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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

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
>