Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Paul Richard Thomas]

Hi Andre,

Committed to trunk as revision 241403.

Thanks for the review.

Paul

On 20 October 2016 at 11:43, Andre Vehreschild  wrote:
> Hi Paul,
>
> after looking at your patch again, I understood why these extra copies are
> needed. May be a comment would prevent future gfortran hackers from trying to
> remove them again.
>
> The patch is ok for me. Thanks for working on this.
>
> Regards,
> Andre
>
>
> On Wed, 19 Oct 2016 20:02:14 +0200
> Andre Vehreschild  wrote:
>
>> Hi Paul,
>>
>> I am not completely through with your patch, but what jumped into my eye was
>> that you copy ref in resolve_select_type and again in fixup_array_ref, when
>> you use it? May be I oversee something. You are more into this code. Is the
>> double copying necessary (line 49 and 82 as well as 95, respectively). IMHO
>> the copy in line 49 could be sufficient.
>>
>> I look into it tomorrow more thoroughly. Please wait before submitting a new
>> version. May be I see something more :-)
>>
>> So far, thanks for working on this.
>>
>> Regards,
>>   Andre
>>
>> On Wed, 19 Oct 2016 09:28:39 +0200
>> Paul Richard Thomas  wrote:
>>
>> > Dear Andre,
>> >
>> > Following our exchange yesterday, I have eliminated the modification
>> > to trans_associate_var and have corrected the offending expressions in
>> > resolve.c(fixup_array_ref).
>> >
>> > Please find attached the corrected patch.
>> >
>> > Cheers
>> >
>> > Paul
>> >
>> > 2016-10-19  Paul Thomas  
>> >
>> > PR fortran/69566
>> > * resolve.c (fixup_array_ref): New function.
>> > (resolve_select_type): Gather up the rank and array reference,
>> > if any, from the selector. Fix up the 'associate name' and the
>> > 'associate entities' as necessary.
>> > * trans-expr.c (gfc_conv_class_to_class): If the symbol backend
>> > decl is a FUNCTION_DECL, use the 'fake_result_decl' instead.
>> >
>> > 2016-10-19  Paul Thomas  
>> >
>> > PR fortran/69566
>> > * gfortran.dg/select_type_37.f03: New test.
>> >
>> > On 18 October 2016 at 18:16, Andre Vehreschild  wrote:
>> > > Hi Paul,
>> > >
>> > >> For reasons I don't understand, sometimes the expression type comes
>> > >> through as BT_DERIVED, whilst the symbol is BT_CLASS. I could repair
>> > >> this in resolve.c(fixup_array_ref) if you think that would be
>> > >> cleaner.
>> > >
>> > > I think that I figured the rule:
>> > >
>> > > - when no _class-ref is present, then the type is BT_CLASS,
>> > > - as soon as a _class-ref is present the type is BT_DERIVED.
>> > >
>> > > There is an attr.is_class. Would that be an alternative? I don't know how
>> > > reliable it is set.
>> > >
>> > >> > I am regression testing my polymorhpic class patch at the moment,
>> > >> > therefore I can't test.
>> > >>
>> > >> OK - I can wait. I have quite a few other things to do :-(
>> > >
>> > > I found an error in my patch that only manifests itself with an
>> > > optimization level great than 0. Now I am searching, never having done
>> > > anything there.
>> > >
>> > > - Andre
>> > > --
>> > > Andre Vehreschild * Email: vehre ad gmx dot de
>> >
>> >
>> >
>>
>>
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de



-- 
The difference between genius and stupidity is; genius has its limits.

Albert Einstein

Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Andre Vehreschild]

Hi Paul,

after looking at your patch again, I understood why these extra copies are
needed. May be a comment would prevent future gfortran hackers from trying to
remove them again. 

The patch is ok for me. Thanks for working on this.

Regards,
Andre


On Wed, 19 Oct 2016 20:02:14 +0200
Andre Vehreschild  wrote:

> Hi Paul,
> 
> I am not completely through with your patch, but what jumped into my eye was
> that you copy ref in resolve_select_type and again in fixup_array_ref, when
> you use it? May be I oversee something. You are more into this code. Is the
> double copying necessary (line 49 and 82 as well as 95, respectively). IMHO
> the copy in line 49 could be sufficient.
> 
> I look into it tomorrow more thoroughly. Please wait before submitting a new
> version. May be I see something more :-)
> 
> So far, thanks for working on this.
> 
> Regards,
>   Andre
> 
> On Wed, 19 Oct 2016 09:28:39 +0200
> Paul Richard Thomas  wrote:
> 
> > Dear Andre,
> > 
> > Following our exchange yesterday, I have eliminated the modification
> > to trans_associate_var and have corrected the offending expressions in
> > resolve.c(fixup_array_ref).
> > 
> > Please find attached the corrected patch.
> > 
> > Cheers
> > 
> > Paul
> > 
> > 2016-10-19  Paul Thomas  
> > 
> > PR fortran/69566
> > * resolve.c (fixup_array_ref): New function.
> > (resolve_select_type): Gather up the rank and array reference,
> > if any, from the selector. Fix up the 'associate name' and the
> > 'associate entities' as necessary.
> > * trans-expr.c (gfc_conv_class_to_class): If the symbol backend
> > decl is a FUNCTION_DECL, use the 'fake_result_decl' instead.
> > 
> > 2016-10-19  Paul Thomas  
> > 
> > PR fortran/69566
> > * gfortran.dg/select_type_37.f03: New test.
> > 
> > On 18 October 2016 at 18:16, Andre Vehreschild  wrote:  
> > > Hi Paul,
> > >
> > >> For reasons I don't understand, sometimes the expression type comes
> > >> through as BT_DERIVED, whilst the symbol is BT_CLASS. I could repair
> > >> this in resolve.c(fixup_array_ref) if you think that would be
> > >> cleaner.
> > >
> > > I think that I figured the rule:
> > >
> > > - when no _class-ref is present, then the type is BT_CLASS,
> > > - as soon as a _class-ref is present the type is BT_DERIVED.
> > >
> > > There is an attr.is_class. Would that be an alternative? I don't know how
> > > reliable it is set.
> > >
> > >> > I am regression testing my polymorhpic class patch at the moment,
> > >> > therefore I can't test.
> > >>
> > >> OK - I can wait. I have quite a few other things to do :-(
> > >
> > > I found an error in my patch that only manifests itself with an
> > > optimization level great than 0. Now I am searching, never having done
> > > anything there.
> > >
> > > - Andre
> > > --
> > > Andre Vehreschild * Email: vehre ad gmx dot de
> > 
> > 
> >   
> 
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Andre Vehreschild]

Hi Paul,

I am not completely through with your patch, but what jumped into my eye was
that you copy ref in resolve_select_type and again in fixup_array_ref, when you
use it? May be I oversee something. You are more into this code. Is the double
copying necessary (line 49 and 82 as well as 95, respectively). IMHO the copy
in line 49 could be sufficient.

I look into it tomorrow more thoroughly. Please wait before submitting a new
version. May be I see something more :-)

So far, thanks for working on this.

Regards,
Andre

On Wed, 19 Oct 2016 09:28:39 +0200
Paul Richard Thomas  wrote:

> Dear Andre,
> 
> Following our exchange yesterday, I have eliminated the modification
> to trans_associate_var and have corrected the offending expressions in
> resolve.c(fixup_array_ref).
> 
> Please find attached the corrected patch.
> 
> Cheers
> 
> Paul
> 
> 2016-10-19  Paul Thomas  
> 
> PR fortran/69566
> * resolve.c (fixup_array_ref): New function.
> (resolve_select_type): Gather up the rank and array reference,
> if any, from the selector. Fix up the 'associate name' and the
> 'associate entities' as necessary.
> * trans-expr.c (gfc_conv_class_to_class): If the symbol backend
> decl is a FUNCTION_DECL, use the 'fake_result_decl' instead.
> 
> 2016-10-19  Paul Thomas  
> 
> PR fortran/69566
> * gfortran.dg/select_type_37.f03: New test.
> 
> On 18 October 2016 at 18:16, Andre Vehreschild  wrote:
> > Hi Paul,
> >  
> >> For reasons I don't understand, sometimes the expression type comes
> >> through as BT_DERIVED, whilst the symbol is BT_CLASS. I could repair
> >> this in resolve.c(fixup_array_ref) if you think that would be cleaner.  
> >
> > I think that I figured the rule:
> >
> > - when no _class-ref is present, then the type is BT_CLASS,
> > - as soon as a _class-ref is present the type is BT_DERIVED.
> >
> > There is an attr.is_class. Would that be an alternative? I don't know how
> > reliable it is set.
> >  
> >> > I am regression testing my polymorhpic class patch at the moment,
> >> > therefore I can't test.  
> >>
> >> OK - I can wait. I have quite a few other things to do :-(  
> >
> > I found an error in my patch that only manifests itself with an optimization
> > level great than 0. Now I am searching, never having done anything there.
> >
> > - Andre
> > --
> > Andre Vehreschild * Email: vehre ad gmx dot de  
> 
> 
> 


-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Paul Richard Thomas]

Dear Andre,

Following our exchange yesterday, I have eliminated the modification
to trans_associate_var and have corrected the offending expressions in
resolve.c(fixup_array_ref).

Please find attached the corrected patch.

Cheers

Paul

2016-10-19  Paul Thomas  

PR fortran/69566
* resolve.c (fixup_array_ref): New function.
(resolve_select_type): Gather up the rank and array reference,
if any, from the selector. Fix up the 'associate name' and the
'associate entities' as necessary.
* trans-expr.c (gfc_conv_class_to_class): If the symbol backend
decl is a FUNCTION_DECL, use the 'fake_result_decl' instead.

2016-10-19  Paul Thomas  

PR fortran/69566
* gfortran.dg/select_type_37.f03: New test.

On 18 October 2016 at 18:16, Andre Vehreschild  wrote:
> Hi Paul,
>
>> For reasons I don't understand, sometimes the expression type comes
>> through as BT_DERIVED, whilst the symbol is BT_CLASS. I could repair
>> this in resolve.c(fixup_array_ref) if you think that would be cleaner.
>
> I think that I figured the rule:
>
> - when no _class-ref is present, then the type is BT_CLASS,
> - as soon as a _class-ref is present the type is BT_DERIVED.
>
> There is an attr.is_class. Would that be an alternative? I don't know how
> reliable it is set.
>
>> > I am regression testing my polymorhpic class patch at the moment, therefore
>> > I can't test.
>>
>> OK - I can wait. I have quite a few other things to do :-(
>
> I found an error in my patch that only manifests itself with an optimization
> level great than 0. Now I am searching, never having done anything there.
>
> - Andre
> --
> Andre Vehreschild * Email: vehre ad gmx dot de



-- 
The difference between genius and stupidity is; genius has its limits.

Albert Einstein
Index: gcc/fortran/resolve.c
===
*** gcc/fortran/resolve.c   (revision 241326)
--- gcc/fortran/resolve.c   (working copy)
*** resolve_assoc_var (gfc_symbol* sym, bool
*** 8327,8332 
--- 8327,8374 
  }
  
  
+ /* Ensure that SELECT TYPE expressions have the correct rank and a full
+array reference, where necessary.  The symbols are artificial and so
+the dimension attribute and arrayspec can also be set.  In addition,
+sometimes the expr1 arrives as BT_DERIVED, when the symbol is BT_CLASS.
+This is corrected here as well.*/
+ 
+ static void
+ fixup_array_ref (gfc_expr **expr1, gfc_expr *expr2,
+int rank, gfc_ref *ref)
+ {
+   gfc_ref *nref = (*expr1)->ref;
+   gfc_symbol *sym1 = (*expr1)->symtree->n.sym;
+   gfc_symbol *sym2 = expr2 ? expr2->symtree->n.sym : NULL;
+   (*expr1)->rank = rank;
+   if (sym1->ts.type == BT_CLASS)
+ {
+   if ((*expr1)->ts.type != BT_CLASS)
+   (*expr1)->ts = sym1->ts;
+ 
+   CLASS_DATA (sym1)->attr.dimension = 1;
+   if (CLASS_DATA (sym1)->as == NULL && sym2)
+   CLASS_DATA (sym1)->as
+   = gfc_copy_array_spec (CLASS_DATA (sym2)->as);
+ }
+   else
+ {
+   sym1->attr.dimension = 1;
+   if (sym1->as == NULL && sym2)
+   sym1->as = gfc_copy_array_spec (sym2->as);
+ }
+ 
+   for (; nref; nref = nref->next)
+ if (nref->next == NULL)
+   break;
+ 
+   if (ref && nref && nref->type != REF_ARRAY)
+ nref->next = gfc_copy_ref (ref);
+   else if (ref && !nref)
+ (*expr1)->ref = gfc_copy_ref (ref);
+ }
+ 
+ 
  /* Resolve a SELECT TYPE statement.  */
  
  static void
*** resolve_select_type (gfc_code *code, gfc
*** 8341,8346 
--- 8383,8390 
gfc_namespace *ns;
int error = 0;
int charlen = 0;
+   int rank = 0;
+   gfc_ref* ref = NULL;
  
ns = code->ext.block.ns;
gfc_resolve (ns);
*** resolve_select_type (gfc_code *code, gfc
*** 8468,8473 
--- 8512,8542 
else
  code->ext.block.assoc = NULL;
  
+   /* Ensure that the selector rank and arrayspec are available to
+  correct expressions in which they might be missing.  */
+   if (code->expr2 && code->expr2->rank)
+ {
+   rank = code->expr2->rank;
+   for (ref = code->expr2->ref; ref; ref = ref->next)
+   if (ref->next == NULL)
+ break;
+   if (ref && ref->type == REF_ARRAY)
+   ref = gfc_copy_ref (ref);
+ 
+   /* Fixup expr1 if necessary.  */
+   if (rank)
+   fixup_array_ref (>expr1, code->expr2, rank, ref);
+ }
+   else if (code->expr1->rank)
+ {
+   rank = code->expr1->rank;
+   for (ref = code->expr1->ref; ref; ref = ref->next)
+   if (ref->next == NULL)
+ break;
+   if (ref && ref->type == REF_ARRAY)
+   ref = gfc_copy_ref (ref);
+ }
+ 
/* Add EXEC_SELECT to switch on type.  */
new_st = gfc_get_code (code->op);
new_st->expr1 = code->expr1;
*** resolve_select_type (gfc_code *code, gfc
*** 8533,8539 
st->n.sym->assoc->target = gfc_get_variable_expr 

Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Andre Vehreschild]

Hi Paul,

> For reasons I don't understand, sometimes the expression type comes
> through as BT_DERIVED, whilst the symbol is BT_CLASS. I could repair
> this in resolve.c(fixup_array_ref) if you think that would be cleaner.

I think that I figured the rule:

- when no _class-ref is present, then the type is BT_CLASS,
- as soon as a _class-ref is present the type is BT_DERIVED. 

There is an attr.is_class. Would that be an alternative? I don't know how
reliable it is set.

> > I am regression testing my polymorhpic class patch at the moment, therefore
> > I can't test.  
> 
> OK - I can wait. I have quite a few other things to do :-(

I found an error in my patch that only manifests itself with an optimization
level great than 0. Now I am searching, never having done anything there.

- Andre
-- 
Andre Vehreschild * Email: vehre ad gmx dot de 

Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Paul Richard Thomas]

Hi Andre,

Thanks for a quick response:

> You can use
>
>|| (e->symtree && UNLIMITED_POLY (e->symtree->n.sym));

Ah yes, you are quite right.

> here. UNLIMITED_POLY does all the checks. I am still wondering whether this is
> necessary? The symtree is set for expr_type == { EXPR_VARIABLE, EXPR_FUNCTION 
> }
> both of those should correctly resolve to an unlimited poly ts. Is this a
> left-over?

For reasons I don't understand, sometimes the expression type comes
through as BT_DERIVED, whilst the symbol is BT_CLASS. I could repair
this in resolve.c(fixup_array_ref) if you think that would be cleaner.

>
> I am regression testing my polymorhpic class patch at the moment, therefore I
> can't test.

OK - I can wait. I have quite a few other things to do :-(

Cheers

Paul


-- 
The difference between genius and stupidity is; genius has its limits.

Albert Einstein

Re: [Patch, fortran] PR69566 - Failure of SELECT TYPE with unlimited polymorphic function result

[Andre Vehreschild]

Hi Paul,

> Index: gcc/fortran/trans-stmt.c
> ===
> *** gcc/fortran/trans-stmt.c  (revision 241273)
> --- gcc/fortran/trans-stmt.c  (working copy)
> *** trans_associate_var (gfc_symbol *sym, gf
> *** 1517,1523 
>   && (gfc_is_class_scalar_expr (e)
>   || gfc_is_class_array_ref (e, NULL));
> 
> !   unlimited = UNLIMITED_POLY (e);
> 
> /* Assignments to the string length need to be generated, when
>( sym is a char array or
> --- 1517,1526 
>   && (gfc_is_class_scalar_expr (e)
>   || gfc_is_class_array_ref (e, NULL));
> 
> !   unlimited = UNLIMITED_POLY (e)
> ! || (e->symtree && e->symtree->n.sym
> ! && e->symtree->n.sym->ts.type == BT_CLASS
> ! && UNLIMITED_POLY (e->symtree->n.sym));> 

You can use 

   || (e->symtree && UNLIMITED_POLY (e->symtree->n.sym));

here. UNLIMITED_POLY does all the checks. I am still wondering whether this is
necessary? The symtree is set for expr_type == { EXPR_VARIABLE, EXPR_FUNCTION }
both of those should correctly resolve to an unlimited poly ts. Is this a
left-over? 

I am regression testing my polymorhpic class patch at the moment, therefore I
can't test.

> 
> /* Assignments to the string length need to be generated, when
>( sym is a char array or

- Andre

On Tue, 18 Oct 2016 16:29:54 +0200
Paul Richard Thomas  wrote:


> Dear All,
> 
> This bug was caused by 'associate name' and 'associate entity'
> expressions being incomplete when the 'selector' was an intrinsic
> function result. I tried to fix this at source, in match_select _type
> and gfc_get_variable_expr, but caused a vast number of breakages.
> Undoubtedly, this would be the ecologically sound way to proceed but
> applying fixups in resolve_select_type, gfc_conv_class_to_class and
> trans_associate_var proved to be the path of least resistance.
> 
> Depending on which form of select type is used, the defective
> expressions either lacked the correct value for rank, a full array
> reference, a symbol with an array spec or had type BT_DERIVED for a
> polymorphic symbol; these either singly or in combination depending of
> the form of select type.
> 
> The patch, taken with the ChangeLogs, is fairly self-explanatory.
> Please note that I have suppressed whitespace changes (suppression of
> trailing blanks), so use patch with the -l option.
> 
> Bootstraps and regtests on FC21/x86_64 - OK for trunk?
> 
> Best regards
> 
> Paul
> 
> 2016-10-18  Paul Thomas  
> 
> PR fortran/69566
> * resolve.c (fixup_array_ref): New function.
> (resolve_select_type): Gather up the rank and array reference,
> if any, from the selector. Fix up the 'associate name' and the
> 'associate entities' as necessary.
> * trans-expr.c (gfc_conv_class_to_class): If the symbol backend
> decl is a FUNCTION_DECL, use the 'fake_result_decl' instead.
> * trans-stmt.c (trans_associate_var): Extend 'unlimited' to
> include expressions which are themsleves not unlimited
> polymorphic but the symbol is.
> 
> 2016-10-18  Paul Thomas  
> 
> PR fortran/69566
> * gfortran.dg/select_type_37.f03: New test.


-- 
Andre Vehreschild * Email: vehre ad gmx dot de