diff mbox series

Fortran: reject MODULE PROCEDURE outside generic module interface [PR99036]

Message ID trinity-fe491b4b-66a9-4d21-b8b1-9af4b907aa10-1679345864578@3c-app-gmx-bap50
State Accepted
Headers
Series Fortran: reject MODULE PROCEDURE outside generic module interface [PR99036] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Harald Anlauf March 20, 2023, 8:57 p.m. UTC 
  Dear all,

the attached trivial patch catches a MODULE PROCEDURE outside of a
module interface before we run into an internal error.

Regtested on x86_64-pc-linux-gnu.  OK for mainline?

This PR is marked as an 11/12/13 regression, so this is a candidate
for backporting.

Thanks,
Harald

Comments

Paul Richard Thomas March 21, 2023, 7:09 a.m. UTC | #1 
Hi Harald,

This is good for trunk and for backporting.

Thanks for the rapid fix.

Paul


On Mon, 20 Mar 2023 at 20:57, Harald Anlauf via Fortran <fortran@gcc.gnu.org>
wrote:

> Dear all,
>
> the attached trivial patch catches a MODULE PROCEDURE outside of a
> module interface before we run into an internal error.
>
> Regtested on x86_64-pc-linux-gnu.  OK for mainline?
>
> This PR is marked as an 11/12/13 regression, so this is a candidate
> for backporting.
>
> Thanks,
> Harald
>
>
Tobias Burnus March 21, 2023, 8:31 a.m. UTC | #2 
On 20.03.23 21:57, Harald Anlauf via Gcc-patches wrote:
> --- a/gcc/fortran/decl.cc
> +++ b/gcc/fortran/decl.cc
> @@ -9998,6 +9998,7 @@ gfc_match_modproc (void)
>     if ((gfc_state_stack->state != COMP_INTERFACE
>          && gfc_state_stack->state != COMP_CONTAINS)
>         || gfc_state_stack->previous == NULL
> +      || !current_interface.type
>         || current_interface.type == INTERFACE_NAMELESS
>         || current_interface.type == INTERFACE_ABSTRACT)
>       {

First, I do not like '!var' comparisons for enum values,
only for Booleans/logicals and pointer.

Secondly, I am not sure that it is really guaranteed that
the value is 0.

I think something like the following makes more sense
and, as just tried, it also regtests (w/ your testcase included).
If you agree, feel free to package and commit it.


diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index c8f0bb83c2c..233bf244d62 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -9996,7 +9996,8 @@ gfc_match_modproc (void)
    gfc_interface *old_interface_head, *interface;

-  if ((gfc_state_stack->state != COMP_INTERFACE
-       && gfc_state_stack->state != COMP_CONTAINS)
-      || gfc_state_stack->previous == NULL
+  if (gfc_state_stack->previous == NULL
+      || (gfc_state_stack->state != COMP_INTERFACE
+         && (gfc_state_stack->state != COMP_CONTAINS
+             || gfc_state_stack->previous->state != COMP_INTERFACE))
        || current_interface.type == INTERFACE_NAMELESS
        || current_interface.type == INTERFACE_ABSTRACT)


Thanks for working on this and all the other issues!

Tobias

-----------------
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
Harald Anlauf March 21, 2023, 7:10 p.m. UTC | #3 
Hi Tobias,

Am 21.03.23 um 09:31 schrieb Tobias Burnus:
> On 20.03.23 21:57, Harald Anlauf via Gcc-patches wrote:
>> --- a/gcc/fortran/decl.cc
>> +++ b/gcc/fortran/decl.cc
>> @@ -9998,6 +9998,7 @@ gfc_match_modproc (void)
>>     if ((gfc_state_stack->state != COMP_INTERFACE
>>          && gfc_state_stack->state != COMP_CONTAINS)
>>         || gfc_state_stack->previous == NULL
>> +      || !current_interface.type
>>         || current_interface.type == INTERFACE_NAMELESS
>>         || current_interface.type == INTERFACE_ABSTRACT)
>>       {
>
> First, I do not like '!var' comparisons for enum values,
> only for Booleans/logicals and pointer.

I was hesitating to do this and thought about adding an
enum value that it 0 numerically, but ...

> Secondly, I am not sure that it is really guaranteed that
> the value is 0.

... had assumed that this would be guaranteed.

> I think something like the following makes more sense
> and, as just tried, it also regtests (w/ your testcase included).
> If you agree, feel free to package and commit it.
>
>
> diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
> index c8f0bb83c2c..233bf244d62 100644
> --- a/gcc/fortran/decl.cc
> +++ b/gcc/fortran/decl.cc
> @@ -9996,7 +9996,8 @@ gfc_match_modproc (void)
>     gfc_interface *old_interface_head, *interface;
>
> -  if ((gfc_state_stack->state != COMP_INTERFACE
> -       && gfc_state_stack->state != COMP_CONTAINS)
> -      || gfc_state_stack->previous == NULL
> +  if (gfc_state_stack->previous == NULL
> +      || (gfc_state_stack->state != COMP_INTERFACE
> +         && (gfc_state_stack->state != COMP_CONTAINS
> +             || gfc_state_stack->previous->state != COMP_INTERFACE))
>         || current_interface.type == INTERFACE_NAMELESS
>         || current_interface.type == INTERFACE_ABSTRACT)
>

Yes, that's a much cleaner solution.  Pushed as:

https://gcc.gnu.org/g:dd282b16bfd3c6e218dffb7798a375365b10ae22
commit r13-6790-gdd282b16bfd3c6e218dffb7798a375365b10ae22

Thanks for the review!

Harald


> Thanks for working on this and all the other issues!
>
> Tobias
>
> -----------------
> 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
>
diff mbox series

Patch

From 9c59709fad91c99041a9cb770b98da17af01d260 Mon Sep 17 00:00:00 2001
From: Harald Anlauf <anlauf@gmx.de>
Date: Mon, 20 Mar 2023 21:50:59 +0100
Subject: [PATCH] Fortran: reject MODULE PROCEDURE outside generic module
 interface [PR99036]

gcc/fortran/ChangeLog:

	PR fortran/99036
	* decl.cc (gfc_match_modproc): Reject MODULE PROCEDURE if not in a
	generic module interface.

gcc/testsuite/ChangeLog:

	PR fortran/99036
	* gfortran.dg/pr99036.f90: New test.
---
 gcc/fortran/decl.cc                   | 1 +
 gcc/testsuite/gfortran.dg/pr99036.f90 | 9 +++++++++
 2 files changed, 10 insertions(+)
 create mode 100644 gcc/testsuite/gfortran.dg/pr99036.f90

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index c8f0bb83c2c..b29f491fe1f 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -9998,6 +9998,7 @@  gfc_match_modproc (void)
   if ((gfc_state_stack->state != COMP_INTERFACE
        && gfc_state_stack->state != COMP_CONTAINS)
       || gfc_state_stack->previous == NULL
+      || !current_interface.type
       || current_interface.type == INTERFACE_NAMELESS
       || current_interface.type == INTERFACE_ABSTRACT)
     {
diff --git a/gcc/testsuite/gfortran.dg/pr99036.f90 b/gcc/testsuite/gfortran.dg/pr99036.f90
new file mode 100644
index 00000000000..a6e396f6f71
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/pr99036.f90
@@ -0,0 +1,9 @@ 
+! { dg-do compile }
+! PR fortran/99036 - ICE in gfc_current_interface_head
+! Contributed by G. Steinmetz
+
+module m
+contains
+  module procedure s ! { dg-error "must be in a generic module interface" }
+  end
+end
--
2.35.3