diff mbox series

[fortran] PR97122 - Spurious FINAL ... must be in the specification part of a MODULE

Message ID CAGkQGi+jrPoiz2m0PYSSRKDy0Rxyv9sw=3hMeK1OrpS8HdRRgg@mail.gmail.com
State New
Headers
Series [fortran] PR97122 - Spurious FINAL ... must be in the specification part of a MODULE |

Commit Message

Paul Richard Thomas May 9, 2023, 3:51 p.m. UTC 
  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?

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.

Comments

Harald Anlauf May 9, 2023, 6:24 p.m. UTC | #1 
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.
Li, Pan2 via Gcc-patches May 9, 2023, 6:29 p.m. UTC | #2 
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.
Harald Anlauf May 9, 2023, 6:35 p.m. UTC | #3 
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.
Li, Pan2 via Gcc-patches May 9, 2023, 6:44 p.m. UTC | #4 
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 mbox series

Patch

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