On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote:
> On Wed, Dec 5, 2018 at 12:00 AM Steve Kargl
> <s...@troutmask.apl.washington.edu> 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
