From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 48) id 572E23858D38; Mon, 19 Jun 2023 18:51:38 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 572E23858D38 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1687200698; bh=FcabdKITI7khK0lIc8Ay4X4ZhrJHNJOqhmH3gjCPxZ4=; h=From:To:Subject:Date:In-Reply-To:References:From; b=bLB+IPWAcz0c8szm2vX/qu4sRD9CO3TzUUCQgK9uV8HYIff43sEdU4JTFSFtkmWbK xFSztZNA5AY9zVyaEUZr0yHJq5XR/VV/iDP4Ep1Qh28yIUDnbpkCjWouvbZjiKZwq1 m+WQwsqDm27H0cGdJiJ3tq0eyPO2I5VO/7QnflXU= From: "anlauf at gcc dot gnu.org" To: gcc-bugs@gcc.gnu.org Subject: [Bug fortran/92887] [F2008] Passing nullified/disassociated pointer or unalloc allocatable to OPTIONAL + VALUE dummy fails Date: Mon, 19 Jun 2023 18:51:37 +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: 10.0 X-Bugzilla-Keywords: wrong-code X-Bugzilla-Severity: normal X-Bugzilla-Who: anlauf at gcc dot gnu.org X-Bugzilla-Status: NEW X-Bugzilla-Resolution: X-Bugzilla-Priority: P3 X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org X-Bugzilla-Target-Milestone: --- 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=3D92887 --- Comment #6 from anlauf at gcc dot gnu.org --- (In reply to Mikael Morin from comment #5) > (In reply to anlauf from comment #4) > >=20 > > I'll need broader feedback, so unless someone adds to this pr, I'll sub= mit > > the present patch - with testcases - to get attention. > >=20 > Here you go: >=20 > > diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc > > index 45a984b6bdb..d9dcc11e5bd 100644 > > --- a/gcc/fortran/trans-expr.cc > > +++ b/gcc/fortran/trans-expr.cc >=20 > > @@ -6396,7 +6399,28 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol= * sym, > > && fsym->ts.type !=3D BT_CLASS > > && fsym->ts.type !=3D BT_DERIVED) > > { > > - if (e->expr_type !=3D EXPR_VARIABLE > > + /* F2018:15.5.2.12 Argument presence and > > + restrictions on arguments not present. */ > > + if (e->expr_type =3D=3D EXPR_VARIABLE > > + && (e->symtree->n.sym->attr.allocatable > > + || e->symtree->n.sym->attr.pointer)) >=20 > Beware of expressions like derived%alloc_comp or derived%pointer_comp whi= ch > don't match the above. Right. This is fixable by using && (gfc_expr_attr (e).allocatable || gfc_expr_attr (e).pointer)) instead. > > @@ -7072,6 +7096,42 @@ gfc_conv_procedure_call (gfc_se * se, gfc_symbol= * sym, > > } > > } > >=20=20 > > + /* F2023:15.5.3, 15.5.4: Actual argument expressions are evaluat= ed > > + before they are associated and a procedure is executed. */ > > + if (e && e->expr_type !=3D EXPR_VARIABLE && !gfc_is_constant_exp= r (e)) > > + { > > + /* Create temporary except for functions returning pointers that > > + can appear in a variable definition context. */ >=20 > Maybe explain *why* we have to create a temporary, that is some data > references may become undefined by the procedure call (intent(out) dummi= es) > so we have to evaluate values depending on them beforehand (PR 92178). That is one reason. Another one, also pointed out in PR92178 by Tobias' re= view of Steve's draft, is the first testcase at https://gcc.gnu.org/legacy-ml/gcc-patches/2019-10/msg01970.html This is reminiscent to an issue reported for the MERGE intrinsic (pr107874, fixed so far, but there is a remaining issue in pr105371). > > + if (e->expr_type !=3D EXPR_FUNCTION > > + || !(gfc_expr_attr (e).pointer || gfc_expr_attr (e).proc_pointe= r)) >=20 > Merge with the outer condition? Yes. The above form was intended more for proof-of-concept and readability than for coding standards. > > + need_temp =3D true; > > + } > > + > > + if (need_temp) > > + { > > + if (cond_temp =3D=3D NULL_TREE) > > + parmse.expr =3D gfc_evaluate_now (parmse.expr, &parmse.pre); >=20 > I'm not sure about this. The condition to set need_temp looks quite gene= ral > (especially it matches non-scalar cases, doesn't it?), but > gfc_conv_expr_reference should already take care of creating a variable, = so > that a temporary is missing only for value dummies, I think. I would rat= her > move this to the place specific to value dummies. I agree in principle. The indentation level is already awful in the specif= ic place, which calls for thoughts about refactoring that mega-loop over the arguments than currently spans far more than 1000 source code lines. > I think this PR is only about scalars with basic types, is there the same > problem with derived types? with classes? > I guess arrays are different as they are always by reference? For the current documentation of the argument passing convention see: https://gcc.gnu.org/onlinedocs/gfortran/Argument-passing-conventions.html "For OPTIONAL dummy arguments, an absent argument is denoted by a NULL poin= ter, except for scalar dummy arguments of intrinsic type which have the VALUE attribute. For those, a hidden Boolean argument (logical(kind=3DC_bool),val= ue) is used to indicate whether the argument is present." My understanding is that for these scalar arguments we do need something th= at can be passed by value. We currently do not support VALUE with array arguments (F2008+), character of length > 1, and character actual arguments are broken unless they are constants. There are several open PRs. > > + else >=20 > I would rather move the else part to the place above where cond_temp is s= et, > so that the code is easier to follow. >=20 > > + { > > + /* "Conditional temporary" to handle variables that possibly > > + cannot be dereferenced. Use null value as fallback. */ > > + tree dflt_temp; > > + gcc_assert (e->ts.type !=3D BT_DERIVED && e->ts.type !=3D BT_CL= ASS); > > + gcc_assert (e->rank =3D=3D 0); > > + dflt_temp =3D gfc_create_var (TREE_TYPE (parmse.expr), "temp"); > > + TREE_STATIC (dflt_temp) =3D 1; > > + TREE_CONSTANT (dflt_temp) =3D 1; > > + TREE_READONLY (dflt_temp) =3D 1; > > + DECL_INITIAL (dflt_temp) =3D build_zero_cst (TREE_TYPE (dflt_te= mp)); > > + parmse.expr =3D fold_build3_loc (input_location, COND_EXPR, > > + TREE_TYPE (parmse.expr), > > + cond_temp, > > + parmse.expr, dflt_temp); > > + parmse.expr =3D gfc_evaluate_now (parmse.expr, &parmse.pre); > > + } > > + } > > + > > + > > if (fsym && need_interface_mapping && e) > > gfc_add_interface_mapping (&mapping, fsym, &parmse, e); > > I agree that it was a bad idea to merge the patches for these two PRs just because temporaries are needed.=