Re: [PATCH] Fortran function calls with arguments

[Simon Marchi <simon.marchi@ericsson.com>]

On 2019-01-29 9:47 a.m., Richard Bunt wrote:
> 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.

Thanks!

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

lval_computed is not something you can simply trigger from the command line, I believe.

It's used when the compiler optimizes and decides to place parts of a value in different
places.  For example (maybe a bit exaggerated), for an 8 bytes value, it could put the
first 3 in memory, the next 4 in a register, and the last one is optimized out.  The location
of this value will be described by DWARF "pieces", which is essentially a sequence of
DWARF opcodes describing sequentially where all the pieces are.  The value in GDB with be
lval_computed.  You can check section "2.6.1.2 Composite Location Descriptions" of DWARF5.pdf
if you want to know more.

It's a bit tricky to test, because you need to need to generate predictable DWARF pieces.
The best way is probably to write the DWARF by hand, as in testsuite/gdb.dwarf2/var-access.exp.

Maybe it would be a bit overkill to include such a test in your test case, I'll let you decide
if it's worth it.

lval_xcallable refers to XMethods:

  https://sourceware.org/gdb/onlinedocs/gdb/Xmethods-In-Python.html#Xmethods-In-Python

For lval_internalvar_component, the comment says "Part of a gdb internal variable (structure field)",
so I assume you need to have a structure in a GDB internal variable (set $foo = ...) and pass a field
of $foo to the function.

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

Yes, that's fine.

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


Thanks,

Simon