Re: Fortran patches

[Steve Kargl <sgk at troutmask dot apl dot washington dot edu>]

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