[PATCH] fortran: Avoid infinite self-recursion [PR105381]

[PATCH] fortran: Avoid infinite self-recursion [PR105381]

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

Hello,

this is a fix for the regression I recently introduced with the PR102043 
patch.  It is an infinite recursion problem.  I can’t see the memory 
consumption that Harald reported; maybe he doesn’t use the default 
optimization level to build the compiler.

Regression tested on x86_64-pc-linux-gnu.
I plan to push it tonight.

[-- Attachment #2: 0001-fortran-Avoid-infinite-self-recursion-PR105381.patch --]
[-- Type: text/x-patch, Size: 2961 bytes --]

From 85d57fb88203697d7e52d5f1f148eab35e4f7486 Mon Sep 17 00:00:00 2001
From: Mikael Morin <mikael@gcc.gnu.org>
Date: Tue, 26 Apr 2022 13:05:32 +0200
Subject: [PATCH] fortran: Avoid infinite self-recursion [PR105381]
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Dummy array decls are local decls different from the argument decl
accessible through GFC_DECL_SAVED_DESCRIPTOR.  If the argument decl has
a DECL_LANG_SPECIFIC set, it is copied over to the local decl at the
time the latter is created, so that the DECL_LANG_SPECIFIC object is
shared between local dummy decl and argument decl, and thus the
GFC_DECL_SAVED_DESCRIPTOR of the argument decl is the argument decl
itself.

The r12-8230-g7964ab6c364c410c34efe7ca2eba797d36525349 change introduced
the non_negative_strides_array_p predicate which recurses through
GFC_DECL_SAVED_DESCRIPTOR to avoid seeing dummy decls as purely local
decls.  As the GFC_DECL_SAVED_DESCRIPTOR of the argument decl is itself,
this can cause infinite recursion.

This change adds a check to avoid infinite recursion.

	PR fortran/102043
	PR fortran/105381

gcc/fortran/ChangeLog:

	* trans-array.cc (non_negative_strides_array_p): Don’t recurse
	if the next argument is the same as the current.

gcc/testsuite/ChangeLog:

	* gfortran.dg/character_array_dummy_1.f90: New test.
---
 gcc/fortran/trans-array.cc                    |  3 ++-
 .../gfortran.dg/character_array_dummy_1.f90   | 21 +++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gfortran.dg/character_array_dummy_1.f90

diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc
index e4b6270ccf8..e0070aa080d 100644
--- a/gcc/fortran/trans-array.cc
+++ b/gcc/fortran/trans-array.cc
@@ -3698,7 +3698,8 @@ non_negative_strides_array_p (tree expr)
   if (DECL_P (expr)
       && DECL_LANG_SPECIFIC (expr))
     if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
-      return non_negative_strides_array_p (orig_decl);
+      if (orig_decl != expr)
+	return non_negative_strides_array_p (orig_decl);
 
   return true;
 }
diff --git a/gcc/testsuite/gfortran.dg/character_array_dummy_1.f90 b/gcc/testsuite/gfortran.dg/character_array_dummy_1.f90
new file mode 100644
index 00000000000..da5ed636f4f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/character_array_dummy_1.f90
@@ -0,0 +1,21 @@
+! { dg-do compile }
+!
+! PR fortran/105381
+! Infinite recursion with array references of character dummy arguments.
+!
+! Contributed by Harald Anlauf <anlauf@gmx.de>
+
+MODULE m
+  implicit none
+  integer,  parameter :: ncrit  =  8
+  integer,  parameter :: nterm  =  7
+contains
+
+  subroutine new_thin_rule (rule1)
+    character(*),intent(in) ,optional :: rule1(ncrit)
+    character(len=8) :: rules (ncrit,nterm)
+    rules = ''
+    if (present (rule1)) rules(:,1) = rule1  ! <-- compile time hog
+  end subroutine new_thin_rule
+
+end module m
-- 
2.35.1

Re: [PATCH] fortran: Avoid infinite self-recursion [PR105381]

[Tobias Burnus]

LGTM - however:

On 26.04.22 14:38, Mikael Morin wrote:
> --- a/gcc/fortran/trans-array.cc
> +++ b/gcc/fortran/trans-array.cc
> @@ -3698,7 +3698,8 @@ non_negative_strides_array_p (tree expr)
>     if (DECL_P (expr)
>         && DECL_LANG_SPECIFIC (expr))
>       if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
> -      return non_negative_strides_array_p (orig_decl);
> +      if (orig_decl != expr)
> +     return non_negative_strides_array_p (orig_decl);

Is the if()if()if() cascade really needed? I can see a reason that an
extra 'if' is preferred for the variable declaration of orig_decl, but
can't we at least put the new 'orig_decl != expr' with an '&&' into the
same if as the decl/in the second if? Besides clearer, it also avoids
further identing the return line.

Thanks,

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

Re: [PATCH] fortran: Avoid infinite self-recursion [PR105381]

[Jakub Jelinek]

On Tue, Apr 26, 2022 at 03:22:08PM +0200, Tobias Burnus wrote:
> LGTM - however:
> 
> On 26.04.22 14:38, Mikael Morin wrote:
> > --- a/gcc/fortran/trans-array.cc
> > +++ b/gcc/fortran/trans-array.cc
> > @@ -3698,7 +3698,8 @@ non_negative_strides_array_p (tree expr)
> >     if (DECL_P (expr)
> >         && DECL_LANG_SPECIFIC (expr))
> >       if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
> > -      return non_negative_strides_array_p (orig_decl);
> > +      if (orig_decl != expr)
> > +     return non_negative_strides_array_p (orig_decl);
> 
> Is the if()if()if() cascade really needed? I can see a reason that an
> extra 'if' is preferred for the variable declaration of orig_decl, but
> can't we at least put the new 'orig_decl != expr' with an '&&' into the
> same if as the decl/in the second if? Besides clearer, it also avoids
> further identing the return line.

I think we can't in C++11/C++14.  The options can be if orig_decl would be declared
earlier, then it can be
    tree orig_decl;
    if (DECL_P (expr)
	&& DECL_LANG_SPECIFIC (expr)
	&& (orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
	&& orig_decl != expr)
      return non_negative_strides_array_p (orig_decl);
but I think this is generally frowned upon,
or one can repeat it like:
    if (DECL_P (expr)
	&& DECL_LANG_SPECIFIC (expr)
	&& GFC_DECL_SAVED_DESCRIPTOR (expr)
	&& GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
      return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR (expr));
or what Mikael wrote, perhaps with the && on one line:
    if (DECL_P (expr) && DECL_LANG_SPECIFIC (expr))
      if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
	if (orig_decl != expr)
	  return non_negative_strides_array_p (orig_decl);
In C++17 and later one can write:
    if (DECL_P (expr) && DECL_LANG_SPECIFIC (expr))
      if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr);
	  orig_decl && orig_decl != expr)
	return non_negative_strides_array_p (orig_decl);

	Jakub

Re: [PATCH] fortran: Avoid infinite self-recursion [PR105381]

[Mikael Morin]

Le 26/04/2022 à 15:32, Jakub Jelinek a écrit :
> On Tue, Apr 26, 2022 at 03:22:08PM +0200, Tobias Burnus wrote:
>> LGTM - however:
>>
>> On 26.04.22 14:38, Mikael Morin wrote:
>>> --- a/gcc/fortran/trans-array.cc
>>> +++ b/gcc/fortran/trans-array.cc
>>> @@ -3698,7 +3698,8 @@ non_negative_strides_array_p (tree expr)
>>>      if (DECL_P (expr)
>>>          && DECL_LANG_SPECIFIC (expr))
>>>        if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
>>> -      return non_negative_strides_array_p (orig_decl);
>>> +      if (orig_decl != expr)
>>> +     return non_negative_strides_array_p (orig_decl);
>>
>> Is the if()if()if() cascade really needed? I can see a reason that an
>> extra 'if' is preferred for the variable declaration of orig_decl, but
>> can't we at least put the new 'orig_decl != expr' with an '&&' into the
>> same if as the decl/in the second if? Besides clearer, it also avoids
>> further identing the return line.
> 
> I think we can't in C++11/C++14.  The options can be if orig_decl would be declared
> earlier, then it can be
>      tree orig_decl;
>      if (DECL_P (expr)
> 	&& DECL_LANG_SPECIFIC (expr)
> 	&& (orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
> 	&& orig_decl != expr)
>        return non_negative_strides_array_p (orig_decl);
> but I think this is generally frowned upon,
> or one can repeat it like:
>      if (DECL_P (expr)
> 	&& DECL_LANG_SPECIFIC (expr)
> 	&& GFC_DECL_SAVED_DESCRIPTOR (expr)
> 	&& GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
>        return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR (expr));

I think I’ll use that.  There are numerous places where macros are 
repeated like this already and everybody seems to be pleased with it.
Thanks for the feedback, and for the suggestions.

> or what Mikael wrote, perhaps with the && on one line:
>      if (DECL_P (expr) && DECL_LANG_SPECIFIC (expr))
>        if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
> 	if (orig_decl != expr)
> 	  return non_negative_strides_array_p (orig_decl);
> In C++17 and later one can write:
>      if (DECL_P (expr) && DECL_LANG_SPECIFIC (expr))
>        if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr);
> 	  orig_decl && orig_decl != expr)
> 	return non_negative_strides_array_p (orig_decl);
> 
> 	Jakub
> 

Re: [PATCH] fortran: Avoid infinite self-recursion [PR105381]

[Jakub Jelinek]

On Tue, Apr 26, 2022 at 07:12:13PM +0200, Mikael Morin wrote:
> > I think we can't in C++11/C++14.  The options can be if orig_decl would be declared
> > earlier, then it can be
> >      tree orig_decl;
> >      if (DECL_P (expr)
> > 	&& DECL_LANG_SPECIFIC (expr)
> > 	&& (orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
> > 	&& orig_decl != expr)
> >        return non_negative_strides_array_p (orig_decl);
> > but I think this is generally frowned upon,
> > or one can repeat it like:
> >      if (DECL_P (expr)
> > 	&& DECL_LANG_SPECIFIC (expr)
> > 	&& GFC_DECL_SAVED_DESCRIPTOR (expr)
> > 	&& GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
> >        return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR (expr));
> 
> I think I’ll use that.  There are numerous places where macros are repeated
> like this already and everybody seems to be pleased with it.
> Thanks for the feedback, and for the suggestions.

Agreed in this case, GFC_DECL_SAVED_DESCRIPTOR is really just a dereference
at least in release compiler.  Doing that when the macro actually calls some
functions is worse.

	Jakub

[PATCH v2] fortran: Avoid infinite self-recursion [PR105381]

[Mikael Morin]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

Le 26/04/2022 à 19:12, Mikael Morin a écrit :
> Le 26/04/2022 à 15:32, Jakub Jelinek a écrit :
>> or one can repeat it like:
>>      if (DECL_P (expr)
>>     && DECL_LANG_SPECIFIC (expr)
>>     && GFC_DECL_SAVED_DESCRIPTOR (expr)
>>     && GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
>>        return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR 
>> (expr));
> 
> I think I’ll use that. 

Here it comes.
Regression tested again. OK?

[-- Attachment #2: 0001-fortran-Avoid-infinite-self-recursion-PR105381.patch --]
[-- Type: text/x-patch, Size: 3176 bytes --]

From 9da696478832bb3fe5ac25542ad9226ce3235368 Mon Sep 17 00:00:00 2001
From: Mikael Morin <mikael@gcc.gnu.org>
Date: Tue, 26 Apr 2022 13:05:32 +0200
Subject: [PATCH v2] fortran: Avoid infinite self-recursion [PR105381]

Dummy array decls are local decls different from the argument decl
accessible through GFC_DECL_SAVED_DESCRIPTOR.  If the argument decl has
a DECL_LANG_SPECIFIC set, it is copied over to the local decl at the
time the latter is created, so that the DECL_LANG_SPECIFIC object is
shared between local dummy decl and argument decl, and thus the
GFC_DECL_SAVED_DESCRIPTOR of the argument decl is the argument decl
itself.

The r12-8230-g7964ab6c364c410c34efe7ca2eba797d36525349 change introduced
the non_negative_strides_array_p predicate which recurses through
GFC_DECL_SAVED_DESCRIPTOR to avoid seeing dummy decls as purely local
decls.  As the GFC_DECL_SAVED_DESCRIPTOR of the argument decl is itself,
this can cause infinite recursion.

This change adds a check to avoid infinite recursion.

	PR fortran/102043
	PR fortran/105381

gcc/fortran/ChangeLog:

	* trans-array.cc (non_negative_strides_array_p): Inline variable
	orig_decl and merge nested if conditions.  Add condition to not
	recurse if the next argument is the same as the current.

gcc/testsuite/ChangeLog:

	* gfortran.dg/character_array_dummy_1.f90: New test.
---
 gcc/fortran/trans-array.cc                    |  7 ++++---
 .../gfortran.dg/character_array_dummy_1.f90   | 21 +++++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/character_array_dummy_1.f90

diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc
index e4b6270ccf8..05134952db4 100644
--- a/gcc/fortran/trans-array.cc
+++ b/gcc/fortran/trans-array.cc
@@ -3696,9 +3696,10 @@ non_negative_strides_array_p (tree expr)
   /* If the array was originally a dummy with a descriptor, strides can be
      negative.  */
   if (DECL_P (expr)
-      && DECL_LANG_SPECIFIC (expr))
-    if (tree orig_decl = GFC_DECL_SAVED_DESCRIPTOR (expr))
-      return non_negative_strides_array_p (orig_decl);
+      && DECL_LANG_SPECIFIC (expr)
+      && GFC_DECL_SAVED_DESCRIPTOR (expr)
+      && GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
+    return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR (expr));
 
   return true;
 }
diff --git a/gcc/testsuite/gfortran.dg/character_array_dummy_1.f90 b/gcc/testsuite/gfortran.dg/character_array_dummy_1.f90
new file mode 100644
index 00000000000..da5ed636f4f
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/character_array_dummy_1.f90
@@ -0,0 +1,21 @@
+! { dg-do compile }
+!
+! PR fortran/105381
+! Infinite recursion with array references of character dummy arguments.
+!
+! Contributed by Harald Anlauf <anlauf@gmx.de>
+
+MODULE m
+  implicit none
+  integer,  parameter :: ncrit  =  8
+  integer,  parameter :: nterm  =  7
+contains
+
+  subroutine new_thin_rule (rule1)
+    character(*),intent(in) ,optional :: rule1(ncrit)
+    character(len=8) :: rules (ncrit,nterm)
+    rules = ''
+    if (present (rule1)) rules(:,1) = rule1  ! <-- compile time hog
+  end subroutine new_thin_rule
+
+end module m
-- 
2.35.1

Re: [PATCH v2] fortran: Avoid infinite self-recursion [PR105381]

[Harald Anlauf]

Hi Mikael,

Am 26.04.22 um 21:10 schrieb Mikael Morin:
> Le 26/04/2022 à 19:12, Mikael Morin a écrit :
>> Le 26/04/2022 à 15:32, Jakub Jelinek a écrit :
>>> or one can repeat it like:
>>>      if (DECL_P (expr)
>>>     && DECL_LANG_SPECIFIC (expr)
>>>     && GFC_DECL_SAVED_DESCRIPTOR (expr)
>>>     && GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
>>>        return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR 
>>> (expr));
>>
>> I think I’ll use that. 
> 
> Here it comes.
> Regression tested again. OK?

works for me.

Thanks for the quick fix!

Harald

Re: [PATCH v2] fortran: Avoid infinite self-recursion [PR105381]

[Harald Anlauf]

Hi Mikael,

Am 26.04.22 um 21:10 schrieb Mikael Morin:
> Le 26/04/2022 à 19:12, Mikael Morin a écrit :
>> Le 26/04/2022 à 15:32, Jakub Jelinek a écrit :
>>> or one can repeat it like:
>>>      if (DECL_P (expr)
>>>     && DECL_LANG_SPECIFIC (expr)
>>>     && GFC_DECL_SAVED_DESCRIPTOR (expr)
>>>     && GFC_DECL_SAVED_DESCRIPTOR (expr) != expr)
>>>        return non_negative_strides_array_p (GFC_DECL_SAVED_DESCRIPTOR
>>> (expr));
>>
>> I think I’ll use that.
>
> Here it comes.
> Regression tested again. OK?

works for me.

Thanks for the quick fix!

Harald