Hi!
On Fri, 8 May 2015 14:24:15 +0300, Ilmir Usmanov <i.usma...@samsung.com> wrote:
> On 06.05.2015 14:38, Thomas Schwinge wrote:
> > On Tue, 5 May 2015 15:38:03 -0400, David Malcolm <dmalc...@redhat.com>
> > wrote:
> >> On Wed, 2015-04-29 at 14:10 +0200, Mikael Morin wrote:
> >>> Le 29/04/2015 02:02, David Malcolm a écrit :
> >>>> diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
> >>>> index 2c7c554..30e4eab 100644
> >>>> --- a/gcc/fortran/parse.c
> >>>> +++ b/gcc/fortran/parse.c
> >>>> @@ -4283,7 +4283,7 @@ parse_oacc_structured_block (gfc_statement acc_st)
> >>>> unexpected_eof ();
> >>>> else if (st != acc_end_st)
> >>>> gfc_error ("Expecting %s at %C", gfc_ascii_statement
> >>>> (acc_end_st));
> >>>> - reject_statement ();
> >>>> + reject_statement ();
> >>>> }
> >>>> while (st != acc_end_st);
> >>>>
> >>> I think this one is a bug; there should be braces around 'gfc_error' and
> >>> 'reject_statement'.
> If 'st' is 'acc_end_st', as it shall be, the statement is rejected. So,
> this is a bug.
>
> >
> >>> At least that's the pattern in 'parse_oacc_loop', and how the
> >>> 'unexpected_statement' function is used.
> >> FWIW, Jeff had approved that patch, so I've committed the patch to trunk
> >> (as r222823), making the indentation reflect the block structure.
> >>
> >> Thomas: should the
> >> reject_statement ();
> >> call in the above be guarded by the
> >> else if (st != acc_end_st)
> >> clause?
> > Indeed, this seems to be a bug that has been introduced very early in the
> > OpenACC Fortran front end development -- see how the
> > parse_oacc_structured_block function evolved in the patches posted in
> > <http://news.gmane.org/find-root.php?message_id=%3C52E1595D.9000007%40samsung.com%3E>
> > and following (Ilmir, CCed "just in case"). I also see that the
> > corresponding OpenMP code, parse_omp_structured_block, just calls
> > unexpected_statement, which Ilmir's initial patch also did, but at some
> > point, he then changed this to the current code: gfc_error followed by
> > reject_statement, as cited above -- I would guess for the reason to get a
> > better error message? (Tobias, should this thus also be done for OpenMP,
> > and/or extend unexpected_statement accordingly?)
> That's true.
> I've checked abandoned openacc-1_0-branch and I used
> unexpected_statement there (there still odd *_acc_* naming presents
> instead of new-and-shiny *_oacc_* one), but, as you mentioned, I've
> changed this for better error reporting... and introduced the bug.
>
> >
> > And then, I'm a bit confused: is it "OK" that despite this presumed logic
> > error, which affects all (?) valid executions of this parsing code, we're
> > not running into any issues with the OpenACC Fortran front end test
> > cases?
> I think, this is OK, since this is an !$ACC END _smth_ statement and it
> shall not present in the AST. So, it is abandoned later anyway ;) (if I
> remember correctly, during gfc_clear_new_st call). Although the bug does
> not affect the logic, it is still a bug.
>
> > OK for trunk?
> From my point of view, OK.
Thanks for your review! Nobody else had any comments, so I have now
committed my patch to trunk in r226246:
commit 1ed4ddb2615c807c239e1b3eb214655a4cc87f1a
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Mon Jul 27 14:26:41 2015 +0000
Fix logic error in Fortran OpenACC parsing
gcc/fortran/
* parse.c (parse_oacc_structured_block): Fix logic error.
Reported by Mikael Morin <mikael.mo...@sfr.fr>.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@226246
138bc75d-0d04-0410-961f-82ee72b054a4
---
gcc/fortran/ChangeLog | 5 +++++
gcc/fortran/parse.c | 6 ++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git gcc/fortran/ChangeLog gcc/fortran/ChangeLog
index 0ed6b9b..e5b7681 100644
--- gcc/fortran/ChangeLog
+++ gcc/fortran/ChangeLog
@@ -1,3 +1,8 @@
+2015-07-27 Thomas Schwinge <tho...@codesourcery.com>
+
+ * parse.c (parse_oacc_structured_block): Fix logic error.
+ Reported by Mikael Morin <mikael.mo...@sfr.fr>.
+
2015-07-24 Mikael Morin <mik...@gcc.gnu.org>
PR fortran/64986
diff --git gcc/fortran/parse.c gcc/fortran/parse.c
index 45ad63f..04b4c80 100644
--- gcc/fortran/parse.c
+++ gcc/fortran/parse.c
@@ -4383,8 +4383,10 @@ parse_oacc_structured_block (gfc_statement acc_st)
if (st == ST_NONE)
unexpected_eof ();
else if (st != acc_end_st)
- gfc_error ("Expecting %s at %C", gfc_ascii_statement (acc_end_st));
- reject_statement ();
+ {
+ gfc_error ("Expecting %s at %C", gfc_ascii_statement (acc_end_st));
+ reject_statement ();
+ }
}
while (st != acc_end_st);
Grüße,
Thomas
pgp8lg16qnyQJ.pgp
Description: PGP signature
