Message ID | c97637b1-b86a-1c32-62b8-d5f3bbedf257@charter.net |
---|---|
State | New |
Headers | show |
Hi Jerry, I have tested your patch and gave it a review and the only thing I like to have is a testcase. Can you provide one from the PR? With a testcase I say the patch is ok for trunk and thanks for the patch. Please note, I don't have review rights in the area the patch addresses, although I am familiar with the matcher having worked in it. This "review" is just a helper for an official reviewer to "second" my opinion, hoping to get your patch faster into trunk. Regards and thanks for the patch, Andre On Wed, 18 May 2016 11:25:57 -0700 Jerry DeLisle <jvdelisle@charter.net> wrote: > Hi all, > > The following patch regression tested on x86-64. The ICE is from an attempt to > free a bad expression after a MATCH_ERROR is returned. I have not been able to > identify an exact cause, there being numerous matchers involved attempting to > match the logical expression. > > Regardless, it is an error on invalid so I suggest we commit this patch and > close the PR. I dont think its a regression as marked in bugzilla. I see the > the internal error as far back as 4.5. If someone has an earlier build and can > see where this does not occur, please let me know. (In case I missed something. > > The results of the patch gives the following: > > $ gfc s.f > s.f:4:9: > > if ( x(1) < 0 .or. > 1 > Error: Can not process after the IF statement shown at (1) > f951: Error: Unexpected end of file in ‘s.f’ > > > OK for trunk? > > Regards, > > Jerry > > 2016-05-18 Jerry DeLisle <jvdelisle@gcc.gnu.org> > > PR fortran/66461 > * match.c (gfc_match_if): Catch unxpected MATCH_ERROR and issue an error > message. > > > diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c > index f3a4a43..85e6f92 100644 > --- a/gcc/fortran/match.c > +++ b/gcc/fortran/match.c > @@ -1560,7 +1560,16 @@ gfc_match_if (gfc_statement *if_type) > if (m == MATCH_ERROR) > return MATCH_ERROR; > > - gfc_match (" if ( %e ) ", &expr); /* Guaranteed to match. */ > + m = gfc_match (" if ( %e ) ", &expr); /* Not always guaranteed to match. */ > + > + if (m == MATCH_ERROR) > + { > + /* Under some invalid conditions like unexpected end of file, one > + can get an error in the match. We bail out here and hope for > + the best (the best being an error reported somewhere else). */ > + gfc_error ("Can not process after the IF statement shown at %C"); > + return MATCH_ERROR; > + } > > m = gfc_match_pointer_assignment (); > if (m == MATCH_YES) > >
On 05/22/2016 04:53 AM, Andre Vehreschild wrote: > Hi Jerry, > > I have tested your patch and gave it a review and the only thing I like > to have is a testcase. Can you provide one from the PR? With a testcase > I say the patch is ok for trunk and thanks for the patch. > > Please note, I don't have review rights in the area the patch > addresses, although I am familiar with the matcher having worked in it. > This "review" is just a helper for an official reviewer to "second" my > opinion, hoping to get your patch faster into trunk. > > Regards and thanks for the patch, > Andre Thanks Andre, I am going to hold off just a bit after some further info from Mikael. We are obviously intercepting the problem in different ways, but the root problem still exists. Regards, Jerry
On 05/22/2016 06:00 PM, Jerry DeLisle wrote: > On 05/22/2016 04:53 AM, Andre Vehreschild wrote: >> Hi Jerry, >> >> I have tested your patch and gave it a review and the only thing I like >> to have is a testcase. Can you provide one from the PR? With a testcase >> I say the patch is ok for trunk and thanks for the patch. >> >> Please note, I don't have review rights in the area the patch >> addresses, although I am familiar with the matcher having worked in it. >> This "review" is just a helper for an official reviewer to "second" my >> opinion, hoping to get your patch faster into trunk. >> >> Regards and thanks for the patch, >> Andre > > Thanks Andre, I am going to hold off just a bit after some further info from Mikael. > > We are obviously intercepting the problem in different ways, but the root > problem still exists. > > Regards, > > Jerry > Committed rev 236627 under simple rule. Also credit Mikael Morin for breaking through the ICE so to speak. Many thanks, Jerry https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=236627
diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c index f3a4a43..85e6f92 100644 --- a/gcc/fortran/match.c +++ b/gcc/fortran/match.c @@ -1560,7 +1560,16 @@ gfc_match_if (gfc_statement *if_type) if (m == MATCH_ERROR) return MATCH_ERROR; - gfc_match (" if ( %e ) ", &expr); /* Guaranteed to match. */ + m = gfc_match (" if ( %e ) ", &expr); /* Not always guaranteed to match. */ + + if (m == MATCH_ERROR) + { + /* Under some invalid conditions like unexpected end of file, one + can get an error in the match. We bail out here and hope for + the best (the best being an error reported somewhere else). */ + gfc_error ("Can not process after the IF statement shown at %C"); + return MATCH_ERROR; + } m = gfc_match_pointer_assignment (); if (m == MATCH_YES)