From patchwork Fri Feb 24 11:31:59 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Tobias Burnus X-Patchwork-Id: 1747424 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4PNSRk05H6z1yYg for ; Fri, 24 Feb 2023 22:32:33 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 48EC0385480D for ; Fri, 24 Feb 2023 11:32:30 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id 705DE385840F; Fri, 24 Feb 2023 11:32:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 705DE385840F Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.97,324,1669104000"; d="diff'?scan'208";a="102085879" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa1.mentor.iphmx.com with ESMTP; 24 Feb 2023 03:32:04 -0800 IronPort-SDR: Q40qcBM7pRcpWTlYrXwOswpfIX3KI8uhJw0P6YVwKSeaLYpTwHs1vKtgjAp5Wqo8/oMbRoQneo Y180EFw1xt7T80TkE+KtzoXZcBFrKYx+1SM0YixmXUj8ovap+lruodChY1qym/iZavRYQcBm7n V5+SI/v6Oi2Ws+avF0h+VzcDtsRE3aV3j14ELWZLX8x2WhWdDU0PmOJ8oWZUBeoMxDe8jArnOh mw8DLP6Wpviegp2OAk9Ozz9YQMeiJMRSZZT149tV82NTdXn7qQOoxyIW3EplN94AR27o0NnGsu W1E= Message-ID: <07be8524-0755-6b77-49bd-af5c688404d5@codesourcery.com> Date: Fri, 24 Feb 2023 12:31:59 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Content-Language: en-US To: gcc-patches , fortran From: Tobias Burnus Subject: [Patch] Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621] X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-15.mgc.mentorg.com (139.181.222.15) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_PASS, TXREP 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.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" [The following is about Fortran pointers as actual argument to a CFI taking procedure.] The issue has been marked as 12/13 regression but the issue is just a diagnostic one. To disentangle: (A) Bogus warning [Now tracked as middle-end https://gcc.gnu.org/PR108906 ] Assume: nullify(p) call bind_c_proc(p) For some reasons, the compiler does not propagate the NULL and thus assumes that if (addr != NULL) could be true. This leads to a may-be-uninit warning with '-Wall -O0'. (And no warning with -Og/-Os/-O1.) We could silence it on the gfortran side, I think, by tweaking some tree properties, but I am not sure we want to to it. (The same kind of code is in GCC 11 but as it is hidden in libgfortran; hence, there are no warnings when compiling user code. Since GCC 12, the compiler does the conversion in place when generating the code.) (B) The attached patch: With 'intent(out)' there is no reason to do the conversions. While for nullified pointers the bounds-conversion loop is skipped, it may still be executed for undefined pointers. (Which is usually harmless.) In either case, not generating this code makes sense. OK for mainline? Regarding GCC 12: I am not really sure as it is no real regression. Besides bogus warnings, there might be an issue for undefined pointers and -fsanitize=undefined, namely if 'ubound - lbound' evaluated on random numbers overflows (such as for ubound = huge(..) and lbound = -huge(..)). But that looks like a rather special case. - Thoughts? Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 Fortran: Skip bound conv in gfc_conv_gfc_desc_to_cfi_desc with intent(out) ptr [PR108621] When the dummy argument of the bind(C) proc is 'pointer, intent(out)', the conversion of the GFC to the CFI bounds can be skipped: it is not needed and avoids issues with noninit memory. Note that the 'cfi->base_addr = gfc->addr' assignment is kept as the C code of a user might assume that a nullified pointer arrives as NULL (or even a specific value). For instance, gfortran.dg/c-interop/section-{1,2}.f90 assumes the value NULL. Note 2: The PR is about a may-be-uninitialized warning with intent(out). In the PR's testcase, the pointer was nullified and should not have produced that warning. That is a diagnostic issue, now tracked as PR middle-end/108906 as the issue in principle still exists (e.g. with 'intent(inout)'). [But no longer for intent(out).] Note 3: With undefined pointers and no 'intent', accessing uninit memory is unavoidable on the caller side as the compiler cannot know what the C function does (but this usage determines whether the pointer is permitted be undefined or whether the bounds must be gfc-to-cfi converted). gcc/fortran/ChangeLog: PR fortran/108621 * trans-expr.cc (gfc_conv_gfc_desc_to_cfi_desc): gcc/testsuite/ChangeLog: PR fortran/108621 * gfortran.dg/c-interop/fc-descriptor-pr108621.f90: New test. gcc/fortran/trans-expr.cc | 6 ++ .../c-interop/fc-descriptor-pr108621.f90 | 65 ++++++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc index e85b53fae85..045c8b00b90 100644 --- a/gcc/fortran/trans-expr.cc +++ b/gcc/fortran/trans-expr.cc @@ -5673,6 +5673,9 @@ gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym) gfc_add_modify (&block, tmp, build_int_cst (TREE_TYPE (tmp), attr)); + /* The cfi-base_addr assignment could be skipped for 'pointer, intent(out)'. + That is very sensible for undefined pointers, but the C code might assume + that the pointer retains the value, in particular, if it was NULL. */ if (e->rank == 0) { tmp = gfc_get_cfi_desc_base_addr (cfi); @@ -5695,6 +5698,9 @@ gfc_conv_gfc_desc_to_cfi_desc (gfc_se *parmse, gfc_expr *e, gfc_symbol *fsym) gfc_add_modify (&block, tmp2, fold_convert (TREE_TYPE (tmp2), tmp)); } + if (fsym->attr.pointer && fsym->attr.intent == INTENT_OUT) + goto done; + /* When allocatable + intent out, free the cfi descriptor. */ if (fsym->attr.allocatable && fsym->attr.intent == INTENT_OUT) { diff --git a/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90 b/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90 new file mode 100644 index 00000000000..9c9062bd62d --- /dev/null +++ b/gcc/testsuite/gfortran.dg/c-interop/fc-descriptor-pr108621.f90 @@ -0,0 +1,65 @@ +! { dg-do compile } +! { dg-additional-options "-fdump-tree-original" } +! +! PR fortran/108621 +! +! If the bind(C) procedure's dummy argument is a POINTER with INTENT(OUT), +! avoid converting the array bounds for the CFI descriptor before the call. +! +! Rational: Fewer code and, esp. for undefined pointers, there might be a +! compile-time warning or a runtime error due to the 'extent' arithmentic +! and integer overflows (i.e. random values and -fsanitize=undefined). +! +! (For disassociated pointers, it would/should be only pointless code as +! the bound setting is guarded by a != NULL condtion. However, as the PR shows, +! a bogus may-use-uninitialized-memory warning might still be shown in that case.) +! +! Without 'intent' (but still intent(out) internally), the same applies but +! there is nothing the compiler can do on the caller side. +! Still, as only uninit memory and not invalid memory it accessed, it should still +! work (at least when run-time checking is turned off). +! +subroutine demo(f) +use, intrinsic :: iso_c_binding, only : c_int +implicit none + +interface + subroutine fun(f_p) bind(c) + import c_int + integer(c_int), pointer, intent(out) :: f_p(:) + end subroutine +end interface + +integer(c_int), pointer :: f(:) + +call fun(f) +end + +! The following ones must be present even with intent(out): +! +! { dg-final { scan-tree-dump "cfi...version = 1;" "original" } } +! { dg-final { scan-tree-dump "cfi...rank = 1;" "original" } } +! { dg-final { scan-tree-dump "cfi...type = 1025;" "original" } } +! { dg-final { scan-tree-dump "cfi...attribute = 0;" "original" } } +! { dg-final { scan-tree-dump "cfi...elem_len = 4;" "original" } } + + +! The following is not needed - but user code might expect that an incoming pointer is NULL +! in this case. - At least the GCC testsuite expects this in the C code at +! gfortran.dg/c-interop/section-{1,2}.f90 +! Thus, it is kept as it does not cause any harm: +! +! { dg-final { scan-tree-dump "cfi...base_addr = f->data;" "original" } } + + +! The following ones are not need with intent(out) and, therefore, shouldn't be there: +! +! cfi.0.dim[idx.1].lower_bound = f->dim[idx.1].lbound; +! cfi.0.dim[idx.1].extent = (f->dim[idx.1].ubound - f->dim[idx.1].lbound) + 1; +! cfi.0.dim[idx.1].sm = f->dim[idx.1].stride * f->span; +! +! Now match those - but using a rather generic pattern as it is a ...-not scan: +! +! { dg-final { scan-tree-dump-not "lower_bound = " "original" } } +! { dg-final { scan-tree-dump-not "extent = " "original" } } +! { dg-final { scan-tree-dump-not "sm = " "original" } }