Message ID | trinity-04843f20-2dab-41c6-87fa-c939f57d02b3-1666987979945@3c-app-gmx-bs25 |
---|---|
State | New |
Headers | show |
Series | Fortran: ordering of hidden procedure arguments [PR107441] | expand |
Le 28/10/2022 à 22:12, Harald Anlauf via Fortran a écrit : > Dear all, > > the passing of procedure arguments in Fortran sometimes requires > ancillary parameters that are "hidden". Examples are string length > and the presence status of scalar variables with optional+value > attribute. > > The gfortran ABI is actually documented: > > https://gcc.gnu.org/onlinedocs/gfortran/Argument-passing-conventions.html > > The reporter found that there was a discrepancy between the > caller and the callee. This is corrected by the attached patch. > Hello, I think some discrepancy remains, as gfc_conv_procedure_call accumulates coarray stuff into the stringargs, while your change accumulates the associated parameter decls separately into hidden_arglist. It's not completely clear to me whether it is really problematic (string length and coarray metadata are both integers anyway), but I suspect it is. Another probable issue is your change to create_function_arglist changes arglist/hidden_arglist without also changing typelist/hidden_typelist accordingly. I think a change to gfc_get_function_type is also necessary: as the function decl is changed, the decl type need to be changed as well. I will see whether I can manage to exhibit testcases for these issues.
Le 30/10/2022 à 20:23, Mikael Morin a écrit : > > I think some discrepancy remains, as gfc_conv_procedure_call accumulates > coarray stuff into the stringargs, while your change accumulates the > associated parameter decls separately into hidden_arglist. It's not > completely clear to me whether it is really problematic (string length > and coarray metadata are both integers anyway), but I suspect it is. > > I will see whether I can manage to exhibit testcases for these issues. > Here is a -fcoarray=lib example. It can be placed in gfortran/coarray. ! { dg-do run } ! ! PR fortran/107441 ! Check that with -fcoarray=lib, coarray metadata arguments are passed ! in the right order to procedures. program p integer :: ci[*] ci = 17 call s(1, ci, "abcd") contains subroutine s(ra, ca, c) integer :: ra, ca[*] character(*) :: c ca[1] = 13 if (ra /= 1) stop 1 if (this_image() == 1) then if (ca /= 13) stop 2 else if (ca /= 17) stop 3 end if if (len(c) /= 4) stop 4 if (c /= "abcd") stop 5 end subroutine s end program p
Le 30/10/2022 à 20:23, Mikael Morin a écrit : > Another probable issue is your change to create_function_arglist changes > arglist/hidden_arglist without also changing typelist/hidden_typelist > accordingly. I think a change to gfc_get_function_type is also > necessary: as the function decl is changed, the decl type need to be > changed as well. > > I will see whether I can manage to exhibit testcases for these issues. > Here is a test for the type vs decl mismatch. ! { dg-do run } ! ! PR fortran/107441 ! Check that procedure types and procedure decls match when the procedure ! has both chaacter-typed and optional value args. program p interface subroutine i(c, o) character(*) :: c integer, optional, value :: o end subroutine i end interface procedure(i), pointer :: pp call pp("abcd") contains subroutine s(c, o) character(*) :: c integer, optional, value :: o if (present(o)) stop 1 if (len(c) /= 4) stop 2 if (c /= "abcd") stop 3 end subroutine s end program p
Le 30/10/2022 à 22:25, Mikael Morin a écrit : > Le 30/10/2022 à 20:23, Mikael Morin a écrit : >> Another probable issue is your change to create_function_arglist >> changes arglist/hidden_arglist without also changing >> typelist/hidden_typelist accordingly. I think a change to >> gfc_get_function_type is also necessary: as the function decl is >> changed, the decl type need to be changed as well. >> >> I will see whether I can manage to exhibit testcases for these issues. >> > Here is a test for the type vs decl mismatch. > > ! { dg-do run } > ! > ! PR fortran/107441 > ! Check that procedure types and procedure decls match when the procedure > ! has both chaacter-typed and optional value args. > > program p > interface > subroutine i(c, o) > character(*) :: c > integer, optional, value :: o > end subroutine i > end interface > procedure(i), pointer :: pp A pointer initialization is missing here: pp => s > call pp("abcd") > contains > subroutine s(c, o) > character(*) :: c > integer, optional, value :: o > if (present(o)) stop 1 > if (len(c) /= 4) stop 2 > if (c /= "abcd") stop 3 > end subroutine s > end program p > With the additional initialization, the test passes, so it's not very useful. The type mismatch is visible in the dump though, so maybe a dump match can be used.
Hi Mikael, thanks a lot, your testcases broke my initial (and incorrect) patch in multiple ways. I understand now that the right solution is much simpler and smaller. I've added your testcases, see attached, with a simple scan of the dump for the generated order of hidden arguments in the function decl for the last testcase. Regtested again on x86_64-pc-linux-gnu. OK now? Thanks, Harald Am 31.10.22 um 10:57 schrieb Mikael Morin: > Le 30/10/2022 à 22:25, Mikael Morin a écrit : >> Le 30/10/2022 à 20:23, Mikael Morin a écrit : >>> Another probable issue is your change to create_function_arglist >>> changes arglist/hidden_arglist without also changing >>> typelist/hidden_typelist accordingly. I think a change to >>> gfc_get_function_type is also necessary: as the function decl is >>> changed, the decl type need to be changed as well. >>> >>> I will see whether I can manage to exhibit testcases for these issues. >>> >> Here is a test for the type vs decl mismatch. >> >> ! { dg-do run } >> ! >> ! PR fortran/107441 >> ! Check that procedure types and procedure decls match when the procedure >> ! has both chaacter-typed and optional value args. >> >> program p >> interface >> subroutine i(c, o) >> character(*) :: c >> integer, optional, value :: o >> end subroutine i >> end interface >> procedure(i), pointer :: pp > A pointer initialization is missing here: > pp => s >> call pp("abcd") >> contains >> subroutine s(c, o) >> character(*) :: c >> integer, optional, value :: o >> if (present(o)) stop 1 >> if (len(c) /= 4) stop 2 >> if (c /= "abcd") stop 3 >> end subroutine s >> end program p >> > > With the additional initialization, the test passes, so it's not very > useful. The type mismatch is visible in the dump though, so maybe a > dump match can be used. >
Le 31/10/2022 à 21:29, Harald Anlauf via Fortran a écrit : > Hi Mikael, > > thanks a lot, your testcases broke my initial (and incorrect) patch > in multiple ways. I understand now that the right solution is much > simpler and smaller. > > I've added your testcases, see attached, with a simple scan of the > dump for the generated order of hidden arguments in the function decl > for the last testcase. > > Regtested again on x86_64-pc-linux-gnu. OK now? > Unfortunately no, the coarray case works, but the other problem remains. The type problem is not visible in the definition of S, it is in the declaration of S's prototype in P. S is defined as: void s (character(kind=1)[1:_c] & restrict c, integer(kind=4) o, logical(kind=1) _o, integer(kind=8) _c) { ... } but P has: void p () { static void s (character(kind=1)[1:] & restrict, integer(kind=4), integer(kind=8), logical(kind=1)); void (*<T63a>) (character(kind=1)[1:] & restrict, integer(kind=4), integer(kind=8), logical(kind=1)) pp; pp = s; ... }
Am 02.11.22 um 18:20 schrieb Mikael Morin: > Unfortunately no, the coarray case works, but the other problem remains. > The type problem is not visible in the definition of S, it is in the > declaration of S's prototype in P. > > S is defined as: > > void s (character(kind=1)[1:_c] & restrict c, integer(kind=4) o, > logical(kind=1) _o, integer(kind=8) _c) > { > ... > } > > but P has: > > void p () > { > static void s (character(kind=1)[1:] & restrict, integer(kind=4), > integer(kind=8), logical(kind=1)); > void (*<T63a>) (character(kind=1)[1:] & restrict, integer(kind=4), > integer(kind=8), logical(kind=1)) pp; > > pp = s; > ... > } Right, now I see it too. Simplified case: program p call s ("abcd") contains subroutine s(c, o) character(*) :: c integer, optional, value :: o end subroutine s end I do see what needs to be done in gfc_get_function_type, which seems in fact very simple. But I get really lost in create_function_arglist when trying to get the typelist right. One thing is I really don't understand how the (hidden_)typelist is managed here. How does that macro TREE_CHAIN work? Can we somehow chain two typelists the same way we chain arguments? (Failing that, I tried to split the loop over the dummy arguments in create_function_arglist into two passes, one for the optional+value variant, and one for the rest. It turned out to be a bad idea...) Harald
Le 02/11/2022 à 22:19, Harald Anlauf via Fortran a écrit : > Am 02.11.22 um 18:20 schrieb Mikael Morin: >> Unfortunately no, the coarray case works, but the other problem remains. >> The type problem is not visible in the definition of S, it is in the >> declaration of S's prototype in P. >> >> S is defined as: >> >> void s (character(kind=1)[1:_c] & restrict c, integer(kind=4) o, >> logical(kind=1) _o, integer(kind=8) _c) >> { >> ... >> } >> >> but P has: >> >> void p () >> { >> static void s (character(kind=1)[1:] & restrict, integer(kind=4), >> integer(kind=8), logical(kind=1)); >> void (*<T63a>) (character(kind=1)[1:] & restrict, integer(kind=4), >> integer(kind=8), logical(kind=1)) pp; >> >> pp = s; >> ... >> } > > Right, now I see it too. Simplified case: > > program p > call s ("abcd") > contains > subroutine s(c, o) > character(*) :: c > integer, optional, value :: o > end subroutine s > end > > I do see what needs to be done in gfc_get_function_type, which seems > in fact very simple. But I get really lost in create_function_arglist > when trying to get the typelist right. > > One thing is I really don't understand how the (hidden_)typelist is > managed here. How does that macro TREE_CHAIN work? Can we somehow > chain two typelists the same way we chain arguments? > TREE_CHAIN is just a linked list "next" pointer like there is in the fortran frontend a "next" pointer in gfc_ref or gfc_actual_arglist structures. Yes, we can chain typelists; the implementation of chainon in tree.cc is just TREE_CHAIN appending under the hood. > (Failing that, I tried to split the loop over the dummy arguments in > create_function_arglist into two passes, one for the optional+value > variant, and one for the rest. It turned out to be a bad idea...) > Not necessarily a bad idea, but one has to be careful to keep linked lists synchronized with argument walking. The most simple, I think, is to move the hidden_typelist advancement for optional, value presence arguments from the main loop to a preliminary loop. I hope it helps.
Am 03.11.22 um 11:06 schrieb Mikael Morin: > Le 02/11/2022 à 22:19, Harald Anlauf via Fortran a écrit : >> Am 02.11.22 um 18:20 schrieb Mikael Morin: >>> Unfortunately no, the coarray case works, but the other problem remains. >>> The type problem is not visible in the definition of S, it is in the >>> declaration of S's prototype in P. >>> >>> S is defined as: >>> >>> void s (character(kind=1)[1:_c] & restrict c, integer(kind=4) o, >>> logical(kind=1) _o, integer(kind=8) _c) >>> { >>> ... >>> } >>> >>> but P has: >>> >>> void p () >>> { >>> static void s (character(kind=1)[1:] & restrict, integer(kind=4), >>> integer(kind=8), logical(kind=1)); >>> void (*<T63a>) (character(kind=1)[1:] & restrict, integer(kind=4), >>> integer(kind=8), logical(kind=1)) pp; >>> >>> pp = s; >>> ... >>> } >> >> Right, now I see it too. Simplified case: >> >> program p >> call s ("abcd") >> contains >> subroutine s(c, o) >> character(*) :: c >> integer, optional, value :: o >> end subroutine s >> end >> >> I do see what needs to be done in gfc_get_function_type, which seems >> in fact very simple. But I get really lost in create_function_arglist >> when trying to get the typelist right. >> >> One thing is I really don't understand how the (hidden_)typelist is >> managed here. How does that macro TREE_CHAIN work? Can we somehow >> chain two typelists the same way we chain arguments? >> > TREE_CHAIN is just a linked list "next" pointer like there is in the > fortran frontend a "next" pointer in gfc_ref or gfc_actual_arglist > structures. > Yes, we can chain typelists; the implementation of chainon in tree.cc is > just TREE_CHAIN appending under the hood. > >> (Failing that, I tried to split the loop over the dummy arguments in >> create_function_arglist into two passes, one for the optional+value >> variant, and one for the rest. It turned out to be a bad idea...) >> > Not necessarily a bad idea, but one has to be careful to keep linked > lists synchronized with argument walking. > > The most simple, I think, is to move the hidden_typelist advancement for > optional, value presence arguments from the main loop to a preliminary > loop. > > I hope it helps. > I've spent some time not only staring at create_function_arglist, but trying several variations handling the declared hidden parms, and applying the necessary adjustments to gfc_get_function_type. (Managing linked trees is not the issue, just understanding them.) I've been unable to get the declarations in sync, and would need help how to debug the mess I've created. Dropping my patch for the time being.
Le 03/11/2022 à 23:03, Harald Anlauf a écrit : > Am 03.11.22 um 11:06 schrieb Mikael Morin: >> Le 02/11/2022 à 22:19, Harald Anlauf via Fortran a écrit : >>> I do see what needs to be done in gfc_get_function_type, which seems >>> in fact very simple. But I get really lost in create_function_arglist >>> when trying to get the typelist right. >>> >>> One thing is I really don't understand how the (hidden_)typelist is >>> managed here. How does that macro TREE_CHAIN work? Can we somehow >>> chain two typelists the same way we chain arguments? >>> >> TREE_CHAIN is just a linked list "next" pointer like there is in the >> fortran frontend a "next" pointer in gfc_ref or gfc_actual_arglist >> structures. >> Yes, we can chain typelists; the implementation of chainon in tree.cc is >> just TREE_CHAIN appending under the hood. >> >>> (Failing that, I tried to split the loop over the dummy arguments in >>> create_function_arglist into two passes, one for the optional+value >>> variant, and one for the rest. It turned out to be a bad idea...) >>> >> Not necessarily a bad idea, but one has to be careful to keep linked >> lists synchronized with argument walking. >> >> The most simple, I think, is to move the hidden_typelist advancement for >> optional, value presence arguments from the main loop to a preliminary >> loop. >> >> I hope it helps. >> > > I've spent some time not only staring at create_function_arglist, > but trying several variations handling the declared hidden parms, > and applying the necessary adjustments to gfc_get_function_type. > (Managing linked trees is not the issue, just understanding them.) > I've been unable to get the declarations in sync, and would need > help how to debug the mess I've created. Dropping my patch for > the time being. > If you want, we can meet on IRC somewhen (tonight?).
From b7646403557eca19612c81437f381d4b4dcd51c8 Mon Sep 17 00:00:00 2001 From: Harald Anlauf <anlauf@gmx.de> Date: Fri, 28 Oct 2022 21:58:08 +0200 Subject: [PATCH] Fortran: ordering of hidden procedure arguments [PR107441] gcc/fortran/ChangeLog: PR fortran/107441 * trans-decl.cc (create_function_arglist): Adjust the ordering of automatically generated hidden procedure arguments to match the documented ABI for gfortran. gcc/testsuite/ChangeLog: PR fortran/107441 * gfortran.dg/optional_absent_6.f90: New test. --- gcc/fortran/trans-decl.cc | 15 +++-- .../gfortran.dg/optional_absent_6.f90 | 60 +++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/optional_absent_6.f90 diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc index 63515b9072a..18842fe2c4b 100644 --- a/gcc/fortran/trans-decl.cc +++ b/gcc/fortran/trans-decl.cc @@ -2508,7 +2508,7 @@ create_function_arglist (gfc_symbol * sym) tree fndecl; gfc_formal_arglist *f; tree typelist, hidden_typelist; - tree arglist, hidden_arglist; + tree arglist, hidden_arglist, optional_arglist, strlen_arglist; tree type; tree parm; @@ -2518,6 +2518,7 @@ create_function_arglist (gfc_symbol * sym) the new FUNCTION_DECL node. */ arglist = NULL_TREE; hidden_arglist = NULL_TREE; + strlen_arglist = optional_arglist = NULL_TREE; typelist = TYPE_ARG_TYPES (TREE_TYPE (fndecl)); if (sym->attr.entry_master) @@ -2644,7 +2645,7 @@ create_function_arglist (gfc_symbol * sym) length = build_decl (input_location, PARM_DECL, get_identifier (name), len_type); - hidden_arglist = chainon (hidden_arglist, length); + strlen_arglist = chainon (strlen_arglist, length); DECL_CONTEXT (length) = fndecl; DECL_ARTIFICIAL (length) = 1; DECL_ARG_TYPE (length) = len_type; @@ -2712,7 +2713,7 @@ create_function_arglist (gfc_symbol * sym) PARM_DECL, get_identifier (name), boolean_type_node); - hidden_arglist = chainon (hidden_arglist, tmp); + optional_arglist = chainon (optional_arglist, tmp); DECL_CONTEXT (tmp) = fndecl; DECL_ARTIFICIAL (tmp) = 1; DECL_ARG_TYPE (tmp) = boolean_type_node; @@ -2863,10 +2864,16 @@ create_function_arglist (gfc_symbol * sym) typelist = TREE_CHAIN (typelist); } + /* Add hidden present status for optional+value arguments. */ + arglist = chainon (arglist, optional_arglist); + /* Add the hidden string length parameters, unless the procedure is bind(C). */ if (!sym->attr.is_bind_c) - arglist = chainon (arglist, hidden_arglist); + arglist = chainon (arglist, strlen_arglist); + + /* Add hidden extra arguments for the gfortran library. */ + arglist = chainon (arglist, hidden_arglist); gcc_assert (hidden_typelist == NULL_TREE || TREE_VALUE (hidden_typelist) == void_type_node); diff --git a/gcc/testsuite/gfortran.dg/optional_absent_6.f90 b/gcc/testsuite/gfortran.dg/optional_absent_6.f90 new file mode 100644 index 00000000000..b8abb06980a --- /dev/null +++ b/gcc/testsuite/gfortran.dg/optional_absent_6.f90 @@ -0,0 +1,60 @@ +! { dg-do run } +! PR fortran/107441 +! +! Test VALUE + OPTIONAL for integer/real/... +! in the presence of non-optional character dummies + +program bugdemo + implicit none + character :: s = 'a' + integer :: t + + t = testoptional(s) + call test2 (s) + call test3 (s) + call test4 (w='123',x=42) + +contains + + function testoptional (w, x) result(t) + character, intent(in) :: w + integer, intent(in), value, optional :: x + integer :: t + print *, 'present(x) is', present(x) + t = 0 + if (present (x)) stop 1 + end function testoptional + + subroutine test2 (w, x) + character, intent(in) :: w + integer, intent(in), value, optional :: x + print*, 'present(x) is', present(x) + if (present (x)) stop 2 + end subroutine test2 + + subroutine test3 (w, x) + character, intent(in), optional :: w + integer, intent(in), value, optional :: x + print *, 'present(w) is', present(w) + print *, 'present(x) is', present(x) + if (.not. present (w)) stop 3 + if (present (x)) stop 4 + end subroutine test3 + + subroutine test4 (r, w, x) + real, value, optional :: r + character(*), intent(in), optional :: w + integer, value, optional :: x + print *, 'present(r) is', present(r) + print *, 'present(w) is', present(w) + print *, 'present(x) is', present(x) + if (present (r)) stop 5 + if (.not. present (w)) stop 6 + if (.not. present (x)) stop 7 + print *, 'x=', x + print *, 'len(w)=', len(w) + if (len(w) /= 3) stop 8 + if (x /= 42) stop 9 + end subroutine test4 + +end program bugdemo -- 2.35.3