From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.smtpout.orange.fr (smtp-17.smtpout.orange.fr [80.12.242.17]) by sourceware.org (Postfix) with ESMTPS id 793E93858D1E for ; Mon, 3 Jul 2023 11:46:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 793E93858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=orange.fr Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=orange.fr Received: from [192.168.1.16] ([86.215.161.51]) by smtp.orange.fr with ESMTPA id GI0bqdQJybpZ4GI0bqMXc3; Mon, 03 Jul 2023 13:46:22 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=orange.fr; s=t20230301; t=1688384782; bh=g8QX1dCanfXfr+jxNrB1Vrs+SHwO6LQ9uaG4T9Z9cB8=; h=Date:From:Subject:To:References:In-Reply-To; b=hRlHQu5VCJ4asGKQP1WwQEAM0hpGVQsM99OgD46KXs6BWjOVrmBeosP+CnhAR7ejE zZrnVqEHZ3Ie2uos/lyeRD5kcyCbeZFBWQTVQswV4edKHqVrydrlshqsaQi6t9zJbq U9Pi5ZC+d/O+f6rR77P7MHbr7HcFUfYKxYTYhm13fpsliBKHKotD91h72huhca8g7Z JQafxv5zlegXzCeOAroAR4RKiK9hwgZkQgEGLDZIfNGzEMnVxDRtMvvID+lIt06Hmr SABzjuyxhHigxJRTao+REUwwooqiYf7U70NbmOYzBV0KAc7uGm9Ne0wZn/sbPmKYxG tdLeYDOMN9HAA== X-ME-Helo: [192.168.1.16] X-ME-Auth: bW9yaW4tbWlrYWVsQG9yYW5nZS5mcg== X-ME-Date: Mon, 03 Jul 2023 13:46:22 +0200 X-ME-IP: 86.215.161.51 Message-ID: <5a5306ae-0db1-c7e2-e744-a3beced17636@orange.fr> Date: Mon, 3 Jul 2023 13:46:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 From: Mikael Morin Subject: Re: [PATCH] Fortran: fixes for procedures with ALLOCATABLE,INTENT(OUT) arguments [PR92178] To: Harald Anlauf , fortran , gcc-patches References: Content-Language: fr In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,JMQ_SPF_NEUTRAL,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,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 List-Id: Hello, Le 02/07/2023 à 22:38, Harald Anlauf via Fortran a écrit : > Dear all, > > the attached patch fixes a long-standing issue with the > order of evaluation of procedure argument expressions and > deallocation of allocatable actual arguments passed to > allocatable dummies with intent(out) attribute. > > It is based on an initial patch by Steve, handles issues > pointed out by Tobias, and includes a suggestion by Tobias > to scan the procedure arguments first to decide whether the > creation of temporaries is needed. > > There is one unresolved issue left that might be more > general: it appears to affect character arguments (only) > in that quite often there still is no temporary generated. > I haven't found the reason why and would like to defer this, > unless someone has a good suggestion. > No problem, let's fix the easier parts first. > Regtested on x86_64-pc-linux-gnu. OK for mainline? > A few thing to double check below. > pr92178.diff > > From 609ba636927811cddc74fb815cb18809c7d33565 Mon Sep 17 00:00:00 2001 > From: Harald Anlauf > Date: Sun, 2 Jul 2023 22:14:19 +0200 > Subject: [PATCH] Fortran: fixes for procedures with ALLOCATABLE,INTENT(OUT) > arguments [PR92178] > > gcc/fortran/ChangeLog: > > PR fortran/92178 > * trans-expr.cc (gfc_conv_procedure_call): Check procedures for > allocatable dummy arguments with INTENT(OUT) and move deallocation > of actual arguments after evaluation of argument expressions before > the procedure is executed. > > gcc/testsuite/ChangeLog: > > PR fortran/92178 > * gfortran.dg/pr92178.f90: New test. > * gfortran.dg/pr92178_2.f90: New test. > > Co-authored-by: Steven G. Kargl > --- > gcc/fortran/trans-expr.cc | 52 ++++++++++++++-- > gcc/testsuite/gfortran.dg/pr92178.f90 | 83 +++++++++++++++++++++++++ > gcc/testsuite/gfortran.dg/pr92178_2.f90 | 46 ++++++++++++++ > 3 files changed, 177 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gfortran.dg/pr92178.f90 > create mode 100644 gcc/testsuite/gfortran.dg/pr92178_2.f90 > > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc > index 30946ba3f63..16e8f037cfc 100644 > --- a/gcc/fortran/trans-expr.cc > +++ b/gcc/fortran/trans-expr.cc (...) > @@ -6117,6 +6118,33 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, > && UNLIMITED_POLY (sym) > && comp && (strcmp ("_copy", comp->name) == 0); > > + /* First scan argument list for allocatable actual arguments passed to > + allocatable dummy arguments with INTENT(OUT). As the corresponding > + actual arguments are deallocated before execution of the procedure, we > + evaluate actual argument expressions to avoid problems with possible > + dependencies. */ > + bool force_eval_args = false; > + gfc_formal_arglist *tmp_formal; > + for (arg = args, tmp_formal = formal; arg != NULL; > + arg = arg->next, tmp_formal = tmp_formal ? tmp_formal->next : NULL) > + { > + e = arg->expr; > + fsym = tmp_formal ? tmp_formal->sym : NULL; > + if (e && fsym > + && e->expr_type == EXPR_VARIABLE > + && fsym->attr.intent == INTENT_OUT > + && (fsym->ts.type == BT_CLASS && fsym->attr.class_ok > + ? CLASS_DATA (fsym)->attr.allocatable > + : fsym->attr.allocatable) > + && e->symtree > + && e->symtree->n.sym > + && gfc_variable_attr (e, NULL).allocatable) > + { > + force_eval_args = true; > + break; > + } > + } > + The function is already big enough, would you mind outlining this to its own function? > /* Evaluate the arguments. */ > for (arg = args, argc = 0; arg != NULL; > arg = arg->next, formal = formal ? formal->next : NULL, ++argc) > @@ -6680,7 +6708,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, > else > tmp = gfc_finish_block (&block); > > - gfc_add_expr_to_block (&se->pre, tmp); > + gfc_add_expr_to_block (&dealloc_blk, tmp); > } > > /* A class array element needs converting back to be a > @@ -6980,7 +7008,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, > build_empty_stmt (input_location)); > } > if (tmp != NULL_TREE) > - gfc_add_expr_to_block (&se->pre, tmp); > + gfc_add_expr_to_block (&dealloc_blk, tmp); > } > > tmp = parmse.expr; > @@ -7004,7 +7032,7 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, > void_type_node, > gfc_conv_expr_present (e->symtree->n.sym), > tmp, build_empty_stmt (input_location)); > - gfc_add_expr_to_block (&se->pre, tmp); > + gfc_add_expr_to_block (&dealloc_blk, tmp); > } > } > } These look good, but I'm surprised that there is no similar change at the 6819 line. This is the class array actual vs class array dummy case. It seems to be checked by the "bar" subroutine in your testcase, except that the intent(out) argument comes last there, whereas it was coming first with the original testcases in the PR. Can you double check? > @@ -7101,6 +7129,21 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol * sym, > } > } > > + /* If any actual argument of the procedure is allocatable and passed > + to an allocatable dummy with INTENT(OUT), we conservatively > + evaluate all actual argument expressions before deallocations are > + performed and the procedure is executed. This ensures we conform > + to F2023:15.5.3, 15.5.4. Create temporaries except for constants, > + variables, and functions returning pointers that can appear in a > + variable definition context. */ > + if (e && fsym && force_eval_args > + && e->expr_type != EXPR_VARIABLE > + && !gfc_is_constant_expr (e) > + && (e->expr_type != EXPR_FUNCTION > + || !(gfc_expr_attr (e).pointer > + || gfc_expr_attr (e).proc_pointer))) > + parmse.expr = gfc_evaluate_now (parmse.expr, &parmse.pre); > + I'm not sure about the guarding condition. It looks like it may miss evaluation in some cases (one testcase below). With a value dummy, it is always safe to evaluate to a temporary variable, and with a non-value dummy, parmse.expr contains a pointer, so it is safe as well to evaluate that to a temporary pointer? At least a || fsym->attr.value condition is missing somewhere, but I think the condition can be reduced to this: if (e && fsym && force_eval_args && !gfc_is_constant_expr (e)) Were there failures that drove to your above guarding conditions? Mikael PS: The testcase (as promised): program p implicit none type t integer :: i integer, pointer :: pi end type t integer, target :: j type(t), allocatable :: ta j = 1 ta = t(2, j) call assign(ta, id(ta%pi)) if (ta%i /= 1) stop 1 if (associated(ta%pi)) stop 2 contains subroutine assign(a, b) type(t), intent(out), allocatable :: a integer, intent(in) , value :: b allocate(a) a%i = b a%pi => null() end subroutine assign function id(a) integer, pointer :: id, a id => a end function id end program p