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

  GDB maintainers:

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 tested on PowerPC and X86-64 with no regression
failures.

Please let me know if this 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 gdb ABI to allow PowerPC to access the value of r3
on entry to a function. The new gdb ABI 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_entries in the
binary file enables GDB can resolve the DW_TAG_call_sites.  This new API
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 ABI call for all architectures
but PowerPC.  The get_return_value function will be used to obtain the
return value on these architectures as is currently being done if
RETURN_BUF is zero.  On PowerPC, the new ABI 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.
---
 gdb/arch-utils.c                            |  6 +++
 gdb/arch-utils.h                            |  4 ++
 gdb/dwarf2/loc.c                            |  2 +-
 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                         | 37 +++++++++++++++++++
 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, 168 insertions(+), 7 deletions(-)
  

Hi Carl,

I'm still working through the patch, but I noticed what I believe
to be incorrect terminology while reading the intro / commit log...

On Thu, 06 Oct 2022 09:37:40 -0700
Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:

...
> This patch adds a new gdb ABI to allow PowerPC to access the value of r3
> on entry to a function. The new gdb ABI attempts to use the DW_OP_entry
...

Rather than saying "gdb ABI", I think you meant to say "gdbarch method".

Also, I'd say something like "On PowerPC, the new gdbarch method attempts
to use DW_OP_entry...".  (I.e., this new method doesn't attempt to access r3
on all architectures.)

Kevin
  

Kevin:

On Fri, 2022-10-07 at 21:36 -0700, Kevin Buettner wrote:
> Hi Carl,
> 
> I'm still working through the patch, but I noticed what I believe
> to be incorrect terminology while reading the intro / commit log...
> 
> On Thu, 06 Oct 2022 09:37:40 -0700
> Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> ...
> > This patch adds a new gdb ABI to allow PowerPC to access the value
> > of r3
> > on entry to a function. The new gdb ABI attempts to use the
> > DW_OP_entry
> ...
> 
> Rather than saying "gdb ABI", I think you meant to say "gdbarch
> method".
> 
> Also, I'd say something like "On PowerPC, the new gdbarch method
> attempts
> to use DW_OP_entry...".  (I.e., this new method doesn't attempt to
> access r3
> on all architectures.)

Thanks for the feedback on the terminology.  

I was wondering if you had finished working thru the patch?  Do you
have any additional thoughts on the patch, particularly on the
functional part of the patch before I post an updated version of the
patch.   

Thanks for looking at the patch.

                       Carl Love
  

Hi Carl,

While running "git am" to apply your patch, I ran into the following
problems...

1) The patch didn't apply cleanly due to some changes in gdb/dwarf2/loc.c.
It seemed to me that it was likely due to this commit:

bd2b40ac129 Change GDB to use frame_info_ptr

After checking out bd2b40ac129~, I was able to apply your patch
via git am, though I did see some whitespace complaints...

2) Whitespace complaints:

Applying: PowerPC, fix support for printing the function return value for non-trivial values.
...patch:161: indent with spaces.
                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
...patch:162: indent with spaces.
                      host_address_to_string (gdbarch->get_return_buf_addr));
...patch:182: indent with spaces.
                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
warning: 3 lines add whitespace errors.

(I substituted some long paths local to my machines w/ '...'.)

See my remaining remarks inline below...

Kevin

On Thu, 06 Oct 2022 09:37:40 -0700
Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:

> 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 gdb ABI to allow PowerPC to access the value of r3

I already mentioned this last week, but...

s/GDB ABI/gdbarch method/

I guess you could also refer to it as an API which you did later on
in your commit remarks.  (What I really object to is calling it an
"ABI", which it definitely is not - that said it's easy to mis-type API
as ABI, so I do understand this mistake.)

> on entry to a function. The new gdb ABI attempts to use the DW_OP_entry

s/The new/On PowerPC, the new/
s/GDB ABI/gdbarch method/

> value for the DWARF entries, when exiting the function, to determine the

Missing '_' between "DW_OP_entry" and "value".  I.e. it's "DW_OP_entry_value",
not "DW_OP_entry value".

> 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_entries in the

I don't think that "DW_OP_entries" is correct here.  Maybe "DW_OP_entry_value
entries"?

> binary file enables GDB can resolve the DW_TAG_call_sites.  This new API

Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say
"allow GDB to resolve the DW_TAG_call_site entries".

> 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 ABI call for all architectures

s/ABI/API/

> but PowerPC.  The get_return_value function will be used to obtain the
> return value on these architectures as is currently being done if
> RETURN_BUF is zero.  On PowerPC, the new ABI 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.
> ---
>  gdb/arch-utils.c                            |  6 +++
>  gdb/arch-utils.h                            |  4 ++
>  gdb/dwarf2/loc.c                            |  2 +-
>  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                         | 37 +++++++++++++++++++
>  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, 168 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 5943965bd75..12d6d1cdd75 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1093,6 +1093,12 @@ default_read_core_file_mappings
>  {
>  }
>  
> +CORE_ADDR
> +default_get_return_buf_addr (struct type *val_type, frame_info *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 f850e5fd6e7..a1944ff9c3c 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 *cur_frame);
>  #endif /* ARCH_UTILS_H */
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index ad45d57a654..25a7499fe9b 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -1332,7 +1332,7 @@ static const struct lval_funcs entry_data_value_funcs =
>     Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if it
>     cannot resolve the parameter for any reason.  */

It appears that the doc above was moved to dwarf2/loc.h.  I think it might be
better to remove the above comment and replace it with "/* See dwarf2/loc.h.  */".

>  
> -static struct value *
> +extern struct value *

Remove 'extern ' here.

>  value_of_dwarf_reg_entry (struct type *type, struct frame_info *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 a9834d32066..f2cf93ba28e 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, struct frame_info *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 *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 ad71bf754de..6c623d668c5 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -845,6 +845,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 *", "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 c7a24704c7c..3e363b3a069 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -435,6 +435,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 *cur_frame);
> +extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info *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 18d46a39d7a..da3e2ad0e36 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));
> @@ -2680,6 +2686,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 *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 b788f454432..7964baa7f92 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);
>  	}
> @@ -1851,8 +1864,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 *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.  */
> @@ -1870,7 +1903,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);
>      }
>  
>    if (execution_direction == EXEC_REVERSE)
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 14effb93210..59145f9f166 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,39 @@ 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 *cur_frame)
> +{
> +  /* On PowerPC, the ABI specifies r3 is the address where an argument

s/specifies/specifies that/

I'm not sure about this phrase:  "where an argument and".  If you mean
to say that r3 is sometimes used for arguments (when it's not used for
the address of the return value buffer), then that's true, but it's
not relevant here.

So, I guess that I'm asking that this comment be reworded somewhat.

> +     and the return value is stored.  The PowerPC ABI does not guarantee
> +     r3 will be preserved when the function returns.  The function returns
> +     the address of the buffer stored in r3.

I find this part confusing.  If the function returns the address of
the buffer, doesn't it do so by returning it in r3?  (And, if so, it
seems to me that this code would be unnecessary - so, clearly, I must
be missing something...)

> +
> +     Attempt 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_frame,
> +							       kind, kind_u));
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      warning ("Cannot determine the function return value.\n"
> +	       "Try compiling with -fvar-tracking.");

Will this warning be printed a lot?  (I'm wondering if there should be a
way to shut it off...)

> +    }
> +  return return_val;
> +}
> diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
> index 44f63b145c6..299c87a169d 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*);
> +
>  /* 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 87b2faad36a..0a4f4458845 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -8235,7 +8235,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 5ab4df1bcf3..a9ebc11dfea 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8468,6 +8468,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"
> -- 
> 2.31.1
> 
>
  

On Thu, 2022-10-13 at 19:49 -0700, Kevin Buettner wrote:
> Hi Carl,
> 
> While running "git am" to apply your patch, I ran into the following
> problems...
> 
> 1) The patch didn't apply cleanly due to some changes in
> gdb/dwarf2/loc.c.
> It seemed to me that it was likely due to this commit:
> 
> bd2b40ac129 Change GDB to use frame_info_ptr
> 
> After checking out bd2b40ac129~, I was able to apply your patch
> via git am, though I did see some whitespace complaints...

Yes, mainline has moved a bit.  I refreshed the patch.  I made the
change mentioned by Bruno c/frame_info */frame_info_ptr/.  I was then
able to get gdb to compile.

> 
> 2) Whitespace complaints:
> 
> Applying: PowerPC, fix support for printing the function return value
> for non-trivial values.
> ...patch:161: indent with spaces.
>                       "gdbarch_dump: get_return_buf_addr = <%s>\n",
> ...patch:162: indent with spaces.
>                       host_address_to_string (gdbarch-
> >get_return_buf_addr));
> ...patch:182: indent with spaces.
>                                  gdbarch_get_return_buf_addr_ftype
> get_return_buf_addr)
> warning: 3 lines add whitespace errors.
> 
> (I substituted some long paths local to my machines w/ '...'.)

Yes, I have seen the complaints about the white spaces.  The whitespace
complaints come from file gdb/gdbarch.c which is a generated file.  As
I assume you know, the file was regenerated by the gdb/gdbarch.py based
on the changes to gdb/gdbarch-components.py.  The regenerated file is
set to read only.  Not sure what I can do to rectify the white spaces? 
Any ideas?

> <snip>

> I already mentioned this last week, but...
> 
> s/GDB ABI/gdbarch method/
> 
> I guess you could also refer to it as an API which you did later on
> in your commit remarks.  (What I really object to is calling it an
> "ABI", which it definitely is not - that said it's easy to mis-type
> API
> as ABI, so I do understand this mistake.)

Yea, I think referring to it as a new gdbarch method is probably better
than GDB API/ABI.  I have changed the GDB API/ABI references to gdbarch
method.  I will need to update that in the message to the GDB
maintainers as well when I send the next version of the patch.

> 
> > on entry to a function. The new gdb ABI attempts to use the
> > DW_OP_entry
> 
> s/The new/On PowerPC, the new/
> s/GDB ABI/gdbarch method/
> 
> > value for the DWARF entries, when exiting the function, to
> > determine the
> 
> Missing '_' between "DW_OP_entry" and "value".  I.e. it's
> "DW_OP_entry_value",
> not "DW_OP_entry value".

Fixed.
> 
> > 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_entries
> > in the
> 
> I don't think that "DW_OP_entries" is correct here.  Maybe
> "DW_OP_entry_value
> entries"?

> > binary file enables GDB can resolve the DW_TAG_call_sites.  This
> > new API
> 
> Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say
> "allow GDB to resolve the DW_TAG_call_site entries".
> 
> 
> > 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 ABI call for all
> > architectures
> 
> s/ABI/API/
> 
> > but PowerPC.  The get_return_value function will be used to obtain
> > the
> > return value on these architectures as is currently being done if
> > RETURN_BUF is zero.  On PowerPC, the new ABI 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.

So, I tried to address all of the above comments about ABI,
"DW_OP_entries" etc.  The paragraph now reads:

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 gdb arch 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.

Which I think covers all of the comments above. 

> > 
> > This patch fixes five testcase failures in gdb.cp/non-trivial-
> > retval.exp.
> > 

< snip >

> > 			      frame_info *cur_frame);
> >  #endif /* ARCH_UTILS_H */
> > diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> > index ad45d57a654..25a7499fe9b 100644
> > --- a/gdb/dwarf2/loc.c
> > +++ b/gdb/dwarf2/loc.c
> > @@ -1332,7 +1332,7 @@ static const struct lval_funcs
> > entry_data_value_funcs =
> >     Function always returns non-NULL value.  It throws
> > NO_ENTRY_VALUE_ERROR if it
> >     cannot resolve the parameter for any reason.  */
> 
> It appears that the doc above was moved to dwarf2/loc.h.  I think it
> might be
> better to remove the above comment and replace it with "/* See
> dwarf2/loc.h.  */".
> 
Fixed.

> >  
> > -static struct value *
> > +extern struct value *
> 
> Remove 'extern ' here.

Fixed.

> 

<snip>

> >    if (execution_direction == EXEC_REVERSE)
> > diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> > index 14effb93210..59145f9f166 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,39 @@ 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 *cur_frame)
> > +{
> > +  /* On PowerPC, the ABI specifies r3 is the address where an
> > argument
> 
> s/specifies/specifies that/
> 
> I'm not sure about this phrase:  "where an argument and".  If you
> mean
> to say that r3 is sometimes used for arguments (when it's not used
> for
> the address of the return value buffer), then that's true, but it's
> not relevant here.
> 
> So, I guess that I'm asking that this comment be reworded somewhat.
> 
> > +     and the return value is stored.  The PowerPC ABI does not
> > guarantee
> > +     r3 will be preserved when the function returns.  The function
> > returns
> > +     the address of the buffer stored in r3.
> 
> I find this part confusing.  If the function returns the address of
> the buffer, doesn't it do so by returning it in r3?  (And, if so, it
> seems to me that this code would be unnecessary - so, clearly, I must
> be missing something...)

> > +
> > +     Attempt 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.  */

OK, so some rework here to make things a bit better.  I changed the
header comment for function ppc64_sysv_get_return_buf_addr to the
following.  Hopefully this addresses the various questions and
comments.

  /* 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.  */

Hopefully that is clearer and clears up any confusion as to the role of r3.

> > +  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.");
> 
> Will this warning be printed a lot?  (I'm wondering if there should
> be a
> way to shut it off...)

Hmm, hard to say how often it will get printed.  It is going to be up
to what the user is trying to do.  If they are stopping on the return
of functions that return non-trivial structures, they will see the
message for every function.  Otherwise, the message should not get
printed. 

Not aware of anyway we could prompt the user to ask them if they want
to suppress this message from the command line.  If there is, can you
give me a pointer to an example?

We could do something like:

catch (const gdb_exception_error &e)
    {
      static int warned = 0;

      if (warned++ < 6)
        warning ("Cannot determine the function return value.\n"
                 "Try compiling with -fvar-tracking.");
    }

So the warning is only printed the first five or whatever number of
times we want before suppressing the message.  Not sure what the right
number of times should be? 

Would adding something like this be agreeable?

                        Carl
  

On Fri, 2022-10-14 at 09:36 +0200, Bruno Larsen via Gdb-patches wrote:
> On 14/10/2022 04:49, Kevin Buettner via Gdb-patches wrote:
> > Hi Carl,
> > 
> > While running "git am" to apply your patch, I ran into the
> > following
> > problems...
> > 
> > 1) The patch didn't apply cleanly due to some changes in
> > gdb/dwarf2/loc.c.
> > It seemed to me that it was likely due to this commit:
> > 
> > bd2b40ac129 Change GDB to use frame_info_ptr
> To expand on this, wherever you used frame_info *, you should switch
> to 
> frame_info_ptr, unless you are dealing with guile. As far as I could
> see 
> in your patch, that simple change should be enough.

Yup, made the change you mentioned.  Sorry, I forgot to explicitly add
you to my reply to Kevin.  

                    Carl
  

On Fri, 14 Oct 2022 16:23:58 -0700
Carl Love <cel@us.ibm.com> wrote:

> > 2) Whitespace complaints:
> > 
> > Applying: PowerPC, fix support for printing the function return value
> > for non-trivial values.
> > ...patch:161: indent with spaces.
> >                       "gdbarch_dump: get_return_buf_addr = <%s>\n",
> > ...patch:162: indent with spaces.
> >                       host_address_to_string (gdbarch-  
> > >get_return_buf_addr));  
> > ...patch:182: indent with spaces.
> >                                  gdbarch_get_return_buf_addr_ftype
> > get_return_buf_addr)
> > warning: 3 lines add whitespace errors.
> > 
> > (I substituted some long paths local to my machines w/ '...'.)  
> 
> Yes, I have seen the complaints about the white spaces.  The whitespace
> complaints come from file gdb/gdbarch.c which is a generated file.  As
> I assume you know, the file was regenerated by the gdb/gdbarch.py based
> on the changes to gdb/gdbarch-components.py.  The regenerated file is
> set to read only.  Not sure what I can do to rectify the white spaces? 
> Any ideas?

If the unwanted white space is generated by gdbarch.py,maybe fix that
script?

That said, if those files already have this problem, I'm okay with
it going in with the whitespace complaints.  (It can be fixed in
a later patch addressing just that problem.)

> > I already mentioned this last week, but...
> > 
> > s/GDB ABI/gdbarch method/
> > 
> > I guess you could also refer to it as an API which you did later on
> > in your commit remarks.  (What I really object to is calling it an
> > "ABI", which it definitely is not - that said it's easy to mis-type
> > API
> > as ABI, so I do understand this mistake.)  
> 
> Yea, I think referring to it as a new gdbarch method is probably better
> than GDB API/ABI.  I have changed the GDB API/ABI references to gdbarch
> method.  I will need to update that in the message to the GDB
> maintainers as well when I send the next version of the patch.

Sounds good.

> > > 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_entries
> > > in the  
> > 
> > I don't think that "DW_OP_entries" is correct here.  Maybe
> > "DW_OP_entry_value
> > entries"?  
> 
> > > binary file enables GDB can resolve the DW_TAG_call_sites.  This
> > > new API  
> > 
> > Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say
> > "allow GDB to resolve the DW_TAG_call_site entries".
> > 
> >   
> > > 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 ABI call for all
> > > architectures  
> > 
> > s/ABI/API/
> >   
> > > but PowerPC.  The get_return_value function will be used to obtain
> > > the
> > > return value on these architectures as is currently being done if
> > > RETURN_BUF is zero.  On PowerPC, the new ABI 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.  
> 
> So, I tried to address all of the above comments about ABI,
> "DW_OP_entries" etc.  The paragraph now reads:
> 
> 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 gdb arch 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.
> 
> Which I think covers all of the comments above. 

Yes, this looks good, though I'd use 'gdbarch' in place of 'gdb arch'.
This is a small nit though; if you'd prefer to make them separate
words, I won't argue about it.

> > >    if (execution_direction == EXEC_REVERSE)
> > > diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> > > index 14effb93210..59145f9f166 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,39 @@ 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 *cur_frame)
> > > +{
> > > +  /* On PowerPC, the ABI specifies r3 is the address where an
> > > argument  
> > 
> > s/specifies/specifies that/
> > 
> > I'm not sure about this phrase:  "where an argument and".  If you
> > mean
> > to say that r3 is sometimes used for arguments (when it's not used
> > for
> > the address of the return value buffer), then that's true, but it's
> > not relevant here.
> > 
> > So, I guess that I'm asking that this comment be reworded somewhat.
> >   
> > > +     and the return value is stored.  The PowerPC ABI does not
> > > guarantee
> > > +     r3 will be preserved when the function returns.  The function
> > > returns
> > > +     the address of the buffer stored in r3.  
> > 
> > I find this part confusing.  If the function returns the address of
> > the buffer, doesn't it do so by returning it in r3?  (And, if so, it
> > seems to me that this code would be unnecessary - so, clearly, I must
> > be missing something...)  
> 
> > > +
> > > +     Attempt 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.  */  
> 
> OK, so some rework here to make things a bit better.  I changed the
> header comment for function ppc64_sysv_get_return_buf_addr to the
> following.  Hopefully this addresses the various questions and
> comments.
> 
>   /* 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.  */
> 
> Hopefully that is clearer and clears up any confusion as to the role of r3.

Thanks for reworking that comment.

> > > +  catch (const gdb_exception_error &e)
> > > +    {
> > > +      warning ("Cannot determine the function return value.\n"
> > > +	       "Try compiling with -fvar-tracking.");  
> > 
> > Will this warning be printed a lot?  (I'm wondering if there should
> > be a
> > way to shut it off...)  
> 
> Hmm, hard to say how often it will get printed.  It is going to be up
> to what the user is trying to do.  If they are stopping on the return
> of functions that return non-trivial structures, they will see the
> message for every function.  Otherwise, the message should not get
> printed. 
> 
> Not aware of anyway we could prompt the user to ask them if they want
> to suppress this message from the command line.  If there is, can you
> give me a pointer to an example?
> 
> We could do something like:
> 
> catch (const gdb_exception_error &e)
>     {
>       static int warned = 0;
> 
>       if (warned++ < 6)
>         warning ("Cannot determine the function return value.\n"
>                  "Try compiling with -fvar-tracking.");
>     }
> 
> So the warning is only printed the first five or whatever number of
> times we want before suppressing the message.  Not sure what the right
> number of times should be? 
> 
> Would adding something like this be agreeable?

Maybe.  If we go that route, I'd recommend printing an additional
message along side the last "Cannot determine the function return value."
message indicating that these messages are now being suppressed.

We have several mechanisms in gdb for suppressing messages already. 
Probably the most used one is the "complaints" facility for
supressing warnings while reading symbols.  (See
gdb/complaints.{h,c}.) I recently added a bfd error supression
mechanism in gdb_bfd.c.  Look for gdb_bfd_error_handler() in that
file.  There's also lim_warning() in ada-lang.c along with an old
FIXME remark indicating that it's doing something similar to that of
complaint().  There may be others, but I don't know for sure.
None of them prompt the user about whether to continue to print
warnings; I'd prefer not to do it that way.  Hard coding it to six (or
something) is okay.  Making it user-settable with some default is
also okay.  That said, this is just one message. It seems like overkill
to add a setting for this one message.

After thinking about this a bit more, I think we should go with your
original code which doesn't suppress any messages.  If we find that
it's too noisy, we can add something along the lines of what you
suggested above.

Kevin
  

On Mon, 2022-10-17 at 18:06 -0700, Kevin Buettner wrote:
> On Fri, 14 Oct 2022 16:23:58 -0700
> Carl Love <cel@us.ibm.com> wrote:
> 
> > > 2) Whitespace complaints:
> > > 
> > > Applying: PowerPC, fix support for printing the function return
> > > value
> > > for non-trivial values.
> > > ...patch:161: indent with spaces.
> > >                       "gdbarch_dump: get_return_buf_addr =
> > > <%s>\n",
> > > ...patch:162: indent with spaces.
> > >                       host_address_to_string (gdbarch-  
> > > > get_return_buf_addr));  
> > > ...patch:182: indent with spaces.
> > >                                  gdbarch_get_return_buf_addr_ftyp
> > > e
> > > get_return_buf_addr)
> > > warning: 3 lines add whitespace errors.
> > > 
> > > (I substituted some long paths local to my machines w/ '...'.)  
> > 
> > Yes, I have seen the complaints about the white spaces.  The
> > whitespace
> > complaints come from file gdb/gdbarch.c which is a generated
> > file.  As
> > I assume you know, the file was regenerated by the gdb/gdbarch.py
> > based
> > on the changes to gdb/gdbarch-components.py.  The regenerated file
> > is
> > set to read only.  Not sure what I can do to rectify the white
> > spaces? 
> > Any ideas?
> 
> If the unwanted white space is generated by gdbarch.py,maybe fix that
> script?
> 
> That said, if those files already have this problem, I'm okay with
> it going in with the whitespace complaints.  (It can be fixed in
> a later patch addressing just that problem.)

I see the same white space issues through out the file.  The script
generates the spaces so yes, it probably needs fixing and then cleanup
the white spaces in the file.  However, that is beyond the scope of
this patch.

<snip
?
> > So, I tried to address all of the above comments about ABI,
> > "DW_OP_entries" etc.  The paragraph now reads:
> > 
> > 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 gdb arch 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.
> > 
> > Which I think covers all of the comments above. 
> 
> Yes, this looks good, though I'd use 'gdbarch' in place of 'gdb
> arch'.
> This is a small nit though; if you'd prefer to make them separate
> words, I won't argue about it.

I intended to use "gdbarch" not "gdb arch".  Fixed.

<snip>

> 
> Thanks for reworking that comment.
> 
> > > > +  catch (const gdb_exception_error &e)
> > > > +    {
> > > > +      warning ("Cannot determine the function return value.\n"
> > > > +	       "Try compiling with -fvar-tracking.");  
> > > 
> > > Will this warning be printed a lot?  (I'm wondering if there
> > > should
> > > be a
> > > way to shut it off...)  
> > 
> > Hmm, hard to say how often it will get printed.  It is going to be
> > up
> > to what the user is trying to do.  If they are stopping on the
> > return
> > of functions that return non-trivial structures, they will see the
> > message for every function.  Otherwise, the message should not get
> > printed. 
> > 
> > Not aware of anyway we could prompt the user to ask them if they
> > want
> > to suppress this message from the command line.  If there is, can
> > you
> > give me a pointer to an example?
> > 
> > We could do something like:
> > 
> > catch (const gdb_exception_error &e)
> >     {
> >       static int warned = 0;
> > 
> >       if (warned++ < 6)
> >         warning ("Cannot determine the function return value.\n"
> >                  "Try compiling with -fvar-tracking.");
> >     }
> > 
> > So the warning is only printed the first five or whatever number of
> > times we want before suppressing the message.  Not sure what the
> > right
> > number of times should be? 
> > 
> > Would adding something like this be agreeable?
> 
> Maybe.  If we go that route, I'd recommend printing an additional
> message along side the last "Cannot determine the function return
> value."
> message indicating that these messages are now being suppressed.
> 
> We have several mechanisms in gdb for suppressing messages already. 
> Probably the most used one is the "complaints" facility for
> supressing warnings while reading symbols.  (See
> gdb/complaints.{h,c}.) I recently added a bfd error supression
> mechanism in gdb_bfd.c.  Look for gdb_bfd_error_handler() in that
> file.  There's also lim_warning() in ada-lang.c along with an old
> FIXME remark indicating that it's doing something similar to that of
> complaint().  There may be others, but I don't know for sure.
> None of them prompt the user about whether to continue to print
> warnings; I'd prefer not to do it that way.  Hard coding it to six
> (or
> something) is okay.  Making it user-settable with some default is
> also okay.  That said, this is just one message. It seems like
> overkill
> to add a setting for this one message.
> 
> After thinking about this a bit more, I think we should go with your
> original code which doesn't suppress any messages.  If we find that
> it's too noisy, we can add something along the lines of what you
> suggested above.

I think that sounds reasonable.  We can always look at suppressing the
messages in the future if it looks like an issue.  I am leaving the
code as it was originally.

Thanks again for the feedback.  Take care.

                          Carl