Re: [Patch] [v3] Fortran: Fix Bind(C) Array-Descriptor Conversion (Move to Front-End Code)

[Harald Anlauf <anlauf@gmx.de>]

Hi Tobias,

Am 13.10.21 um 18:01 schrieb Tobias Burnus:
> Dear all,
>
> a minor update [→ v3].

this has become an impressive work.

> I searched for XFAIL in Sandra's c-interop/ and found
> two remaining true** xfails, now fixed:
>
> - gfortran.dg/c-interop/typecodes-scalar-basic.f90
>    The conversion of scalars of type(c_ptr) was mishandled;
>    fixed now; the fix did run into issues converting a string_cst,
>    which has also been fixed.
>
> - gfortran.dg/c-interop/fc-descriptor-7.f90
>    this one uses TRANSPOSE which did not work [now mostly* does]
>    → PR fortran/101309 now also fixed.
>
> I forgot what the exact issue for the latter was. However, when
> looking at the testcase and extending it, I did run into the
> following issue - and at the end the testcase does now pass.
> The issue I had was that when a contiguous check was requested
> (i.e. only copy in when needed) it failed to work, when
> parmse->expr was (a pointer to) a descriptor. I fixed that and
> now most* things work.
>
> OK for mainline? Comments? Suggestions? More PRs which fixes
> this patch? Regressions? Test results?

Doesn't break my own codes so far.

If nobody else responds within the next days, assume an OK
from my side.

This will also provide Gerhard with a new playground.  ;-)

Thanks for the patch!

Harald

> Tobias
>
> PS: I intent to commit this patch to the OG11 (devel/omp/gcc-11)
> branch, in case someone wants to test it there.
>
> PPS: Nice to have an extensive testcase suite - kudos to Sandra
> in this case. I am sure Gerald will find more issues and once
> it is in, I think I/we have to check some PRs + José's patches
> whether for additional testcases + follow-up fixes.
>
> (*) I write most as passing a (potentially) noncontiguous
> assumed-rank array to a CONTIGUOUS assumed-rank array causes
> an ICE as the scalarizer does not handle dynamic ranks alias
> expr->rank == -1 / ss->dimen = -1.
> I decided that that's a separate issue and filled:
> https://gcc.gnu.org/PR102729
> BTW, my impression is that fixing that PR might fix will solve
> the trans*.c part of https://gcc.gnu.org/PR102641 - but I have
> not investigated.
>
> (**) There are still some 'xfail' in comments (outside dg-*)
> whose tests now pass. And those where for two bugs in the same
> statement, only one is reported - and the other only after fixing
> the first one, which is fine.
>
> On 09.10.21 23:48, Tobias Burnus wrote:
>> Hi all,
>>
>> attached is the updated version. Changes:
>> * Handle noncontiguous arrays – with BIND(C), (g)Fortran needs to make it
>>   contiguous in the caller but also handle noncontiguous in the callee.
>> * Fixes/handle 'character(len=*)' with BIND(C); those always use an
>>   array descriptor - also with explicit-size and assumed-size arrays
>> * Fixed a bunch of bugs, found when writing extensive testcases.
>> * Fixed type(*) handling - those now pass properly type and elem_len
>>   on when calling a new function (bind(C) or not).
>>
>> Besides adding the type itself (which is rather straight forward),
>> this patch only had minor modifications – and then the two big
>> conversion functions.
>>
>> While it looks intimidating, it should be comparably simple to
>> review as everything is on one place and hopefully sufficiently
>> well documented.
>>
>> OK – for mainline?  Other comments? More PRs which are fixed?
>> Issues not yet fixed (which are inside the scope of this patch)?
>>
>> (If this patch is too long, I also have a nine-day old pending patch
>> at https://gcc.gnu.org/pipermail/gcc-patches/2021-October/580624.html )
>>
>> Tobias
>>
>> PS: The following still applies.
>>
>> On 06.09.21 12:52, Tobias Burnus wrote:
>>> gfortran's internal array descriptor (xgfc descriptor) and
>>> the descriptor used with BIND(C) (CFI descriptor, ISO_Fortran_binding.h
>>> of TS29113 / Fortran 2018) are different. Thus, when calling a BIND(C)
>>> procedure the gfc descriptor has to be converted to cfi – and when a
>>> BIND(C) procedure is implemented in Fortran, the argument has to be
>>> converted back from CFI to gfc.
>>>
>>> The current implementation handles part in the FE and part in
>>> libgfortran,
>>> but there were several issues, e.g. PR101635 failed due to alias issues,
>>> debugging wasn't working well, uninitialized memory was used in some
>>> cases
>>> etc.
>>>
>>> This patch now moves descriptor conversion handling to the FE – which
>>> also
>>> can make use of compile-time knowledge, useful both for diagnostic
>>> and to
>>> optimize the code.
>>>
>>> Additionally:
>>> - Some cases where TS29113 mandates that the array descriptor should be
>>>   used now use the array descriptor, in particular character scalars
>>> with
>>>   'len=*' and allocatable/pointer scalars.
>>> - While debugging the alias issue, I simplified 'select rank'. While
>>> some
>>>   special case is needed for assumed-shape arrays, those cannot
>>> appear when
>>>   the argument has the pointer or allocatable attribute. That's not
>>> only a
>>>   missed optimization, pointer/allocatable arrays can also be NULL -
>>> such
>>>   that accessing desc->dim.ubound[rank-1] can be uninitialized memory
>>> ...
>>>
>>> OK?  Comments? Suggestions?
>>>
>>>  * * *
>>>
>>> For some more dumps, see the discussion about the alias issue at:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578364.html
>>> ("[RFH] ME optimizes variable assignment away / Fortran bind(C)
>>> descriptor conversion")
>>> plus the original emails:
>>> - https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578271.html
>>> - and (correct dump)
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578274.html
>>>
>>> Debugging - not ideal but not too bad either. For
>>>   subroutine f(x) bind(C)
>>>     integer :: x(:)
>>> with an uninitialized size-4 array as argument:
>>>
>>> m::f (_x=...) at foo4.f90:3
>>> 3       subroutine f(x) bind(C)
>>> (gdb) p x
>>> Cannot access memory at address 0x38
>>> (gdb) p _x
>>> $6 = ( base_addr = 0x7fffffffe2c0, elem_len = 4, version = 1, rank =
>>> 1 '\001', attribute = 2 '\002', type = 1025, dim = (( lower_bound =
>>> 0, extent = 5, sm = 4 )) )
>>> (gdb) s
>>> 5         x(1) = 5
>>> (gdb) p x
>>> $7 = (0, 0, 0, -670762413, 0)
>>>
>>>
>>> Tobias
>>>
>>> PS: This patch fixes but not necessarily fully the following PRs:
>>> PR fortran/102086 - [F2008][TS29113] Accepts invalid scalar TYPE(*)
>>> as actual argument to assumed-rank
>>> PR fortran/92189 - Fortran-written bind(C) function with allocatable
>>> argument does not update C descriptor on exit
>>> PR fortran/92621 - Problems with memory handling with allocatable
>>> intent(out) arrays with bind(c)
>>> PR fortran/101308 - Bind(C): gfortran does not create C descriptors
>>> for scalar pointer/allocatable arguments
>>> PR fortran/101635 - FAIL: gfortran.dg/PR93963.f90 – alias-handling
>>> issue with BIND(C)'s _gfortran_cfi_desc_to_gfc_desc
>>> PR fortran/92482 - BIND(C) with array-descriptor mishandled for type
>>> character
>>> and possibly some more.
>>>
>>> PPS: I should add some additional testcases – I try to do this as
>>> Part 2 of this patch.
>>>
>>> PPPS: Once the patch is in, some audit needs to be done which parts
>>> of those PRs remain
>>> as follow-up work. I think some still existing issues are covered by
>>> José's pending
>>> patches + for those which are now fixed, the testcase might still be
>>> added.
>>>
>>> -----------------
>>> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße
>>> 201, 80634 München; Gesellschaft mit beschränkter Haftung;
>>> Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der
>>> Gesellschaft: München; Registergericht München, HRB 106955
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201,
> 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer:
> Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München;
> Registergericht München, HRB 106955