Re: [PATCH v4] gdb: add a symbol* argument to get_return_value

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2022-02-02 16:59, Lancelot SIX via Gdb-patches wrote:
> Hi,
> 
> Here is a revised version of the patch.
> 
> Since V3:
> 
> - Update documentation of get_return_value and move in inferior.h.
> 
> Best,
> Lancelot.
> 
> ---
> 
> Add an argument to the get_return_value function to indicate the symbol
> of the function the debuggee is returning from.  This will be used by
> the following patch.
> 
> No user visible change after this patch.

Hi Lancelot,

Every time I look I keep finding nits, sorry :(

> 
> Tested on x86_64-linux.
> 
> Change-Id: Idf1279f1f7199f5022738a6679e0fa63fbd22edc
> ---
>  gdb/infcmd.c                     |  9 ++++-----
>  gdb/inferior.h                   | 10 +++++++++-
>  gdb/python/py-finishbreakpoint.c | 29 ++++++++++++++++++++---------
>  3 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 994dd5b32a3..6be126aedab 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1405,12 +1405,11 @@ advance_command (const char *arg, int from_tty)
>    until_break_command (arg, from_tty, 1);
>  }
>  \f
> -/* Return the value of the result of a function at the end of a 'finish'
> -   command/BP.  DTOR_DATA (if not NULL) can represent inferior registers
> -   right after an inferior call has finished.  */
> +/* See inferior.h.  */
>  
>  struct value *
> -get_return_value (struct value *function, struct type *value_type)
> +get_return_value (struct symbol *func_symbol, struct value *function,
> +		  struct type *value_type)

I have the feeling that value_type is redundant with func_symbol, since
the return value type initially comes from the symbol.  So I think we
could remove value_type.  In fact, it might have been redundant already,
since from `struct value *function`, you should also be able to get the
function type.  But there may be edge cases I don't know about.

Do you think this change below (that builds on top of your patch) would
work?  Tests gdb.*/*finish*.exp pass here.


From 7062c0afb88b5a845f8d44e4ad3bae20acbd0dd3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Wed, 2 Feb 2022 20:00:04 -0500
Subject: [PATCH] fixup

Change-Id: I3644b669ab6a4e32cf59129f5c4b8e01fe90196e
---
 gdb/infcmd.c                     |  8 +++----
 gdb/inferior.h                   |  8 +++----
 gdb/python/py-finishbreakpoint.c | 37 ++++++++++++--------------------
 3 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 6be126aedabb..9a3214374e3a 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1408,14 +1408,14 @@ advance_command (const char *arg, int from_tty)
 /* See inferior.h.  */
 
 struct value *
-get_return_value (struct symbol *func_symbol, struct value *function,
-		  struct type *value_type)
+get_return_value (struct symbol *func_symbol, struct value *function)
 {
   regcache *stop_regs = get_current_regcache ();
   struct gdbarch *gdbarch = stop_regs->arch ();
   struct value *value;
 
-  value_type = check_typedef (value_type);
+  struct type *value_type
+    = check_typedef (TYPE_TARGET_TYPE (SYMBOL_TYPE (func_symbol)));
   gdb_assert (value_type->code () != TYPE_CODE_VOID);
 
   /* FIXME: 2003-09-27: When returning from a nested inferior function
@@ -1576,7 +1576,7 @@ 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, rv->type);
+	  rv->value = get_return_value (function, func);
 	  if (rv->value != NULL)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
diff --git a/gdb/inferior.h b/gdb/inferior.h
index ef18fcb2482b..45de3c2d9c8b 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -222,13 +222,11 @@ extern void notice_new_inferior (struct thread_info *, bool, int);
 /* Return the value of the result of a function at the end of a 'finish'
    command/BP.  If the result's value cannot be retrieved, return NULL.
 
-   FUNC_SYMBOL is the symbol of the function being returned from, FUNCTION is
-   a value containing the address of the function, and VALUE_TYPE the type of
-   the value the function returns.  */
+   FUNC_SYMBOL is the symbol of the function being returned from.  FUNCTION is
+   a value containing the address of the function.  */
 
 extern struct value *get_return_value (struct symbol *func_symbol,
-				       struct value *function,
-				       struct type *value_type);
+				       struct value *function);
 
 /* Prepare for execution command.  TARGET is the target that will run
    the command.  BACKGROUND determines whether this is a foreground
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index a322918dfca6..034f8e69c0a2 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -40,15 +40,15 @@ struct finish_breakpoint_object
 {
   /* gdb.Breakpoint base class.  */
   gdbpy_breakpoint_object py_bp;
+
   /* gdb.Symbol object of the function finished by this breakpoint.  */
   PyObject *func_symbol;
-  /* gdb.Type object of the value return by the breakpointed function.
-     May be NULL if no debug information was available or return type
-     was VOID.  */
-  PyObject *return_type;
-  /* gdb.Value object of the function finished by this breakpoint.  Will be
-     NULL if return_type is NULL.  */
+
+  /* gdb.Value object of the function finished by this breakpoint.
+
+     nullptr if no debug information was available or return type was VOID.  */
   PyObject *function_value;
+
   /* When stopped at this FinishBreakpoint, gdb.Value object returned by
      the function; Py_None if the value is not computable; NULL if GDB is
      not stopped at a FinishBreakpoint.  */
@@ -84,7 +84,6 @@ bpfinishpy_dealloc (PyObject *self)
 
   Py_XDECREF (self_bpfinish->func_symbol);
   Py_XDECREF (self_bpfinish->function_value);
-  Py_XDECREF (self_bpfinish->return_type);
   Py_XDECREF (self_bpfinish->return_value);
   Py_TYPE (self)->tp_free (self);
 }
@@ -102,7 +101,7 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
   /* Can compute return_value only once.  */
   gdb_assert (!self_finishbp->return_value);
 
-  if (!self_finishbp->return_type)
+  if (self_finishbp->func_symbol == nullptr)
     return;
 
   try
@@ -111,10 +110,8 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
 	symbol_object_to_symbol (self_finishbp->func_symbol);
       struct value *function =
 	value_object_to_value (self_finishbp->function_value);
-      struct type *value_type =
-	type_object_to_type (self_finishbp->return_type);
       struct value *ret =
-	get_return_value (func_symbol, function, value_type);
+	get_return_value (func_symbol, function);
 
       if (ret)
 	{
@@ -245,7 +242,6 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 
   /* Find the function we will return from.  */
   self_bpfinish->func_symbol = nullptr;
-  self_bpfinish->return_type = nullptr;
   self_bpfinish->function_value = nullptr;
 
   try
@@ -255,22 +251,20 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	  struct symbol *function = find_pc_function (pc);
 	  if (function != nullptr)
 	    {
-	      self_bpfinish->func_symbol = symbol_to_symbol_object (function);
 	      struct type *ret_type =
 		check_typedef (TYPE_TARGET_TYPE (SYMBOL_TYPE (function)));
 
 	      /* Remember only non-void return types.  */
 	      if (ret_type->code () != TYPE_CODE_VOID)
 		{
-		  struct value *func_value;
-
 		  /* Ignore Python errors at this stage.  */
-		  self_bpfinish->return_type = type_to_type_object (ret_type);
-		  PyErr_Clear ();
-		  func_value = read_var_value (function, NULL, frame);
-		  self_bpfinish->function_value =
-		      value_to_value_object (func_value);
+		  value *func_value = read_var_value (function, NULL, frame);
+		  self_bpfinish->function_value
+		    = value_to_value_object (func_value);
 		  PyErr_Clear ();
+
+		  self_bpfinish->func_symbol
+		    = symbol_to_symbol_object (function);
 		}
 	    }
 	}
@@ -282,16 +276,13 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
     }
 
   if (self_bpfinish->func_symbol == nullptr
-      || self_bpfinish->return_type == nullptr
       || self_bpfinish->function_value == nullptr)
     {
       /* Won't be able to compute return value.  */
       Py_XDECREF (self_bpfinish->func_symbol);
-      Py_XDECREF (self_bpfinish->return_type);
       Py_XDECREF (self_bpfinish->function_value);
 
       self_bpfinish->func_symbol = nullptr;
-      self_bpfinish->return_type = nullptr;
       self_bpfinish->function_value = nullptr;
     }
 

base-commit: 36a13a0e62bc672f59c6d27bc2b963edee32b488
prerequisite-patch-id: 2ac1882ff2c8573a1f4abfe6ec081c23bad1f2fe
-- 
2.34.1