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

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

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.