Re: [PATCH] Fortran function calls with arguments

[Richard Bunt <Richard.Bunt@arm.com>]

Hi Simon,

Many thanks for the review comments.

On 1/21/19 10:03 PM, Simon Marchi wrote:
> 
> 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.

My definition of correct for this patch has been gleaned from two sources:

1. The GNU Fortran argument passing conventions which can be found here:

https://gcc.gnu.org/onlinedocs/gfortran/Argument-passing-conventions.html

I've attempted to capture as much of this page as possible in the test case.

2. Valid source code. A user should be able to paste their Fortran function
call from their source code into GDB and receive the expected result.

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

I've addressed all style and code/comment repositioning issues for v2 of
the patch.

> 
>> +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)
> 
> ?
> 

Yes, this makes more sense. That error is indeed hit if a function call is made
where one of the arguments is a register. v2 of this patch now works in this case.

I am not able to see how the user would be able to express the other types of
lvalue (e.g. computed) from the user interface. Do you have any pointers on
this? As it would be useful to add this to the test case if it is indeed possible
for the user to provoke this.

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

> 
> Could you file bugs for the various setup_kfail?
> 

I've filed the bug report for the optional attribute here:

https://sourceware.org/bugzilla/show_bug.cgi?id=24147

However, I think the second bug report should wait until the point this
patch passes review, as the bug only exists in GDB HEAD with this patch applied.
I've included what the bug report would consist of below to explain why this
bug exists.

Fortran allows function parameters to be tagged with a "value" attribute which
indicates that an argument is to be passed by value, rather than the default
of by reference. For example:

{noformat}
integer(kind=4) function one_arg_value(x)
    integer(kind=4), value :: x
    one_arg_value = x
end function
{noformat}

p one_arg_value(10)
$19 = 6318016

Garbage is returned when this function is called with a version of GDB which
has this patch applied. Most likely this is the location of 10 in the inferior
placed into a 4-byte integer, since GDB is passing a pointer to this value
rather than the value. NOTE: This use case will work on GDB without this patch
applied, as it flips the default calling convention to that outlined in 
https://gcc.gnu.org/onlinedocs/gfortran/Argument-passing-conventions.html. This
way functions calls will mostly work out of the box rather than mostly not work.

If this function call was working as expected it would return 10.

This was tested with:
* A build of 36c25ffa1ab5d6d5ee0fa3fc32f128a58e78e7a2 + the following patch 
from the mailing list https://sourceware.org/ml/gdb-patches/2019-01/msg00448.html
* On Ubuntu 16.04.
* On x86 64.
* Fortran programs compiled with GCC 8.2.


Many thanks,

Rich