[PATCH] [AArch64] Properly extract the reference to a return value from x8
Andrew Burgess
aburgess@redhat.com
Thu Jan 13 15:18:01 GMT 2022
* 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
More information about the Gdb-patches
mailing list