fortran: gfc_trans_subcomponent_assign fixes [PR113503]

  Hi!

The r14-870 changes broke xtb package tests (reduced testcase is the first
one below) and caused ICEs on a test derived from that (the second one).
For the
  x = T(u = trim (us(1)))
statement, before that change gfortran used to emit weird code with
2 trim calls:
      _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
      if (len.2 > 0)
        {
          __builtin_free ((void *) pstr.1);
        }
      D.4275 = len.2;
      t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) D.4275, 1>);
      t.0._u_length = D.4275;
      _gfortran_string_trim (&len.4, (void * *) &pstr.3, 20, &us[0]);
      (void) __builtin_memcpy ((void *) t.0.u, (void *) pstr.3, (unsigned long) NON_LVALUE_EXPR <len.4>);
      if (len.4 > 0)
        {
          __builtin_free ((void *) pstr.3);
        }
That worked at runtime, though it is wasteful.
That commit changed it to:
      slen.3 = len.2;
      t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.3, 1>);
      t.0._u_length = slen.3;
      _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
      (void) __builtin_memcpy ((void *) t.0.u, (void *) pstr.1, (unsigned long) NON_LVALUE_EXPR <len.2>);
      if (len.2 > 0)
        {
          __builtin_free ((void *) pstr.1);
        }
which results in -Wuninitialized warning later on and if one is unlucky and
the uninitialized len.2 variable is smaller than the trimmed length, it
results in heap overflow and often crashes later on.
The bug above is clear, len.2 is only initialized in the
_gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
call, but used before that.  Now, the
      slen.3 = len.2;
      t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.3, 1>);
      t.0._u_length = slen.3;
statements come from the alloc_scalar_allocatable_subcomponent call,
while
      _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]);
from the gfc_conv_expr (&se, expr); call which is done before the
alloc_scalar_allocatable_subcomponent call, but is only appended later on
with gfc_add_block_to_block (&block, &se.pre);
Now, obviously the alloc_scalar_allocatable_subcomponent emitted statements
can depend on the se.pre sequence statements which can compute variables
used by alloc_scalar_allocatable_subcomponent like the length.
On the other side, I think the se.pre sequence really shouldn't depend
on the changes done by alloc_scalar_allocatable_subcomponent, that is
initializing the FIELD_DECLs of the destination allocatable subcomponent
only, the gfc_conv_expr statements are already created, so all they could
in theory depend above is on t.0.u or t.0._u_length, but I believe if the
rhs dependened on the lhs content (which is allocated by those statements
but really uninitialized), it would need to be discovered by the dependency
analysis and forced into a temporary.
So, in order to fix the first testcase, the second hunk of the patch just
emits the se.pre block before the alloc_scalar_allocatable_subcomponent
changes rather than after it.

The second problem is an ICE on the second testcase.  expr in the caller
(expr2 inside of alloc_scalar_allocatable_subcomponent) has
expr2->ts.u.cl->backend_decl already set, INTEGER_CST 20, but
alloc_scalar_allocatable_subcomponent overwrites it to a new VAR_DECL
which it assigns a value to before the malloc.  That can work if the only
places the expr2->ts is ever used are in the same local block or its
subblocks (and only if it is dominated by the code emitted by
alloc_scalar_allocatable_subcomponent, so e.g. not if that call is inside
of a conditional code and use later unconditional), but doesn't work
if expr2->ts is used before that block or after it.  So, the exact ICE is
because of:
  slen.1 = 20;
    static character(kind=1) us[1][1:20] = {"foo                 "};
  x.u = 0B;
  x._u_length = 0;
  {
    struct t t.0;
    struct t D.4308;

    {
      integer(kind=8) slen.1;

      slen.1 = 20;
      t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.1, 1>);
      t.0._u_length = slen.1;
      (void) __builtin_memcpy ((void *) t.0.u, (void *) &us[0], 20);
    }
where the first slen.1 = 20; is emitted because it sees us has a VAR_DECL
ts.u.cl->backend_decl and so it wants to initialize it to the actual length.
This is invalid GENERIC, because the slen.1 variable is only declared inside
of a {} later on and so uses outside of it are wrong.  Similarly wrong would
be if it is used later on.  E.g. in the same testcase if it has
  type(T) :: x, y
  x = T(u = us(1))
  y%u = us(1)
then there is
    {
      integer(kind=8) slen.1;

      slen.1 = 20;
      t.0.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.1, 1>);
      t.0._u_length = slen.1;
      (void) __builtin_memcpy ((void *) t.0.u, (void *) &us[0], 20);
    }
...
    if (y.u != 0B) goto L.1;
    y.u = (character(kind=1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.1, 1>);
i.e. another use of slen.1, this time after slen.1 got out of scope.

I really don't understand why the code modifies
expr2->ts.u.cl->backend_decl, expr2 isn't used there anywhere except for
expr2->ts.u.cl->backend_decl expressions, so hacks like save the previous
value, overwrite it temporarily over some call that will use expr2 and
restore afterwards aren't needed - there are no such calls, so the
following patch fixes it just by not messing up with
expr2->ts.u.cl->backend_decl, only set it to size variable and overwrite
that with a temporary if needed.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-02-17  Jakub Jelinek  <jakub@redhat.com>

	PR fortran/113503
	* trans-expr.cc (alloc_scalar_allocatable_subcomponent): Don't
	overwrite expr2->ts.u.cl->backend_decl, instead set size to
	expr2->ts.u.cl->backend_decl first and use size instead of
	expr2->ts.u.cl->backend_decl.
	(gfc_trans_subcomponent_assign): Emit se.pre into block
	before calling alloc_scalar_allocatable_subcomponent instead of
	after it.

	* gfortran.dg/pr113503_1.f90: New test.
	* gfortran.dg/pr113503_2.f90: New test.


	Jakub
  

Hi Jakub,

On 2/17/24 10:02, Jakub Jelinek wrote:
> Hi!
>
> The r14-870 changes broke xtb package tests (reduced testcase is the first
> one below) and caused ICEs on a test derived from that (the second one).
[...]

thanks for your detailed analysis and for the patch, which puts
things in straight order to actually fix two issues here!

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK for trunk, except for the minor nit below.

> --- gcc/testsuite/gfortran.dg/pr113503_1.f90.jj	2024-02-16 14:16:17.937153094 +0100
> +++ gcc/testsuite/gfortran.dg/pr113503_1.f90	2024-02-16 14:16:10.124258815 +0100
> @@ -0,0 +1,18 @@
> +! PR fortran/113503
> +! { dg-do compile }
> +! { dg-options "-O2 -fno-inline -Wuninitialized" }
> +
> +program pr113503
> +  implicit none
> +  type :: T
> +    character(len=:), allocatable :: u
> +  end type
> +  character(len=20) :: us(1) = 'foobar'
> +  type(T) :: x
> +  x = T(u = trim (us(1)))	! { dg-bogus "is used uninitialized" }
                             ^^^^ tab here not allowed in Fortran

My newsreader shows a tab here, giving a warning when running the test.
Also, applying your patch on top of r14-9045 I do not see the
uninitialized warning, which could have been fixed by r14-8947.
Please recheck and adjust accordingly.

> +  call foo
> +contains
> +  subroutine foo
> +    if (x%u /= 'foobar') stop 1
> +  end subroutine
> +end

Thanks,
Harald
  

On Sat, Feb 17, 2024 at 04:05:16PM +0100, Harald Anlauf wrote:
> > +program pr113503
> > +  implicit none
> > +  type :: T
> > +    character(len=:), allocatable :: u
> > +  end type
> > +  character(len=20) :: us(1) = 'foobar'
> > +  type(T) :: x
> > +  x = T(u = trim (us(1)))	! { dg-bogus "is used uninitialized" }
>                             ^^^^ tab here not allowed in Fortran
> 
> My newsreader shows a tab here, giving a warning when running the test.
> Also, applying your patch on top of r14-9045 I do not see the
> uninitialized warning, which could have been fixed by r14-8947.
> Please recheck and adjust accordingly.

I certainly see if I apply just the testsuite part of the patch to current
trunk
make check-gfortran RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=pr113503*.f90'
...
FAIL: gfortran.dg/pr113503_1.f90   -O   (test for bogus messages, line 12)
FAIL: gfortran.dg/pr113503_2.f90   -O  (internal compiler error: in gimplify_var_or_parm_decl, at gimplify.cc:3280)
FAIL: gfortran.dg/pr113503_2.f90   -O  (test for excess errors)

		=== gfortran Summary for unix/-m32 ===

# of expected passes		1
# of unexpected failures	3
...
FAIL: gfortran.dg/pr113503_1.f90   -O   (test for bogus messages, line 12)
FAIL: gfortran.dg/pr113503_2.f90   -O  (internal compiler error: in gimplify_var_or_parm_decl, at gimplify.cc:3280)
FAIL: gfortran.dg/pr113503_2.f90   -O  (test for excess errors)

		=== gfortran Summary for unix/-m64 ===

# of expected passes		1
# of unexpected failures	3
and that is all gone after I apply the trans-expr.cc patch as well and
make before the above command.

I have replaced the tab with a space, but from what I can see, there is no
warning/error with the options it is compiled, warning with -Wtabs or -Wall
and error with -pedantic-errors, but those options aren't used.

Thanks.

	Jakub