From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 97539 invoked by alias); 5 Dec 2018 21:49:18 -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 97223 invoked by uid 89); 5 Dec 2018 21:49:02 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=fritz, Fritz, Kargl, HTo:D*washington.edu X-HELO: mail-it1-f173.google.com Received: from mail-it1-f173.google.com (HELO mail-it1-f173.google.com) (209.85.166.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Dec 2018 21:48:52 +0000 Received: by mail-it1-f173.google.com with SMTP id x19so23899699itl.1; Wed, 05 Dec 2018 13:48:43 -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; bh=tFOucl/THF2lsbBfC4ORv4KGk9DvpQbQvQvrkMRAsrw=; b=kkIjQBMAhgNvuxw+NqOjs1OaTcdcO4gWv1g0lbgLHSLd/9+6K+EDL3C1kfKRqvoVhQ kU3wns4m7BZvmhQLLNwNy5/yo43tD1M7uesdluWw52SnFu3zRlSxjIsi9/8GMwAvodb9 7hZRtb4OnDx1dZXETfgR6R0nLdqiU776fXMTo1cDUsnnEtWuabQN754aovPTEL1fUz95 gxwxr9LFNyCdiY00dG5hObeMnzx5t+6yXtGk5iUGBKaG+UVBdosP2kCDt71wi8c/dI+t 2HfPm0GEXwW+AKNFxubP2tO3dg44B0U87jc55O975J8+0D1l52KL+2cejxxMnbE08aws FxCA== MIME-Version: 1.0 References: <20181205045945.GA40221@troutmask.apl.washington.edu> In-Reply-To: <20181205045945.GA40221@troutmask.apl.washington.edu> From: Fritz Reese Date: Wed, 05 Dec 2018 21:49:00 -0000 Message-ID: Subject: Re: Fortran patches To: Steve Kargl Cc: fortran , gcc-patches Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2018-12/txt/msg00317.txt.bz2 On Wed, Dec 5, 2018 at 12:00 AM Steve Kargl wrote: > > I intend to commit the attached patch on Saturday. Thanks for the work. I assume the patch bootstraps and passes regression tests? 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. 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. RE: > PR fortran/88205 > * io.c (gfc_match_open): STATUS must be CHARACTER type. [...] >@@ -2161,6 +2167,12 @@ gfc_match_open (void) > > if (!open->file && open->status) > { >+ if (open->status->ts.type != BT_CHARACTER) >+ { >+ gfc_error ("STATUS must be a default character type at %C"); >+ goto cleanup; >+ } >+ > if (open->status->expr_type == EXPR_CONSTANT > && gfc_wide_strncasecmp (open->status->value.character.string, > "scratch", 7) != 0) Both resolve_tag() and is_char_type() actually already catch type mismatches for STATUS (the latter for constant expressions). The real problem is the following condition which checks STATUS before it has been processed yet, since NEWUNIT is processed before STATUS. I think the correct thing to do is actually to move the NEWUNIT/UNIT if-block after the STATUS if-block, rather than adding a new phrasing for the same error. Then we should see: pr88205.f90:13:29: open (newunit=n, status=status) 1 Error: STATUS requires a scalar-default-char-expr at (1) RE: > PR fortran/88328 > * io.c (resolve_tag_format): Detect zero-sized array. [...] >Index: gcc/fortran/io.c >=================================================================== >--- gcc/fortran/io.c (revision 266718) >+++ gcc/fortran/io.c (working copy) >@@ -1636,6 +1636,12 @@ resolve_tag_format (gfc_expr *e) > gfc_expr *r; > gfc_char_t *dest, *src; > >+ if (e->value.constructor == NULL) >+ { >+ gfc_error ("FORMAT tag at %C cannot be a zero-sized array"); >+ return false; >+ } >+ > n = 0; > c = gfc_constructor_first (e->value.constructor); > len = c->expr->value.character.length; [...] >@ -3231,12 +3257,21 @@ gfc_resolve_dt (gfc_dt *dt, locus *loc) > { > gfc_expr *e; > io_kind k; >+ locus loc_tmp; > > /* This is set in any case. */ > gcc_assert (dt->dt_io_kind); > k = dt->dt_io_kind->value.iokind; > >- RESOLVE_TAG (&tag_format, dt->format_expr); >+ loc_tmp = gfc_current_locus; >+ gfc_current_locus = *loc; >+ if (!resolve_tag (&tag_format, dt->format_expr)) >+ { >+ gfc_current_locus = loc_tmp; >+ return false; >+ } >+ gfc_current_locus = loc_tmp; >+ > RESOLVE_TAG (&tag_rec, dt->rec); > RESOLVE_TAG (&tag_spos, dt->pos); > RESOLVE_TAG (&tag_advance, dt->advance); Is it really true that resolve_tag_format needs the locus at gfc_resolve_dt::loc instead of e->where as with the other errors in resolve_tag_format? If so, are the other errors also incorrect in using e->where? Might it then be better to pass loc from gfc_resolve_dt down to resolve_tag/RESOLVE_TAG and then resolve_tag_format, instead of swapping gfc_current_locus? RE: > PR fortran/88048 > * resolve.c (check_data_variable): Convert gfc_internal_error to > an gfc_error. Add a nearby missing 'return false;' [...] > PR fortran/88025 > * expr.c (gfc_apply_init): Remove asserts and check for valid > ts->u.cl->length. [...] > PR fortran/88116 > * simplify.c: Remove internal error and return gfc_bad_expr. These look good. A few pedantic comments: RE: > PR fortran/88269 > * io.c (io_constraint): Update macro. Remove incompatible use > of io_constraint and give explicit error. [...] There should be two separate references to io_constraint and check_io_constraints: > * io.c (io_constraint): Update macro. > (check_io_constraints) Remove incompatible use > of io_constraint and give explicit error. RE: > #define io_constraint(condition,msg,arg)\ > if (condition) \ > {\ >- gfc_error(msg,arg);\ >+ if ((arg)->lb != NULL) \ >+ gfc_error(msg,arg);\ >+ else \ >+ gfc_error(msg,&gfc_current_locus);\ > m = MATCH_ERROR;\ > } I think you could safely follow style conventions here (Comma-Space and Function-Space-Parenthesis): -#define io_constraint(condition,msg,arg)\ +#define io_constraint(condition, msg, arg)\ if (condition) \ {\ - gfc_error(msg,arg);\ + if ((arg)->lb != NULL) \ + gfc_error (msg, arg);\ + else \ + gfc_error (msg, &gfc_current_locus);\ m = MATCH_ERROR;\ } RE: >@@ -3741,11 +3779,14 @@ if (condition) \ > if (expr && expr->ts.type != BT_CHARACTER) > { > >- io_constraint (gfc_pure (NULL) && (k == M_READ || k == M_WRITE), >- "IO UNIT in %s statement at %C must be " >+ if (gfc_pure (NULL) && (k == M_READ || k == M_WRITE)) >+ { >+ gfc_error ("IO UNIT in %s statement at %C must be " > "an internal file in a PURE procedure", > io_kind_name (k)); >- >+ return MATCH_ERROR; >+ } >+ > if (k == M_READ || k == M_WRITE) > gfc_unset_implicit_pure (NULL); > } Trailing whitespace on line 3789 (post-patch). RE: > PR fortran/87945 > * decl.c (var_element): Inquiry parameter cannot be a data object. > (match_data_constant): Inquiry parameter can be a data > in a data statement. [...] >@@ -391,6 +399,14 @@ match_data_constant (gfc_expr **result) > } > else if (m == MATCH_YES) > { >+ /* If a parameter inquiry ends up here, symtree is NULL but **result >+ contains the right constant expression. Check here. */ >+ if ((*result)->symtree == NULL >+ && (*result)->expr_type == EXPR_CONSTANT >+ && ((*result)->ts.type == BT_INTEGER >+ || (*result)->ts.type == BT_REAL)) >+ return m; >+ > /* F2018:R845 data-stmt-constant is initial-data-target. > A data-stmt-constant shall be ... initial-data-target if and > only if the corresponding data-stmt-object has the POINTER Trailing whitespace on line 406 (post-patch). Cheers, Fritz