Skip to content

Improve the packing mechanism used to build Dynlink#2268

Merged
mshinwell merged 24 commits intoocaml:trunkfrom
mshinwell:dynlink_packing
Mar 19, 2019
Merged

Improve the packing mechanism used to build Dynlink#2268
mshinwell merged 24 commits intoocaml:trunkfrom
mshinwell:dynlink_packing

Conversation

@mshinwell
Copy link
Copy Markdown
Contributor

@mshinwell mshinwell commented Feb 26, 2019

This pull request improves the packing mechanism used in the Dynlink makefile so that we can reference compilerlibs from both the bytecode and native dynlink implementations. (At present, it can only be referenced from the bytecode one.) In turn, this means that we can improve the types that are used to describe the format of .cmxs files, as will feature in another patch I am preparing. This is all on the path to merging the gdb work.

This patch means that the dynlink code is no longer "special" as regards what can be referenced, which makes it easier to modify and reduces code duplication.

If you are interested in the details, which are quite grim, please see the various comments that are added in this patch to otherlibs/dynlink/Makefile. These explain the problems with the existing packing mechanism and various nuances that have to be dealt with.

Splitting the .cmxs format from the .cmx format descriptions in asmcomp/ was done both because I was going to do it in another patch; and because it substantially decreases the amount of modules that have to form part of the dynlink-specific pack.

@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Feb 26, 2019

P.S. This is based on the old version of #2265; I will rebase once that's merged.

@mshinwell mshinwell mentioned this pull request Mar 1, 2019
Merged
@mshinwell mshinwell requested a review from dra27 March 1, 2019 14:18
@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 1, 2019

There seem to be some problems with this - I've discussed with @stedolan and hopefully have a solution, which I shall put in place next week.

@mshinwell mshinwell force-pushed the dynlink_packing branch 2 times, most recently from 2b10e1a to c06a0fa Compare March 4, 2019 09:58
@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 4, 2019

@dra27 This is now ready for you to look at.

dra27
dra27 reviewed Mar 5, 2019
View reviewed changes
@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 6, 2019

I've made the change about -slash and removed the .depend files + the two subdirectories. Hopefully this will work this time.

xavierleroy pushed a commit that referenced this pull request Mar 13, 2019
@mshinwell @xavierleroy
Remove support for compiler plugins (#2276)
36c1632 
After consultation on the core developers' list I am proposing this patch to remove support for compiler plugins.

The main motivations for removing compiler plugins are:
- They are a potential security risk.
 - They increase the complexity of the build system and make maintenance of the Dynlink libraries more difficult (although actually, this complexity could probably be reduced after #2268 is merged).
 - Many applications of plugins should be able to be expressed by building custom compiler drivers that link against compilerlibs.

* Remove compiler plugins and hooks
* Add new function Dynlink.unsafe_get_global_symbol but keep it outside the documented API.
* Remove otherlibs/dynlink/nodynlink.ml
* Update Changes
@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 13, 2019 •
edited
Loading

@dra27 Please approve and merge once CI passes.

dra27
dra27 reviewed Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two (dull) questions regarding copyright headers and two REMOVE_ME comment headers which should have gone during the rebase

@dra27
Copy link
Copy Markdown
Member

dra27 commented Mar 13, 2019

Apologies - the email notification of that review may have included a whole load of outdated comments from days ago which GitHub hid from the editor and then magically restored when I finished it!

dra27
dra27 approved these changes Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precheck #221 (the layout alterations haven't been through precheck) and assuming green/blue all good to merge.

@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 13, 2019

This is not to be merged yet, I need to investigate a failure on ppc64le.

This was referenced Mar 15, 2019
Merged
Merged
@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 18, 2019

I've rebased this and sent it to precheck (#225). I hope it's going to work this time. Assuming it does and the CI on this page is green, this can be merged.

@mshinwell
Copy link
Copy Markdown
Contributor Author

mshinwell commented Mar 19, 2019

There was one failure on precheck, but it appears to have been a github-related failure that also occurred for another pull request being tested. The relevant platform passed precheck for this GPR on the previous round, so I'm certain enough that this is ok to merge now.

@mshinwell mshinwell merged commit dbede46 into ocaml:trunk Mar 19, 2019
@jhjourdan jhjourdan mentioned this pull request Nov 29, 2019
Closed
@gasche
Copy link
Copy Markdown
Member

gasche commented Dec 25, 2019

I just tracked a dependency problem in our Makefiles that comes from this PR. It introduces .mli-only files, in particular file_format/cmxs_format.mli, that are not tracked properly as dependencies of the compilerlibs/*.cma archives -- they are not rebuilt when rebuilding the archives. (It's not trivial to fix this, compilerlibs archives are defined by listing only their .cmo files in the root Makefile, and adding a .cmi file to eg. $(COMMON) would not be correct.)

Then some tools, in particular tools/ocamlobjinfo, depend on cmxs_format.mli, but the Makefiles only enforces their dependencies on compilerlibs archives. (There is no dependency-tracking from the tools Makefile to the root Makefile, but tools/Makefile is only run when the compilerlibs archives have been built.) The dependency on cmxs_format.mli is missing, so the corresponding .cmi is not rebuilt, yet it is accessed in objinfo.ml, creating .cmi incompatibilities issue.

To test this, build make core; make ocamltools, then add a blank line at the top of utils/misc.mli and rebuild make ocamltools. You will see a failure message such as the following:

File "/home/gasche/Prog/ocaml/github-trunk/tools/objinfo.ml", line 1:
Error: The files /home/gasche/Prog/ocaml/github-trunk/utils/misc.cmi
       and /home/gasche/Prog/ocaml/github-trunk/file_formats/cmxs_format.cmi
       make inconsistent assumptions over interface Misc

@gasche
Copy link
Copy Markdown
Member

gasche commented Dec 25, 2019

I think that the simplest fix for this issue would be to turn thse .mli-only files in .ml-only files (I don't really see the benefit for .mli-only files), so that they end up as archives in a .cma files like everything else.

Another approarch would be to split compilerlibs-archive-variables in _CMO and _CMI variants.

If we had a concept of "cmi archive", we could only expose the .cmia and .cmi files of compilerlibs and not have any other -I directive, and this kind of failure could not possibly happen. (Here the problem is that we access a .cmi in a -I directory without checking that the archive that is intended to be more recent is indeed more recent.)

@gasche gasche mentioned this pull request Dec 26, 2019
Merged
@dra27 dra27 mentioned this pull request Jan 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dra27 dra27 dra27 approved these changes

Assignees

No one assigned

Labels

debugging-prerequisite

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mshinwell @dra27 @gasche