diff mbox

Fix logic error in Fortran OpenACC parsing

Message ID 87d1zdzna3.fsf@schwinge.name
State New
Headers show

Commit Message

Thomas Schwinge July 27, 2015, 2:29 p.m. UTC 
Hi!

On Fri, 8 May 2015 14:24:15 +0300, Ilmir Usmanov <i.usmanov@samsung.com> wrote:
> On 06.05.2015 14:38, Thomas Schwinge wrote:
> > On Tue, 5 May 2015 15:38:03 -0400, David Malcolm <dmalcolm@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.morin@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(-)



Grüße,
 Thomas
diff mbox

Patch

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  <thomas@codesourcery.com>
+
+	* parse.c (parse_oacc_structured_block): Fix logic error.
+	Reported by Mikael Morin <mikael.morin@sfr.fr>.
+
 2015-07-24  Mikael Morin  <mikael@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);