[fortran] PR107900 Select type with intrinsic type inside associate causes ICE / Segmenation fault

Hi All,

The attached patch is amply described by the comments and the
changelog. It also includes the fix for the memory leak in decl.cc, as
promised some days ago.

OK for trunk?

Regards

Paul

PS This leaves 89645 and 99065 as the only real blockers to PR87477.
These will take a little while to fix. They come about because the
type of the associate name is determined by that of a derived type
function that hasn't been parsed at the time that component references
are being parsed. If the order of the contained procedures is
reversed, both test cases compile correctly. The fix will comprise
matching the component name to the accessible derived types, while
keeping track of all the references in case the match is ambiguous and
has to be fixed up later.

On Sat, Jun 17, 2023 at 10:14:43AM +0100, Paul Richard Thomas via Fortran wrote:
> 
> PS This leaves 89645 and 99065 as the only real blockers to PR87477.
>  

Hate to be the bearer of bad news, but Neil Carlson has added
a couple PRs about associate that may not be listed in 87447.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110224
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110033


For the second one, if I suppress the bogus error message,
the compiler runs into a new error message.  The second
error message indicates the associate-name is not an
unexpected coarray.  I haven't determined how to propagate
the information that the selector is a coarray to the 
associate-name.

Hi Paul,

On 6/17/23 11:14, Paul Richard Thomas via Gcc-patches wrote:
> Hi All,
>
> The attached patch is amply described by the comments and the
> changelog. It also includes the fix for the memory leak in decl.cc, as
> promised some days ago.
>
> OK for trunk?

I hate to say it, but you forgot to add the testcase again... :-(

The patch fixes your "extended" testcase in

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107900#c2

but the original one in comment ICEs for me here:

% gfc-14 pr107900.f90
f951: internal compiler error: Segmentation fault
0x1025c2f crash_signal
         ../../gcc-trunk/gcc/toplev.cc:314
0x9d31d3 resolve_select_type
         ../../gcc-trunk/gcc/fortran/resolve.cc:9791
0x9cef5e gfc_resolve_code(gfc_code*, gfc_namespace*)
         ../../gcc-trunk/gcc/fortran/resolve.cc:12588
0x9d2431 resolve_codes
         ../../gcc-trunk/gcc/fortran/resolve.cc:18057
0x9d24fe gfc_resolve(gfc_namespace*)
         ../../gcc-trunk/gcc/fortran/resolve.cc:18092
0x9cf0ee gfc_resolve(gfc_namespace*)
         ../../gcc-trunk/gcc/fortran/resolve.cc:18077
0x9cf0ee resolve_block_construct
         ../../gcc-trunk/gcc/fortran/resolve.cc:10971
0x9cf0ee gfc_resolve_code(gfc_code*, gfc_namespace*)
         ../../gcc-trunk/gcc/fortran/resolve.cc:12596
0x9d2431 resolve_codes
         ../../gcc-trunk/gcc/fortran/resolve.cc:18057
0x9d24fe gfc_resolve(gfc_namespace*)
         ../../gcc-trunk/gcc/fortran/resolve.cc:18092
0x9b11f1 resolve_all_program_units
         ../../gcc-trunk/gcc/fortran/parse.cc:6864
0x9b11f1 gfc_parse_file()
         ../../gcc-trunk/gcc/fortran/parse.cc:7120
0xa033ef gfc_be_parse_file
         ../../gcc-trunk/gcc/fortran/f95-lang.cc:229

It hits an assert here:

9790          st = gfc_find_symtree (ns->sym_root, name);
9791          gcc_assert (st->n.sym->assoc);

My tree is slightly modified, but the changes should not have
any effect here.

Can you please have a look, too?

Thanks,
Harald

> Regards
>
> Paul
>
> PS This leaves 89645 and 99065 as the only real blockers to PR87477.
> These will take a little while to fix. They come about because the
> type of the associate name is determined by that of a derived type
> function that hasn't been parsed at the time that component references
> are being parsed. If the order of the contained procedures is
> reversed, both test cases compile correctly. The fix will comprise
> matching the component name to the accessible derived types, while
> keeping track of all the references in case the match is ambiguous and
> has to be fixed up later.

Hi Harald,

Fixing the original testcase in this PR turned out to be slightly more
involved than I expected. However, it resulted in an open door to fix
some other PRs and the attached much larger patch.

This time, I did remember to include the testcases in the .diff :-)

I believe that, between the Change.Logs and the comments, it is
reasonably self-explanatory.

OK for trunk?

Regards

Paul

Fortran: Fix some bugs in associate [PR87477]

2023-06-20  Paul Thomas  <pault@gcc.gnu.org>

gcc/fortran
PR fortran/87477
PR fortran/88688
PR fortran/94380
PR fortran/107900
PR fortran/110224
* decl.cc (char_len_param_value): Fix memory leak.
(resolve_block_construct): Remove unnecessary static decls.
* expr.cc (gfc_is_ptr_fcn): New function.
(gfc_check_vardef_context): Use it to permit pointer function
result selectors to be used for associate names in variable
definition context.
* gfortran.h: Prototype for gfc_is_ptr_fcn.
* match.cc (build_associate_name): New function.
(gfc_match_select_type): Use the new function to replace inline
version and to build a new associate name for the case where
the supplied associate name is already used for that purpose.
* resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
associate names with pointer function targets to be used in
variable definition context.
* trans-decl.cc (gfc_get_symbol_decl): Unlimited polymorphic
variables need deferred initialisation of the vptr.
(gfc_trans_deferred_vars): Do the vptr initialisation.
* trans-stmt.cc (trans_associate_var): Ensure that a pointer
associate name points to the target of the selector and not
the selector itself.

gcc/testsuite/
PR fortran/87477
PR fortran/107900
* gfortran.dg/pr107900.f90 : New test

PR fortran/110224
* gfortran.dg/pr110224.f90 : New test

PR fortran/88688
* gfortran.dg/pr88688.f90 : New test

PR fortran/94380
* gfortran.dg/pr94380.f90 : New test

PR fortran/95398
* gfortran.dg/pr95398.f90 : Set -std=f2008, bump the line
numbers in the error tests by two and change the text in two.

Hi Paul,

On 6/20/23 12:54, Paul Richard Thomas via Gcc-patches wrote:
> Hi Harald,
> 
> Fixing the original testcase in this PR turned out to be slightly more
> involved than I expected. However, it resulted in an open door to fix
> some other PRs and the attached much larger patch.
> 
> This time, I did remember to include the testcases in the .diff :-)

indeed! :-)

I've only had a superficial look so far although it looks very good.
(I have to trust your experience with unlimited polymorphism.)

However, I was wondering about the following helper function:

+bool
+gfc_is_ptr_fcn (gfc_expr *e)
+{
+  return e != NULL && e->expr_type == EXPR_FUNCTION
+	      && (gfc_expr_attr (e).pointer
+		  || (e->ts.type == BT_CLASS
+		      && CLASS_DATA (e)->attr.class_pointer));
+}
+
+
  /* Copy a shape array.  */

Is there a case where gfc_expr_attr (e).pointer returns false
and you really need the || part?  Looking at gfc_expr_attr
and the present context, it might just not be necessary.

> I believe that, between the Change.Logs and the comments, it is
> reasonably self-explanatory.
> 
> OK for trunk?

OK from my side.

Thanks for the patch!

Harald

> Regards
> 
> Paul
> 
> Fortran: Fix some bugs in associate [PR87477]
> 
> 2023-06-20  Paul Thomas  <pault@gcc.gnu.org>
> 
> gcc/fortran
> PR fortran/87477
> PR fortran/88688
> PR fortran/94380
> PR fortran/107900
> PR fortran/110224
> * decl.cc (char_len_param_value): Fix memory leak.
> (resolve_block_construct): Remove unnecessary static decls.
> * expr.cc (gfc_is_ptr_fcn): New function.
> (gfc_check_vardef_context): Use it to permit pointer function
> result selectors to be used for associate names in variable
> definition context.
> * gfortran.h: Prototype for gfc_is_ptr_fcn.
> * match.cc (build_associate_name): New function.
> (gfc_match_select_type): Use the new function to replace inline
> version and to build a new associate name for the case where
> the supplied associate name is already used for that purpose.
> * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
> associate names with pointer function targets to be used in
> variable definition context.
> * trans-decl.cc (gfc_get_symbol_decl): Unlimited polymorphic
> variables need deferred initialisation of the vptr.
> (gfc_trans_deferred_vars): Do the vptr initialisation.
> * trans-stmt.cc (trans_associate_var): Ensure that a pointer
> associate name points to the target of the selector and not
> the selector itself.
> 
> gcc/testsuite/
> PR fortran/87477
> PR fortran/107900
> * gfortran.dg/pr107900.f90 : New test
> 
> PR fortran/110224
> * gfortran.dg/pr110224.f90 : New test
> 
> PR fortran/88688
> * gfortran.dg/pr88688.f90 : New test
> 
> PR fortran/94380
> * gfortran.dg/pr94380.f90 : New test
> 
> PR fortran/95398
> * gfortran.dg/pr95398.f90 : Set -std=f2008, bump the line
> numbers in the error tests by two and change the text in two.

Committed as r14-2022-g577223aebc7acdd31e62b33c1682fe54a622ae27

Thanks for the help and the review Harald. Thanks to Steve too for
picking up Neil Carlson's bugs.

Cheers

Paul

On Tue, 20 Jun 2023 at 22:57, Harald Anlauf <anlauf@gmx.de> wrote:
>
> Hi Paul,
>
> On 6/20/23 12:54, Paul Richard Thomas via Gcc-patches wrote:
> > Hi Harald,
> >
> > Fixing the original testcase in this PR turned out to be slightly more
> > involved than I expected. However, it resulted in an open door to fix
> > some other PRs and the attached much larger patch.
> >
> > This time, I did remember to include the testcases in the .diff :-)
>
> indeed! :-)
>
> I've only had a superficial look so far although it looks very good.
> (I have to trust your experience with unlimited polymorphism.)
>
> However, I was wondering about the following helper function:
>
> +bool
> +gfc_is_ptr_fcn (gfc_expr *e)
> +{
> +  return e != NULL && e->expr_type == EXPR_FUNCTION
> +             && (gfc_expr_attr (e).pointer
> +                 || (e->ts.type == BT_CLASS
> +                     && CLASS_DATA (e)->attr.class_pointer));
> +}
> +
> +
>   /* Copy a shape array.  */
>
> Is there a case where gfc_expr_attr (e).pointer returns false
> and you really need the || part?  Looking at gfc_expr_attr
> and the present context, it might just not be necessary.
>
> > I believe that, between the Change.Logs and the comments, it is
> > reasonably self-explanatory.
> >
> > OK for trunk?
>
> OK from my side.
>
> Thanks for the patch!
>
> Harald
>
> > Regards
> >
> > Paul
> >
> > Fortran: Fix some bugs in associate [PR87477]
> >
> > 2023-06-20  Paul Thomas  <pault@gcc.gnu.org>
> >
> > gcc/fortran
> > PR fortran/87477
> > PR fortran/88688
> > PR fortran/94380
> > PR fortran/107900
> > PR fortran/110224
> > * decl.cc (char_len_param_value): Fix memory leak.
> > (resolve_block_construct): Remove unnecessary static decls.
> > * expr.cc (gfc_is_ptr_fcn): New function.
> > (gfc_check_vardef_context): Use it to permit pointer function
> > result selectors to be used for associate names in variable
> > definition context.
> > * gfortran.h: Prototype for gfc_is_ptr_fcn.
> > * match.cc (build_associate_name): New function.
> > (gfc_match_select_type): Use the new function to replace inline
> > version and to build a new associate name for the case where
> > the supplied associate name is already used for that purpose.
> > * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
> > associate names with pointer function targets to be used in
> > variable definition context.
> > * trans-decl.cc (gfc_get_symbol_decl): Unlimited polymorphic
> > variables need deferred initialisation of the vptr.
> > (gfc_trans_deferred_vars): Do the vptr initialisation.
> > * trans-stmt.cc (trans_associate_var): Ensure that a pointer
> > associate name points to the target of the selector and not
> > the selector itself.
> >
> > gcc/testsuite/
> > PR fortran/87477
> > PR fortran/107900
> > * gfortran.dg/pr107900.f90 : New test
> >
> > PR fortran/110224
> > * gfortran.dg/pr110224.f90 : New test
> >
> > PR fortran/88688
> > * gfortran.dg/pr88688.f90 : New test
> >
> > PR fortran/94380
> > * gfortran.dg/pr94380.f90 : New test
> >
> > PR fortran/95398
> > * gfortran.dg/pr95398.f90 : Set -std=f2008, bump the line
> > numbers in the error tests by two and change the text in two.
>

On Wed, Jun 21, 2023 at 05:12:22PM +0100, Paul Richard Thomas wrote:
> Committed as r14-2022-g577223aebc7acdd31e62b33c1682fe54a622ae27
> 
> Thanks for the help and the review Harald. Thanks to Steve too for
> picking up Neil Carlson's bugs.
> 

It's only natural.  You fix bugs in a long desired feature,
and people will start to use that feature more. 

I always look at Neil's bug reports.  They're typically
concise code snippets and have cross references to the
Fortran standard.  Unfortunately, I lack the ability to
fix them. :(

Hi Paul,

while I only had a minor question regarding gfc_is_ptr_fcn(),
can you still try to enlighten me why that second part
was necessary?  (I believed it to be redundant and may have
overlooked the obvious.)

Cheers,
Harald

On 6/21/23 18:12, Paul Richard Thomas via Gcc-patches wrote:
> Committed as r14-2022-g577223aebc7acdd31e62b33c1682fe54a622ae27
>
> Thanks for the help and the review Harald. Thanks to Steve too for
> picking up Neil Carlson's bugs.
>
> Cheers
>
> Paul
>
> On Tue, 20 Jun 2023 at 22:57, Harald Anlauf <anlauf@gmx.de> wrote:
>>
>> Hi Paul,
>>
>> On 6/20/23 12:54, Paul Richard Thomas via Gcc-patches wrote:
>>> Hi Harald,
>>>
>>> Fixing the original testcase in this PR turned out to be slightly more
>>> involved than I expected. However, it resulted in an open door to fix
>>> some other PRs and the attached much larger patch.
>>>
>>> This time, I did remember to include the testcases in the .diff :-)
>>
>> indeed! :-)
>>
>> I've only had a superficial look so far although it looks very good.
>> (I have to trust your experience with unlimited polymorphism.)
>>
>> However, I was wondering about the following helper function:
>>
>> +bool
>> +gfc_is_ptr_fcn (gfc_expr *e)
>> +{
>> +  return e != NULL && e->expr_type == EXPR_FUNCTION
>> +             && (gfc_expr_attr (e).pointer
>> +                 || (e->ts.type == BT_CLASS
>> +                     && CLASS_DATA (e)->attr.class_pointer));
>> +}
>> +
>> +
>>    /* Copy a shape array.  */
>>
>> Is there a case where gfc_expr_attr (e).pointer returns false
>> and you really need the || part?  Looking at gfc_expr_attr
>> and the present context, it might just not be necessary.
>>
>>> I believe that, between the Change.Logs and the comments, it is
>>> reasonably self-explanatory.
>>>
>>> OK for trunk?
>>
>> OK from my side.
>>
>> Thanks for the patch!
>>
>> Harald
>>
>>> Regards
>>>
>>> Paul
>>>
>>> Fortran: Fix some bugs in associate [PR87477]
>>>
>>> 2023-06-20  Paul Thomas  <pault@gcc.gnu.org>
>>>
>>> gcc/fortran
>>> PR fortran/87477
>>> PR fortran/88688
>>> PR fortran/94380
>>> PR fortran/107900
>>> PR fortran/110224
>>> * decl.cc (char_len_param_value): Fix memory leak.
>>> (resolve_block_construct): Remove unnecessary static decls.
>>> * expr.cc (gfc_is_ptr_fcn): New function.
>>> (gfc_check_vardef_context): Use it to permit pointer function
>>> result selectors to be used for associate names in variable
>>> definition context.
>>> * gfortran.h: Prototype for gfc_is_ptr_fcn.
>>> * match.cc (build_associate_name): New function.
>>> (gfc_match_select_type): Use the new function to replace inline
>>> version and to build a new associate name for the case where
>>> the supplied associate name is already used for that purpose.
>>> * resolve.cc (resolve_assoc_var): Call gfc_is_ptr_fcn to allow
>>> associate names with pointer function targets to be used in
>>> variable definition context.
>>> * trans-decl.cc (gfc_get_symbol_decl): Unlimited polymorphic
>>> variables need deferred initialisation of the vptr.
>>> (gfc_trans_deferred_vars): Do the vptr initialisation.
>>> * trans-stmt.cc (trans_associate_var): Ensure that a pointer
>>> associate name points to the target of the selector and not
>>> the selector itself.
>>>
>>> gcc/testsuite/
>>> PR fortran/87477
>>> PR fortran/107900
>>> * gfortran.dg/pr107900.f90 : New test
>>>
>>> PR fortran/110224
>>> * gfortran.dg/pr110224.f90 : New test
>>>
>>> PR fortran/88688
>>> * gfortran.dg/pr88688.f90 : New test
>>>
>>> PR fortran/94380
>>> * gfortran.dg/pr94380.f90 : New test
>>>
>>> PR fortran/95398
>>> * gfortran.dg/pr95398.f90 : Set -std=f2008, bump the line
>>> numbers in the error tests by two and change the text in two.
>>
>
>

Hi!

First of all, many thanks for the patch!
If i may, i have a question concerning the chosen style in the error
message and one nitpick concerning a return type though, the latter
primarily prompting this reply.

On Tue, 20 Jun 2023 11:54:25 +0100
Paul Richard Thomas via Fortran <fortran@gcc.gnu.org> wrote:

> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
> index d5cfbe0cc55..c960dfeabd9 100644
> --- a/gcc/fortran/expr.cc
> +++ b/gcc/fortran/expr.cc

> @@ -6470,6 +6480,22 @@ gfc_check_vardef_context (gfc_expr* e, bool pointer, bool alloc_obj,
>  	    }
>  	  return false;
>  	}
> +      else if (context && gfc_is_ptr_fcn (assoc->target))
> +	{
> +	  if (!gfc_notify_std (GFC_STD_F2018, "%qs at %L associated to "
> +			       "pointer function target being used in a "
> +			       "variable definition context (%s)", name,
> +			       &e->where, context))

I'm curious why you decided to put context in braces and not simply use
quotes as per %qs?

> +	    return false;
> +	  else if (gfc_has_vector_index (e))
> +	    {
> +	      gfc_error ("%qs at %L associated to vector-indexed target"
> +			 " cannot be used in a variable definition"
> +			 " context (%s)",
> +			 name, &e->where, context);

Ditto.

> diff --git a/gcc/fortran/match.cc b/gcc/fortran/match.cc
> index e7be7fddc64..0e4b5440393 100644
> --- a/gcc/fortran/match.cc
> +++ b/gcc/fortran/match.cc
> @@ -6377,6 +6377,39 @@ build_class_sym:
>  }
> 
> 
> +/* Build the associate name  */
> +static int
> +build_associate_name (const char *name, gfc_expr **e1, gfc_expr **e2)
> +{

> +    return 1;

> +  return 0;
> +}

I've gone through the frontend recently and changed several such
boolean functions to use bool where appropriate. May i ask folks to use
narrower types in new code, please?
Iff later in the pipeline it is considered appropriate or benefical to
promote types, these will eventually be promoted.

> diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
> index e6a4337c0d2..18589e17843 100644
> --- a/gcc/fortran/trans-decl.cc
> +++ b/gcc/fortran/trans-decl.cc

> @@ -1906,6 +1915,7 @@ gfc_get_symbol_decl (gfc_symbol * sym)
>  	gcc_assert (!sym->value || sym->value->expr_type == EXPR_NULL);
>      }
> 
> +

ISTM that the addition of vertical whitespace like here is in
contradiction with the coding style.

Please kindly excuse my comment and, again, thanks!

>    gfc_finish_var_decl (decl, sym);
> 
>    if (sym->ts.type == BT_CHARACTER)

Hi Both,

> while I only had a minor question regarding gfc_is_ptr_fcn(),
> can you still try to enlighten me why that second part
> was necessary?  (I believed it to be redundant and may have
> overlooked the obvious.)

Blast! I forgot about checking that. Lurking in the back of my mind
and going back to the first days of OOP in gfortran is a distinction
between a class entity with the pointer attribute and one whose data
component has the class_pointer attribute. I'll check it out and do
whatever is needed.


> > +      else if (context && gfc_is_ptr_fcn (assoc->target))
> > +     {
> > +       if (!gfc_notify_std (GFC_STD_F2018, "%qs at %L associated to "
> > +                            "pointer function target being used in a "
> > +                            "variable definition context (%s)", name,
> > +                            &e->where, context))
>
> I'm curious why you decided to put context in braces and not simply use
> quotes as per %qs?

That's the way it's done in the preceding errors. I had to keep the
context in context, so to speak.

> > +/* Build the associate name  */
> > +static int
> > +build_associate_name (const char *name, gfc_expr **e1, gfc_expr **e2)
> > +{
>
> > +    return 1;
>
> > +  return 0;
> > +}
>
> I've gone through the frontend recently and changed several such
> boolean functions to use bool where appropriate. May i ask folks to use
> narrower types in new code, please?
> Iff later in the pipeline it is considered appropriate or benefical to
> promote types, these will eventually be promoted.
>
> > diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
> > index e6a4337c0d2..18589e17843 100644
> > --- a/gcc/fortran/trans-decl.cc
> > +++ b/gcc/fortran/trans-decl.cc
>
> > @@ -1906,6 +1915,7 @@ gfc_get_symbol_decl (gfc_symbol * sym)
> >       gcc_assert (!sym->value || sym->value->expr_type == EXPR_NULL);
> >      }
> >
> > +
>

'twas accidental. There had previously been another version of the fix
that I commented out and the extra line crept in when I deleted it.
Thanks for the spot.

>
> Please kindly excuse my comment and, again, thanks!
>
> >    gfc_finish_var_decl (decl, sym);
> >
> >    if (sym->ts.type == BT_CHARACTER)

Regards

Paul