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

Refactor Compile/Optcompile #1703

Merged
merged 2 commits into from Jul 28, 2018

Conversation

Projects
None yet
9 participants
@Drup
Copy link
Contributor

Drup commented Apr 6, 2018

I always felt that the Compile and Optcompile modules were a slight violation of the DRY principle, so this patchset removes them fixes them.

This patchset factorize the common content of Compile and Optcompile into a third module, Libcompile. It also make the whole pipeline significantly more modular and, I hope, easier to tinker with.

I originally made this patch for the eliom fork, where each file is first typechecked, split and untyped into two parsetrees and then typechecked again and compiled. Given the current pipeline code, this was impossible to write reasonably.
With this patchset, it's just a matter of plumbing: https://github.com/ocsigen/ocaml-eliom/blob/master/driver/compile.ml#L62-L76

I believe this could be useful in a wider context, to easily build custom pipelines or tinker with the existing one. For example to test untypedast, I made a compiler which types, untypes and retypes.
Finally, it also avoids a big chunk of code duplication.

The patches are not exactly atomic. I'll squash when the time arrives.

I'm not exactly sure how to test this. At least it bootstraps.

Changes Outdated
@@ -350,6 +350,10 @@ Working version
- GPR#1679 : remove Pbittest from primitives in lambda
(Hugo Heuzard, review by Mark Shinwell)

- GPR#__ : Add the module Libcompile, which factorize the common
part in Compile and Optcompile. This also make the pipeline more modular.

This comment has been minimized.

@nojb

nojb Apr 6, 2018

Contributor

s/factorize/factorizes/
s/make/makes/

This comment has been minimized.

@Drup

Drup Apr 6, 2018

Author Contributor

Fixed, thanks!

@Drup Drup force-pushed the Drup:driver branch from 374e16e to 80c4b06 Apr 6, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

I will read this.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

I had a look at this using patdiff, which is excellent for checking this sort of patch, and didn't notice anything semantically wrong. However I'm only reasonably confident about that. One thing which might simplify matters further is to make the libcompile code into a functor (I think this will eliminate more duplication). It also avoids the kind of "wrap" notion which always seems slightly dubious.

I would maybe think of another name rather than "libcompile" as it's not really a separate library.

A couple of stylistic points: I think blank lines in functions should be avoided (if the function is unclear or too long, refactor it in another way) and spaces before record field semicolons are unusual. Also, it would be nice if all of the functions (etc.) in the libcompile interface were documented.

This patch would seem to me to be a very good step along the way to making ocamlopt emit bytecode as well as native code. Maybe that's worth trying? In particular you could apply the same kind of simplifications as you have here to main.ml and optmain.ml -- there are various unnecessary inconsistencies there at present (e.g. unset versus clear). If you merge all the options together, with bytecode-only or native-only ones annotated for the -help output, then I suspect there isn't much more work to do in order to reach that goal. I believe @damiendoligez is a particular fan of achieving that.

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 9, 2018

Thanks for the review! Indeed, the diff is quite hard to read (in fact, I had to port this from 4.03, and I ended up partially rewriting it because the diffs were impossible to work with).

What about renaming Compile to Bytecompile, and making this new module Compile ? It's not very consistent with the rest of the driver module but ....

I will look into turning this into a functor, but I'm not so convinced. The pipeline after typing differs significantly between byte and opt. Unless we standardize over a common API, I don't see how we can improve the situation further. I also would prefer to keep the "combinator" approach used here, as I think it's more flexible.

I completely agree with your last point about merging the byte and opt driver, this is the end goal.
I remember asking @chambart about having the opt compiler emit bytecode. I don't remember the exact answer, but it was not positive. In any cases, all this is probably better left for the next PRs!

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 10, 2018

The next step should probably be to do some large-scale testing of this on OPAM. Maybe talk to the OCaml Labs guys to see if they can assist with setting up a build?

@Drup Drup force-pushed the Drup:driver branch from 80c4b06 to 408d146 Apr 10, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 10, 2018

After taking another look, I'm really not sure how to improve it by using a functor. We could define a functor just for the wrap function, but I don't really see that as an improvement. The flexibility offered by the combinator version is essential for defining non-orthodox pipelines. Could you clarify what you had in mind?

I fixed your stylistic remarks and added some more documentation comments.

I asked @avsm and @AltGr. They said they have some testing platform that should be available soon to test compilers, so I'll wait for that.

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 11, 2018

@dra27 Is the appveyor failure relevant? I can't find what's wrong in the log. If not, can you relaunch the job?

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 12, 2018

Thanks to @kit-ty-kate, the output of the opam run is here. Apparently, there is an issue with annot files.

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 12, 2018

@shindere I would like to write a test that tries to compile a file with ocamlc and ocamlopt, and in each case verifies that .annot files have been generated. I have tried to look at similar things in the testsuite but without success, and I don't really understand the DSL used in test files. How do I do something like that ?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 12, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 12, 2018

@shindere Thanks! That's very helpful. I have something that works in the case the compiler succeed, but I would also like to allow the compiler to fail. How do I do that?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 12, 2018

@Drup Drup force-pushed the Drup:driver branch from 33ea071 to f2ac45e Apr 12, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 12, 2018

@shindere Perfect. It works nicely now.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 12, 2018

@Drup Drup force-pushed the Drup:driver branch from f2ac45e to 04b636d Apr 13, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Apr 13, 2018

So, the new driver code didn't emit .annot files when a compilation was successful. This is now fixed and I added a test that checks that .annot files are emitted if a compilation is a success, a failure or if -i is provided. The new opam run is identical to trunk and travis is happy.
For some reason, the "typeonly" test fails on appveyor. I have no idea why.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented May 28, 2018

For some reason, the "typeonly" test fails on appveyor. I have no idea why.

I'm investigating this.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented May 28, 2018

It fails because of a subtle interaction between -i, -annot, and the -o added by ocamltest, and the fact that executable files have an extension under Windows and not under Unix.

The solution (credit @shindere) is to add a line compile_only = "true" right after the flags = ... line in the test file, to prevent ocamltest from adding the -o typeonly.byte.exe option.

@Drup Drup force-pushed the Drup:driver branch from 15d911f to eaf2106 May 28, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented May 28, 2018

@damiendoligez and @shindere Thanks! I added the appropriate line in typeonly.ml and rebased. Let's see the results.

@Drup Drup force-pushed the Drup:driver branch from eaf2106 to bf09bce May 28, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented May 28, 2018

Tests are finally passing. @damiendoligez Are you merging as-is?

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 29, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented May 29, 2018

It's already rebased. The multiple commits are for review purposes. I'll squash when it's approved.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 29, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented May 30, 2018

@Drup I tried to read the rest of this. I couldn't find a separate changeset which fixed the stylistic/documentation issues you said you had attended to. Do you know what's happened there?

Some other remarks:

  • #!/usr/bin/sh at the top of the annot-checking script seems unusual/wrong. Also, I recommend always having "set -eu" (and probably "set -eu -o pipefail") at the top of every shell script, no matter how short it may be at the moment.

  • There is one unsightly "end ;" instead of "end;" in libcompile.ml`.

Please ensure this isn't squashed (or similar) before merging as it will be difficult to review.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 30, 2018

@Drup Drup force-pushed the Drup:driver branch from deaedf0 to f427a85 Jul 4, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 16, 2018

4.07 is released, the world cup is finished. @mshinwell, can I get a review? :)

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 24, 2018

@mshinwell one down, one to go ? :)

val emit_signature : info -> Parsetree.signature -> Typedtree.signature -> unit
(** [emit_signature info parsetree typedtree] emits the [.cmi] file
containing the signature.
*)

This comment has been minimized.

@mshinwell

mshinwell Jul 27, 2018

Contributor

"the signature" -> "the given signature"


val print_if :
info -> bool ref -> (Format.formatter -> 'a -> unit) -> 'a -> 'a
(** [print_if i flag fmt x] prints [x] with [fmt] on [i] if [b] is true. *)

This comment has been minimized.

@mshinwell

mshinwell Jul 27, 2018

Contributor

This should move elsewhere, maybe to Misc.

@@ -0,0 +1,90 @@
(**************************************************************************)

This comment has been minimized.

@mshinwell

mshinwell Jul 27, 2018

Contributor

I still think the name of this module isn't clear. What about Compile_common or similar?

val cmx : info -> string
val obj : info -> string
val annot : info -> string
(** Return the filename of some compiler products associated

This comment has been minimized.

@mshinwell

mshinwell Jul 27, 2018

Contributor

"products" -> "build artifacts"

val typecheck_impl :
info -> Parsetree.structure -> Typedtree.structure * Typedtree.module_coercion
(** [typecheck_impl info parsetree] typechecks an implementation and returns
the typedtree of the associated module, along with a coercion again

This comment has been minimized.

@mshinwell

mshinwell Jul 27, 2018

Contributor

"again" -> "against"

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Jul 27, 2018

Can we please run this against OPAM once again after addressing the final review comments and rebasing?

@Drup Drup force-pushed the Drup:driver branch 2 times, most recently from ebed095 to 99ff164 Jul 27, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 27, 2018

@mshinwell I addressed all your comments.

Rebasing after #1913 was extremely painful and I was more or less forced to squash everything. I do not want to have to rebase this patchset again. Whatever the opam repository has to say, it'll be minor and I can fix it afterwards. I insist that we merge as soon as CI passes.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Jul 27, 2018

OK, that's fair enough. Let's get an OPAM run going at that point though, in case there is something we've missed.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Jul 27, 2018

ah, and Changes needs the new entry formatting properly.

@Drup Drup force-pushed the Drup:driver branch from 99ff164 to 32bf1c3 Jul 27, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 27, 2018

ah, and Changes needs the new entry formatting properly.

Done.

@sliquister

This comment has been minimized.

Copy link
Contributor

sliquister commented Jul 27, 2018

@Drup sorry, I've had my dose of conflicts too. Would you please rename the field info.ppf as info.ppf_dump?

@Drup Drup force-pushed the Drup:driver branch from 32bf1c3 to 2ed35b3 Jul 27, 2018

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 27, 2018

@Drup sorry, I've had my dose of conflicts too. Would you please rename the field info.ppf as info.ppf_dump?

Done.

@Drup Drup force-pushed the Drup:driver branch from 2ed35b3 to 0b0e86e Jul 27, 2018

Drup added some commits May 3, 2016

@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 28, 2018

Everything is green!

@gasche gasche merged commit 3b6b2cd into ocaml:trunk Jul 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Drup

This comment has been minimized.

Copy link
Contributor Author

Drup commented Jul 28, 2018

🎉

@Drup Drup deleted the Drup:driver branch Jul 28, 2018

~always:(fun () -> Stypes.dump (Some (outputprefix ^ ".annot")))
))
let implementation ~backend ~sourcefile ~outputprefix =
Compmisc.with_ppf_dump ~fileprefix:(outputprefix ^ ".cmo") @@ fun ppf_dump ->

This comment has been minimized.

@sliquister

sliquister Jul 31, 2018

Contributor

This should be .cmx, not .cmo.

let cmo i = i.outputprefix ^ ".cmo"
let annot i = i.outputprefix ^ ".annot"

let init ppf_dump ~init_path ~tool_name ~sourcefile ~outputprefix =

This comment has been minimized.

@sliquister

sliquister Jul 31, 2018

Contributor

~init_path is a strange name. Shouldn't this be called ~native?

This comment has been minimized.

@Drup

Drup Jul 31, 2018

Author Contributor

Well, it's the boolean that control if paths should be initialized (which is apparently needed for native). I wouldn't object renaming it.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Feb 18, 2019

Note: this PR introduced a regression reported in MPR#7918.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 19, 2019

I propose a fix for the check_fatal regression in #2257.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment