diff mbox series

[aarch64/arm] Properly extract the return value returned in memory

Message ID 20220310095721.84432-1-luis.machado@arm.com
State New
Headers show
Series [aarch64/arm] Properly extract the return value returned in memory | expand

Commit Message

Luis Machado March 10, 2022, 9:57 a.m. UTC
From: Luis Machado <luis.machado@linaro.org>

When running gdb.cp/non-trivial-retval.exp, the following shows up for
both aarch64-linux and armhf-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
backends don't extract the return value from the correct location. GDB should
fetch a pointer to the memory location from X8 for aarch64 and r0 for armhf.

With the patch, gdb.cp/non-trivial-retval.exp has full passes on
aarch64-linux and armhf-linux on 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) and fixes FAIL's in
gdb.ada/mi_var_array.exp.

A new testcase is provided, and it exercises GDB's ability to "finish" a
function that returns a large struct (> 16 bytes) and display the
contents of this struct correctly. This has always been incorrect for
these backends, but no testcase exercised this particular scenario.
---
 gdb/aarch64-tdep.c                            | 21 ++++++++-
 gdb/arm-tdep.c                                | 32 +++++++++++--
 gdb/testsuite/gdb.base/retval-large-struct.c  | 45 +++++++++++++++++++
 .../gdb.base/retval-large-struct.exp          | 37 +++++++++++++++
 4 files changed, 130 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/retval-large-struct.c
 create mode 100644 gdb/testsuite/gdb.base/retval-large-struct.exp

Comments

Tom Tromey March 10, 2022, 5:01 p.m. UTC | #1
>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

Luis> From: Luis Machado <luis.machado@linaro.org>
Luis> When running gdb.cp/non-trivial-retval.exp, the following shows up for
Luis> both aarch64-linux and armhf-linux:

Thanks for the patch.

I don't know much about these arches, but I did look at the patch and
(1) there were no red flags for me, and (2) it looks similar to what is
done in other -tdep files.  I also appreciate the new test case.

Tom
Luis Machado March 11, 2022, 1:29 p.m. UTC | #2
On 3/10/22 17:01, Tom Tromey wrote:
>>>>>> "Luis" == Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Luis> From: Luis Machado <luis.machado@linaro.org>
> Luis> When running gdb.cp/non-trivial-retval.exp, the following shows up for
> Luis> both aarch64-linux and armhf-linux:
> 
> Thanks for the patch.
> 
> I don't know much about these arches, but I did look at the patch and
> (1) there were no red flags for me, and (2) it looks similar to what is
> done in other -tdep files.  I also appreciate the new test case.
> 
> Tom

Thanks Tom! I'll push it soon if there are no further comments.
Luis Machado March 14, 2022, 10:39 a.m. UTC | #3
On 3/11/22 13:29, Luis Machado via Gdb-patches wrote:
> On 3/10/22 17:01, Tom Tromey wrote:
>>>>>>> "Luis" == Luis Machado via Gdb-patches 
>>>>>>> <gdb-patches@sourceware.org> writes:
>>
>> Luis> From: Luis Machado <luis.machado@linaro.org>
>> Luis> When running gdb.cp/non-trivial-retval.exp, the following shows 
>> up for
>> Luis> both aarch64-linux and armhf-linux:
>>
>> Thanks for the patch.
>>
>> I don't know much about these arches, but I did look at the patch and
>> (1) there were no red flags for me, and (2) it looks similar to what is
>> done in other -tdep files.  I also appreciate the new test case.
>>
>> Tom
> 
> Thanks Tom! I'll push it soon if there are no further comments.

Pushed now.
diff mbox series

Patch

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b3efb3ebaff..3f3a65240c0 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;
 	}
     }
 
diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d36856e1f3b..8e245648f23 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -8075,7 +8075,8 @@  arm_return_in_memory (struct gdbarch *gdbarch, struct type *type)
     {
       /* The AAPCS says all aggregates not larger than a word are returned
 	 in a register.  */
-      if (TYPE_LENGTH (type) <= ARM_INT_REGISTER_SIZE)
+      if (TYPE_LENGTH (type) <= ARM_INT_REGISTER_SIZE
+	  && language_pass_by_reference (type).trivially_copyable)
 	return 0;
 
       return 1;
@@ -8086,7 +8087,8 @@  arm_return_in_memory (struct gdbarch *gdbarch, struct type *type)
 
       /* All aggregate types that won't fit in a register must be returned
 	 in memory.  */
-      if (TYPE_LENGTH (type) > ARM_INT_REGISTER_SIZE)
+      if (TYPE_LENGTH (type) > ARM_INT_REGISTER_SIZE
+	  || !language_pass_by_reference (type).trivially_copyable)
 	return 1;
 
       /* In the ARM ABI, "integer" like aggregate types are returned in
@@ -8307,9 +8309,33 @@  arm_return_value (struct gdbarch *gdbarch, struct value *function,
       || valtype->code () == TYPE_CODE_UNION
       || valtype->code () == TYPE_CODE_ARRAY)
     {
+      /* From the AAPCS document:
+
+	 Result return:
+
+	 A Composite Type larger than 4 bytes, or whose size cannot be
+	 determined statically by both caller and callee, is stored in memory
+	 at an address passed as an extra argument when the function was
+	 called (Parameter Passing, rule A.4). The memory to be used for the
+	 result may be modified at any point during the function call.
+
+	 Parameter Passing:
+
+	 A.4: If the subroutine is a function that returns a result in memory,
+	 then the address for the result is placed in r0 and the NCRN is set
+	 to r1.  */
       if (tdep->struct_return == pcc_struct_return
 	  || arm_return_in_memory (gdbarch, valtype))
-	return RETURN_VALUE_STRUCT_CONVENTION;
+	{
+	  if (readbuf)
+	    {
+	      CORE_ADDR addr;
+
+	      regcache->cooked_read (ARM_A1_REGNUM, &addr);
+	      read_memory (addr, readbuf, TYPE_LENGTH (valtype));
+	    }
+	  return RETURN_VALUE_ABI_RETURNS_ADDRESS;
+	}
     }
   else if (valtype->code () == TYPE_CODE_COMPLEX)
     {
diff --git a/gdb/testsuite/gdb.base/retval-large-struct.c b/gdb/testsuite/gdb.base/retval-large-struct.c
new file mode 100644
index 00000000000..ae459741220
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retval-large-struct.c
@@ -0,0 +1,45 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct big_struct_t
+{
+  int int_array[5];
+  double double_array[5];
+  char char_array[5];
+};
+
+struct big_struct_t big_struct =
+{
+  {1, 2, 3, 4, 5},
+  {3.25, 5.0, 6.25, 1.325, -1.95},
+  "abcde"
+};
+
+struct big_struct_t return_large_struct (void)
+{
+  return big_struct;
+}
+
+int
+main (int argc, char **argv)
+{
+  struct big_struct_t test_struct;
+
+  test_struct = return_large_struct ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/retval-large-struct.exp b/gdb/testsuite/gdb.base/retval-large-struct.exp
new file mode 100644
index 00000000000..11bc4d5fbdf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/retval-large-struct.exp
@@ -0,0 +1,37 @@ 
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite
+#
+# Test if "finish" behaves correctly when a function returns a
+# large (> 16 bytes) struct.
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+set pattern ".* = \\{int_array = \\{1, 2, 3, 4, 5\\}, double_array = \\{3.25, 5, 6.25, 1.325, -1.95\\}, char_array = \"abcde\"\\}"
+
+gdb_test "p return_large_struct ()" $pattern
+
+gdb_breakpoint "return_large_struct"
+gdb_continue_to_breakpoint "Break in print_large_struct"
+gdb_test "finish" $pattern "finish from return_large_struct"