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

Move complugin and friends from BYTECOMP to COMP #1216

Merged
merged 3 commits into from Aug 16, 2017

Conversation

lpw25
Copy link
Contributor

@lpw25 lpw25 commented Jun 28, 2017

This moves Complugin and its dependencies from the BYTECOMP Makefile section to the COMP Makefile section because they are used by parts of ocamlopt. In particular, without this change ocamlopttoplevel.cmxa cannot be linked without ocamlbytecomp.cmxa -- which is a regression from 4.03.0.

@gasche
Copy link
Member

gasche commented Jun 28, 2017

The way Compdynlink is built is tricky, the .cmx and .cmo don't use the same source (the .cmo uses .mlbyte, and the .cmx uses .mlopt or .mlno if natdynlink is unavailable), so they also don't have the same dependencies. Why would the opt-related .cmxa include bytecomp/meta.cmx if they don't depend on it?

@mshinwell
Copy link
Contributor

This may need looking at in conjunction with #1063, which changes some of the dynlink source files around a bit.

@lpw25
Copy link
Contributor Author

lpw25 commented Jun 29, 2017

Why would the opt-related .cmxa include bytecomp/meta.cmx if they don't depend on it?

Are you suggesting moving meta.cmo from COMP to BYTECOMP? But then wouldn't ocamlcommon.cma require linking ocamlbytecomp.cma?

@lpw25
Copy link
Contributor Author

lpw25 commented Jul 19, 2017

@gasche Could you clarify what you would like to be done differently? (or if you're happy with it approve it for merge)

@gasche
Copy link
Member

gasche commented Jul 19, 2017

On my first (incompelte) read, I thought that "only compplugin" had moved. I understand that its dependencies had to be moved as well, but I am uncomfortable with the fact that some dependencies that look "very bytecode" to me (meta.cmo in particular) is now moved to COMP. This changes the PR from "somethin small in scale touching a relatively new module" to "touches old old stuff in ways that feel a bit strange to me and that I don't understand". I don't feel like I am informed enough to review/approve the change.

My remark in the previous comment can be explicited: if I understand correctly, there are two implementations of compplugin, and only the bytecode one needs meta. I wondered if the fact that compplugin does not depend on Meta when used for native compilation meant that it was possible to keep meta in BYTECOMP instead of moving it to the shared zone. But I guess that does not work, as some things may use COMP but neither of ASMCOMP and BYTECOMP, and those would lack dependencies.

@mshinwell
Copy link
Contributor

The point is that some of the files in the bytecomp directory are required to dynamically link plugins when the compiler itself is built as bytecode (irrespective as to whether the compiler produces bytecode or native code). It seems better to adopt the approach in this patch, whereby these few small dependencies live in COMP together with accepting the fact that they will be linked when not needed (i.e. when the compiler itself is built as native code), rather than (a) having things that are clearly not at all related (e.g. emission of bytecode instructions) to the native compiler linked into it or (b) having the .cmo version of the dynamic-link support code be in a different library from the .cmx version.

This patch also fixes a regression if I understand correctly.

This is OK subject to the tests passing.

@mshinwell mshinwell merged commit b3f54cb into ocaml:trunk Aug 16, 2017
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: cuihtlauac <cuihtlauac@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants