[PATCH, Fortran] -- PR fortran/87923 -- fix ICE when resolving I/O tags and simplify io.c

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

[PATCH, Fortran] -- PR fortran/87923 -- fix ICE when resolving I/O tags and simplify io.c

gcc - fortran mailing list
All,

The attached patch fixes PR 87923 while also simplifying the code in
io.c. I do say this patch simplifies io.c because it is true. This
patch causes more deletions than insertions to the file -- a rare
sight:

 gcc/fortran/io.c | 859 ++++++++++++++++++++++++-------------------------------
 1 file changed, 381 insertions(+), 478 deletions(-)

Over the years various special cases have been introduced which are
not necessary. The constraints for I/O statements are verified in
several different places, and in fact some constraints (like whether
an I/O tag is a scalar default-kind character) are checked as many as
three times. This patches simplifies the code by moving all checks not
necessary for matching out of the matching phase and into the
resolution phase. The resolve_tag function already checks several
constraints for I/O tags, including type and kind, which were
previously checked redundantly in other places. This patch also
improves error reporting by providing the correct locus for I/O
statement error messages, and by providing a more detailed error
message when a tag which requires an init-expr is not a valid
init-expr.

The patch also increases test coverage of I/O statements, especially
for I/O tags, by incorporating testcases provided in PRs from the past
which the removed code originally addressed: specifically, PRs 66724
and 66725. With the patch applied on the current master
(c72a1b6f8b2...) all regression tests with check-fortran pass,
including the new ones.

OK for master?


commit 5a403f4e8e77123994ca1ed05e8f10877423fe55
Author: Fritz Reese <[hidden email]>
Date:   Mon Apr 6 12:13:48 2020 -0400

    Fix fortran/87923 -- ICE(s) when resolving I/O tags.

    2020-04-06  Fritz Reese  <[hidden email]>

    This patch also reorganizes I/O checking code. Checks which were done
    in the matching phase which do not affect the match result are moved to the
    resolution phase. Checks which were duplicated in both the
matching phase and
    resolution phase have been reduced to one check in the resolution phase.

    Another section of code which used a global async_io_dt flag to
check for and
    assign the asynchronous attribute to variables used in
asynchronous I/O has been
    simplified.

    Furthermore, this patch improves error reporting and expands test
coverage of
    I/O tags:

     - "TAG must be an initialization expression" reported by io.c
       (check_io_constraints) is replaced with an error from expr.c
       (gfc_reduce_init_expr) indicating _why_ the expression is not a valid
       initialization expression.

     - Several distinct error messages regarding the check for scalar
+ character
       + default kind have been unified to one message reported by resolve_tag
       or check_*_constraints.

    gcc/fortran/ChangeLog:

            PR fortran/87923
            * gfortran.h (gfc_resolve_open, gfc_resolve_close): Add
            locus parameter.
            (gfc_resolve_dt): Add code parameter.
            * io.c (async_io_dt, check_char_variable, is_char_type): Removed.
            (resolve_tag_format): Add locus to error message regarding
zero-sized
            array in FORMAT tag.
            (check_open_constraints, check_close_constraints): New
functions called
            at resolution time.
            (gfc_match_open, gfc_match_close, match_io): Move checks which don't
            affect the match result to new functions check_open_constraints,
            check_close_constraints, check_io_constraints.
            (gfc_resolve_open, gfc_resolve_close): Call new functions
            check_open_constraints, check_close_constraints after all
tags have been
            independently resolved.  Remove duplicate constraints
which are already
            verified by resolve_tag. Explicitly pass locus to all error reports.
            (compare_to_allowed_values): Add locus parameter and
provide explicit
            locus all error reports.
            (match_open_element, match_close_element, match_file_element,
            match_dt_element, match_inquire_element): Remove redundant
special cases
            for ASYNCHRONOUS and IOMSG tags.
            (gfc_resolve_dt): Remove redundant special case for format
expression.
            Call check_io_constraints, forwarding an I/O list as the io_code
            parameter if present.
            (check_io_constraints): Change return type to bool. Pass
explicit locus
            to error reports. Move generic checks after tag-specific
checks, since
            errors are no longer buffered.  Move simplification of
format string to
            match_io.  Remove redundant checks which are verified by
resolve_tag.
            Remove usage of async_io_dt flag and explicitly mark symbols used in
            asynchronous I/O with the asynchronous attribute.
            * resolve.c (resolve_transfer, resolve_fl_namelist):
Remove checks for
            async_io_dt flag. This is now done in io.c (check_io_constraints).
            (gfc_resolve_code): Pass code locus to gfc_resolve_open,
            gfc_resolve_close, gfc_resolve_dt.

    gcc/testsuite/ChangeLog:

            PR fortran/87923
            * gfortran.dg/f2003_io_8.f03: Fix expected error messages.
            * gfortran.dg/io_constraints_8.f90: Likewise.
            * gfortran.dg/iomsg_2.f90: Likewise.
            * gfortran.dg/pr66725.f90: Likewise.
            * gfortran.dg/pr88205.f90: Likewise.
            * gfortran.dg/write_check4.f90: Likewise.
            * gfortran.dg/asynchronous_5.f03: New test.
            * gfortran.dg/io_constraints_15.f90: Likewise.
            * gfortran.dg/io_constraints_16.f90: Likewise.
            * gfortran.dg/io_constraints_17.f90: Likewise.
            * gfortran.dg/io_constraints_18.f90: Likewise.
            * gfortran.dg/io_tags_1.f90: Likewise.
            * gfortran.dg/io_tags_10.f90: Likewise.
            * gfortran.dg/io_tags_2.f90: Likewise.
            * gfortran.dg/io_tags_3.f90: Likewise.
            * gfortran.dg/io_tags_4.f90: Likewise.
            * gfortran.dg/io_tags_5.f90: Likewise.
            * gfortran.dg/io_tags_6.f90: Likewise.
            * gfortran.dg/io_tags_7.f90: Likewise.
            * gfortran.dg/io_tags_8.f90: Likewise.
            * gfortran.dg/io_tags_9.f90: Likewise.
            * gfortran.dg/write_check5.f90: Likewise.

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

Re: [PATCH, Fortran] -- PR fortran/87923 -- fix ICE when resolving I/O tags and simplify io.c

gcc - fortran mailing list
Tobias,

Thanks very much for the review.

On Thu, Apr 9, 2020 at 5:21 AM Tobias Burnus <[hidden email]> wrote:

>
> Hi,
>
> On 4/6/20 7:25 PM, Fritz Reese via Fortran wrote:
>
> > The attached patch fixes PR 87923 while also simplifying the code in
> > io.c.
>
> Thanks for the work, which looks great; it is a bit lengthy
> but mostly moving code or mechanical (%C → %L).
> It also has an impressive amount of new test cases!

I also wished the patch could be easier on the eyes, but alas
sometimes this is the price of progress. :-)

> * First line in git commit "Fix fortran/87923 -- ICE(s) when resolving I/O tags."
>    It helps with doing patch archeology if they are the same – or if the GIT one
>    is a substring of the email subject. (If it is about a PR, searching for the PR
>    usually works, but also not al emails have the PR number in the subject.)
>    For this patch, you use:
>    email: "[PATCH, Fortran] -- PR fortran/87923 -- fix ICE when resolving I/O tags and simplify io.c"
>    GIT: "Fix fortran/87923 -- ICE(s) when resolving I/O tags."

Yes, that is a good point. I will alter the commit summary to match
the email subject.

> * Please check whether the ChangeLog lines are too long; I didn't count
>    but it looks as if they might be too long. For sure, they
>    were too long for your mail program …

I copied the git commit message from the log, which git formats with
an extra level of indentation. I wrapped the raw ChangeLog entries and
commit message to 80 characters, but after the extra indentation my
mail client indeed wrapped the lines during post-processing. I suppose
I should wrap these each to 76 to account for git's indentation. That
certainly makes "git log" look better.

> * In the following comment, you have two empty tailing lines:
>
> +   Tag expressions are already resolved by resolve_tag, which includes
> +   verifying the type, that they are scalar, and verifying that BT_CHARACTER
> +   tags are of default kind.
> +
> +   */

Oops!


I will commit the patch with these fixes rebased on master after one
final build+test. Thanks again for taking a look.

Cheers,

---
Fritz Reese
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, Fortran] -- PR fortran/87923 -- fix ICE when resolving I/O tags and simplify io.c

gcc - fortran mailing list
On Fri, Apr 10, 2020 at 8:14 AM Rainer Orth <[hidden email]> wrote:
>
> Hi Fritz,
[...]

> one new testcases comes up as UNRESOLVED everywhere:
>
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-not original "volatile.*?ivar_noasync"
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-times original "volatile.*?ccvar_async" 1
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-times original "volatile.*?darrvar_async" 1
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-times original "volatile.*?dvar_async" 1
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-times original "volatile.*?ivar_async" 1
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-times original "volatile.*?lvar_async" 1
> +UNRESOLVED: gfortran.dg/asynchronous_5.f03   -O   scan-tree-dump-times original "volatile.*?rvar_async" 1
>
> gfortran.dg/asynchronous_5.f03   -O  : dump file does not exist
>
> It has several scan-tree-dump* checks, but no corresponding
> -fdump-tree-* option.  Please fix (and make sure not to look only for
> FAILs during regtesting in the future).
>
>         Rainer

Ah! My mistake... I will fix and look for this in the future.

Fritz