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

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

* 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