[Patch, fortran] PR79685 - [12/13/14/15 Regression] ICE on valid code in gfc_match_structure_constructor

[Patch, fortran] PR79685 - [12/13/14/15 Regression] ICE on valid code in gfc_match_structure_constructor

[Paul Richard Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 807 bytes --]

This patch is straightforward but I am still puzzled as to why it is
necessary for the particular case. Having looked at all the other chunks of
frontend code involving use renaming, it seems that the process just works
everywhere else. I tried putting the new code in gfc_find_symtree but it
caused some regressions unless I pinned it down to the specific case of a
structure constructor.

OK for mainline and backporting at a later date?

Paul

Fortran: Fix ICE with structure constructor in data statement [PR79685]

2024-07-27  Paul Thomas  <pault@gcc.gnu.org>

gcc/fortran
PR fortran/79685
* primary.cc (gfc_match_structure_constructor): See if there is
a use renamed symtree before calling gfc_get_ha_sym_tree. If so
use it.

gcc/testsuite/
PR fortran/79685
* gfortran.dg/use_rename_12.f90: New test.

[-- Attachment #2: submit.diff --]
[-- Type: text/x-patch, Size: 2510 bytes --]

diff --git a/gcc/fortran/primary.cc b/gcc/fortran/primary.cc
index 76f6bcb8a78..30ea01961a3 100644
--- a/gcc/fortran/primary.cc
+++ b/gcc/fortran/primary.cc
@@ -3524,14 +3524,33 @@ gfc_match_structure_constructor (gfc_symbol *sym, gfc_expr **result)
 {
   match m;
   gfc_expr *e;
-  gfc_symtree *symtree;
+  gfc_symtree *symtree = NULL;
   bool t = true;
+  gfc_use_list *use_stmts = gfc_current_ns->use_stmts;
 
-  gfc_get_ha_sym_tree (sym->name, &symtree);
+  /* Check if we have a usable symtree that is use renamed.  */
+  for (; use_stmts; use_stmts = use_stmts->next)
+    {
+      if (!use_stmts->rename || use_stmts->rename->use_name[0] == 0)
+	continue;
+
+      if (!strcmp (use_stmts->rename->use_name, sym->name))
+	{
+	  symtree = gfc_find_symtree (gfc_current_ns->sym_root,
+				      use_stmts->rename->local_name);
+	  if (symtree && symtree->name[0] != 0
+	      && symtree->n.sym->attr.flavor == FL_PROCEDURE)
+	    break;
+	}
+    }
+
+  /* Otherwise find or create the symtree.  */
+  if (!symtree)
+    gfc_get_ha_sym_tree (sym->name, &symtree);
 
   e = gfc_get_expr ();
-  e->symtree = symtree;
   e->expr_type = EXPR_FUNCTION;
+  e->symtree = symtree;
   e->where = gfc_current_locus;
 
   gcc_assert (gfc_fl_struct (sym->attr.flavor)
diff --git a/gcc/testsuite/gfortran.dg/use_rename_12.f90 b/gcc/testsuite/gfortran.dg/use_rename_12.f90
new file mode 100644
index 00000000000..97f26f42f76
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/use_rename_12.f90
@@ -0,0 +1,37 @@
+! { dg-do compile }
+!
+! Test the fix for pr79685, which failed as in the comments below.
+!
+! Contributed by Juergen Reuter  <juergen.reuter@desy.de>
+!
+module omega_color
+  implicit none
+
+  type omega_color_factor
+     integer :: i
+  end type
+
+  type(omega_color_factor), parameter :: op = omega_color_factor (199)
+
+end module
+
+module foo
+  use omega_color, ocf => omega_color_factor, ocfp => op
+  implicit none
+
+  type(ocf) :: table_color_factors1 = ocf(42)
+  type(ocf) :: table_color_factors2
+  type(ocf) :: table_color_factors3 (2)
+  type(ocf) :: table_color_factors4
+  data table_color_factors2 / ocf(99) /        ! This failed in gfc_match_structure_constructor.
+  data table_color_factors3 / ocf(1), ocf(2) / ! ditto.
+  data table_color_factors4 / ocfp /
+end module
+
+  use foo
+  if (table_color_factors1%i .ne. 42) stop 1
+  if (table_color_factors2%i .ne. 99) stop 2
+  if (any (table_color_factors3%i .ne. [1,2])) stop 3
+  if (table_color_factors4%i .ne. 199) stop 4
+end
+

Re: *** SPAM *** [Patch, fortran] PR79685 - [12/13/14/15 Regression] ICE on valid code in gfc_match_structure_constructor

[Mikael Morin]

Hello,

Le 27/07/2024 à 11:25, Paul Richard Thomas a écrit :
> This patch is straightforward but I am still puzzled as to why it is 
> necessary for the particular case. Having looked at all the other chunks 
> of frontend code involving use renaming, it seems that the process just 
> works everywhere else. I tried putting the new code in gfc_find_symtree 
> but it caused some regressions unless I pinned it down to the specific 
> case of a structure constructor.
> 
I think it works as expected, symtrees have the local names, and symbols 
the original name, so if all that is available is the symbol, name 
lookup may not work correctly with renaming.  And I think that there are 
other places where it happens.

In this case, it seems the caller can provide the local name, which 
would avoid processing the use statements.  Did you try that?

Mikael

Re: *** SPAM *** [Patch, fortran] PR79685 - [12/13/14/15 Regression] ICE on valid code in gfc_match_structure_constructor

[Paul Richard Thomas]

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

Hi Mikael,

You were absolutely right. I looked at the caller and "just didn't get it".
Thanks. I will resubmit when I get back from a business trip.

Cordialement

Paul



On Sat, 27 Jul 2024 at 12:35, Mikael Morin <morin-mikael@orange.fr> wrote:

> Hello,
>
> Le 27/07/2024 à 11:25, Paul Richard Thomas a écrit :
> > This patch is straightforward but I am still puzzled as to why it is
> > necessary for the particular case. Having looked at all the other chunks
> > of frontend code involving use renaming, it seems that the process just
> > works everywhere else. I tried putting the new code in gfc_find_symtree
> > but it caused some regressions unless I pinned it down to the specific
> > case of a structure constructor.
> >
> I think it works as expected, symtrees have the local names, and symbols
> the original name, so if all that is available is the symbol, name
> lookup may not work correctly with renaming.  And I think that there are
> other places where it happens.
>
> In this case, it seems the caller can provide the local name, which
> would avoid processing the use statements.  Did you try that?
>
> Mikael
>

Re: [Patch, fortran] PR79685 - [12/13/14/15 Regression] ICE on valid code in gfc_match_structure_constructor

[Paul Richard Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 2014 bytes --]

I have attached the updated and rather simpler patch.

Thanks for the prod, Mikael!

Paul

Fortran: Fix ICE with structure constructor in data statement [PR79685]

2024-07-28  Paul Thomas  <pault@gcc.gnu.org>

gcc/fortran
PR fortran/79685
* decl.cc (match_data_constant): Find the symtree instead of
the symbol so the use renamed symbols are found. Pass this and
the derived type to gfc_match_structure_constructor.
* match.h: Update prototype of gfc_match_structure_contructor.
* primary.cc (gfc_match_structure_constructor): Remove call to
gfc_get_ha_sym_tree and use caller supplied symtree instead.

gcc/testsuite/
PR fortran/79685
* gfortran.dg/use_rename_12.f90: New test.


On Sat, 27 Jul 2024 at 22:04, Paul Richard Thomas <
paul.richard.thomas@gmail.com> wrote:

> Hi Mikael,
>
> You were absolutely right. I looked at the caller and "just didn't get
> it". Thanks. I will resubmit when I get back from a business trip.
>
> Cordialement
>
> Paul
>
>
>
> On Sat, 27 Jul 2024 at 12:35, Mikael Morin <morin-mikael@orange.fr> wrote:
>
>> Hello,
>>
>> Le 27/07/2024 à 11:25, Paul Richard Thomas a écrit :
>> > This patch is straightforward but I am still puzzled as to why it is
>> > necessary for the particular case. Having looked at all the other
>> chunks
>> > of frontend code involving use renaming, it seems that the process just
>> > works everywhere else. I tried putting the new code in gfc_find_symtree
>> > but it caused some regressions unless I pinned it down to the specific
>> > case of a structure constructor.
>> >
>> I think it works as expected, symtrees have the local names, and symbols
>> the original name, so if all that is available is the symbol, name
>> lookup may not work correctly with renaming.  And I think that there are
>> other places where it happens.
>>
>> In this case, it seems the caller can provide the local name, which
>> would avoid processing the use statements.  Did you try that?
>>
>> Mikael
>>
>

[-- Attachment #2: resubmit.diff --]
[-- Type: text/x-patch, Size: 3992 bytes --]

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index b8308aeee55..119c9dffa03 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -376,6 +376,7 @@ match_data_constant (gfc_expr **result)
   gfc_expr *expr;
   match m;
   locus old_loc;
+  gfc_symtree *symtree;
 
   m = gfc_match_literal_constant (&expr, 1);
   if (m == MATCH_YES)
@@ -436,9 +437,11 @@ match_data_constant (gfc_expr **result)
   if (m != MATCH_YES)
     return m;
 
-  if (gfc_find_symbol (name, NULL, 1, &sym))
+  if (gfc_find_sym_tree (name, NULL, 1, &symtree))
     return MATCH_ERROR;
 
+  sym = symtree->n.sym;
+
   if (sym && sym->attr.generic)
     dt_sym = gfc_find_dt_in_generic (sym);
 
@@ -452,7 +455,7 @@ match_data_constant (gfc_expr **result)
       return MATCH_ERROR;
     }
   else if (dt_sym && gfc_fl_struct (dt_sym->attr.flavor))
-    return gfc_match_structure_constructor (dt_sym, result);
+    return gfc_match_structure_constructor (dt_sym, symtree, result);
 
   /* Check to see if the value is an initialization array expression.  */
   if (sym->value->expr_type == EXPR_ARRAY)
diff --git a/gcc/fortran/match.h b/gcc/fortran/match.h
index c2b7d69c37c..519ad194a15 100644
--- a/gcc/fortran/match.h
+++ b/gcc/fortran/match.h
@@ -101,7 +101,7 @@ match gfc_match_call (void);
 /* We want to use this function to check for a common-block-name
    that can exist in a bind statement, so removed the "static"
    declaration of the function in match.cc. */
- 
+
 match gfc_match_common_name (char *name);
 
 match gfc_match_common (void);
@@ -302,7 +302,7 @@ match gfc_match_bind_c_stmt (void);
 match gfc_match_bind_c (gfc_symbol *, bool);
 
 /* primary.cc.  */
-match gfc_match_structure_constructor (gfc_symbol *, gfc_expr **);
+match gfc_match_structure_constructor (gfc_symbol *, gfc_symtree *, gfc_expr **);
 match gfc_match_variable (gfc_expr **, int);
 match gfc_match_equiv_variable (gfc_expr **);
 match gfc_match_actual_arglist (int, gfc_actual_arglist **, bool = false);
diff --git a/gcc/fortran/primary.cc b/gcc/fortran/primary.cc
index 76f6bcb8a78..9000b9cbbbc 100644
--- a/gcc/fortran/primary.cc
+++ b/gcc/fortran/primary.cc
@@ -3520,18 +3520,16 @@ gfc_convert_to_structure_constructor (gfc_expr *e, gfc_symbol *sym, gfc_expr **c
 
 
 match
-gfc_match_structure_constructor (gfc_symbol *sym, gfc_expr **result)
+gfc_match_structure_constructor (gfc_symbol *sym, gfc_symtree *symtree,
+				 gfc_expr **result)
 {
   match m;
   gfc_expr *e;
-  gfc_symtree *symtree;
   bool t = true;
 
-  gfc_get_ha_sym_tree (sym->name, &symtree);
-
   e = gfc_get_expr ();
-  e->symtree = symtree;
   e->expr_type = EXPR_FUNCTION;
+  e->symtree = symtree;
   e->where = gfc_current_locus;
 
   gcc_assert (gfc_fl_struct (sym->attr.flavor)
diff --git a/gcc/testsuite/gfortran.dg/use_rename_12.f90 b/gcc/testsuite/gfortran.dg/use_rename_12.f90
new file mode 100644
index 00000000000..97f26f42f76
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/use_rename_12.f90
@@ -0,0 +1,37 @@
+! { dg-do compile }
+!
+! Test the fix for pr79685, which failed as in the comments below.
+!
+! Contributed by Juergen Reuter  <juergen.reuter@desy.de>
+!
+module omega_color
+  implicit none
+
+  type omega_color_factor
+     integer :: i
+  end type
+
+  type(omega_color_factor), parameter :: op = omega_color_factor (199)
+
+end module
+
+module foo
+  use omega_color, ocf => omega_color_factor, ocfp => op
+  implicit none
+
+  type(ocf) :: table_color_factors1 = ocf(42)
+  type(ocf) :: table_color_factors2
+  type(ocf) :: table_color_factors3 (2)
+  type(ocf) :: table_color_factors4
+  data table_color_factors2 / ocf(99) /        ! This failed in gfc_match_structure_constructor.
+  data table_color_factors3 / ocf(1), ocf(2) / ! ditto.
+  data table_color_factors4 / ocfp /
+end module
+
+  use foo
+  if (table_color_factors1%i .ne. 42) stop 1
+  if (table_color_factors2%i .ne. 99) stop 2
+  if (any (table_color_factors3%i .ne. [1,2])) stop 3
+  if (table_color_factors4%i .ne. 199) stop 4
+end
+

Re: [Patch, fortran] PR79685 - [12/13/14/15 Regression] ICE on valid code in gfc_match_structure_constructor

[Mikael Morin]

Le 28/07/2024 à 11:24, Paul Richard Thomas a écrit :
> I have attached the updated and rather simpler patch.
> 
OK for master and backport.
Thanks for the patch.

> Thanks for the prod, Mikael!
> 
> Paul
> 
> Fortran: Fix ICE with structure constructor in data statement [PR79685]
> 
> 2024-07-28  Paul Thomas  <pault@gcc.gnu.org <mailto:pault@gcc.gnu.org>>
> 
> gcc/fortran
> PR fortran/79685
> * decl.cc (match_data_constant): Find the symtree instead of
> the symbol so the use renamed symbols are found. Pass this and
> the derived type to gfc_match_structure_constructor.
> * match.h: Update prototype of gfc_match_structure_contructor.
> * primary.cc (gfc_match_structure_constructor): Remove call to
> gfc_get_ha_sym_tree and use caller supplied symtree instead.
> 
> gcc/testsuite/
> PR fortran/79685
> * gfortran.dg/use_rename_12.f90: New test.
> 
> 
> On Sat, 27 Jul 2024 at 22:04, Paul Richard Thomas 
> <paul.richard.thomas@gmail.com <mailto:paul.richard.thomas@gmail.com>> 
> wrote:
> 
>     Hi Mikael,
> 
>     You were absolutely right. I looked at the caller and "just didn't
>     get it". Thanks. I will resubmit when I get back from a business trip.
> 
>     Cordialement
> 
>     Paul
> 
> 
> 
>     On Sat, 27 Jul 2024 at 12:35, Mikael Morin <morin-mikael@orange.fr
>     <mailto:morin-mikael@orange.fr>> wrote:
> 
>         Hello,
> 
>         Le 27/07/2024 à 11:25, Paul Richard Thomas a écrit :
>          > This patch is straightforward but I am still puzzled as to
>         why it is
>          > necessary for the particular case. Having looked at all the
>         other chunks
>          > of frontend code involving use renaming, it seems that the
>         process just
>          > works everywhere else. I tried putting the new code in
>         gfc_find_symtree
>          > but it caused some regressions unless I pinned it down to the
>         specific
>          > case of a structure constructor.
>          >
>         I think it works as expected, symtrees have the local names, and
>         symbols
>         the original name, so if all that is available is the symbol, name
>         lookup may not work correctly with renaming.  And I think that
>         there are
>         other places where it happens.
> 
>         In this case, it seems the caller can provide the local name, which
>         would avoid processing the use statements.  Did you try that?
> 
>         Mikael
>