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

[Bruno Larsen <blarsen@redhat.com>]

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