[prev in list] [next in list] [prev in thread] [next in thread]
List: gdb-patches
Subject: Re: [PATCH 2/2 ver 2] PowerPC, fix support for printing the function return value for non-trivial va
From: Carl Love via Gdb-patches <gdb-patches () sourceware ! org>
Date: 2022-10-31 16:07:50
Message-ID: db69e3d3fb522ab29285c7644a4b5044443cc133.camel () us ! ibm ! com
[Download RAW message or body]
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"
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic