From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id AD27A385E012; Sat, 17 Feb 2024 15:57:13 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org AD27A385E012 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1708185433; bh=01o+NOSXl7yWBJJg+7SWTzHNWnOaG6wKtJZI7DTGxZU=; h=From:To:Subject:Date:In-Reply-To:References:From; b=N4/0H10k60NuY3htD6rk8zOhACiDCo78CFm+jTMVwBrgjjaWeganoZXaFATpZ29u/ Dm/XQ5JF4EybF6OJ0exTuXk40h0MCcW1nq+z9Wl9jT1bIZ3u0E2bxGhVV8NXWLR6Bc 819AQUlg6vqP+MaIDh1oxI4nvVotvR5F7bUflBSQ= From: "cvs-commit at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug fortran/113503] [14 Regression] xtb test miscompilation starting with r14-870 Date: Sat, 17 Feb 2024 15:57:11 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: gcc X-Bugzilla-Component: fortran X-Bugzilla-Version: 14.0 X-Bugzilla-Keywords: wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: cvs-commit at gcc dot gnu.org X-Bugzilla-Status: ASSIGNED X-Bugzilla-Resolution: X-Bugzilla-Priority: P1 X-Bugzilla-Assigned-To: jakub at gcc dot gnu.org X-Bugzilla-Target-Milestone: 14.0 X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 List-Id: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D113503 --- Comment #7 from GCC Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:296284a9dbb7df4485cc5f1d3e975fdb4b8a10b8 commit r14-9049-g296284a9dbb7df4485cc5f1d3e975fdb4b8a10b8 Author: Jakub Jelinek 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 fi= rst one below) and caused ICEs on a test derived from that (the second one). For the x =3D T(u =3D 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 =3D len.2; t.0.u =3D (character(kind=3D1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) D.4275, 1>); t.0._u_length =3D D.4275; _gfortran_string_trim (&len.4, (void * *) &pstr.3, 20, &us[0]); (void) __builtin_memcpy ((void *) t.0.u, (void *) pstr.3, (unsign= ed long) NON_LVALUE_EXPR ); if (len.4 > 0) { __builtin_free ((void *) pstr.3); } That worked at runtime, though it is wasteful. That commit changed it to: slen.3 =3D len.2; t.0.u =3D (character(kind=3D1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.3, 1>); t.0._u_length =3D slen.3; _gfortran_string_trim (&len.2, (void * *) &pstr.1, 20, &us[0]); (void) __builtin_memcpy ((void *) t.0.u, (void *) pstr.1, (unsign= ed long) NON_LVALUE_EXPR ); 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 =3D len.2; t.0.u =3D (character(kind=3D1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.3, 1>); t.0._u_length =3D 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 statem= ents 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 cou= ld in theory depend above is on t.0.u or t.0._u_length, but I believe if t= he rhs dependened on the lhs content (which is allocated by those statemen= ts but really uninitialized), it would need to be discovered by the depend= ency analysis and forced into a temporary. So, in order to fix the first testcase, the second hunk of the patch ju= st 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 on= ly 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 insi= de 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 =3D 20; static character(kind=3D1) us[1][1:20] =3D {"foo "}; x.u =3D 0B; x._u_length =3D 0; { struct t t.0; struct t D.4308; { integer(kind=3D8) slen.1; slen.1 =3D 20; t.0.u =3D (character(kind=3D1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.1, 1>); t.0._u_length =3D slen.1; (void) __builtin_memcpy ((void *) t.0.u, (void *) &us[0], 20); } where the first slen.1 =3D 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 =3D T(u =3D us(1)) y%u =3D us(1) then there is { integer(kind=3D8) slen.1; slen.1 =3D 20; t.0.u =3D (character(kind=3D1)[1:0] *) __builtin_malloc (MAX_EXPR <(sizetype) slen.1, 1>); t.0._u_length =3D slen.1; (void) __builtin_memcpy ((void *) t.0.u, (void *) &us[0], 20); } ... if (y.u !=3D 0B) goto L.1; y.u =3D (character(kind=3D1)[1:0] *) __builtin_malloc (MAX_EXPR <(s= izetype) 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 previo= us 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 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.=