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

ocamllex: Exact check for the absence of memory instructions. #1713

Merged
merged 3 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@maranget
Copy link
Contributor

maranget commented Apr 10, 2018

The PR corrects MPR 7760: Segfault in ocamllex-generated code using 'shortest'.

ocamllex: exact check for the absence of memory instructions.
Lexing automata come in two flavors
  (a) Ordinary DFA.
  (b) Extended DFA that additionally record positions from the buffer,
      so as to implement ocamlex 'as' construct.

For each entry, case (b) is selected when (1) there is come code to
record position in the lexer tables (2) the states that are reachable from
this entry may record positions in memory cell (as shown by the vector
of memory size being of non-zero size).

In case (b), a frech vector of memory cells is allocated and the new lexing
engine (ocaml_new_lex_engine) is called; in case (a) a simpler lexing engine
(ocaml_new_lex_engine) is called.
@gasche
Copy link
Member

gasche left a comment

I did a non-expert review of the code (I had never looked at it before), and the patch makes sense from the explanation that Luc gave. I had two questions while reading the code:

  1. Why is only output changed when outputbis contains the same code? (output generates C code, outputbis generates ML code, as the names suggest :-) The answer is that only output uses compaction (why?), as can be seen in main.ml.

  2. This PR disables emission of memory instruction when an action was using memory, but the compaction pass completely removed all memory actions. Wouldn't there be an intermediate case where some memory actions remain, but the ones for the state that we are processing were removed? Is there a risk of bug in this case?

Changes Outdated
@@ -475,6 +475,10 @@ Working version
- GPR#1538: Make pattern matching compilation more robust to ill-typed columns
(Gabriel Scherer and Thomas Refis, review by Luc Maranget)

- MPR#7760: Exact selecton of lexing engine, or "Segfault in
ocamllex-generated code using 'shortest'
(Luc Maranget, reported by def (?), review by (?))

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

The reporter is Stephen Dolan (see MPR#7760). If you consider that the analysis work that Frédéric Bour ("def") did help you write your fix, you can credit him with "Luc Maranget, Frédéric Bour, report by Stephen Dolan".

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

(Also, the change entry could have the GPR number as well: MPR#7760, GPR#1713.)

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

Also, you chose the wrong category for Changes. You are in the "4.06 maintenance branch" which should in fact be marked "4.06.1" (I'll fix this), you should go in the "OCaml 4.07" part, just below (by numeric MPR ordering):

- MPR#7751, GPR#1657: The toplevel prints some concrete types as abstract
  (Jacques Garrigue, report by Matej Kosik)

This comment has been minimized.

@maranget

maranget Apr 10, 2018

Author Contributor

Ok I change Change again.

@maranget maranget force-pushed the maranget:shortest branch from 2b44bba to 4f99046 Apr 10, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 10, 2018

I removed the suggestion to change Outputbis as I found it unconvincing when looking at the code again.

@gasche

gasche approved these changes Apr 10, 2018

Copy link
Member

gasche left a comment

Approved. I would still like to check that the CI passes before this PR is merged.

Changes Outdated
@@ -423,6 +423,10 @@ Working version
- MPR#7751, GPR#1657: The toplevel prints some concrete types as abstract
(Jacques Garrigue, report by Matej Kosik)

- PR#7760, GPR#1713: Exact selection of lexing engine, that is
correct "Segfault in ocamllex-generated code using 'shortest'"
(Luc Maranget, Frédéric Bour, report by Stephen Dolan, Review by ?)

This comment has been minimized.

@gasche

gasche Apr 10, 2018

Member

I guess I'm ?.

@maranget

This comment has been minimized.

Copy link
Contributor Author

maranget commented Apr 10, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 10, 2018

For now the CI results are not in: "Some checks haven't completed yet". You can either wait for them to finish, and then correct and merge (you can squash all your commits together with git rebase -i trunk for example), or you update the branch, and then we wait for the restarted CI to complete again -- I can take care of the merge.

@maranget maranget force-pushed the maranget:shortest branch from 4f99046 to 36c295b Apr 10, 2018

@maranget maranget force-pushed the maranget:shortest branch from 36c295b to 0fb23c1 Apr 10, 2018

@maranget maranget merged commit 6dddb9d into ocaml:trunk Apr 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.