[binutils-gdb] Add new overload of gdbarch_return_value

[binutils-gdb] Add new overload of gdbarch_return_value

[Tom Tromey]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4e1d2f5814b29048d1dd1cea2cb50570e6c8f1f8

commit 4e1d2f5814b29048d1dd1cea2cb50570e6c8f1f8
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Sep 7 08:39:52 2022 -0600

    Add new overload of gdbarch_return_value
    
    The gdbarch "return_value" can't correctly handle variably-sized
    types.  The problem here is that the TYPE_LENGTH of such a type is 0,
    until the type is resolved, which requires reading memory.  However,
    gdbarch_return_value only accepts a buffer as an out parameter.
    
    Fixing this requires letting the implementation of the gdbarch method
    resolve the type and return a value -- that is, both the contents and
    the new type.
    
    After an attempt at this, I realized I wouldn't be able to correctly
    update all implementations (there are ~80) of this method.  So,
    instead, this patch adds a new method that falls back to the current
    method, and it updates gdb to only call the new method.  This way it's
    possible to incrementally convert the architectures that I am able to
    test.

Diff:
---
 gdb/arch-utils.c          | 18 ++++++++++++++++++
 gdb/arch-utils.h          |  6 ++++++
 gdb/elfread.c             |  4 ++--
 gdb/gdbarch-components.py | 36 ++++++++++++++++++++++++++++++++++--
 gdb/gdbarch-gen.h         | 21 +++++++++++++++++++--
 gdb/gdbarch.c             | 35 ++++++++++++++++++++++++-----------
 gdb/infcall.c             |  7 +++----
 gdb/infcmd.c              | 18 +++++++++---------
 gdb/stack.c               |  7 ++++---
 gdb/value.c               |  4 ++--
 10 files changed, 121 insertions(+), 35 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index a6d0035f973..3ca193085cb 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1098,6 +1098,24 @@ default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
   return 0;
 }
 
+enum return_value_convention
+default_gdbarch_return_value
+     (struct gdbarch *gdbarch, struct value *function, struct type *valtype,
+      struct regcache *regcache, struct value **read_value,
+      const gdb_byte *writebuf)
+{
+  gdb_byte *readbuf = nullptr;
+
+  if (read_value != nullptr)
+    {
+      *read_value = allocate_value (valtype);
+      readbuf = value_contents_raw (*read_value).data ();
+    }
+
+  return gdbarch_return_value (gdbarch, function, valtype, regcache,
+			       readbuf, writebuf);
+}
+
 /* Non-zero if we want to trace architecture code.  */
 
 #ifndef GDBARCH_DEBUG
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 9671e50c0dc..56690f0fd43 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -308,4 +308,10 @@ extern void default_read_core_file_mappings
 /* Default implementation of gdbarch default_get_return_buf_addr method.  */
 extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
 					      frame_info_ptr cur_frame);
+
+extern enum return_value_convention default_gdbarch_return_value
+     (struct gdbarch *gdbarch, struct value *function, struct type *valtype,
+      struct regcache *regcache, struct value **read_value,
+      const gdb_byte *writebuf);
+
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/elfread.c b/gdb/elfread.c
index ceaf81f0fca..ccee37406c8 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -1038,8 +1038,8 @@ elf_gnu_ifunc_resolver_return_stop (code_breakpoint *b)
   set_value_address (func_func, b->loc->related_address);
 
   value = allocate_value (value_type);
-  gdbarch_return_value (gdbarch, func_func, value_type, regcache,
-			value_contents_raw (value).data (), NULL);
+  gdbarch_return_value_as_value (gdbarch, func_func, value_type, regcache,
+				 &value, NULL);
   resolved_address = value_as_address (value);
   resolved_pc = gdbarch_convert_from_func_ptr_addr
     (gdbarch, resolved_address, current_inferior ()->top_target ());
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index ca9c3bceb3d..0e1ca40102d 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -854,6 +854,9 @@ If WRITEBUF is not NULL, it contains a return value which will be
 stored into the appropriate register.  This can be used when we want
 to force the value returned by a function (see the "return" command
 for instance).
+
+NOTE: it is better to implement return_value_as_value instead, as that
+method can properly handle variably-sized types.
 """,
     type="enum return_value_convention",
     name="return_value",
@@ -864,8 +867,37 @@ for instance).
         ("gdb_byte *", "readbuf"),
         ("const gdb_byte *", "writebuf"),
     ],
-    predicate=True,
-    invalid=True,
+    invalid=False,
+)
+
+Method(
+    comment="""
+Return the return-value convention that will be used by FUNCTION
+to return a value of type VALTYPE.  FUNCTION may be NULL in which
+case the return convention is computed based only on VALTYPE.
+
+If READ_VALUE is not NULL, extract the return value and save it in
+this pointer.
+
+If WRITEBUF is not NULL, it contains a return value which will be
+stored into the appropriate register.  This can be used when we want
+to force the value returned by a function (see the "return" command
+for instance).
+""",
+    type="enum return_value_convention",
+    name="return_value_as_value",
+    params=[
+        ("struct value *", "function"),
+        ("struct type *", "valtype"),
+        ("struct regcache *", "regcache"),
+        ("struct value **", "read_value"),
+        ("const gdb_byte *", "writebuf"),
+    ],
+    predefault="default_gdbarch_return_value",
+    # If we're using the default, then the other method must be set;
+    # but if we aren't using the default here then the other method
+    # must not be set.
+    invalid="(gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr)",
 )
 
 Function(
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index c2299dcf6e6..128efa10bef 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -433,14 +433,31 @@ extern void set_gdbarch_integer_to_address (struct gdbarch *gdbarch, gdbarch_int
    If WRITEBUF is not NULL, it contains a return value which will be
    stored into the appropriate register.  This can be used when we want
    to force the value returned by a function (see the "return" command
-   for instance). */
+   for instance).
 
-extern bool gdbarch_return_value_p (struct gdbarch *gdbarch);
+   NOTE: it is better to implement return_value_as_value instead, as that
+   method can properly handle variably-sized types. */
 
 typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
 
+/* Return the return-value convention that will be used by FUNCTION
+   to return a value of type VALTYPE.  FUNCTION may be NULL in which
+   case the return convention is computed based only on VALTYPE.
+
+   If READ_VALUE is not NULL, extract the return value and save it in
+   this pointer.
+
+   If WRITEBUF is not NULL, it contains a return value which will be
+   stored into the appropriate register.  This can be used when we want
+   to force the value returned by a function (see the "return" command
+   for instance). */
+
+typedef enum return_value_convention (gdbarch_return_value_as_value_ftype) (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf);
+extern enum return_value_convention gdbarch_return_value_as_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf);
+extern void set_gdbarch_return_value_as_value (struct gdbarch *gdbarch, gdbarch_return_value_as_value_ftype *return_value_as_value);
+
 /* Return the address at which the value being returned from
    the current function will be stored.  This routine is only
    called if the current function uses the the "struct return
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 5469b1355ce..353040cf020 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -116,6 +116,7 @@ struct gdbarch
   gdbarch_address_to_pointer_ftype *address_to_pointer = unsigned_address_to_pointer;
   gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
   gdbarch_return_value_ftype *return_value = nullptr;
+  gdbarch_return_value_as_value_ftype *return_value_as_value = default_gdbarch_return_value;
   gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = default_get_return_buf_addr;
   gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
   gdbarch_skip_prologue_ftype *skip_prologue = 0;
@@ -369,7 +370,9 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of pointer_to_address, invalid_p == 0 */
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
-  /* Skip verify of return_value, has predicate.  */
+  /* Skip verify of return_value, invalid_p == 0 */
+  if ((gdbarch->return_value_as_value == default_gdbarch_return_value) == (gdbarch->return_value == nullptr))
+    log.puts ("\n\treturn_value_as_value");
   /* Skip verify of get_return_buf_addr, invalid_p == 0 */
   /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
   if (gdbarch->skip_prologue == 0)
@@ -776,12 +779,12 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
 	      "gdbarch_dump: integer_to_address = <%s>\n",
 	      host_address_to_string (gdbarch->integer_to_address));
-  gdb_printf (file,
-	      "gdbarch_dump: gdbarch_return_value_p() = %d\n",
-	      gdbarch_return_value_p (gdbarch));
   gdb_printf (file,
 	      "gdbarch_dump: return_value = <%s>\n",
 	      host_address_to_string (gdbarch->return_value));
+  gdb_printf (file,
+	      "gdbarch_dump: return_value_as_value = <%s>\n",
+	      host_address_to_string (gdbarch->return_value_as_value));
   gdb_printf (file,
 	      "gdbarch_dump: get_return_buf_addr = <%s>\n",
 	      host_address_to_string (gdbarch->get_return_buf_addr));
@@ -2568,13 +2571,6 @@ set_gdbarch_integer_to_address (struct gdbarch *gdbarch,
   gdbarch->integer_to_address = integer_to_address;
 }
 
-bool
-gdbarch_return_value_p (struct gdbarch *gdbarch)
-{
-  gdb_assert (gdbarch != NULL);
-  return gdbarch->return_value != NULL;
-}
-
 enum return_value_convention
 gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf)
 {
@@ -2592,6 +2588,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
   gdbarch->return_value = return_value;
 }
 
+enum return_value_convention
+gdbarch_return_value_as_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, struct value **read_value, const gdb_byte *writebuf)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->return_value_as_value != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_return_value_as_value called\n");
+  return gdbarch->return_value_as_value (gdbarch, function, valtype, regcache, read_value, writebuf);
+}
+
+void
+set_gdbarch_return_value_as_value (struct gdbarch *gdbarch,
+				   gdbarch_return_value_as_value_ftype return_value_as_value)
+{
+  gdbarch->return_value_as_value = return_value_as_value;
+}
+
 CORE_ADDR
 gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame)
 {
diff --git a/gdb/infcall.c b/gdb/infcall.c
index ec2aa180daa..acaaf0c029d 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -485,10 +485,9 @@ get_call_return_value (struct call_return_meta_info *ri)
     }
   else
     {
-      retval = allocate_value (ri->value_type);
-      gdbarch_return_value (ri->gdbarch, ri->function, ri->value_type,
-			    get_current_regcache (),
-			    value_contents_raw (retval).data (), NULL);
+      gdbarch_return_value_as_value (ri->gdbarch, ri->function, ri->value_type,
+				     get_current_regcache (),
+				     &retval, NULL);
       if (stack_temporaries && class_or_union_p (ri->value_type))
 	{
 	  /* Values of class type returned in registers are copied onto
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 0c1765d418d..0497ad05091 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1497,15 +1497,14 @@ get_return_value (struct symbol *func_symbol, struct value *function)
      inferior function call code.  In fact, when inferior function
      calls are made async, this will likely be made the norm.  */
 
-  switch (gdbarch_return_value (gdbarch, function, value_type,
-				nullptr, nullptr, nullptr))
+  switch (gdbarch_return_value_as_value (gdbarch, function, value_type,
+					 nullptr, nullptr, nullptr))
     {
     case RETURN_VALUE_REGISTER_CONVENTION:
     case RETURN_VALUE_ABI_RETURNS_ADDRESS:
     case RETURN_VALUE_ABI_PRESERVES_ADDRESS:
-      value = allocate_value (value_type);
-      gdbarch_return_value (gdbarch, function, value_type, stop_regs,
-			    value_contents_raw (value).data (), nullptr);
+      gdbarch_return_value_as_value (gdbarch, function, value_type, stop_regs,
+				     &value, nullptr);
       break;
     case RETURN_VALUE_STRUCT_CONVENTION:
       value = nullptr;
@@ -1889,10 +1888,11 @@ finish_command (const char *arg, int from_tty)
       struct type * val_type
 	= check_typedef (sm->function->type ()->target_type ());
 
-      return_value = gdbarch_return_value (gdbarch,
-					   read_var_value (sm->function, nullptr,
-							   callee_frame),
-					   val_type, nullptr, nullptr, nullptr);
+      return_value
+	= gdbarch_return_value_as_value (gdbarch,
+					 read_var_value (sm->function, nullptr,
+							 callee_frame),
+					 val_type, nullptr, nullptr, nullptr);
 
       if (return_value == RETURN_VALUE_STRUCT_CONVENTION
 	  && val_type->code () != TYPE_CODE_VOID)
diff --git a/gdb/stack.c b/gdb/stack.c
index 7f7478ea323..0fd797836dc 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2838,9 +2838,10 @@ return_command (const char *retval_exp, int from_tty)
 
       gdb_assert (rv_conv != RETURN_VALUE_STRUCT_CONVENTION
 		  && rv_conv != RETURN_VALUE_ABI_RETURNS_ADDRESS);
-      gdbarch_return_value (cache_arch, function, return_type,
-			    get_current_regcache (), NULL /*read*/,
-			    value_contents (return_value).data () /*write*/);
+      gdbarch_return_value_as_value
+	(cache_arch, function, return_type,
+	 get_current_regcache (), NULL /*read*/,
+	 value_contents (return_value).data () /*write*/);
     }
 
   /* If we are at the end of a call dummy now, pop the dummy frame
diff --git a/gdb/value.c b/gdb/value.c
index 7d14dc4a5d8..e3f60e7e989 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3957,8 +3957,8 @@ struct_return_convention (struct gdbarch *gdbarch,
     error (_("Function return type unknown."));
 
   /* Probe the architecture for the return-value convention.  */
-  return gdbarch_return_value (gdbarch, function, value_type,
-			       NULL, NULL, NULL);
+  return gdbarch_return_value_as_value (gdbarch, function, value_type,
+					NULL, NULL, NULL);
 }
 
 /* Return true if the function returning the specified type is using