From patchwork Sat Feb 17 09:02:04 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 202588 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp222794dyc; Sat, 17 Feb 2024 01:03:16 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCWmjjRgIO+nhofdU9bPuU3gGx4JEGltrat95XU/q3MdupXGpuh6LXGdbTuFzyv3qasSeD53FER+QPBeY7XVbdbBTTaUJg== X-Google-Smtp-Source: AGHT+IFsa//9OjiSBQO4FKN/jXjJcvRpcXbnxvPB431WLpy4AX5c6zGzmWGhGEhUtvt9FaGPsX69 X-Received: by 2002:a05:6214:2621:b0:68c:977b:465f with SMTP id gv1-20020a056214262100b0068c977b465fmr8850078qvb.57.1708160595838; Sat, 17 Feb 2024 01:03:15 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708160595; cv=pass; d=google.com; s=arc-20160816; b=lQfbAOcbRTjk4MioaDXF0W54A8xXyBbCB6yT2Jq4/cKdY4HFPNACZ5Ub7vZnhLQvAz 4LEqCy90WNXZ+vGMZaEQT3G6z3pnNheHvDKyVdIqRiMTAHUUO0bMepMzCug1qHomxqQg hZ4X7Cqmmk0iVYeNuBz6phC4NssUo1k4Y0eXAzdVDtOJEIq72Z6/Lq80fvf8fknrtR/u Kx0srJ09BQxNk34mxkMx7T+OH8LaWrJBe0aZ2d3XJueCNHp5vmiFfBMLmIgccRkJIElY rwypHESO6GebqwJetIuqCy7CQdtKP6PwPGoEBEKRgTHIR2q6il/b7fti4ZU+a01nsRNc alpQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=errors-to:reply-to:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:content-disposition :mime-version:message-id:subject:cc:to:from:date:dkim-signature :arc-filter:dmarc-filter:delivered-to; bh=PyHRYk8ttZOlDUPlGmcBdwms/OzfjBSmBbo/HfUPVVM=; fh=QDzG+soQb1iOtqAf13lTft+gQ93WNBIOPyoknohDtsY=; b=YxqDjTFSmWsmzt+ojlBiboZM3O4D8YTgUieoWw4dmllbkWG6XLCejQz3tqTzHes4g4 vTtQFBFNv7w3O+1eIQ03O/4oaZsBVhyRVpVL2xafxBKGHJME9eOy6ZTaS41dLx3Oygje hWSSXBfKkVaOWLPrBbjDt6/FAk5C81/SGZX+cjiQh2u/qxtD3aeGPyMpChM8oUi8BATO OYFr5zgV22+f9d0vXKeG1CDeuiWD/4Rj7+2UroNn0xq+W3OG4o3xhT12XEVX7s+5Yr5K 6GmC1A2NP/ZugUmiEzMcSZQeC6dxtnZ9YI8wfepKhUlnRJCE5VYSJ7XGfeIv00qu9pob bYow==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=CSQNP0eh; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from server2.sourceware.org (server2.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id gu3-20020a056214260300b0068caccd5f77si1768977qvb.214.2024.02.17.01.03.15 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 17 Feb 2024 01:03:15 -0800 (PST) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=CSQNP0eh; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 86A7A385DC3E for ; Sat, 17 Feb 2024 09:03:15 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 65DB03858D33 for ; Sat, 17 Feb 2024 09:02:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 65DB03858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 65DB03858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708160536; cv=none; b=LKgqwFyzm/saXZC3japSO4+PJ/am9fKCQND5ZfzcK9fvM+FvRS1wlnA+sPLe+wAf8nUaw/ab1Wvqf2lRGS6S4J6nPWr83K1iwCxS9nDy056pFkpGpFDKAMTN42kcK8l4tnQaBFTFDL9+ljSSGtHSRKH+ZioIfhZgiGxTD+YAQ2U= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708160536; c=relaxed/simple; bh=3auCQ/lGinfnGmpAynTbInCSAuwde8jKai1Jx8zjPa0=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=d8qOARxYSfnX+0jJh9X6wZJSlwobVKbgbNCLz9OhnIEBGPOkD6SfRp3Cb6Im4smYG9upE5/UosWVb6PA1Kjz4XrgapyCL6L5tuTOeI1z5DdQ/9AMqtHy8tXHNn4veQ4Hd/buDcrlYnp1S0fX7S6Men4fAQmVbTcoswI9iaI2m70= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708160530; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type; bh=PyHRYk8ttZOlDUPlGmcBdwms/OzfjBSmBbo/HfUPVVM=; b=CSQNP0ehqzUyu3DINPEZYWdr0C61T2qiu1aIQqYhKAXRZ7h2bxx40Zz++AceFd4HbYFMQz /chstTar9UR7F0d0rPoAyJtzJUiSo+N/Y53K2Zc6w6XNwjW5ocs1Zj1tIsTUYy//xYpYSD iMJoDLgDqA5cbDIVP93sAZFdtHdq6VU= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-204-_7MYSu74OyWpjBx0rJUybw-1; Sat, 17 Feb 2024 04:02:08 -0500 X-MC-Unique: _7MYSu74OyWpjBx0rJUybw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 5451129AB3F3; Sat, 17 Feb 2024 09:02:08 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.8]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 18639C185C0; Sat, 17 Feb 2024 09:02:07 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 41H925nL3035687 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Sat, 17 Feb 2024 10:02:06 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 41H925aS3035686; Sat, 17 Feb 2024 10:02:05 +0100 Date: Sat, 17 Feb 2024 10:02:04 +0100 From: Jakub Jelinek To: fortran@gcc.gnu.org Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] fortran: gfc_trans_subcomponent_assign fixes [PR113503] Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Jakub Jelinek Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791136204616614795 X-GMAIL-MSGID: 1791136204616614795 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 ); 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 ); 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 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 --- gcc/fortran/trans-expr.cc.jj 2024-02-14 14:26:19.764810614 +0100 +++ gcc/fortran/trans-expr.cc 2024-02-16 13:58:22.544770967 +0100 @@ -9059,13 +9059,10 @@ alloc_scalar_allocatable_subcomponent (s if (cm->ts.type == BT_CHARACTER && cm->ts.deferred) { gcc_assert (expr2->ts.type == BT_CHARACTER); - if (!expr2->ts.u.cl->backend_decl - || !VAR_P (expr2->ts.u.cl->backend_decl)) - expr2->ts.u.cl->backend_decl = gfc_create_var (TREE_TYPE (slen), - "slen"); - gfc_add_modify (block, expr2->ts.u.cl->backend_decl, slen); - size = expr2->ts.u.cl->backend_decl; + if (!size || !VAR_P (size)) + size = gfc_create_var (TREE_TYPE (slen), "slen"); + gfc_add_modify (block, size, slen); gfc_deferred_strlen (cm, &tmp); lhs_cl_size = fold_build3_loc (input_location, COMPONENT_REF, @@ -9253,19 +9250,20 @@ gfc_trans_subcomponent_assign (tree dest || (cm->ts.type == BT_CLASS && CLASS_DATA (cm)->attr.allocatable && expr->ts.type != BT_CLASS))) { + tree size; + gfc_init_se (&se, NULL); gfc_conv_expr (&se, expr); - tree size; + /* The remainder of these instructions follow the if (cm->attr.pointer) + if (!cm->attr.dimension) part above. */ + gfc_add_block_to_block (&block, &se.pre); /* Take care about non-array allocatable components here. The alloc_* routine below is motivated by the alloc_scalar_allocatable_for_ assignment() routine, but with the realloc portions removed and different input. */ alloc_scalar_allocatable_subcomponent (&block, dest, cm, expr, se.string_length); - /* The remainder of these instructions follow the if (cm->attr.pointer) - if (!cm->attr.dimension) part above. */ - gfc_add_block_to_block (&block, &se.pre); if (expr->symtree && expr->symtree->n.sym->attr.proc_pointer && expr->symtree->n.sym->attr.dummy) --- 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" } + call foo +contains + subroutine foo + if (x%u /= 'foobar') stop 1 + end subroutine +end --- gcc/testsuite/gfortran.dg/pr113503_2.f90.jj 2024-02-16 14:17:01.197567699 +0100 +++ gcc/testsuite/gfortran.dg/pr113503_2.f90 2024-02-16 14:16:51.215702778 +0100 @@ -0,0 +1,12 @@ +! PR fortran/113503 +! { dg-do compile } + +program pr113503 + implicit none + type :: T + character(len=:), allocatable :: u + end type + character(len=20) :: us(1) = 'foo' + type(T) :: x + x = T(u = us(1)) +end