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

Message ID 20220111212219.4018309-1-luis.machado@linaro.org
State New
Headers show
Series
  • [AArch64] Properly extract the reference to a return value from x8
Related show

Commit Message

Simon Marchi via Gdb-patches Jan. 11, 2022, 9:22 p.m.
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(-)

-- 
2.25.1

Comments

Simon Marchi via Gdb-patches Jan. 12, 2022, 11:14 a.m. | #1
* 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
Simon Marchi via Gdb-patches Jan. 13, 2022, 2:19 p.m. | #2
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

>
Simon Marchi via Gdb-patches Jan. 13, 2022, 3:18 p.m. | #3
* 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
Simon Marchi via Gdb-patches Jan. 13, 2022, 3:22 p.m. | #4
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.

Patch

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;
 	}
     }