Re: Fix logic error in Fortran OpenACC parsing

[Ilmir Usmanov <i dot usmanov at samsung dot com>]

Hi!

On 06.05.2015 14:38, Thomas Schwinge wrote:
> Hi!
>
> 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.

> commit 068eebfa63b2b4c8849ed5fd2c9d0a130586dfb0
> Author: Thomas Schwinge <thomas@codesourcery.com>
> Date:   Wed May 6 13:18:18 2015 +0200
>
>      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>.
> ---
>   gcc/fortran/parse.c |    6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git gcc/fortran/parse.c gcc/fortran/parse.c
> index 30e4eab..e977498 100644
> --- gcc/fortran/parse.c
> +++ gcc/fortran/parse.c
> @@ -4282,8 +4282,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
--
Ilmir.