Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parsing of OCaml code by ocamlyacc #1012

Merged
merged 10 commits into from May 17, 2017
Merged

Fix parsing of OCaml code by ocamlyacc #1012

merged 10 commits into from May 17, 2017

Conversation

@DemiMarie
Copy link
Contributor

DemiMarie commented Jan 15, 2017

ocamlyacc would missparse certain valid OCaml code. As shown in MPR#7454 and MPR#7455, this can cause valid .mly files to be rejected, and for certain invalid files to be compiled to OCaml code that segfaults, without the .mly file containing any unsafe functions.

This patch fixes both problems.

@DemiMarie DemiMarie force-pushed the DemiMarie:fix-yacc branch from 6d2d8f2 to 5c864c3 Jan 15, 2017
vrow += varsetsize;
#endif

This comment has been minimized.

@dra27

dra27 Jan 15, 2017 Contributor

What is this change doing?

This comment has been minimized.

@DemiMarie

DemiMarie Jan 15, 2017 Author Contributor

The Clang static analyzer said that the store on line 114 is dead.

This comment has been minimized.

@dra27

dra27 Jan 15, 2017 Contributor

So it is - but in which case the line can be deleted (in the tidying up commit)...

yacc/main.c Outdated
@@ -138,7 +138,7 @@ void done(int k)

void onintr(int dummy)
{
done(1);
done(128 + dummy);

This comment has been minimized.

@dra27

dra27 Jan 15, 2017 Contributor

Similarly - what's this alteration for?

This comment has been minimized.

@DemiMarie

DemiMarie Jan 15, 2017 Author Contributor

Unix convention is that signals cause the program to exit with status 128 + the signal number.

This comment has been minimized.

@dra27

dra27 Jan 15, 2017 Contributor

That may well be so, but that's definitely not something to be smuggled inside another patch. Could this change at least be put on its own (I'm not entirely convinced that that belongs in this pull request at all)

}
}


This comment has been minimized.

@dra27

dra27 Jan 15, 2017 Contributor

Is eliminating this part of the patch, or just tidying?

This comment has been minimized.

@DemiMarie

DemiMarie Feb 11, 2017 Author Contributor

It is tidying.

@dra27
Copy link
Contributor

dra27 commented Jan 15, 2017

Thanks for the patch - a few things, though, please. There are various things which look unrelated to the two PRs (sorry if I'm misreading the code) - this single commit seems to combine some tidying, some necessary refactoring and fixing two PRs. I think I'm correct that fixing the actual code changes for the two PRs are not directly related, so please could you split it into four commits to create a slightly more bisect-able story:

  1. Tidying any superfluous code (is that just a legacy of its original porting from yacc?)
  2. Refactoring: the moving of caml_ident_start, etc. and moving unchanged bodies of code out to process_quoted_string, etc.
  3. Fix one PR
  4. Fix the other PR

Please also do a Changes entry (again, these aren't related so it should be really be two entries - it's fine fixing in one pull request, of course) and if you could bear to turn the two sample files you sent into testsuite tests, that would be great (I can do that afterwards if you don't have time/inclination).

@DemiMarie DemiMarie force-pushed the DemiMarie:fix-yacc branch from 5c864c3 to b77f8e6 Jan 15, 2017
@DemiMarie DemiMarie force-pushed the DemiMarie:fix-yacc branch 2 times, most recently from 110dd0f to 9dfcaa7 Jan 29, 2017
@DemiMarie DemiMarie force-pushed the DemiMarie:fix-yacc branch from f9ccd63 to e2c0c86 Feb 11, 2017
DemiMarie added 10 commits Jan 15, 2017
The %ident and %union directives don't make sense for OCaml,
and generated broken OCaml code.  Just drop support for them.
The processing of apostrophes, quoted strings and comments was
duplicated in at least two places.  This refactors both into
functions.

Also move the bitmap code, for use by later changes.
This adds support for parsing of raw OCaml strings.
This uses an explicit stack to keep track of parentheses and curly
braces in actions.
A'"'" was misparsed (the ' was not associated with the A as it should
be)
This adds a partial testcase for the better lexing of
OCaml code by ocamlyacc, as well as a bug fix.
It was left from the conversion of Berkely Yacc to ocamlyacc
@DemiMarie DemiMarie force-pushed the DemiMarie:fix-yacc branch from 3a638dd to f2950b5 Apr 4, 2017
@DemiMarie
Copy link
Contributor Author

DemiMarie commented Apr 8, 2017

@dra27 done.

@dra27
Copy link
Contributor

dra27 commented May 16, 2017

Sorry for the delay looking at this - many thanks for the refactoring and the extra parts! I'm just running it through the full CI at Inria, as that includes a few other C compilers, but I propose merging once that's complete.

@dra27 dra27 added the approved label May 16, 2017
@dra27
Copy link
Contributor

dra27 commented May 17, 2017

OK, the failures in precheck on mingw32/mingw64 are unrelated, so merging.

@dra27 dra27 merged commit 472ee2c into ocaml:trunk May 17, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche
Copy link
Member

gasche commented Jul 12, 2017

In the Changes file, this change is marked as compatibility-breaking (use of the * bullet instead of -). @DemiMarie, how does it break compatibility (besides fixing wrong behaviors)?

@DemiMarie
Copy link
Contributor Author

DemiMarie commented Jul 14, 2017

@gasche
Copy link
Member

gasche commented Jul 14, 2017

Thanks, I fixed the Changes in #1241.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.