fortran: Avoid infinite self-recursion [PR105381]
Commit Message
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
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
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
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
>
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
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
@@ -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;
}
new file mode 100644
@@ -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