https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113503
--- Comment #7 from GCC Commits <cvs-commit at gcc dot gnu.org> --- The master branch has been updated by Jakub Jelinek <ja...@gcc.gnu.org>: https://gcc.gnu.org/g:296284a9dbb7df4485cc5f1d3e975fdb4b8a10b8 commit r14-9049-g296284a9dbb7df4485cc5f1d3e975fdb4b8a10b8 Author: Jakub Jelinek <ja...@redhat.com> Date: Sat Feb 17 16:54:08 2024 +0100 fortran: gfc_trans_subcomponent_assign fixes [PR113503] 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. 2024-02-17 Jakub Jelinek <ja...@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.
