On Thu, Feb 22, 2024 at 09:22:37PM +0100, Harald Anlauf wrote:
> Hi Steve!
>
> On 2/22/24 01:52, Steve Kargl wrote:
> > On Wed, Feb 21, 2024 at 01:42:32PM -0800, Steve Kargl wrote:
> > > On Wed, Feb 21, 2024 at 10:20:43PM +0100, Harald Anlauf wrote:
> > > > On 2/21/24 22:00, Steve Kargl wrote:
> > > > > memleak vs ICE. I think I'll take one over the other.
> > > > > Probably need to free code->expr3 before the copy.
> > > >
> > > > Yep.
> > > >
> > > > > I tried gfc_replace_expr in an earlier patch. It did not
> > > > > work.
> >
> > I tried freeing code->expr3 before assigning the new expression.
> > That leads to
> >
> > % gfcx -c ~/gcc/gccx/gcc/testsuite/gfortran.dg/allocate_with_source_28.f90
> > pid 69473 comm f951 has trashed its stack, killing
> > gfortran: internal compiler error: Illegal instruction signal terminated
> > program f951
>
> Right. I also don't see what the lifetimes of the expressions are.
>
> But is the gfc_copy_expr really needed? Wouldn't the following suffice?
>
> code->expr3 = gfc_get_parentheses (code->expr3);
It's been awhile since I use gfc_copy_expr, gfc_replace_expr, etc.
I did not try the above. If that works, then we should use that
for simplicity.
> > If I don't free code->expr3 but simply assign the new
> > expression from gfc_get_parentheses(), your example
> > now compiles are executes are expected. It now
> > allocate_with_source_28.f90. Caveat: I don't know
> > how to test the CLASS uu.
> >
> > > > > > - it still fails on the following code, because the traversal
> > > > > > of the refs is incomplete / wrong:
> > > > > >
> > > > > > program foo
> > > > > > implicit none
> > > > > > complex :: cmp(3)
> > > > > > real, pointer :: pp(:)
> > > > > > class(*), allocatable :: uu(:)
> > > > > > type t
> > > > > > real :: re
> > > > > > real :: im
> > > > > > end type t
> > > > > > type u
> > > > > > type(t) :: tt(3)
> > > > > > end type u
> > > > > > type(u) :: cc
> > > > > >
> > > > > > cmp = (3.45,6.78)
> > > > > > cc% tt% re = cmp% re
> > > > > > cc% tt% im = cmp% im
> > > > > > allocate (pp, source = cc% tt% im) ! ICE
> > > > >
> > > > > cc%tt%im isn't a complex-part-ref, so this seems to
> > > > > be a different (maybe related) issue. Does the code
> > > > > compile with 'source = (cc%tt%im)'? If so, perhaps,
> > > > > detecting a component reference and doing the simply
> > > > > wrapping with parentheses can be done.
> > > >
> > > > Yes, that's why I tried to make up the above example.
> > > > I think %re and %im are not too special, they work
> > > > here pretty much like component refs elsewhere.
> > > >
> > >
> > > I see. The %re and %im complex-part-ref correspond to
> > > ref->u.i == INQUIRY_RE and INQUIRY_IM, respectively.
> > > A part-ref for a user-defined type doesn't have an
> > > INQUIRY_xxx, so we'll need to see if there is a way to
> > > easily identify, e.g., cc%tt%re from your testcase.
> >
> > The attach patch uses ref->type == REF_COMPONENT to deal
> > with the above code.
>
> I actually wanted to draw your attention away from the
> real/complex stuff, because that is not really the point.
> When do we actually need to enforce the parentheses?
This is essentially my concern. I was inserting parentheses
only if I determined they were needed (to avoid unnecessary
temporary variable). The code paththat enters the else portion
of the following if-else-stmt, where a temporary is created.
That is,
allocate(x, source=z%re) becomes allocate(x, source=(z%re))
and from code generation viewpoint this is
tmp = (z%re)
allocate(x, sourcer=tmp)
deallocate(tmp)
> I tried the following, and it seems to work:
>
> if (code->expr3->expr_type == EXPR_VARIABLE
> && is_subref_array (code->expr3))
> code->expr3 = gfc_get_parentheses (code->expr3);
>
> (Beware: this is not regtested!)
>
> On the positive side, it not only seems to fix the cases in question,
> but also substring references etc., like the following:
If the above passes a regression test, then by all means we should
use it. I did not consider the substring case. Even if unneeded
parentheses are inserted, which may cause generation of a temporary
variable, I hope users are not using 'allocate(x,source=z%re)' is
some deeply nested crazy loops structure.
BTW, my patch and I suspect your improved patch also
fixes 'allocate(x,mold=z%re)'. Consider,
complex z(3)
real, allocatable :: x(:)
z = 42
allocate(x, mold=z%re)
print *, size(x)
end
% gfortran13 -o z a.f90
a.f90:9:25:
9 | allocate(x, mold=z%re)
| 1
internal compiler error: in retrieve_last_ref, at fortran/trans-array.cc:6070
0x247d7a679 __libc_start1
/usr/src/lib/libc/csu/libc_start1.c:157
% gfcx -o z a.f90 && ./z
3
--
Steve
