Hi Tobias,
I can only echo Harald's comment that this is an impressive bit of work.
I spent some time messing with fc-descriptor-7.f90/gc-descriptor-7-c.cc
because it kept failing on me. This came about because I missed one of the
chunks not applying in the C component of the test; namely:
for (int j = 0; j < 5; ++j)
for (int i = 0; i < 10; ++i)
{
subscripts[0] = j; subscripts[1] = i;
if (*(int *) CFI_address (a, subscripts) != (i+1) + 100*(j+1))
abort ();
}
This set me to wondering whether or not the user should be aware that the
result of the transpose intrinsic being passed in this way should not
generate a warning that the CFI API must be used in this case and not to
depend on the data being transposed?
Apart from this I have no other comments, still less corrections :-)
Many thanks for the patch - OK for mainline.
Paul
On Wed, 13 Oct 2021 at 21:11, Harald Anlauf <anl...@gmx.de> wrote:
> 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
>
>
--
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein
