From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73676 invoked by alias); 6 Dec 2018 19:09:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 72911 invoked by uid 89); 6 Dec 2018 19:09:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=beat, H*Ad:U*tkoenig, 717, F2003 X-HELO: mail-it1-f177.google.com Received: from mail-it1-f177.google.com (HELO mail-it1-f177.google.com) (209.85.166.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 19:09:09 +0000 Received: by mail-it1-f177.google.com with SMTP id a6so3224342itl.4; Thu, 06 Dec 2018 11:09:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=THuPJYbMtTEQyFBG9mIe0//cDX30pCvkCPLj5qX2plo=; b=LYytVc0yMSyXdCDqUdu05NUpNorIZHzfbvABLo5R+m/8Xj08Tpa3Q5rljgIGdTmTn+ JeBEgDJRgfsZPSDcU4lAPMDEGKKI/mD210Kudoi4WPAA4RfR3ErIEioXTsIgHGZnE1wp qoj7wcJ+Urv5dLLJj0xtJGdmNXnti7QcVtODTNCLiq6e514N+8aOeMTNpWm7OP6rjAms wlnLO+uiSeBeMQEMsp95OQ1eNwaePN6b+GWjtCi/ygXEaeYDp5BfHGB0XKcLfVQCIV8R BUhLGAiwd4calgUe/qBm9iYk/pj96id6hM4irCHyQ22iK0QJz4A9YJSZ2E7oUs6BPoEF dPvQ== MIME-Version: 1.0 References: <20181205045945.GA40221@troutmask.apl.washington.edu> <20181206000315.GA47513@troutmask.apl.washington.edu> In-Reply-To: <20181206000315.GA47513@troutmask.apl.washington.edu> From: Fritz Reese Date: Thu, 06 Dec 2018 19:09:00 -0000 Message-ID: Subject: Re: Fortran patches To: Steve Kargl Cc: fortran , gcc-patches , Thomas Koenig Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-SW-Source: 2018-12/txt/msg00392.txt.bz2 On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl wrote: > > On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote: [...] > > RE: > > > PR fortran/88228 > > > * expr.c (check_null, check_elemental): Work around -fdec and > > > initialization with logical operators operating on integers. > > > > I plan to review this section of the patch later today -- though the > > patch hides the segfault from the PR, I need more time to determine > > whether it is correct and complete. > > By the time the gfc_expr is given to check_check and check_elemental, > it has been reduced to a EXPR_CONSTANT, which neither routine expected. > I simply return early in that case. It appears the correct solution is simply the following patch: diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c index b2090218d48..775a5c52c65 100644 --- a/gcc/fortran/resolve.c +++ b/gcc/fortran/resolve.c @@ -4004,7 +4004,7 @@ resolve_operator (gfc_expr *e) if (op2->ts.type !=3D e->ts.type || op2->ts.kind !=3D e->ts.kind) gfc_convert_type (op2, &e->ts, 1); e =3D logical_to_bitwise (e); - return resolve_function (e); + break; } sprintf (msg, _("Operands of logical operator %%<%s%%> at %%L are %s/%s"), @@ -4020,7 +4020,7 @@ resolve_operator (gfc_expr *e) e->ts.type =3D BT_INTEGER; e->ts.kind =3D op1->ts.kind; e =3D logical_to_bitwise (e); - return resolve_function (e); + break; } if (op1->ts.type =3D=3D BT_LOGICAL) Returning immediately short-circuits various checks and simplifications which are done in the remainder of resolve_operator, including gfc_simplify_expr which handles the EXPR_CONSTANT case. The comments on gfc_reduce_init_expr indicate that check_null and check_elemental should never get EXPR_CONSTANT anyway if gfc_resolve_expr is correct. Regression tests verify this patch is correct. Please use this patch instead for PR 88228, or if you prefer I can submit/commit the patch myself. > > > RE: > > > PR fortran/88139 > > > * dump-parse-tree.c (write_proc): Alternate return. > > I dissent with this patch. The introduced error is meaningless and, as > > mentioned by comment #3 in the PR, avoiding the ICE in dump-parse-tree > > is not directly the issue. The code should be rejected in parsing. In > > gcc-8.1 the invalid code is accepted (without an ICE) even without the > > -fc-prototypes flag: I haven't finished building the compiler with > > your changes yet to see whether that is still true afterwards, but at > > least the test case doesn't try this, so I strongly suspect the patch > > is incomplete to fix the PR. > > Comment #3 does not contain a patch to fix the problem elsewhere. > > In F2003, 15.2.6 "Interoperability of procedures and procedure interfaces= ", > I cannot find a prohibition on an alternate return in a subroutine > interface with BIND(C). > > I'm disinclined to let a patch fester in bugzilla to only attain > the same fate as my patch to PR68544. According to F2008 =C2=A715.3.7.2(5): > any dummy argument without the VALUE attribute [...] is interoperable wit= h an entity of the > referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the forma= l parameter Regardless of whether or not we accept alternate returns in BIND(C) procedures, the compiler must be at least consistent: if we accept them (which gfortran currently does), then we should be able to dump the C prototype (with -fc-prototypes), providing a formal parameter interoperable with the type of the alternate return dummy argument; if we reject them, then we should issue the error in parsing (before handling by -fc-prototypes). In either case, the error message should not be obscure or meaningless. Even so, the patch here is inconsistent since we accept the code, but issue an error when attempting to dump the C prototype. However, gfortran does not implement alternate return dummy arguments as actual arguments, but rather using an integer return code (regardless of the number of alternate return parameters in the interface). One interpretation of the consequences of this are that BIND(C) should be rejected, since there is no interoperable formal parameter which can be used to mirror the dummy argument (required by 15.3.7.2.5 above). An alternate interpretation is that we can continue to accept BIND(C) with alternate return dummy arguments, but just ignore the alternate return arguments. The former is perhaps more "correct"; the latter is perhaps more useful albeit potentially error-prone. To patch support for the latter case, rather than issuing an error in write_proc for procedures with alternate return arguments, we should output the actual interoperable prototype: in this case we would output 'int' as the return type (rather than void, as usual for subroutines) and alternate return dummy arguments would be ignored (not output). So the output for the example in the PR should really be 'int f()'. Something like this should do it: diff --git a/gcc/fortran/dump-parse-tree.c b/gcc/fortran/dump-parse-tree.c index af64588786a..9d6c3945cc5 100644 --- a/gcc/fortran/dump-parse-tree.c +++ b/gcc/fortran/dump-parse-tree.c @@ -3239,19 +3239,41 @@ write_proc (gfc_symbol *sym) gfc_formal_arglist *f; const char *sym_name; const char *intent_in; + bool has_alternate_returns; if (sym->binding_label) sym_name =3D sym->binding_label; else sym_name =3D sym->name; - if (sym->ts.type =3D=3D BT_UNKNOWN) + /* Look for alternate return placeholders. */ + for (f =3D gfc_sym_get_dummy_args (sym); f; f =3D f->next) + { + if (f->sym =3D=3D NULL) + { + has_alternate_returns =3D true; + break; + } + } + + gfc_typespec ts =3D sym->ts; + gfc_array_spec *as =3D sym->as; + if (has_alternate_returns) + { + /* Alternate returns are implemented as an integer return code from + an otherwise void subroutine; override this here. */ + ts.type =3D BT_INTEGER; + ts.kind =3D gfc_c_int_kind; + as =3D NULL; + } + + if (!has_alternate_returns && sym->ts.type =3D=3D BT_UNKNOWN) { fprintf (dumpfile, "void "); fputs (sym_name, dumpfile); } else - write_decl (&(sym->ts), sym->as, sym_name, true, &sym->declared_at); + write_decl (&ts, as, sym_name, true, &sym->declared_at); fputs (" (", dumpfile); @@ -3259,6 +3281,12 @@ write_proc (gfc_symbol *sym) { gfc_symbol *s; s =3D f->sym; + + /* Ignore alternate return dummy arguments, since they are handled + as an integer return value. */ + if (!s) + continue; + rok =3D get_c_type_name (&(s->ts), NULL, &pre, &type_name, &asterisk, &post, false); if (rok =3D=3D T_ERROR) EDIT: It appears Thomas beat me to a reply - I believe he's suggested something like what the above diff should provide. Perhaps this will be a useful starting point for him. > > RE: > > > PR fortran/88205 > > > * io.c (gfc_match_open): STATUS must be CHARACTER type. [...] > If I used e->where one gets > > a.f90:2:32: > > 2 | character(3), parameter :: a(0) =3D [character(3)::] > | 1 > Error: FORMAT tag at (1) cannot be a zero-sized array > > Now, imagine a few hundred lines separating the two statements. > I think the latter error locus is preferable. Yes, I agree. Swapping gfc_current_locus definitely works, but is possibly less readable(+maintainable) than my other suggestion of passing loc down as an argument... But that suggestion touches more code, so there are merits to either approach. In either case I have no real issue with this part of the patch regardless of implementation of the locus workaround. Fritz