diff mbox series

fortran: Avoid infinite self-recursion [PR105381]

Message ID 8f082ce4-a6a9-72c1-a882-4663426adaff@orange.fr
State New
Headers
Series fortran: Avoid infinite self-recursion [PR105381] |

Commit Message

Mikael Morin April 26, 2022, 12:38 p.m. UTC 
  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.

Comments

Tobias Burnus April 26, 2022, 1:22 p.m. UTC | #1 
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
Jakub Jelinek April 26, 2022, 1:32 p.m. UTC | #2 
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
Mikael Morin April 26, 2022, 5:12 p.m. UTC | #3 
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
>
Jakub Jelinek April 26, 2022, 5:28 p.m. UTC | #4 
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
diff mbox series

Patch

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