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

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

[Lancelot SIX]

Hi,

This is a V2 for https://sourceware.org/pipermail/gdb-patches/2022-January/185301.html

The main change since V1 is an improvement of the messages shown to the
user (including the name of the function that is marked as not following
the calling convention).

In order to do this, this series move the get_function_name (COREADDR)
function from infcall.c where it currently resides (as a static
function) to a more generic place accessible to other parts of GDB.

I unfortunately did not find the obvious place where this function
should live.  The first commit of this series moves the declaration of the
function in gdb/symtab.h and its declaration in gdb/blockframe.c in
absence of anything better.  I am happy moving this somewhere else if
someone sees a better location in mind.

All feedback welcome.
Best,
Lancelot.

Lancelot SIX (2):
  gdb: Move most of get_function_name (COREADDR) in symtab.h
  gdb: Respect the DW_CC_nocall attribute

 gdb/blockframe.c                              | 29 ++++++
 gdb/gdbtypes.c                                | 11 +++
 gdb/gdbtypes.h                                |  9 ++
 gdb/infcall.c                                 | 43 +-------
 gdb/infcmd.c                                  | 19 ++++
 gdb/stack.c                                   | 16 ++-
 gdb/symtab.h                                  | 15 +++
 gdb/testsuite/gdb.dwarf2/calling-convention.c | 36 +++++++
 .../gdb.dwarf2/calling-convention.exp         | 97 +++++++++++++++++++
 9 files changed, 234 insertions(+), 41 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 v2 1/2] gdb: Move most of get_function_name (COREADDR) in symtab.h

[Lancelot SIX]

A later commit will need to be able to print the name of a function
given its address.  This functionality is currently implemented in a
static function in gdb/infcall.c.

In order to make this functionality available to other parts of GDB,
this commit moves it to gdb/symtab.h and gdb/blockframe.c.

No user visible change after this commit.

Tested on x86_64.

Change-Id: I3d32d64f298bdb49acc9e224fdeff33dcc7aa6cb
---
 gdb/blockframe.c | 29 +++++++++++++++++++++++++++++
 gdb/infcall.c    | 38 --------------------------------------
 gdb/symtab.h     | 15 +++++++++++++++
 3 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/gdb/blockframe.c b/gdb/blockframe.c
index 88595d4fcb0..bf557690f61 100644
--- a/gdb/blockframe.c
+++ b/gdb/blockframe.c
@@ -479,3 +479,32 @@ block_innermost_frame (const struct block *block)
 
   return NULL;
 }
+
+/* See symtab.h.  */
+
+const char *
+get_function_name (CORE_ADDR funaddr, char *buf, int buf_size)
+{
+  {
+    struct symbol *symbol = find_pc_function (funaddr);
+
+    if (symbol)
+      return symbol->print_name ();
+  }
+
+  {
+    /* Try the minimal symbols.  */
+    struct bound_minimal_symbol msymbol = lookup_minimal_symbol_by_pc (funaddr);
+
+    if (msymbol.minsym)
+      return msymbol.minsym->print_name ();
+  }
+
+  {
+    std::string tmp = string_printf (_(RAW_FUNCTION_ADDRESS_FORMAT),
+				     hex_string (funaddr));
+
+    gdb_assert (tmp.length () + 1 <= buf_size);
+    return strcpy (buf, tmp.c_str ());
+  }
+}
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 05cf18f0a7f..571e790fbee 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -44,11 +44,6 @@
 #include "gdbsupport/scope-exit.h"
 #include <list>
 
-/* If we can't find a function's name from its address,
-   we print this instead.  */
-#define RAW_FUNCTION_ADDRESS_FORMAT "at 0x%s"
-#define RAW_FUNCTION_ADDRESS_SIZE (sizeof (RAW_FUNCTION_ADDRESS_FORMAT) \
-				   + 2 * sizeof (CORE_ADDR))
 
 /* NOTE: cagney/2003-04-16: What's the future of this code?
 
@@ -374,39 +369,6 @@ error_call_unknown_return_type (const char *func_name)
 	     "cast the call to its declared return type"));
 }
 
-/* Fetch the name of the function at FUNADDR.
-   This is used in printing an error message for call_function_by_hand.
-   BUF is used to print FUNADDR in hex if the function name cannot be
-   determined.  It must be large enough to hold formatted result of
-   RAW_FUNCTION_ADDRESS_FORMAT.  */
-
-static const char *
-get_function_name (CORE_ADDR funaddr, char *buf, int buf_size)
-{
-  {
-    struct symbol *symbol = find_pc_function (funaddr);
-
-    if (symbol)
-      return symbol->print_name ();
-  }
-
-  {
-    /* Try the minimal symbols.  */
-    struct bound_minimal_symbol msymbol = lookup_minimal_symbol_by_pc (funaddr);
-
-    if (msymbol.minsym)
-      return msymbol.minsym->print_name ();
-  }
-
-  {
-    std::string tmp = string_printf (_(RAW_FUNCTION_ADDRESS_FORMAT),
-				     hex_string (funaddr));
-
-    gdb_assert (tmp.length () + 1 <= buf_size);
-    return strcpy (buf, tmp.c_str ());
-  }
-}
-
 /* All the meta data necessary to extract the call's return value.  */
 
 struct call_return_meta_info
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 55d1b772c45..aa951706c16 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1821,6 +1821,21 @@ extern bool find_function_entry_range_from_pc (CORE_ADDR pc,
 
 extern struct type *find_function_type (CORE_ADDR pc);
 
+/* If we can't find a function's name from its address,
+   we print this instead.  */
+#define RAW_FUNCTION_ADDRESS_FORMAT "at 0x%s"
+#define RAW_FUNCTION_ADDRESS_SIZE (sizeof (RAW_FUNCTION_ADDRESS_FORMAT) \
+				   + 2 * sizeof (CORE_ADDR))
+
+/* Fetch the name of the function at FUNADDR.
+
+   BUF is used to print FUNADDR in hex if the function name cannot be
+   determined.  It must be large enough to hold formatted result of
+   RAW_FUNCTION_ADDRESS_FORMAT.  */
+
+extern const char *get_function_name (CORE_ADDR funaddr, char *buf,
+				      int buf_size);
+
 /* See if we can figure out the function's actual type from the type
    that the resolver returns.  RESOLVER_FUNADDR is the address of the
    ifunc resolver.  */
-- 
2.25.1

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

[Lancelot SIX]

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                                |  9 ++
 gdb/infcall.c                                 |  5 +
 gdb/infcmd.c                                  | 19 ++++
 gdb/stack.c                                   | 16 ++-
 gdb/testsuite/gdb.dwarf2/calling-convention.c | 36 +++++++
 .../gdb.dwarf2/calling-convention.exp         | 97 +++++++++++++++++++
 7 files changed, 190 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..c8c45fa2817 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2852,4 +2852,13 @@ extern enum bfd_endian type_byte_order (const struct type *type);
 
 extern unsigned int overload_debug;
 
+/* The function 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 571e790fbee..f2afe0d35e0 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -775,6 +775,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 994dd5b32a3..186b1a2024a 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1419,6 +1419,25 @@ get_return_value (struct value *function, struct type *value_type)
   value_type = check_typedef (value_type);
   gdb_assert (value_type->code () != TYPE_CODE_VOID);
 
+  if (is_nocall_function (check_typedef (::value_type (function))))
+    {
+      CORE_ADDR funaddr = value_address (function);
+      const char *fname = nullptr;
+      char buf[RAW_FUNCTION_ADDRESS_SIZE];
+      if (funaddr != 0)
+	fname = get_function_name (funaddr, buf, sizeof (buf));
+
+      if (fname != nullptr)
+	warning (_("The function '%s' does not follow the target calling "
+		   "convention, cannot determine its returned value."),
+		 fname);
+      else
+	warning (_("Cannot determine the return value of a function which "
+		   "does not follow the target's calling convention."));
+
+      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..e95459d7767 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,15 @@ 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 ("The 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 +2824,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..48c71185a1f
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/calling-convention.c
@@ -0,0 +1,36 @@
+/* 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 ()
+{
+  asm ("foo_label: .globl foo_label");
+  return 42;
+}
+
+/* Dummy main function.  */
+
+int
+main()
+{
+  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..0e77396d553
--- /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 ".*The 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: The 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 v2 1/2] gdb: Move most of get_function_name (COREADDR) in symtab.h

[Bruno Larsen]

On 1/28/22 11:29, Lancelot SIX via Gdb-patches wrote:
> A later commit will need to be able to print the name of a function
> given its address.  This functionality is currently implemented in a
> static function in gdb/infcall.c.
> 
> In order to make this functionality available to other parts of GDB,
> this commit moves it to gdb/symtab.h and gdb/blockframe.c.
> 
> No user visible change after this commit.
> 
> Tested on x86_64.
> 
> Change-Id: I3d32d64f298bdb49acc9e224fdeff33dcc7aa6cb
> ---
>   gdb/blockframe.c | 29 +++++++++++++++++++++++++++++
>   gdb/infcall.c    | 38 --------------------------------------
>   gdb/symtab.h     | 15 +++++++++++++++
>   3 files changed, 44 insertions(+), 38 deletions(-)
> 
> diff --git a/gdb/blockframe.c b/gdb/blockframe.c
> index 88595d4fcb0..bf557690f61 100644
> --- a/gdb/blockframe.c
> +++ b/gdb/blockframe.c
> @@ -479,3 +479,32 @@ block_innermost_frame (const struct block *block)
>   
>     return NULL;
>   }
> +
> +/* See symtab.h.  */
> +
> +const char *
> +get_function_name (CORE_ADDR funaddr, char *buf, int buf_size)
> +{
> +  {
> +    struct symbol *symbol = find_pc_function (funaddr);
> +
> +    if (symbol)
> +      return symbol->print_name ();
> +  }
> +
> +  {
> +    /* Try the minimal symbols.  */
> +    struct bound_minimal_symbol msymbol = lookup_minimal_symbol_by_pc (funaddr);
> +
> +    if (msymbol.minsym)
> +      return msymbol.minsym->print_name ();
> +  }
> +
> +  {
> +    std::string tmp = string_printf (_(RAW_FUNCTION_ADDRESS_FORMAT),
> +				     hex_string (funaddr));
> +
> +    gdb_assert (tmp.length () + 1 <= buf_size);
> +    return strcpy (buf, tmp.c_str ());
> +  }
> +}
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 05cf18f0a7f..571e790fbee 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -44,11 +44,6 @@
>   #include "gdbsupport/scope-exit.h"
>   #include <list>
>   
> -/* If we can't find a function's name from its address,
> -   we print this instead.  */
> -#define RAW_FUNCTION_ADDRESS_FORMAT "at 0x%s"
> -#define RAW_FUNCTION_ADDRESS_SIZE (sizeof (RAW_FUNCTION_ADDRESS_FORMAT) \
> -				   + 2 * sizeof (CORE_ADDR))
>   
>   /* NOTE: cagney/2003-04-16: What's the future of this code?
>   
> @@ -374,39 +369,6 @@ error_call_unknown_return_type (const char *func_name)
>   	     "cast the call to its declared return type"));
>   }
>   
> -/* Fetch the name of the function at FUNADDR.
> -   This is used in printing an error message for call_function_by_hand.
> -   BUF is used to print FUNADDR in hex if the function name cannot be
> -   determined.  It must be large enough to hold formatted result of
> -   RAW_FUNCTION_ADDRESS_FORMAT.  */
> -
> -static const char *
> -get_function_name (CORE_ADDR funaddr, char *buf, int buf_size)
> -{
> -  {
> -    struct symbol *symbol = find_pc_function (funaddr);
> -
> -    if (symbol)
> -      return symbol->print_name ();
> -  }
> -
> -  {
> -    /* Try the minimal symbols.  */
> -    struct bound_minimal_symbol msymbol = lookup_minimal_symbol_by_pc (funaddr);
> -
> -    if (msymbol.minsym)
> -      return msymbol.minsym->print_name ();
> -  }
> -
> -  {
> -    std::string tmp = string_printf (_(RAW_FUNCTION_ADDRESS_FORMAT),
> -				     hex_string (funaddr));
> -
> -    gdb_assert (tmp.length () + 1 <= buf_size);
> -    return strcpy (buf, tmp.c_str ());
> -  }
> -}
> -
>   /* All the meta data necessary to extract the call's return value.  */
>   
>   struct call_return_meta_info
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 55d1b772c45..aa951706c16 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1821,6 +1821,21 @@ extern bool find_function_entry_range_from_pc (CORE_ADDR pc,
>   
>   extern struct type *find_function_type (CORE_ADDR pc);
>   
> +/* If we can't find a function's name from its address,
> +   we print this instead.  */
> +#define RAW_FUNCTION_ADDRESS_FORMAT "at 0x%s"
> +#define RAW_FUNCTION_ADDRESS_SIZE (sizeof (RAW_FUNCTION_ADDRESS_FORMAT) \
> +				   + 2 * sizeof (CORE_ADDR))
> +
> +/* Fetch the name of the function at FUNADDR.
> +
> +   BUF is used to print FUNADDR in hex if the function name cannot be
> +   determined.  It must be large enough to hold formatted result of
> +   RAW_FUNCTION_ADDRESS_FORMAT.  */
> +
> +extern const char *get_function_name (CORE_ADDR funaddr, char *buf,
> +				      int buf_size);
> +
>   /* See if we can figure out the function's actual type from the type
>      that the resolver returns.  RESOLVER_FUNADDR is the address of the
>      ifunc resolver.  */

Hi Lancelot,

LGTM, but I can't approve for pushing.

-- 
Cheers!
Bruno Larsen

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

[Bruno Larsen]

Hi Lancelot,

Thanks for looking at this. The general direction looks good, just a few minor nits.

On 1/28/22 11:29, Lancelot SIX via Gdb-patches wrote:
> 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                                |  9 ++
>   gdb/infcall.c                                 |  5 +
>   gdb/infcmd.c                                  | 19 ++++
>   gdb/stack.c                                   | 16 ++-
>   gdb/testsuite/gdb.dwarf2/calling-convention.c | 36 +++++++
>   .../gdb.dwarf2/calling-convention.exp         | 97 +++++++++++++++++++
>   7 files changed, 190 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..c8c45fa2817 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -2852,4 +2852,13 @@ extern enum bfd_endian type_byte_order (const struct type *type);
>   
>   extern unsigned int overload_debug;
>   
> +/* The function 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 571e790fbee..f2afe0d35e0 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -775,6 +775,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 994dd5b32a3..186b1a2024a 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1419,6 +1419,25 @@ get_return_value (struct value *function, struct type *value_type)
>     value_type = check_typedef (value_type);
>     gdb_assert (value_type->code () != TYPE_CODE_VOID);
>   
> +  if (is_nocall_function (check_typedef (::value_type (function))))
> +    {
> +      CORE_ADDR funaddr = value_address (function);
> +      const char *fname = nullptr;
> +      char buf[RAW_FUNCTION_ADDRESS_SIZE];
> +      if (funaddr != 0)
> +	fname = get_function_name (funaddr, buf, sizeof (buf));
> +
> +      if (fname != nullptr)
> +	warning (_("The function '%s' does not follow the target calling "
> +		   "convention, cannot determine its returned value."),
> +		 fname);
> +      else
> +	warning (_("Cannot determine the return value of a function which "
> +		   "does not follow the target's calling convention."));
> +
> +      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..e95459d7767 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,15 @@ 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 ("The 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 ());
> +

I feel like this if statement could have a set of { } around it. The assignment and call are big enough to be confusing on a quick glance

>   	  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 +2824,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..48c71185a1f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/calling-convention.c
> @@ -0,0 +1,36 @@
> +/* 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 ()
> +{
> +  asm ("foo_label: .globl foo_label");
> +  return 42;
> +}

According to the testsuite coding standards, this should be foo (void), instead of foo ().

> +
> +/* Dummy main function.  */
> +
> +int
> +main()
> +{
> +  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..0e77396d553
> --- /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
> +}

Following Keith Seitz's request to be more pedantic about TCL formatting, I think this should be

if {![runto_main]} {

> +
> +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 ".*The 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: The 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"]

Like I said, I can't approve for pushing, but LGTM.

-- 
Cheers!
Bruno Larsen

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

[Simon Marchi]

On 2022-01-31 08:19, Bruno Larsen via Gdb-patches wrote:
> Hi Lancelot,
> 
> Thanks for looking at this. The general direction looks good, just a few minor nits.

Some more comments.

Git gave me this warning when applying, maybe something to fix:

Applying: gdb: Respect the DW_CC_nocall attribute
.git/rebase-apply/patch:148: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index 994dd5b32a3..186b1a2024a 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -1419,6 +1419,25 @@ get_return_value (struct value *function, struct type *value_type)
>>     value_type = check_typedef (value_type);
>>     gdb_assert (value_type->code () != TYPE_CODE_VOID);
>>   +  if (is_nocall_function (check_typedef (::value_type (function))))
>> +    {
>> +      CORE_ADDR funaddr = value_address (function);
>> +      const char *fname = nullptr;
>> +      char buf[RAW_FUNCTION_ADDRESS_SIZE];
>> +      if (funaddr != 0)
>> +    fname = get_function_name (funaddr, buf, sizeof (buf));
>> +
>> +      if (fname != nullptr)

My understand is that fname will be nullptr if funaddr is 0.  Can you
explain when funaddr is 0, and what that means?  Just trying to
understand how we can get there.

My other comments about this would be:

 - One caller of this command, finish_command_fsm::should_stop, starts
   from a struct symbol.  Getting the name from that is easy, you can
   simply call 'print_name' on it (as shown by your change in
   return_command).
 - The other caller of get_return_value is bpfinishpy_pre_stop_hook.
   bpfinishpy_pre_stop_hook does not have access to a struct symbol
   directly, but maybe it could, if we saved the symbol used to set
   finish_breakpoint_object::function_value, bpfinishpy_init.

So if we can always have access to the symbol somehow, maybe we can
avoid using get_function_name in get_return_value.

>> +    warning (_("The function '%s' does not follow the target calling "

I think that when naming the thing, you should omit the "The" in front.
For example, you'd say "President Biden", or "The president", but not
"The president Biden".  Maybe I'm wrong, I'm not a native english
speaker.

Simon

RE: [PATCH v2 2/2] gdb: Respect the DW_CC_nocall attribute

[Six, Lancelot]

[AMD Official Use Only]

Hi,

> >         else if (thisfun != NULL)
> >       {
> > +       if (is_nocall_function (check_typedef (value_type (function))))
> > +         query_prefix
> > +           = string_printf ("The 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 ());
> > +
>
> I feel like this if statement could have a set of { } around it. The assignment and call are big enough to be confusing on a quick glance

Will do.

> > +
> > +int
> > +foo ()
> > +{
> > +  asm ("foo_label: .globl foo_label");
> > +  return 42;
> > +}
>
> According to the testsuite coding standards, this should be foo (void), instead of foo ()

OK

> > +if ![runto_main] {
> > +    return -1
> > +}
>
> Following Keith Seitz's request to be more pedantic about TCL formatting, I think this should be
> 
> if {![runto_main]} {

Will change that as well.  Thanks for pointing this out.

Thanks for the feedbacks.

Best,
Lancelot.

> Like I said, I can't approve for pushing, but LGTM.
>
> --
> Cheers!
> Bruno Larsen

RE: [PATCH v2 2/2] gdb: Respect the DW_CC_nocall attribute

[Six, Lancelot]

[AMD Official Use Only]

> Some more comments.
>
> Git gave me this warning when applying, maybe something to fix:
> 
> Applying: gdb: Respect the DW_CC_nocall attribute
> .git/rebase-apply/patch:148: new blank line at EOF.
> +
> warning: 1 line adds whitespace errors.

Thanks for pointing this out, I'll look into it before V3.

>>> +      if (funaddr != 0)
>>> +    fname = get_function_name (funaddr, buf, sizeof (buf));
>>> +
>>> +      if (fname != nullptr)
>
> My understand is that fname will be nullptr if funaddr is 0.  Can you explain when funaddr is 0, and what that means?  Just trying to understand how we can get there.

Yes.

I do not think this is possible, but I am not sure I can guaranty it (0 is still a value return value of value_address).   Anyway, given your next comment, if I can get a symbol* down to this point this question will not have to be addressed, and it will simplify things.

>
> My other comments about this would be:
>
> - One caller of this command, finish_command_fsm::should_stop, starts
>    from a struct symbol.  Getting the name from that is easy, you can
>   simply call 'print_name' on it (as shown by your change in
>   return_command).
> - The other caller of get_return_value is bpfinishpy_pre_stop_hook.
>   bpfinishpy_pre_stop_hook does not have access to a struct symbol
>   directly, but maybe it could, if we saved the symbol used to set
>   finish_breakpoint_object::function_value, bpfinishpy_init.
>
> So if we can always have access to the symbol somehow, maybe we can avoid using get_function_name in get_return_value.

This was my first approach as well.  However, I did this to avoid diving into changing the python stuff to make the symbol available to bpfinishpy_pre_stop_hook.  I'll look a bit more at what it takes to have the symbol passed down to this point.  This might very well be easier (and avoid having to rely on the first patch).

>>> +    warning (_("The function '%s' does not follow the target calling "
>
> I think that when naming the thing, you should omit the "The" in front.
> For example, you'd say "President Biden", or "The president", but not "The president Biden".  Maybe I'm wrong, I'm not a native english speaker.

I am no native English speaker, so I do not know what the proper wording is either 😉 I'll update this.

Thanks for the comment,
Lancelot.

> Simon

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

[Simon Marchi]

On 2022-01-31 09:34, Six, Lancelot wrote:
> This was my first approach as well.  However, I did this to avoid diving into changing the python stuff to make the symbol available to bpfinishpy_pre_stop_hook.  I'll look a bit more at what it takes to have the symbol passed down to this point.  This might very well be easier (and avoid having to rely on the first patch).

Hmm true, the only remaining use of get_function_name would be in
infcall.c then, so get_function_name could stay static in that file.

Simon