Message ID | CAGkQGi+jrPoiz2m0PYSSRKDy0Rxyv9sw=3hMeK1OrpS8HdRRgg@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | [fortran] PR97122 - Spurious FINAL ... must be in the specification part of a MODULE | expand |
Hi Paul, On 5/9/23 17:51, Paul Richard Thomas via Gcc-patches wrote: > Hi All, > > Thanks to Steve Kargl for the fix. It caused finalize_8.f03 to fail because > this testcase checked that finalizable derived types could not be specified > in a submodule. I have replaced the original test with a test of the patch. > > Thanks also to Malcolm Cohen for guidance on this. > > OK for trunk? the patch looks good to me. However: @@ -11637,8 +11637,9 @@ gfc_match_final_decl (void) block = gfc_state_stack->previous->sym; gcc_assert (block); - if (!gfc_state_stack->previous || !gfc_state_stack->previous->previous - || gfc_state_stack->previous->previous->state != COMP_MODULE) + if (gfc_state_stack->previous->previous + && gfc_state_stack->previous->previous->state != COMP_MODULE + && gfc_state_stack->previous->previous->state != COMP_SUBMODULE) { gfc_error ("Derived type declaration with FINAL at %C must be in the" " specification part of a MODULE"); I am wondering if we should keep the protection against a potential NULL pointer dereference (i.e. gfc_state_stack->previous == NULL) for possibly invalid code. I have failed to produce a simple testcase, but others may have "better" ideas. I'll leave it to you to amend the patch or leave as is. Thanks, Harald > Paul > > Fortran: Allow declaration of finalizable DT in a submodule [PR97122] > > 2023-05-09 Paul Thomas <pault@gcc.gnu.org> > Steven G. Kargl <kargl@gcc.gnu.org> > > gcc/fortran > PR fortran/97122 > * decl.cc (variable_decl): Clean up white space issues. > (gfc_match_final_decl): Declaration of finalizable derived type > is allowed in a submodule. > > gcc/testsuite/ > PR fortran/97122 > * gfortran.dg/finalize_8.f03 : Replace testcase that checks > declaration of finalizable derived types in submodules works.
On Tue, May 09, 2023 at 08:24:16PM +0200, Harald Anlauf wrote: > Hi Paul, > > On 5/9/23 17:51, Paul Richard Thomas via Gcc-patches wrote: > > Hi All, > > > > Thanks to Steve Kargl for the fix. It caused finalize_8.f03 to fail because > > this testcase checked that finalizable derived types could not be specified > > in a submodule. I have replaced the original test with a test of the patch. > > > > Thanks also to Malcolm Cohen for guidance on this. > > > > OK for trunk? > > the patch looks good to me. However: > > @@ -11637,8 +11637,9 @@ gfc_match_final_decl (void) > block = gfc_state_stack->previous->sym; ^^^^^^^^^^^^^^^^^^^^^^^^^ See below. > gcc_assert (block); > > - if (!gfc_state_stack->previous || !gfc_state_stack->previous->previous > - || gfc_state_stack->previous->previous->state != COMP_MODULE) > + if (gfc_state_stack->previous->previous > + && gfc_state_stack->previous->previous->state != COMP_MODULE > + && gfc_state_stack->previous->previous->state != COMP_SUBMODULE) > { > gfc_error ("Derived type declaration with FINAL at %C must be in > the" > " specification part of a MODULE"); > > I am wondering if we should keep the protection against a potential > NULL pointer dereference (i.e. gfc_state_stack->previous == NULL) for > possibly invalid code. I have failed to produce a simple testcase, > but others may have "better" ideas. It's not needed. See above. gfc_state_stack->previous is referenced a few lines above the if-stmt. The reference will segfault if the pointer is NULL.
On 5/9/23 20:29, Steve Kargl via Gcc-patches wrote: > On Tue, May 09, 2023 at 08:24:16PM +0200, Harald Anlauf wrote: >> Hi Paul, >> >> On 5/9/23 17:51, Paul Richard Thomas via Gcc-patches wrote: >>> Hi All, >>> >>> Thanks to Steve Kargl for the fix. It caused finalize_8.f03 to fail because >>> this testcase checked that finalizable derived types could not be specified >>> in a submodule. I have replaced the original test with a test of the patch. >>> >>> Thanks also to Malcolm Cohen for guidance on this. >>> >>> OK for trunk? >> >> the patch looks good to me. However: >> >> @@ -11637,8 +11637,9 @@ gfc_match_final_decl (void) >> block = gfc_state_stack->previous->sym; > ^^^^^^^^^^^^^^^^^^^^^^^^^ > See below. > >> gcc_assert (block); >> >> - if (!gfc_state_stack->previous || !gfc_state_stack->previous->previous >> - || gfc_state_stack->previous->previous->state != COMP_MODULE) >> + if (gfc_state_stack->previous->previous >> + && gfc_state_stack->previous->previous->state != COMP_MODULE >> + && gfc_state_stack->previous->previous->state != COMP_SUBMODULE) >> { >> gfc_error ("Derived type declaration with FINAL at %C must be in >> the" >> " specification part of a MODULE"); >> >> I am wondering if we should keep the protection against a potential >> NULL pointer dereference (i.e. gfc_state_stack->previous == NULL) for >> possibly invalid code. I have failed to produce a simple testcase, >> but others may have "better" ideas. > > It's not needed. See above. gfc_state_stack->previous is referenced > a few lines above the if-stmt. The reference will segfault if the > pointer is NULL. > You're absolutely right. So it is OK as is.
On Tue, May 09, 2023 at 08:35:00PM +0200, Harald Anlauf wrote: > On 5/9/23 20:29, Steve Kargl via Gcc-patches wrote: > > > > It's not needed. See above. gfc_state_stack->previous is referenced > > a few lines above the if-stmt. The reference will segfault if the > > pointer is NULL. > > > > You're absolutely right. So it is OK as is. Thanks for keeping us honest and the review.
diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc index 233bf244d62..6d6ce0854de 100644 --- a/gcc/fortran/decl.cc +++ b/gcc/fortran/decl.cc @@ -2698,7 +2698,7 @@ variable_decl (int elem) } gfc_seen_div0 = false; - + /* F2018:C830 (R816) An explicit-shape-spec whose bounds are not constant expressions shall appear only in a subprogram, derived type definition, BLOCK construct, or interface body. */ @@ -2769,7 +2769,7 @@ variable_decl (int elem) if (e->expr_type != EXPR_CONSTANT) { n = gfc_copy_expr (e); - if (!gfc_simplify_expr (n, 1) && gfc_seen_div0) + if (!gfc_simplify_expr (n, 1) && gfc_seen_div0) { m = MATCH_ERROR; goto cleanup; @@ -2784,12 +2784,12 @@ variable_decl (int elem) if (e->expr_type != EXPR_CONSTANT) { n = gfc_copy_expr (e); - if (!gfc_simplify_expr (n, 1) && gfc_seen_div0) + if (!gfc_simplify_expr (n, 1) && gfc_seen_div0) { m = MATCH_ERROR; goto cleanup; } - + if (n->expr_type == EXPR_CONSTANT) gfc_replace_expr (e, n); else @@ -11637,8 +11637,9 @@ gfc_match_final_decl (void) block = gfc_state_stack->previous->sym; gcc_assert (block); - if (!gfc_state_stack->previous || !gfc_state_stack->previous->previous - || gfc_state_stack->previous->previous->state != COMP_MODULE) + if (gfc_state_stack->previous->previous + && gfc_state_stack->previous->previous->state != COMP_MODULE + && gfc_state_stack->previous->previous->state != COMP_SUBMODULE) { gfc_error ("Derived type declaration with FINAL at %C must be in the" " specification part of a MODULE"); diff --git a/gcc/testsuite/gfortran.dg/finalize_8.f03 b/gcc/testsuite/gfortran.dg/finalize_8.f03 index b2027a0ba6d..b7fa10dda31 100644 --- a/gcc/testsuite/gfortran.dg/finalize_8.f03 +++ b/gcc/testsuite/gfortran.dg/finalize_8.f03 @@ -1,35 +1,49 @@ -! { dg-do compile } - -! Parsing of finalizer procedure definitions. -! Check that FINAL-declarations are only allowed on types defined in the -! specification part of a module. - -MODULE final_type +! { dg-do run } +! +! PR97122: Declaration of a finalizable derived type in a submodule +! IS allowed. +! +! Contributed by Ian Harvey <ian_harvey@bigpond.com> +! +MODULE m IMPLICIT NONE -CONTAINS + INTERFACE + MODULE SUBROUTINE other(i) + IMPLICIT NONE + integer, intent(inout) :: i + END SUBROUTINE other + END INTERFACE - SUBROUTINE bar - IMPLICIT NONE + integer :: mi - TYPE :: mytype - INTEGER, ALLOCATABLE :: fooarr(:) - REAL :: foobar - CONTAINS - FINAL :: myfinal ! { dg-error "in the specification part of a MODULE" } - END TYPE mytype - - CONTAINS +END MODULE m - SUBROUTINE myfinal (el) - TYPE(mytype) :: el - END SUBROUTINE myfinal +SUBMODULE (m) s + IMPLICIT NONE - END SUBROUTINE bar + TYPE :: t + integer :: i + CONTAINS + FINAL :: final_t ! Used to be an error here + END TYPE t -END MODULE final_type +CONTAINS -PROGRAM finalizer - IMPLICIT NONE - ! Do nothing here -END PROGRAM finalizer + SUBROUTINE final_t(arg) + TYPE(t), INTENT(INOUT) :: arg + mi = -arg%i + END SUBROUTINE final_t + + module subroutine other(i) ! 'ti' is finalized + integer, intent(inout) :: i + type(t) :: ti + ti%i = i + END subroutine other +END SUBMODULE s + + use m + integer :: i = 42 + call other(i) + if (mi .ne. -i) stop 1 +end