Re: [Patch, fortran] PR99602 - [11 regression] runtime error: pointer actual argument not associated

2021-03-29 Thread Paul Richard Thomas via Fortran
Hi Tobias,

An earlier version of the patch, without the exclusion of unlimited
polymorphic expressions caused several regressions. However, omitting the
exclusion now causes no regressions.  I forgot to go back to this wrinkle.
I have included your testcases with appropriate attribution and pushed as
297363774e6a5dca2f46a85ab086f1d9e59431ac .

Thanks for the review and the additional testcases.

Paul



On Fri, 26 Mar 2021 at 11:22, Tobias Burnus  wrote:

> Hi Paul,
>
> I do not understand the !UNLIMITED_POLY(fsym) part of the patch.
> In particular, your patch causes foo.f90 to fail by wrongly diagnosting:
>
>Fortran runtime error: Pointer actual argument 'cptr' is not associated
>
> I have only did some light tests – but it seems that just removing
> '&& !UNLIMITED_POLY(fsym)' seems to be enough. (But I did not run
> the testsuite.)
>
> Hence:
> - Please include the attached testcases or some variants of them.
> - Check that removing !UNLIMITED_POLY does not cause any regressions
>
> If that works: OK for mainline
>
> Thanks for looking into this issue and working on the patches.
>
> Tobias
>
> On 26.03.21 07:58, Paul Richard Thomas via Fortran wrote:
> > This patch is straightforward but the isolation of the problem was rather
> > less so. Many thanks to Juergen for testcase reduction.
> >
> > Regtested on FC33/x86_64 - OK for master?
> >
> > Paul
> >
> > Fortran: Fix problem with runtime pointer chack [PR99602].
> >
> > 2021-03-26  Paul Thomas  
> >
> > gcc/fortran/ChangeLog
> >
> > PR fortran/99602
> > * trans-expr.c (gfc_conv_procedure_call): Use the _data attrs
> > for class expressions and detect proc pointer evaluations by
> > the non-null actual argument list.
> >
> > gcc/testsuite/ChangeLog
> >
> > PR fortran/99602
> > * gfortran.dg/pr99602.f90: New test.
> > * gfortran.dg/pr99602a.f90: New test.
> > * gfortran.dg/pr99602b.f90: New test.
> -
> Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
> Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
> Thürauf
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein


Re: [Patch, fortran] 99307 - FAIL: gfortran.dg/class_assign_4.f90 execution test

2021-03-29 Thread Tobias Burnus

Hi all,

as preremark I want to note that the testcase class_assign_4.f90
was added for PR83118/PR96012 (fixes problems in handling class objects, Dec 
18, 2020)
and got revised for PR99124 (class defined operators, Feb 23, 2021).
Both patches were then also applied to GCC 9 and 10.

On 26.03.21 17:30, Paul Richard Thomas via Gcc-patches wrote:

This patch comes in two versions: submit.diff with Change.Logs or
submit2.diff with Change2.Logs.
The first fixes the problem by changing array temporaries from class
expressions into class temporaries. This permits the use of
gfc_get_class_from_expr to obtain the vptr for these temporaries and all
the good things that come with that when handling dynamic types. The second
part of the fix is to use the array element length from the class
descriptor, when reallocating on assignment. This is needed because the
vptr is being set too early. I will set about trying to track down why this
is happening and fix it after release.

The second version does the same as the first but puts in place a load of
tidying up that is permitted by the fix to class array temporaries.



I couldn't readily see how to prepare a testcase - ideas?
Both regtest on FC33/x86_64. The first was tested by Dominique (see the
PR). OK for master?


Typo – underscore-'c' should be a dot-'c' – both changelog files


  * trans-expr_c (gfc_trans_scalar_assign): Make use of pre and


I think the second longer version is nicer in general, but at least for
GCC 9/GCC10 the first version is simpler and, hence, less error prone.

As you only ask about mainline, I would prefer the second one.

However, I am not happy about gfc_find_and_cut_at_last_class_ref:


+ of refs following. If ts is non-null the cut is at the class entity
+ or component that is followed by an array reference, which is not +
an element. */ ... + + if (ts) + { + if (e->symtree + &&
e->symtree->n.sym->ts.type == BT_CLASS) + *ts =
&e->symtree->n.sym->ts; + else + *ts = NULL; + } + for (ref = e->ref;
ref; ref = ref->next) { + if (ts && ref->type == REF_COMPONENT + &&
ref->u.c.component->ts.type == BT_CLASS + && ref->next &&
ref->next->type == REF_COMPONENT + && strcmp
(ref->next->u.c.component->name, "_data") == 0 + && ref->next->next +
&& ref->next->next->type == REF_ARRAY + && ref->next->next->u.ar.type
!= AR_ELEMENT) + { + *ts = &ref->u.c.component->ts; + class_ref = ref;
+ break; + } + + if (ts && *ts == NULL) + return NULL; +

Namely, if there is:
  type1%array_class2 → array_class2 is used for 'ts' and later (ok)
  type1%type%array_class2 → NULL is returned  (why?)
  class1%type%array_class2 → ts = class1 but array2_class is used later on 
(ups!)
  class1%...%scalar_class2 → ts = class1 but scalar_class2 is used
etc.

Thus this either needs to be cleaned up (separate 'ref' loop for
ts != NULL) – including the wording in the description which tells what
happens if 'ts' is passed as arg but the expr has rank == 0 – and
what value is assigned to 'ts'. (You can then also fix 'class.c::' to
'class.c: ' in the description above the function.)

Alternatively, you can leave the current code ref handling code in place
at build_class_array_ref, which might be the simpler alternative.

Otherwise, it looks sensible to me.

Tobias

-
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München 
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank 
Thürauf