Re: [PATCH 2/2 ver 2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love via Gdb-patches <gdb-patches@sourceware.org>]

Kevin, Bruno:

I haven't seen any comments on the updated patch.  Just wanted to check
and see if the updated patches look OK to you. Thanks.

                   Carl 

On Tue, 2022-10-18 at 11:55 -0700, Carl Love wrote:
> GDB maintainers:
> 
> Version 2, updated comments per Kevin's comments.  Changed
> "frame_info
> *" to frame_info_ptr per Bruno's comments.  Discussed supressing the
> new warning after printing it with Kevin.  In the end we decided to
> not
> suppress the warnings.  Suppression can be added later if deemed
> necessary.  The white space introduced by the gdb/gdbarch-
> components.py 
> needs to be addressed by a future patch to fix the script and the
> generated files.
> 
> On PowerPC, a non-trivial return value for a function cannot be
> reliably determined with the current PowerPC ABI.  The issue is the
> address of the return buffer for a non-trivial value is passed to the
> function in register r3.  However, at the end of the function the ABI
> does not guarantee r3 will not be changed.  Hence it cannot be
> reliably
> used to get the address of the return buffer.
> 
> This patch adds a new GDB ABI to allow GDB to obtain the input value
> of
> r3 when the function returns.  This is done by using the DW_OP_entry
> value for the DWARF entries to obtain the value of r3 on entry to the
> function.  The ABI allows GDB to get the correct address for the
> return
> buffer on exit from the function.
> 
> This patch fixes the five test failures in gdb.cp/non-trivial-
> retval.exp.
> 
> The patch has been re-tested on PowerPC and X86-64 with no regression
> failures.
> 
> Please let me know if this version of the patch is acceptable for GDB
> mainline.  Thanks.
> 
>                          Carl Love
> 
> 
> -----------------------------------
> PowerPC, fix support for printing the function return value for non-
> trivial values.
> 
> Currently, a non-trivial return value from a function cannot
> currently be
> reliably determined on PowerPC.  This is due to the fact that the
> PowerPC
> ABI uses register r3 to store the address of the buffer containing
> the
> non-trivial return value when the function is called.  The PowerPC
> ABI
> does not guarantee the value in register r3 is not modified in the
> function.  Thus the value in r3 cannot be reliably used to obtain the
> return addreses on exit from the function.
> 
> This patch adds a new gdbarch method to allow PowerPC to access the
> value
> of r3 on entry to a function. On PowerPC, the new gdbarch method
> attempts
> to use the DW_OP_entry_value for the DWARF entries, when exiting the
> function, to determine the value of r3 on entry to the
> function.  This
> requires the use of the -fvar-tracking compiler option to compile the
> user application thus generating the DW_OP_entry_value in the
> binary.  The
> DW_OP_entry_value entries in the binary file allows GDB to resolve
> the
> DW_TAG_call_site entries.  This new gdbarch method is used to get the
> return buffer address, in the case of a function returning a
> nontrivial
> data type, on exit from the function.  The GDB function should_stop
> checks
> to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set
> to
> zero by the new gdbarch method call for all architectures except
> PowerPC.
> The get_return_value function will be used to obtain the return value
> on
> all other architectures as is currently being done if RETURN_BUF is
> zero.
> On PowerPC, the new gdbarch method will return a nonzero address in
> RETURN_BUF if the value can be determined.  The value_at function
> uses the
> return buffer address to get the return value.
> 
> This patch fixes five testcase failures in gdb.cp/non-trivial-
> retval.exp.
> The correct function return values are now reported.
> 
> Note this patch is dependent on patch: "PowerPC, function
> ppc64_sysv_abi_return_value add missing return value convention".
> 
> This patch has been tested on Power 10 and x86-64 with no
> regressions.
> 
> Signed-off-by: Carl Love <cel@us.ibm.com>
> ---
>  gdb/arch-utils.c                            |  6 +++
>  gdb/arch-utils.h                            |  4 ++
>  gdb/dwarf2/loc.c                            | 10 +----
>  gdb/dwarf2/loc.h                            | 11 ++++++
>  gdb/gdbarch-components.py                   | 15 ++++++++
>  gdb/gdbarch-gen.h                           | 11 ++++++
>  gdb/gdbarch.c                               | 23 ++++++++++++
>  gdb/infcmd.c                                | 41
> +++++++++++++++++++--
>  gdb/ppc-sysv-tdep.c                         | 41
> +++++++++++++++++++++
>  gdb/ppc-tdep.h                              |  2 +
>  gdb/rs6000-tdep.c                           |  6 ++-
>  gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
>  gdb/testsuite/lib/gdb.exp                   |  8 ++++
>  13 files changed, 173 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 6a72b3f5efd..1358987c02f 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1092,6 +1092,12 @@ default_read_core_file_mappings
>  {
>  }
> 
> +CORE_ADDR
> +default_get_return_buf_addr (struct type *val_type, frame_info_ptr
> cur_frame)
> +{
> +  return 0;
> +}
> +
>  /* 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 f6229f434fd..46018a6fbbb 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
>     struct bfd *cbfd,
>     read_core_file_mappings_pre_loop_ftype pre_loop_cb,
>     read_core_file_mappings_loop_ftype loop_cb);
> +
> +/* 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);
>  #endif /* ARCH_UTILS_H */
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index 791648d6e7e..b88226db610 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -1325,14 +1325,8 @@ static const struct lval_funcs
> entry_data_value_funcs =
>    entry_data_value_free_closure
>  };
> 
> -/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND
> and KIND_U
> -   are used to match DW_AT_location at the caller's
> -   DW_TAG_call_site_parameter.
> -
> -   Function always returns non-NULL value.  It throws
> NO_ENTRY_VALUE_ERROR if it
> -   cannot resolve the parameter for any reason.  */
> -
> -static struct value *
> +/* See dwarf2/loc.h.  */
> +struct value *
>  value_of_dwarf_reg_entry (struct type *type, frame_info_ptr frame,
>  			  enum call_site_parameter_kind kind,
>  			  union call_site_parameter_u kind_u)
> diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
> index d6709f2e342..9156e1ee533 100644
> --- a/gdb/dwarf2/loc.h
> +++ b/gdb/dwarf2/loc.h
> @@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
>     dwarf2_per_objfile *per_objfile, frame_info_ptr frame,
>     struct type *type, bool resolve_abstract_p = false);
> 
> +/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND
> and KIND_U
> +   are used to match DW_AT_location at the caller's
> +   DW_TAG_call_site_parameter.
> +
> +   Function always returns non-NULL value.  It throws
> NO_ENTRY_VALUE_ERROR if
> +   it cannot resolve the parameter for any reason.  */
> +
> +extern struct value *value_of_dwarf_reg_entry (struct type *type,
> +					       struct frame_info_ptr
> frame,
> +					       enum
> call_site_parameter_kind kind,
> +					       union
> call_site_parameter_u kind_u);
>  #endif /* DWARF2LOC_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 46e7565f293..325801256a2 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -868,6 +868,21 @@ for instance).
>      invalid=True,
>  )
> 
> +Function(
> +    comment="""
> +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
> +convention".
> +
> +May return 0 when unable to determine that address.""",
> +    type="CORE_ADDR",
> +    name="get_return_buf_addr",
> +    params=[("struct type *", "val_type"), ("frame_info_ptr",
> "cur_frame")],
> +    predefault="default_get_return_buf_addr",
> +    invalid=False,
> +)
> +
>  Method(
>      comment="""
>  Return true if the return value of function is stored in the first
> hidden
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 840de585869..66e2e761385 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -441,6 +441,17 @@ typedef enum return_value_convention
> (gdbarch_return_value_ftype) (struct gdbarc
>  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 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
> +   convention".
> +
> +   May return 0 when unable to determine that address. */
> +
> +typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type
> *val_type, frame_info_ptr cur_frame);
> +extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch
> *gdbarch, struct type *val_type, frame_info_ptr cur_frame);
> +extern void set_gdbarch_get_return_buf_addr (struct gdbarch
> *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
> +
>  /* Return true if the return value of function is stored in the
> first hidden
>     parameter.  In theory, this feature should be language-dependent, 
> specified
>     by language and its ABI, such as C++.  Unfortunately, compiler
> may
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 559e92dee58..3bbb70e7be9 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -116,6 +116,7 @@ struct gdbarch
>    gdbarch_address_to_pointer_ftype *address_to_pointer = nullptr;
>    gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
>    gdbarch_return_value_ftype *return_value = nullptr;
> +  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = nullptr;
>    gdbarch_return_in_first_hidden_param_p_ftype
> *return_in_first_hidden_param_p = nullptr;
>    gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
>    gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
> @@ -313,6 +314,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
>    gdbarch->value_from_register = default_value_from_register;
>    gdbarch->pointer_to_address = unsigned_pointer_to_address;
>    gdbarch->address_to_pointer = unsigned_address_to_pointer;
> +  gdbarch->get_return_buf_addr = default_get_return_buf_addr;
>    gdbarch->return_in_first_hidden_param_p =
> default_return_in_first_hidden_param_p;
>    gdbarch->breakpoint_from_pc = default_breakpoint_from_pc;
>    gdbarch->sw_breakpoint_from_kind = NULL;
> @@ -465,6 +467,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>    /* 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 get_return_buf_addr, invalid_p == 0 */
>    /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0
> */
>    if (gdbarch->skip_prologue == 0)
>      log.puts ("\n\tskip_prologue");
> @@ -877,6 +880,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct
> ui_file *file)
>    gdb_printf (file,
>                        "gdbarch_dump: return_value = <%s>\n",
>                        host_address_to_string (gdbarch-
> >return_value));
> +  gdb_printf (file,
> +                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
> +                      host_address_to_string (gdbarch-
> >get_return_buf_addr));
>    gdb_printf (file,
>                        "gdbarch_dump: return_in_first_hidden_param_p
> = <%s>\n",
>                        host_address_to_string (gdbarch-
> >return_in_first_hidden_param_p));
> @@ -2684,6 +2690,23 @@ set_gdbarch_return_value (struct gdbarch
> *gdbarch,
>    gdbarch->return_value = return_value;
>  }
> 
> +CORE_ADDR
> +gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type
> *val_type, frame_info_ptr cur_frame)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->get_return_buf_addr != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
> +  return gdbarch->get_return_buf_addr (val_type, cur_frame);
> +}
> +
> +void
> +set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
> +                                 gdbarch_get_return_buf_addr_ftype
> get_return_buf_addr)
> +{
> +  gdbarch->get_return_buf_addr = get_return_buf_addr;
> +}
> +
>  int
>  gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
> struct type *type)
>  {
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index d729732c81c..2e7cca1a1aa 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -55,6 +55,7 @@
>  #include "gdbsupport/gdb_optional.h"
>  #include "source.h"
>  #include "cli/cli-style.h"
> +#include "dwarf2/loc.h"
> 
>  /* Local functions: */
> 
> @@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
>       return value.  */
>    struct return_value_info return_value_info {};
> 
> +  /* If the current function uses the "struct return convention",
> +     this holds the address at which the value being returned will
> +     be stored, or zero if that address could not be determined or
> +     the "struct return convention" is not being used.  */
> +  CORE_ADDR return_buf;
> +
>    explicit finish_command_fsm (struct interp *cmd_interp)
>      : thread_fsm (cmd_interp)
>    {
> @@ -1636,7 +1643,13 @@ finish_command_fsm::should_stop (struct
> thread_info *tp)
>  	  struct value *func;
> 
>  	  func = read_var_value (function, NULL, get_current_frame ());
> -	  rv->value = get_return_value (function, func);
> +
> +	  if (return_buf != 0)
> +	    /* Retrieve return value from the buffer where it was
> saved.  */
> +	      rv->value = value_at (rv->type, return_buf);
> +	  else
> +	      rv->value = get_return_value (function, func);
> +
>  	  if (rv->value != NULL)
>  	    rv->value_history_index = record_latest_value (rv->value);
>  	}
> @@ -1852,8 +1865,28 @@ finish_command (const char *arg, int from_tty)
>      }
> 
>    /* Find the function we will return from.  */
> -
> -  sm->function = find_pc_function (get_frame_pc (get_selected_frame
> (NULL)));
> +  frame_info_ptr callee_frame = get_selected_frame (NULL);
> +  sm->function = find_pc_function (get_frame_pc (callee_frame));
> +
> +  /* Determine the return convention.  If it is
> RETURN_VALUE_STRUCT_CONVENTION,
> +     attempt to determine the address of the return buffer.  */
> +  enum return_value_convention return_value;
> +  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> +
> +  struct type * val_type
> +    = check_typedef (sm->function->type()->target_type ());
> +
> +  return_value = gdbarch_return_value (gdbarch,
> +				       read_var_value (sm->function,
> NULL,
> +						       callee_frame),
> +				       val_type, NULL, NULL, NULL);
> +
> +  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
> +      && val_type->code() != TYPE_CODE_VOID)
> +    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
> +						  callee_frame);
> +  else
> +    sm->return_buf = 0;
> 
>    /* Print info on the selected frame, including level number but
> not
>       source.  */
> @@ -1871,7 +1904,7 @@ finish_command (const char *arg, int from_tty)
>  	  gdb_printf (_("Run till exit from "));
>  	}
> 
> -      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> +      print_stack_frame (callee_frame, 1, LOCATION, 0);
>      }
>    frame.reinflate ();
> 
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 14effb93210..f720e87e63b 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -28,6 +28,7 @@
>  #include "objfiles.h"
>  #include "infcall.h"
>  #include "dwarf2.h"
> +#include "dwarf2/loc.h"
>  #include "target-float.h"
>  #include <algorithm>
> 
> @@ -2156,3 +2157,43 @@ ppc64_sysv_abi_return_value (struct gdbarch
> *gdbarch, struct value *function,
>    return RETURN_VALUE_STRUCT_CONVENTION;
>  }
> 
> +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
> +					  frame_info_ptr cur_frame)
> +{
> +  /* The PowerPC ABI specifies aggregates that are not returned by
> value
> +     are returned in a storage buffer provided by the caller.  The
> +     address of the storage buffer is provided as a hidden first
> input
> +     arguement in register r3.  The PowerPC ABI does not guarantee
> that
> +     register r3 will not be changed while executing the
> function.  Hence, it
> +     cannot be assumed that r3 will still contain the address of the
> storage
> +     buffer when execution reaches the end of the function.
> +
> +     This function attempts to determine the value of r3 on entry to
> the
> +     function using the DW_OP_entry_value DWARF entries.  This
> requires
> +     compiling the user program with -fvar-tracking to resolve the
> +     DW_TAG_call_sites in the binary file.  */
> +
> +  union call_site_parameter_u kind_u;
> +  enum call_site_parameter_kind kind;
> +  CORE_ADDR return_val = 0;
> +
> +  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in
> r3.  */
> +  kind = CALL_SITE_PARAMETER_DWARF_REG;
> +
> +  /* val_type is the type of the return value.  Need the pointer
> type
> +     to the return value.  */
> +  val_type = lookup_pointer_type (val_type);
> +
> +  try
> +    {
> +      return_val = value_as_address (value_of_dwarf_reg_entry
> (val_type,
> +							       cur_fram
> e,
> +							       kind,
> kind_u));
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      warning ("Cannot determine the function return value.\n"
> +	       "Try compiling with -fvar-tracking.");
> +    }
> +  return return_val;
> +}
> diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
> index 34b250849b9..cbccd87820d 100644
> --- a/gdb/ppc-tdep.h
> +++ b/gdb/ppc-tdep.h
> @@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct
> regset *regset,
>  				  const struct regcache *regcache,
>  				  int regnum, void *vsxregs, size_t
> len);
> 
> +extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*,
> frame_info_ptr);
> +
>  /* Private data that this module attaches to struct gdbarch.  */
> 
>  /* ELF ABI version used by the inferior.  */
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 8b6d666bbe7..637cbb41651 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -8234,7 +8234,11 @@ rs6000_gdbarch_init (struct gdbarch_info info,
> struct gdbarch_list *arches)
>    set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
> 
>    if (wordsize == 8)
> -    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
> +    {
> +      set_gdbarch_return_value (gdbarch,
> ppc64_sysv_abi_return_value);
> +      set_gdbarch_get_return_buf_addr (gdbarch,
> +				       ppc64_sysv_get_return_buf_addr);
> +    }
>    else
>      set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
> 
> diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> index 21c390bc937..93a3a6832f7 100644
> --- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> @@ -15,11 +15,18 @@
> 
>  # This file is part of the gdb testsuite
> 
> +set additional_flags ""
> +
>  if {[skip_cplus_tests]} { continue }
> 
>  standard_testfile .cc
> 
> -if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> {debug c++}]} {
> +if {[have_fvar_tracking]} {
> +    set additional_flags "additional_flags= -fvar-tracking"
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> [list debug c++ $additional_flags]]} {
> +
>      return -1
>  }
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index bfa9fec628e..d3b77b4869b 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8582,6 +8582,14 @@ gdb_caching_proc have_fuse_ld_gold {
>      return [gdb_simple_compile $me $src executable $flags]
>  }
> 
> +# Return 1 if compiler supports fvar-tracking, otherwise return 0.
> +gdb_caching_proc have_fvar_tracking {
> +    set me "have_fvar_tracking"
> +    set flags "additional_flags=-fvar-tracking"
> +    set src { int main() { return 0; } }
> +    return [gdb_simple_compile $me $src executable $flags]
> +}
> +
>  # Return 1 if linker supports -Ttext-segment, otherwise return 0.
>  gdb_caching_proc linker_supports_Ttext_segment_flag {
>      set me "linker_supports_Ttext_segment_flag"