Re: [PATCH] Fortran function calls with arguments

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 2019-01-21 10:05 a.m., Richard Bunt wrote:
> Prior to this patch, calling functions on the inferior with arguments and
> then using these arguments within a function resulted in an invalid
> memory access. This is because Fortran arguments are typically passed as
> pointers to values.
> 
> It is possible to call Fortran functions, but memory must be allocated in
> the inferior, so a pointer can be passed to the function, and the
> language must be set to C to enable C-style casting. This is cumbersome
> and not a pleasant debug experience.
> 
> This patch implements the GNU Fortran argument passing conventions with
> caveats. Firstly, it does not handle the VALUE attribute as there is
> insufficient DWARF information to determine when this is the case.
> Secondly, functions with optional parameters can only be called with all
> parameters present. Both these cases are marked as KFAILS in the test.
> 
> Since the GNU Fortran argument passing convention has been implemented,
> there is no guarantee that this patch will work correctly, in all cases,
> with other compilers.
> 
> Despite these limitations, this patch improves the ease with which
> functions can be called in many cases, without taking away the existing
> approach of calling with the language set to C.
> 
> It should be noted that bugs were found with printing complex numbers
> while developing this patch. However, fixing this issue was considered
> outside the scope of this patch. The test case for calling functions with
> complex numbers should remain as a KFAIL.
> 
> Regression tested on x86_64, aarch64 and POWER9 with GCC 7.3.0.
> Regression tested with Ada on x86_64.
> Regression tested with native-extended-gdbserver target board.

Hi Richard,

Thanks for the patch.  I don't know Fortran, so I can't assess whether the
behavior you implement is the right one.  If other maintainers have this
knowledge, they are welcome to complement this review.  Otherwise, I am
ready to trust you on that matter.

Here are a few more high level comments in the mean time.

> gdb/ChangeLog:
> 2018-10-27  Richard Bunt  <richard.bunt@arm.com>
> 	Dirk Schubert  <dirk.schubert@arm.com>
> 	Chris January  <chris.january@arm.com>
> 
> 	* gdb/eval.c (evaluate_subexp_standard): Call Fortran argument
> 	wrapping logic.
> 	* gdb/f-lang.c (struct value): A value which can be passed into a
> 	Fortran function call.
> 	(fortran_argument_convert): Wrap Fortran arguments in a pointer
> 	where appropriate.
> 	(struct type): Value ready for a Fortran function call.
> 	(fortran_preserve_arg_pointer): Undo check_typedef, the pointer
> 	is needed.
> 	* gdb/f-lang.h (fortran_argument_convert): Declaration.
> 	(fortran_preserve_arg_pointer): Declaration.
> 	* gdb/infcall.c (value_arg_coerce): Call Fortran argument
> 	logic.

Remove the gdb/ prefixes (the filenames should be relative to the ChangeLog
file itself).

> 
> gdb/testsuite/ChangeLog:
> 2018-10-27  Richard Bunt  <richard.bunt@arm.com>
> 
> 	* gdb/testsuite/gdb.fortran/function-calls.exp: New file.
> 	* gdb/testsuite/gdb.fortran/function-calls.f90: New test.

Same here.

> ---
>  gdb/eval.c                                   |  13 +-
>  gdb/f-lang.c                                 |  48 ++++++
>  gdb/f-lang.h                                 |   5 +-
>  gdb/infcall.c                                |   5 +-
>  gdb/testsuite/gdb.fortran/function-calls.exp | 104 ++++++++++++
>  gdb/testsuite/gdb.fortran/function-calls.f90 | 242 +++++++++++++++++++++++++++
>  6 files changed, 414 insertions(+), 3 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.fortran/function-calls.exp
>  create mode 100644 gdb/testsuite/gdb.fortran/function-calls.f90
> 
> diff --git a/gdb/eval.c b/gdb/eval.c
> index 47d08a656c0229ace5f2004f73eabb30c90a96a8..083e9531e103ba995910c26c12e4961024dab3b8 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -1987,7 +1987,18 @@ evaluate_subexp_standard (struct type *expect_type,
>  	  argvec[0] = arg1;
>  	  tem = 1;
>  	  for (; tem <= nargs; tem++)
> -	    argvec[tem] = evaluate_subexp_with_coercion (exp, pos, noside);
> +	    {
> +	      argvec[tem] = evaluate_subexp_with_coercion (exp, pos, noside);
> +	      /* Arguments in Fortran are passed by address.  Coerce the
> +		 arguments here rather than in value_arg_coerce as otherwise
> +		 the call to malloc to place the non-lvalue parameters in
> +		 target memory is hit by this Fortran specific logic.  This
> +		 results in malloc being called with a pointer to an integer
> +		 followed by an attempt to malloc the arguments to malloc in
> +		 target memory.  Infinite recursion ensues.  */
> +	      argvec[tem] = fortran_argument_convert (argvec[tem],
> +		TYPE_FIELD_ARTIFICIAL (value_type (arg1), tem-1));

Spaces are the minus sign.

Normally, when wrapping arguments, the wrapped ones should be aligned with the
first one (according to the GNU style).  It's a bit hard here because of the length
of the expression and the fact that we are pretty nested.  To solve it, I would do:

              bool is_artifical
                = TYPE_FIELD_ARTIFICIAL (value_type (arg1), tem - 1);
	      argvec[tem] = fortran_argument_convert (argvec[tem], is_artificial);

> +	    }
>  	  argvec[tem] = 0;	/* signal end of arglist */
>  	  if (noside == EVAL_SKIP)
>  	    return eval_skip_value (exp);
> diff --git a/gdb/f-lang.c b/gdb/f-lang.c
> index 4ff828ba75300ae5667313e74cfb9fcaffa9df2f..0c3118a511d55b20cb287a9e69fccb476579391c 100644
> --- a/gdb/f-lang.c
> +++ b/gdb/f-lang.c
> @@ -27,6 +27,7 @@
>  #include "parser-defs.h"
>  #include "language.h"
>  #include "varobj.h"
> +#include "gdbcore.h"
>  #include "f-lang.h"
>  #include "valprint.h"
>  #include "value.h"
> @@ -371,3 +372,50 @@ _initialize_f_language (void)
>  {
>    f_type_data = gdbarch_data_register_post_init (build_fortran_types);
>  }
> +
> +/* Ensures that function argument VALUE is in the appropriate form to
> +   pass to a Fortran function.  Returns a possibly new value that should
> +   be used instead of VALUE.
> +
> +   When IS_ARTIFICIAL is true this indicates an artificial argument,
> +   e.g. hidden string lengths which the GNU Fortran argument passing
> +   convention specifies as being passed by value.
> +
> +   When IS_ARTIFICIAL is false, the argument is passed by pointer.  If the
> +   value is already in target memory then return a value that is a pointer
> +   to VALUE.  If VALUE is not in memory (e.g. an integer literal), allocate
> +   space in the target, copy VALUE in, and return a pointer to the in
> +   memory copy.  */

Please move the comment to f-lang.h, and put this here:

/* See f-lang.h.  */

> +struct value *
> +fortran_argument_convert (struct value *value, const bool is_artificial)
> +{
> +  if (!is_artificial)
> +    {
> +      if (VALUE_LVAL (value) == not_lval
> +	  || VALUE_LVAL (value) == lval_internalvar)

Just wondering, have you considered all lval_types here?  If you were to pass
a variable that is in a register (lval_register) or composed of multiple pieces
(lval_computed), I guess we would need to allocate them too.  Not sure about the
other ones.  In fact, everything that is not lval_memory would likely hit this
assert in value_addr:

  if (VALUE_LVAL (arg1) != lval_memory)
    error (_("Attempt to take address of value not located in memory."));

So maybe this should be

    if (VALUE_LVAL (value) != lval_memory)

?

> +	{
> +	  struct value *val = value_copy (value);
> +	  const int length = TYPE_LENGTH (value_type (val));
> +	  const CORE_ADDR addr
> +	    = value_as_long (value_allocate_space_in_inferior (length));
> +	  VALUE_LVAL (val) = lval_memory;
> +	  set_value_address (val, addr);
> +	  write_memory (addr, value_contents (val), length);
> +	  return value_addr (val);

> +	}
> +      else
> +	return value_addr (value);
> +    }
> +    return value;
> +}
> +
> +/* All arguments in Fortran are pointers.  If it is a pointer, preserve it so
> +   that the address is passed rather than the value.  */

Same (move comment to f-lang.h).  Also, can you describe what ARG and TYPE are?
How are they related?

> +struct type *
> +fortran_preserve_arg_pointer (struct value *arg,
> +			      struct type *type)
> +{
> +  if (TYPE_CODE (value_type (arg)) == TYPE_CODE_PTR)
> +    return value_type (arg);
> +  return type;
> +}
> diff --git a/gdb/f-lang.h b/gdb/f-lang.h
> index cab06f9806d4bed15f73b4c8da717d1ede2a7d5f..bccf082dae4c306c57b49f6a91306fc76ba3de54 100644
> --- a/gdb/f-lang.h
> +++ b/gdb/f-lang.h
> @@ -78,4 +78,7 @@ struct builtin_f_type
>  
>  /* Return the Fortran type table for the specified architecture.  */
>  extern const struct builtin_f_type *builtin_f_type (struct gdbarch *gdbarch);
> -
> +extern struct value *fortran_argument_convert (struct value *value,
> +					       const bool is_artificial);
> +extern struct type *fortran_preserve_arg_pointer (struct value *arg,
> +						  struct type *type);
> diff --git a/gdb/infcall.c b/gdb/infcall.c
> index 14b0cbc716907ac790fd9c4fee89172fa2a33020..367048b7411900486855ddac5949aa655ea77853 100644
> --- a/gdb/infcall.c
> +++ b/gdb/infcall.c
> @@ -33,6 +33,7 @@
>  #include "command.h"
>  #include "dummy-frame.h"
>  #include "ada-lang.h"
> +#include "f-lang.h"
>  #include "gdbthread.h"
>  #include "event-top.h"
>  #include "observable.h"
> @@ -129,7 +130,7 @@ show_unwind_on_terminating_exception_p (struct ui_file *file, int from_tty,
>  }
>  
>  /* Perform the standard coercions that are specified
> -   for arguments to be passed to C or Ada functions.
> +   for arguments to be passed to C, Ada or Fortran functions.
>  
>     If PARAM_TYPE is non-NULL, it is the expected parameter type.
>     IS_PROTOTYPED is non-zero if the function declaration is prototyped.
> @@ -148,6 +149,8 @@ value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
>    /* Perform any Ada-specific coercion first.  */
>    if (current_language->la_language == language_ada)
>      arg = ada_convert_actual (arg, type);
> +  else if (current_language->la_language == language_fortran)
> +    type = fortran_preserve_arg_pointer (arg, type);

The comment just above would need to be updated to mention Fortran.

>  
>    /* Force the value to the target if we will need its address.  At
>       this point, we could allocate arguments on the stack instead of
> diff --git a/gdb/testsuite/gdb.fortran/function-calls.exp b/gdb/testsuite/gdb.fortran/function-calls.exp
> new file mode 100644
> index 0000000000000000000000000000000000000000..e75336651b3947534e6a11766d4fdc5fa9b87d5d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.fortran/function-calls.exp
> @@ -0,0 +1,104 @@
> +# Copyright 2019 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/> .
> +
> +# Exercise passing and returning arguments in Fortran. This test case
> +# is based on the GNU Fortran Argument passing conventions.
> +
> +if {[skip_fortran_tests]} { return -1 }
> +
> +standard_testfile ".f90"
> +
> +if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug f90}]} {
> +    return -1
> +}
> +
> +if {![runto [gdb_get_line_number "post_init"]]} then {
> +    perror "couldn't run to breakpoint post_init"
> +    continue
> +}
> +
> +# Use inspired by gdb.base/callfuncs.exp.
> +gdb_test_no_output "set unwindonsignal on"
> +
> +# Baseline: function and subroutine call with no arguments.
> +gdb_test "p no_arg()" " = .TRUE."
> +gdb_test_no_output "call no_arg_subroutine()"
> +
> +# Argument class: literal, inferior variable, convenience variable,
> +# function call return value, function.
> +# Paragraph 3: Variables are passed by reference.
> +gdb_test "p one_arg(.TRUE.)" " = .TRUE."
> +gdb_test "p one_arg(untrue)" " = .FALSE."
> +gdb_test_no_output "set \$var = .FALSE."
> +gdb_test "p one_arg(\$var)" " = .FALSE."
> +gdb_test "p one_arg(one_arg(.TRUE.))" " = .TRUE."
> +gdb_test "p one_arg(one_arg(.FALSE.))" " = .FALSE."
> +gdb_test_no_output "call run(no_arg_subroutine)"
> +
> +# Return: constant.
> +gdb_test "p return_constant()" " = 17"
> +# Return derived type and call a function in a module.
> +gdb_test "p derived_types_and_module_calls::build_cart(7,8)" \
> +	 " = \\\( x = 7, y = 8 \\\)"
> +
> +# Two hidden arguments. 1. returned string and 2. string length.
> +# Paragraph 1.
> +gdb_test "p return_string(returned_string_debugger, 40)" ""
> +gdb_test "p returned_string_debugger" "'returned in hidden first argument       '"
> +
> +# Argument type: real(kind=4), complex, array, pointer, derived type,
> +# derived type with allocatable, nested derived type.
> +# Paragraph 4: pointer.
> +gdb_test "p pointer_function(int_pointer)" " = 87"
> +# Paragraph 4: array.
> +gdb_test "call array_function(integer_array)" " = 17"
> +gdb_test "p derived_types_and_module_calls::pass_cart(c)" \
> +	 " = \\\( x = 2, y = 4 \\\)"
> +# Allocatable elements in a derived type. Technical report ISO/IEC 15581.
> +gdb_test "p derived_types_and_module_calls::pass_cart_nd(c_nd)" " = 4"
> +gdb_test "p derived_types_and_module_calls::pass_nested_cart(nested_c)" \
> +	  "= \\\( d = \\\( x = 1, y = 2 \\\), z = 3 \\\)"
> +# Result within some tolerance.
> +gdb_test "p real4_argument(real4)" " = 3.${decimal}"
> +
> +# Printing complex numbers through GDB doesn't work in general.
> +# Paragraph 2.
> +setup_kfail "gdb/NNNN" *-*-*
> +gdb_test "p complex_argument(fft)" " = (2.${decimal},3.${decimal})"
> +
> +# Function with optional arguments.
> +# Paragraph 10: Option reference arguments.
> +gdb_test "p sum_some(1,2,3)" " = 6"
> +# There is currently no mechanism to call a function without all
> +# optional parameters present.
> +setup_kfail "gdb/NNNN" *-*-*
> +gdb_test "p sum_some(1,2)" " = 3"
> +
> +# Paragraph 10: optional value arguments. There is insufficient DWARF
> +# information to reliably make this case work.
> +setup_kfail "gdb/NNNN" *-*-*
> +gdb_test "p one_arg_value(10)" " = 10"
> +
> +# DW_AT_artificial formal parameters must be passed manually. This
> +# assert will fail if the length of the string is wrapped in a pointer.
> +# Paragraph 7: Character type.
> +gdb_test "p hidden_string_length('arbitrary string', 16)" " = 16"
> +
> +# Several arguments.
> +gdb_test "p several_arguments(2, 3, 5)" " = 10"
> +gdb_test "p mix_of_scalar_arguments(5, .TRUE., 3.5)" " = 9"
> +
> +# Calling other functions: Recursive call.
> +gdb_test "p fibonacci(6)" " = 8"

Could you file bugs for the various setup_kfail?

Thanks,

Simon