[PATCH] [AArch64] Properly extract the reference to a return value from x8
Luis Machado
luis.machado@linaro.org
Thu Jan 13 15:22:31 GMT 2022
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.
More information about the Gdb-patches
mailing list