From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 41968 invoked by alias); 6 Dec 2018 00:03:28 -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 41948 invoked by uid 89); 6 Dec 2018 00:03:27 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.9 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=audit, Reese, reese, H*f:sk:CAE4aFA X-HELO: troutmask.apl.washington.edu Received: from troutmask.apl.washington.edu (HELO troutmask.apl.washington.edu) (128.95.76.21) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 00:03:17 +0000 Received: from troutmask.apl.washington.edu (localhost [127.0.0.1]) by troutmask.apl.washington.edu (8.15.2/8.15.2) with ESMTPS id wB603FJO050084 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Wed, 5 Dec 2018 16:03:15 -0800 (PST) (envelope-from sgk@troutmask.apl.washington.edu) Received: (from sgk@localhost) by troutmask.apl.washington.edu (8.15.2/8.15.2/Submit) id wB603FKa050083; Wed, 5 Dec 2018 16:03:15 -0800 (PST) (envelope-from sgk) Date: Thu, 06 Dec 2018 00:03:00 -0000 From: Steve Kargl To: Fritz Reese Cc: fortran , gcc-patches Subject: Re: Fortran patches Message-ID: <20181206000315.GA47513@troutmask.apl.washington.edu> Reply-To: sgk@troutmask.apl.washington.edu References: <20181205045945.GA40221@troutmask.apl.washington.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-SW-Source: 2018-12/txt/msg00327.txt.bz2 On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote: > 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? The patch passed regression testing on i586-*-freebsd. I'll also do regression testing on x86_64-*-freebsd prior to the commit. > 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. > 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. > 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. OK. I'll check to see if this works. > 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? program p character(3), parameter :: a(0) = [character(3)::] print a end With the patch using loc I get a.f90:3:10: 3 | print a | 1 Error: FORMAT tag at (1) cannot be a zero-sized array If I used e->where one gets a.f90:2:32: 2 | character(3), parameter :: a(0) = [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. I did not audit the other uses of e->where to see where the locus ends up pointing if those errors are triggered. > 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: > I'll address these before the commit. -- Steve