[PATCH v3 0/2] Make GDB respect the DW_CC_nocall attribute

[PATCH v3 0/2] Make GDB respect the DW_CC_nocall attribute

[Lancelot SIX via Gdb-patches]

Hi,

This is a V3 following
https://sourceware.org/pipermail/gdb-patches/2022-January/185563.html.

Noticeable changes since V2:

- Instead of using get_function_name (COREADDR, ...) in order to format
  messages displayed to the user in get_return_value, this series now
  ensures that the function's symbol is passed to get_return_value.
  With the symbol available, getting the function name is a matter of
  calling 'symbol::print_name'.  This overall simplifies the
  implementation.

  To simplify the review process, changes required to have the symbol in
  get_return_value are done in a preparatory patch.

- Fix coding style issues reported by Bruno.

- Remove newline at the end of the testcase.

Thanks to Bruno and Simon for the comments.

Tested on x86_64-linux with no regression observed.

All feedback welcome.

Best,
Lancelot.

Lancelot SIX (2):
  gdb: add a symbol* argument to get_return_value
  gdb: Respect the DW_CC_nocall attribute

 gdb/gdbtypes.c                                | 11 +++
 gdb/gdbtypes.h                                | 10 ++
 gdb/infcall.c                                 |  5 +
 gdb/infcmd.c                                  | 14 ++-
 gdb/inferior.h                                |  3 +-
 gdb/python/py-finishbreakpoint.c              | 29 ++++--
 gdb/stack.c                                   | 18 +++-
 gdb/testsuite/gdb.dwarf2/calling-convention.c | 35 +++++++
 .../gdb.dwarf2/calling-convention.exp         | 97 +++++++++++++++++++
 9 files changed, 207 insertions(+), 15 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/calling-convention.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/calling-convention.exp


base-commit: 0d8cbc5f2fcbcb9eb207f12507fdfe04f3d3ae14
-- 
2.25.1

[PATCH v3 1/2] gdb: add a symbol* argument to get_return_value

[Lancelot SIX via Gdb-patches]

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.

Tested on x86_64-linux.

Change-Id: Idf1279f1f7199f5022738a6679e0fa63fbd22edc
---
 gdb/infcmd.c                     |  5 +++--
 gdb/inferior.h                   |  3 ++-
 gdb/python/py-finishbreakpoint.c | 29 ++++++++++++++++++++---------
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 994dd5b32a3..66667d67b21 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1410,7 +1410,8 @@ advance_command (const char *arg, int from_tty)
    right after an inferior call has finished.  */
 
 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)
 {
   regcache *stop_regs = get_current_regcache ();
   struct gdbarch *gdbarch = stop_regs->arch ();
@@ -1577,7 +1578,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 (func, rv->type);
+	  rv->value = get_return_value (function, func, rv->type);
 	  if (rv->value != NULL)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
diff --git a/gdb/inferior.h b/gdb/inferior.h
index ec0fb6e8b16..5fdff0f9358 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -219,7 +219,8 @@ extern void detach_command (const char *, int);
 
 extern void notice_new_inferior (struct thread_info *, bool, int);
 
-extern struct value *get_return_value (struct value *function,
+extern struct value *get_return_value (struct symbol *func_symbol,
+				       struct value *function,
 				       struct type *value_type);
 
 /* Prepare for execution command.  TARGET is the target that will run
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 03bd4934506..a322918dfca 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -40,6 +40,8 @@ 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.  */
@@ -80,6 +82,7 @@ bpfinishpy_dealloc (PyObject *self)
   struct finish_breakpoint_object *self_bpfinish =
 	(struct finish_breakpoint_object *) 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);
@@ -104,11 +107,14 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
 
   try
     {
+      struct symbol *func_symbol =
+	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 (function, value_type);
+      struct value *ret =
+	get_return_value (func_symbol, function, value_type);
 
       if (ret)
 	{
@@ -165,7 +171,6 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   PyObject *internal = NULL;
   int internal_bp = 0;
   CORE_ADDR pc;
-  struct symbol *function;
 
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords,
 					&frame_obj, &internal))
@@ -239,16 +244,18 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
     }
 
   /* Find the function we will return from.  */
-  self_bpfinish->return_type = NULL;
-  self_bpfinish->function_value = NULL;
+  self_bpfinish->func_symbol = nullptr;
+  self_bpfinish->return_type = nullptr;
+  self_bpfinish->function_value = nullptr;
 
   try
     {
       if (get_frame_pc_if_available (frame, &pc))
 	{
-	  function = find_pc_function (pc);
-	  if (function != NULL)
+	  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)));
 
@@ -274,14 +281,18 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	 remain NULL.  */
     }
 
-  if (self_bpfinish->return_type == NULL || self_bpfinish->function_value == NULL)
+  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->return_type = NULL;
-      self_bpfinish->function_value = NULL;
+      self_bpfinish->func_symbol = nullptr;
+      self_bpfinish->return_type = nullptr;
+      self_bpfinish->function_value = nullptr;
     }
 
   bppy_pending_object = &self_bpfinish->py_bp;
-- 
2.25.1

[PATCH v3 2/2] gdb: Respect the DW_CC_nocall attribute

[Lancelot SIX via Gdb-patches]

It is possible for a compiler to optimize a function in a such ways that
the function does not follow the calling convention of the target.  In
such situation, the compiler can use the DW_AT_calling_convention
attribute with the value DW_CC_nocall to tell the debugger that it is
unsafe to call the function.  The DWARF5 standard states, in 3.3.1.1:

  > If the value of the calling convention attribute is the constant
  > DW_CC_nocall, the subroutine does not obey standard calling
  > conventions, and it may not be safe for the debugger to call this
  > subroutine.

Non standard calling convention can affect GDB's assumptions in multiple
ways, including how arguments are passed to the function, how values are
returned, and so on.  For this reason, it is unsafe for GDB to try to do
the following operations on a function with marked with DW_CC_nocall:

- call / print an expression requiring the function to be evaluated,
- inspect the value a function returns using the 'finish' command,
- force the value returned by a function using the 'return' command.

This patch ensures that if a command which relies on GDB's knowledge of
the target's calling convention is used on a function marked nocall, GDB
prints an appropriate message to the user and does not proceed with the
operation which is unreliable.

Note that it is still possible for someone to use a vendor specific
value for the DW_AT_calling_convention attribute for example to indicate
the use of an alternative calling convention.  This commit does not
prevent this, and target dependent code can be adjusted if one wanted to
support multiple calling conventions.

Tested on x86_64-Linux, with no regression observed.

Change-Id: I72970dae68234cb83edbc0cf71aa3d6002a4a540
---
 gdb/gdbtypes.c                                | 11 +++
 gdb/gdbtypes.h                                | 10 ++
 gdb/infcall.c                                 |  5 +
 gdb/infcmd.c                                  |  9 ++
 gdb/stack.c                                   | 18 +++-
 gdb/testsuite/gdb.dwarf2/calling-convention.c | 35 +++++++
 .../gdb.dwarf2/calling-convention.exp         | 97 +++++++++++++++++++
 7 files changed, 182 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/calling-convention.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/calling-convention.exp

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 8af96c79e6c..0c2586e28f2 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3903,6 +3903,17 @@ type_byte_order (const struct type *type)
   return byteorder;
 }
 
+/* See gdbtypes.h.  */
+
+bool
+is_nocall_function (const struct type *type)
+{
+  gdb_assert (type->code () == TYPE_CODE_FUNC
+	      || type->code () == TYPE_CODE_METHOD);
+
+  return TYPE_CALLING_CONVENTION (type) == DW_CC_nocall;
+}
+
 \f
 /* Overload resolution.  */
 
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 7238873e4db..f8e8481f84d 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2852,4 +2852,14 @@ extern enum bfd_endian type_byte_order (const struct type *type);
 
 extern unsigned int overload_debug;
 
+/* Return whether the function type represented by TYPE is marked as unsafe
+   to call by the debugger.
+
+   This usually indicates that the function does not follow the target's
+   standard calling convention.
+
+   The TYPE argument must be of code TYPE_CODE_FUNC or TYPE_CODE_METHOD.  */
+
+extern bool is_nocall_function (const struct type *type);
+
 #endif /* GDBTYPES_H */
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 05cf18f0a7f..573504ee6a1 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -813,6 +813,11 @@ call_function_by_hand_dummy (struct value *function,
   type *values_type;
   CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
 
+  if (is_nocall_function (ftype))
+    error (_("Cannot call the function '%s' which does not follow the "
+	     "target calling convention."),
+	   get_function_name (funaddr, name_buf, sizeof (name_buf)));
+
   if (values_type == NULL)
     values_type = default_return_type;
   if (values_type == NULL)
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 66667d67b21..310bd430fcf 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1420,6 +1420,15 @@ get_return_value (struct symbol *func_symbol, struct value *function,
   value_type = check_typedef (value_type);
   gdb_assert (value_type->code () != TYPE_CODE_VOID);
 
+  if (is_nocall_function (check_typedef (::value_type (function))))
+    {
+      warning (_("Function '%s' does not follow the target calling "
+		 "convention, cannot determine its returned value."),
+	       func_symbol->print_name ());
+
+      return nullptr;
+    }
+
   /* FIXME: 2003-09-27: When returning from a nested inferior function
      call, it's possible (with no help from the architecture vector)
      to locate and return/print a "struct return" value.  This is just
diff --git a/gdb/stack.c b/gdb/stack.c
index c7269e2ac32..3ab9d7f684b 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2737,7 +2737,7 @@ return_command (const char *retval_exp, int from_tty)
   struct symbol *thisfun;
   struct value *return_value = NULL;
   struct value *function = NULL;
-  const char *query_prefix = "";
+  std::string query_prefix;
 
   thisframe = get_selected_frame ("No selected frame.");
   thisfun = get_frame_function (thisframe);
@@ -2793,6 +2793,17 @@ return_command (const char *retval_exp, int from_tty)
 	return_value = NULL;
       else if (thisfun != NULL)
 	{
+	  if (is_nocall_function (check_typedef (value_type (function))))
+	    {
+	      query_prefix =
+		string_printf ("Function '%s' does not follow the target "
+			       "calling convention.\n"
+			       "If you continue, setting the return value "
+			       "will probably lead to unpredictable "
+			       "behaviors.\n",
+			       thisfun->print_name ());
+	    }
+
 	  rv_conv = struct_return_convention (gdbarch, function, return_type);
 	  if (rv_conv == RETURN_VALUE_STRUCT_CONVENTION
 	      || rv_conv == RETURN_VALUE_ABI_RETURNS_ADDRESS)
@@ -2815,12 +2826,13 @@ return_command (const char *retval_exp, int from_tty)
 
       if (thisfun == NULL)
 	confirmed = query (_("%sMake selected stack frame return now? "),
-			   query_prefix);
+			   query_prefix.c_str ());
       else
 	{
 	  if (TYPE_NO_RETURN (thisfun->type))
 	    warning (_("Function does not return normally to caller."));
-	  confirmed = query (_("%sMake %s return now? "), query_prefix,
+	  confirmed = query (_("%sMake %s return now? "),
+			     query_prefix.c_str (),
 			     thisfun->print_name ());
 	}
       if (!confirmed)
diff --git a/gdb/testsuite/gdb.dwarf2/calling-convention.c b/gdb/testsuite/gdb.dwarf2/calling-convention.c
new file mode 100644
index 00000000000..0068ccdd6b4
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/calling-convention.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Dummy foo function.  */
+
+int
+foo (void)
+{
+  asm ("foo_label: .globl foo_label");
+  return 42;
+}
+
+/* Dummy main function.  */
+
+int
+main (void)
+{
+  asm ("main_label: .globl main_label");
+  foo ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/calling-convention.exp b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
new file mode 100644
index 00000000000..26b1f5eafd6
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
@@ -0,0 +1,97 @@
+# Copyright 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This testcase checks that if a function has the DW_AT_calling_convention
+# attribute with the value DW_CC_nocall, then will not:
+# - call the function,
+# - try to access the value returned by the function when using the finish
+#   command,
+# - force a user-provided return value when using the return command.
+#
+# In every case, GDB prints a message to the user indicating the issue.  For
+# the return command, GDB asks the user to confirm if the specified value
+# should be forced.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c .S
+
+# First compile the .c file so we can ask GDB what is sizeof(int).
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    untested "failed to compile"
+    return -1
+}
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name "calling-convention"}
+	} {
+	    declare_labels int_label
+
+	    int_label: base_type {
+		{byte_size [get_sizeof "int" 4] sdata}
+		{encoding @DW_ATE_signed}
+		{name "int"}
+	    }
+
+	    subprogram {
+		{MACRO_AT_func { foo }}
+		{type :$int_label}
+		{calling_convention @DW_CC_nocall}
+	    }
+
+	    subprogram {
+		{MACRO_AT_func { main }}
+		{type :$int_label}
+	    }
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_test "call foo ()" \
+    "Cannot call the function 'foo' which does not follow the target calling convention."
+gdb_breakpoint "foo"
+gdb_continue_to_breakpoint "foo"
+
+gdb_test_multiple "return 35" "" {
+    -re ".*Function 'foo' does not follow the target calling convention.\r\nIf you continue, setting the return value will probably lead to unpredictable behaviors.\r\nMake foo return now?.*\\(y or n\\) $" {
+	send_gdb "n\n"
+	pass $gdb_test_name
+    }
+}
+
+gdb_test "finish" [multi_line \
+    "Run till exit from #0  $hex in foo \\(\\)" \
+    "warning: Function 'foo' does not follow the target calling convention, cannot determine its returned value\." \
+    "$hex in main \\(\\)" \
+    "Value returned has type: int. Cannot determine contents"]
-- 
2.25.1

Re: [PATCH v3 1/2] gdb: add a symbol* argument to get_return_value

[Simon Marchi]

On 2022-02-02 1:41 p.m., Lancelot SIX via Gdb-patches wrote:
> 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.
> 
> Tested on x86_64-linux.
> 
> Change-Id: Idf1279f1f7199f5022738a6679e0fa63fbd22edc
> ---
>  gdb/infcmd.c                     |  5 +++--
>  gdb/inferior.h                   |  3 ++-
>  gdb/python/py-finishbreakpoint.c | 29 ++++++++++++++++++++---------
>  3 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 994dd5b32a3..66667d67b21 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1410,7 +1410,8 @@ advance_command (const char *arg, int from_tty)
>     right after an inferior call has finished.  */
>  
>  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)

get_return_value's comment (above this) is quite out of date, referring things
that don't exist anymore.  While at it, can you update it, move it to the header
file, and add the /* See ... */ comment here?

Simon

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

[Lancelot SIX via Gdb-patches]

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.

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)
 {
   regcache *stop_regs = get_current_regcache ();
   struct gdbarch *gdbarch = stop_regs->arch ();
@@ -1577,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 (func, rv->type);
+	  rv->value = get_return_value (function, func, rv->type);
 	  if (rv->value != NULL)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
diff --git a/gdb/inferior.h b/gdb/inferior.h
index ec0fb6e8b16..ef18fcb2482 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -219,7 +219,15 @@ extern void detach_command (const char *, int);
 
 extern void notice_new_inferior (struct thread_info *, bool, int);
 
-extern struct value *get_return_value (struct value *function,
+/* 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.  */
+
+extern struct value *get_return_value (struct symbol *func_symbol,
+				       struct value *function,
 				       struct type *value_type);
 
 /* Prepare for execution command.  TARGET is the target that will run
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 03bd4934506..a322918dfca 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -40,6 +40,8 @@ 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.  */
@@ -80,6 +82,7 @@ bpfinishpy_dealloc (PyObject *self)
   struct finish_breakpoint_object *self_bpfinish =
 	(struct finish_breakpoint_object *) 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);
@@ -104,11 +107,14 @@ bpfinishpy_pre_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
 
   try
     {
+      struct symbol *func_symbol =
+	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 (function, value_type);
+      struct value *ret =
+	get_return_value (func_symbol, function, value_type);
 
       if (ret)
 	{
@@ -165,7 +171,6 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   PyObject *internal = NULL;
   int internal_bp = 0;
   CORE_ADDR pc;
-  struct symbol *function;
 
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords,
 					&frame_obj, &internal))
@@ -239,16 +244,18 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
     }
 
   /* Find the function we will return from.  */
-  self_bpfinish->return_type = NULL;
-  self_bpfinish->function_value = NULL;
+  self_bpfinish->func_symbol = nullptr;
+  self_bpfinish->return_type = nullptr;
+  self_bpfinish->function_value = nullptr;
 
   try
     {
       if (get_frame_pc_if_available (frame, &pc))
 	{
-	  function = find_pc_function (pc);
-	  if (function != NULL)
+	  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)));
 
@@ -274,14 +281,18 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	 remain NULL.  */
     }
 
-  if (self_bpfinish->return_type == NULL || self_bpfinish->function_value == NULL)
+  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->return_type = NULL;
-      self_bpfinish->function_value = NULL;
+      self_bpfinish->func_symbol = nullptr;
+      self_bpfinish->return_type = nullptr;
+      self_bpfinish->function_value = nullptr;
     }
 
   bppy_pending_object = &self_bpfinish->py_bp;

base-commit: 0d8cbc5f2fcbcb9eb207f12507fdfe04f3d3ae14
-- 
2.25.1

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

[Simon Marchi via Gdb-patches]

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

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

[Six, Lancelot via Gdb-patches]

[AMD Official Use Only]

> Hi Lancelot,
>
> Every time I look I keep finding nits, sorry :(

No worry.

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

To be honest, I did have a similar impression about the function parameter.  Once you have the function symbol, you should be able to get a value containing the function address, so this parameter is probably also redundant.  The reason I did not make this change is because the function's address is obtained with a call to:

    struct value *function = read_var_value (func_symbol, NULL, frame);

Moving this call within get_return_value might mean we use a different FRAME argument (get_current_frame ()).  I do not expect it to change anything, but because I am not 100% sure, I did not change that.

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

I'll probably try to remove both the value* and type* arguments and see if I have any regression (unless anyone is aware on some edge cases where this can be problematic).

WDYT?

Thanks,
Lancelot.

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

[Simon Marchi via Gdb-patches]

On 2022-02-03 06:10, Six, Lancelot wrote:
> [AMD Official Use Only]
> 
>> Hi Lancelot,
>>
>> Every time I look I keep finding nits, sorry :(
> 
> No worry.
> 
>> 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.
> 
> To be honest, I did have a similar impression about the function parameter.  Once you have the function symbol, you should be able to get a value containing the function address, so this parameter is probably also redundant.  The reason I did not make this change is because the function's address is obtained with a call to:
> 
>     struct value *function = read_var_value (func_symbol, NULL, frame);
> 
> Moving this call within get_return_value might mean we use a different FRAME argument (get_current_frame ()).  I do not expect it to change anything, but because I am not 100% sure, I did not change that.
> 
>> Do you think this change below (that builds on top of your patch) would work?  Tests gdb.*/*finish*.exp pass here.
> 
> I'll probably try to remove both the value* and type* arguments and see if I have any regression (unless anyone is aware on some edge cases where this can be problematic).
> 
> WDYT?

I wouldn't get rid of the `value *` argument, as you said this is
obtained using read_var_value and the current frame.  So if we don't
pass the `value *`, we would need to pass the frame, there's no point in
doing that change.

I'm just talking about removing the `type *` argument, which in both
callers of get_return_value, is obtained using:

  TYPE_TARGET_TYPE (SYMBOL_TYPE (function));

where `function` is the struct symbol that we also pass to
get_return_value.

Simon

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

[Lancelot SIX via Gdb-patches]

> -		  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);

Shouldn’t there be a call to PyErr_Clear () here? (I did forget it initially).

Lancelot

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

[Lancelot SIX via Gdb-patches]

Hi,

Here is an updated version which includes the changes proposed by Simon.
I have also made sure that PyErr_Clear is properly called in
bpfinishpy_init.

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.

Since the function return type can be deduced from the symbol remove the
value_type argument which becomes redundant.

No user visible change after this patch.

Tested on x86_64-linux.

Change-Id: Idf1279f1f7199f5022738a6679e0fa63fbd22edc
Co-authored-by: Simon Marchi <simon.marchi@polymtl.ca>
---
 gdb/infcmd.c                     | 11 +++----
 gdb/inferior.h                   | 10 ++++--
 gdb/python/py-finishbreakpoint.c | 55 +++++++++++++++++---------------
 3 files changed, 42 insertions(+), 34 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 994dd5b32a3..9a3214374e3 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1405,18 +1405,17 @@ 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)
 {
   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
@@ -1577,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 (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 ec0fb6e8b16..45de3c2d9c8 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -219,8 +219,14 @@ extern void detach_command (const char *, int);
 
 extern void notice_new_inferior (struct thread_info *, bool, int);
 
-extern struct value *get_return_value (struct value *function,
-				       struct type *value_type);
+/* 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.  */
+
+extern struct value *get_return_value (struct symbol *func_symbol,
+				       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 03bd4934506..2298c179ba8 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -40,13 +40,15 @@ struct finish_breakpoint_object
 {
   /* gdb.Breakpoint base class.  */
   gdbpy_breakpoint_object py_bp;
-  /* 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.Symbol object of the function finished by this breakpoint.  */
+  PyObject *func_symbol;
+
+  /* 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.  */
@@ -80,8 +82,8 @@ bpfinishpy_dealloc (PyObject *self)
   struct finish_breakpoint_object *self_bpfinish =
 	(struct finish_breakpoint_object *) 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);
 }
@@ -99,16 +101,17 @@ 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
     {
+      struct symbol *func_symbol =
+	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 (function, value_type);
+      struct value *ret =
+	get_return_value (func_symbol, function);
 
       if (ret)
 	{
@@ -165,7 +168,6 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   PyObject *internal = NULL;
   int internal_bp = 0;
   CORE_ADDR pc;
-  struct symbol *function;
 
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords,
 					&frame_obj, &internal))
@@ -239,15 +241,15 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
     }
 
   /* Find the function we will return from.  */
-  self_bpfinish->return_type = NULL;
-  self_bpfinish->function_value = NULL;
+  self_bpfinish->func_symbol = nullptr;
+  self_bpfinish->function_value = nullptr;
 
   try
     {
       if (get_frame_pc_if_available (frame, &pc))
 	{
-	  function = find_pc_function (pc);
-	  if (function != NULL)
+	  struct symbol *function = find_pc_function (pc);
+	  if (function != nullptr)
 	    {
 	      struct type *ret_type =
 		check_typedef (TYPE_TARGET_TYPE (SYMBOL_TYPE (function)));
@@ -255,14 +257,14 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	      /* 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);
+		  value *func_value = read_var_value (function, NULL, frame);
+		  self_bpfinish->function_value
+		    = value_to_value_object (func_value);
 		  PyErr_Clear ();
-		  func_value = read_var_value (function, NULL, frame);
-		  self_bpfinish->function_value =
-		      value_to_value_object (func_value);
+
+		  self_bpfinish->func_symbol
+		    = symbol_to_symbol_object (function);
 		  PyErr_Clear ();
 		}
 	    }
@@ -274,14 +276,15 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	 remain NULL.  */
     }
 
-  if (self_bpfinish->return_type == NULL || self_bpfinish->function_value == NULL)
+  if (self_bpfinish->func_symbol == nullptr
+      || self_bpfinish->function_value == nullptr)
     {
       /* Won't be able to compute return value.  */
-      Py_XDECREF (self_bpfinish->return_type);
+      Py_XDECREF (self_bpfinish->func_symbol);
       Py_XDECREF (self_bpfinish->function_value);
 
-      self_bpfinish->return_type = NULL;
-      self_bpfinish->function_value = NULL;
+      self_bpfinish->func_symbol = nullptr;
+      self_bpfinish->function_value = nullptr;
     }
 
   bppy_pending_object = &self_bpfinish->py_bp;

base-commit: 0d8cbc5f2fcbcb9eb207f12507fdfe04f3d3ae14
-- 
2.25.1

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

[Simon Marchi via Gdb-patches]

> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index 03bd4934506..2298c179ba8 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -40,13 +40,15 @@ struct finish_breakpoint_object
>  {
>    /* gdb.Breakpoint base class.  */
>    gdbpy_breakpoint_object py_bp;
> -  /* 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.Symbol object of the function finished by this breakpoint.  */
> +  PyObject *func_symbol;
> +
> +  /* 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;

Actually I would mention the "nullptr if no debug..." part in
func_symbol's documentation as well, since in the code we do check
whether func_symbol is nullptr.

LGTM with that fixed.

And I have a question I didn't bring up before, because I don't want to
increase the scope of this patch, but I am asking just for the sake of
discussing.  Do you know why we wrapped return_type, and now
func_symbol, in Python objects?  Any reason why we don't save the struct
type / struct symbol pointer directly?  To protect against the
underlying object disappearing while the FinishBreakpoint exists, maybe?

Simon

RE: [PATCH v5] gdb: add a symbol* argument to get_return_value

[Six, Lancelot via Gdb-patches]

[AMD Official Use Only]

> > +  /* 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;
>
> Actually I would mention the "nullptr if no debug..." part in func_symbol's documentation as well, since in the code we do check whether func_symbol is nullptr.
>
> LGTM with that fixed.
>
> And I have a question I didn't bring up before, because I don't want to increase the scope of this patch, but I am asking just for the sake of discussing.  Do you know why we wrapped return_type, and now func_symbol, in Python objects?  Any reason why we don't save the struct type / struct symbol pointer directly?  To protect against the underlying object disappearing while the FinishBreakpoint exists, maybe?

I guess it is out of consistency (but am really unsure). The return_value field is exposed as part of the python API (see bpfinishpy_get_returnvalue[1]), exposing the other fields is just a matter of adding the get function.  I also did ask this myself but since all fields of finish_breakpoint_object were not exposed I thought not exposing the symbol either was OK.  Having a quick look at the log, it seems that this did not change for quite some time (~10 years) so I am not sure we will get a definitive answer.

Lancelot

[1] https://www.sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/python/py-finishbreakpoint.c;h=03bd49345060fa48dec07298af3e4b4d50d775fa;hb=refs/heads/master#l63

>
> Simon
>

Re: [PATCH v3 2/2] gdb: Respect the DW_CC_nocall attribute

[Simon Marchi via Gdb-patches]

Lancelot pointed out that this was still the latest version of patch #2,
candidate for merging.

> diff --git a/gdb/testsuite/gdb.dwarf2/calling-convention.exp b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
> new file mode 100644
> index 00000000000..26b1f5eafd6
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/calling-convention.exp
> @@ -0,0 +1,97 @@
> +# Copyright 2022 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This testcase checks that if a function has the DW_AT_calling_convention
> +# attribute with the value DW_CC_nocall, then will not:

I think we are missing a word in the sentence above.

LGTM otherwise.

Simon

Re: [PATCH v3 2/2] gdb: Respect the DW_CC_nocall attribute

[Lancelot SIX via Gdb-patches]

Hi

>> +# This testcase checks that if a function has the DW_AT_calling_convention
>> +# attribute with the value DW_CC_nocall, then will not:
> 
> I think we are missing a word in the sentence above.

Fixed

> 
> LGTM otherwise.
> 

I pushed the series with:
- The final adjustments requested (on comments),
- Rebased on top of current master, with trivial adjustments to account 
for 5f9c5a63ce38b103f778f54394c6a3d43b7ade90 (gdb: remove SYMBOL_TYPE 
macro).

Thanks,
Lancelot.