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

Improve the packing mechanism used to build Dynlink #2268

Merged
merged 24 commits into from Mar 19, 2019

Conversation

@mshinwell
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

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

@mshinwell mshinwell requested a review from dra27 Mar 1, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

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 mshinwell:dynlink_packing branch 2 times, most recently from 2b10e1a to c06a0fa Mar 4, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

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

otherlibs/dynlink/Makefile Outdated Show resolved Hide resolved
@mshinwell mshinwell force-pushed the mshinwell:dynlink_packing branch from c06a0fa to d86d2c7 Mar 6, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

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.

@mshinwell mshinwell force-pushed the mshinwell:dynlink_packing branch from d86d2c7 to 722887d Mar 7, 2019
xavierleroy added a commit that referenced this pull request Mar 13, 2019
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 mshinwell force-pushed the mshinwell:dynlink_packing branch from 42dc4a4 to 121595b Mar 13, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@dra27 Please approve and merge once CI passes.

Copy link
Contributor

left a comment

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

otherlibs/dynlink/Makefile Show resolved Hide resolved
asmcomp/cmxs_format.mli Outdated Show resolved Hide resolved
otherlibs/dynlink/native/dynlink.ml Outdated Show resolved Hide resolved
otherlibs/dynlink/dynlink_common.ml Outdated Show resolved Hide resolved
@dra27

This comment has been minimized.

Copy link
Contributor

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
Copy link
Contributor

left a comment

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

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

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

@mshinwell mshinwell force-pushed the mshinwell:dynlink_packing branch from 37762d8 to c462b40 Mar 18, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.