Fortran patches

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Fortran patches

Steve Kargl
I intend to commit the attached patch on Saturday.

2018-12-02  Steven G. Kargl  <[hidden email]>

        PR fortran/87922
        * io.c (gfc_match_open): ASYNCHRONOUS must be scalar.

        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.

        PR fortran/88139
        * dump-parse-tree.c (write_proc): Alternate return.

        PR fortran/88025
        * expr.c (gfc_apply_init): Remove asserts and check for valid
        ts->u.cl->length.

        PR fortran/88048
        * resolve.c (check_data_variable): Convert gfc_internal_error to
        an gfc_error.  Add a nearby missing 'return false;'

        PR fortran/88116
        * simplify.c: Remove internal error and return gfc_bad_expr.

        PR fortran/88205
        * io.c (gfc_match_open): STATUS must be CHARACTER type.

        PR fortran/88206
        * match.c (gfc_match_type_spec): REAL can be an intrinsic function.

        PR fortran/88228
        * expr.c (check_null, check_elemental): Work around -fdec and
        initialization with logical operators operating on integers.

        PR fortran/88249
        * gfortran.h: Update prototype for gfc_resolve_filepos
        * io.c (gfc_resolve_filepos): Accept the locus to include in errors.
        * resolve.c (gfc_resolve_code): Pass locus.
 
        PR fortran/88269
        * io.c (io_constraint): Update macro.  Remove incompatible use
        of io_constraint and give explicit error.
 
        PR fortran/88328
        * io.c (resolve_tag_format): Detect zero-sized array.

2018-12-02  Steven G. Kargl  <[hidden email]>

        PR fortran/87922
        * gfortran.dg/pr87922.f90: New test.

        PR fortran/887945
        * gfortran.dg/pr87945_1.f90: New test.
        * gfortran.dg/pr87945_2.f90: New test.

        PR fortran/87994
        * gfortran.dg/pr87994_1.f90: New test.
        * gfortran.dg/pr87994_2.f90: New test.
        * gfortran.dg/pr87994_3.f90: New test.

        PR fortran/88025
        * gfortran.dg/pr88025.f90: New test.

        PR fortran/88048
        * gfortran.dg/pr88048.f90: New test.

        PR fortran/88116
        * gfortran.dg/pr88116_1.f90: New test.
        * gfortran.dg/pr88116_2.f90: New test.

        PR fortran/88139
        * gfortran.dg/pr88139.f90: New test.

        PR fortran/88205
        * gfortran.dg/pr88205.f90: New test.

        PR fortran/88206
        * gfortran.dg/pr88206.f90: New test.

        PR fortran/88228
        * gfortran.dg/pr88228.f90: New test.

        PR fortran/88249
        * gfortran.dg/pr88249.f90: New test.

        PR fortran/88269
        * gfortran.dg/pr88269.f90: New test.

        PR fortran/88328
        * gfortran.dg/pr88328.f90: New test.

--
Steve

big.diff (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fortran patches

Steve Kargl
On Wed, Dec 05, 2018 at 04:48:28PM -0500, Fritz Reese wrote:
> On Wed, Dec 5, 2018 at 12:00 AM Steve Kargl
> <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Fortran patches

Thomas Koenig-6
Hi Steve,

>>>         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.

I know :-)

> 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 also does not allow this, and does not offer a valid interpretation
of what it should mean.

If it has a meaning, it should be translatable into something prescribed
by the standard with -fc-prototypes.

I have assigned the error to myself, so I will not forget to fix
it before the gcc 9 release.

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: Fortran patches

Steve Kargl
In reply to this post by Steve Kargl
On Thu, Dec 06, 2018 at 02:08:54PM -0500, Fritz Reese wrote:

> On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl
> <[hidden email]> 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 != e->ts.type || op2->ts.kind != e->ts.kind)
>             gfc_convert_type (op2, &e->ts, 1);
>           e = 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 = BT_INTEGER;
>           e->ts.kind = op1->ts.kind;
>           e = logical_to_bitwise (e);
> -         return resolve_function (e);
> +         break;
>         }

Intersting.  I wonder why resolve_function() appears here.  
Hmmm, 'svn annotate' points to r241535 with the committer
being foreese. :-)  Log message says you are trying to convert
logical op on integers for DEC.  Were trying to catch the
logical functions like NOT()?

I'll include the above in my patch Saturday activities.


> > > >        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 §15.3.7.2(5):
>
> > any dummy argument without the VALUE attribute [...] is interoperable with an entity of the
> > referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the formal parameter

Yep.

> 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

I think we should determine what other compilers do with
BIND(C) and alternate return dummy arguments, and follow
suit.  


> > > >        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) = [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.

I agree that this could get messy, but I'm hoping only
format strings need this special handling.  A Fortran
string must contain '(' and ')', so zero-sized arrays
can never appear as a fmt=array.  

--
Steve
Reply | Threaded
Open this post in threaded view
|

Re: Fortran patches

Steve Kargl
In reply to this post by Steve Kargl
On Thu, Dec 06, 2018 at 02:08:54PM -0500, Fritz Reese wrote:

> On Wed, Dec 5, 2018 at 7:03 PM Steve Kargl
> >
> > > 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 §15.3.7.2(5):
>
> > any dummy argument without the VALUE attribute [...] is interoperable with an entity of the
> > referenced type (ISO/IEC 9899:1999, 6.2.5, 7.17, and 7.18.1) of the formal 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.

Here's an alternative patch that would reject a subroutine
with an alternate return dummy argument with the bind(c)
attributes.  I'm still trying to determine if the code
should be legal.  The c.l.f thread I started isn't helping :(

Index: decl.c
===================================================================
--- decl.c (revision 266766)
+++ decl.c (working copy)
@@ -7467,6 +7467,7 @@ gfc_match_subroutine (void)
   match is_bind_c;
   char peek_char;
   bool allow_binding_name;
+  locus loc;
 
   if (gfc_current_state () != COMP_NONE
       && gfc_current_state () != COMP_INTERFACE
@@ -7532,6 +7533,8 @@ gfc_match_subroutine (void)
   /* Here, we are just checking if it has the bind(c) attribute, and if
      so, then we need to make sure it's all correct.  If it doesn't,
      we still need to continue matching the rest of the subroutine line.  */
+  gfc_gobble_whitespace ();
+  loc = gfc_current_locus;
   is_bind_c = gfc_match_bind_c (sym, allow_binding_name);
   if (is_bind_c == MATCH_ERROR)
     {
@@ -7543,6 +7546,8 @@ gfc_match_subroutine (void)
 
   if (is_bind_c == MATCH_YES)
     {
+      gfc_formal_arglist *arg;
+
       /* The following is allowed in the Fortran 2008 draft.  */
       if (gfc_current_state () == COMP_CONTAINS
   && sym->ns->proc_name->attr.flavor != FL_MODULE
@@ -7556,8 +7561,17 @@ gfc_match_subroutine (void)
           gfc_error ("Missing required parentheses before BIND(C) at %C");
           return MATCH_ERROR;
         }
-      if (!gfc_add_is_bind_c (&(sym->attr), sym->name,
-      &(sym->declared_at), 1))
+
+      /* Scan the dummy arguments for an alternate return.  */
+      for (arg = sym->formal; arg; arg = arg->next)
+ if (!arg->sym)
+  {
+    gfc_error ("Alternate return dummy argument cannot appear in a "
+       "SUBROUTINE with the BIND(C) attribute at %L", &loc);
+    return MATCH_ERROR;
+  }
+
+      if (!gfc_add_is_bind_c (&(sym->attr), sym->name, &(sym->declared_at), 1))
         return MATCH_ERROR;
     }

--
steve