Added a Plugin function to install hooks
Changes PlannedPublic

Authored by angerman on Nov 29 2014, 8:14 PM.

Details

Summary

This change is based on the ongoing OutOfProcessTemplateHaskell work put in place against 7.8 by luite/angerman and it enables hooks to be installed using the standard Plugin infrastructure (-fplugin=...) making it quite easy to experiment.

I would like to propose this change for 7.10 and a subsequent revision of the design for 7.12.

Test Plan

validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
ghc-jit
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 2360
Build 2370: GHC Patch Validation (amd64/Linux)
rodlogic updated this revision to Diff 1773.Nov 29 2014, 8:14 PM
rodlogic retitled this revision from to Added a Plugin function to install hooks.
rodlogic updated this object.
rodlogic edited the test plan for this revision. (Show Details)
rodlogic added reviewers: hvr, luite, austin, angerman.
angerman accepted this revision.Nov 30 2014, 4:23 AM
angerman edited edge metadata.

LGTM.

I've also adjusted the ghc-7.8 patch (https://gist.github.com/7db11c24f8935c73fcf5) to incorporate some of the changes from this patch and adjusted the installHooks signature. So that both patches use the same interface.

This revision is now accepted and ready to land.Nov 30 2014, 4:23 AM
luite edited edge metadata.Nov 30 2014, 4:44 AM

A few things to note:

  • This will likely not work for cross compilers in general, since plugins are loaded from haskell libraries rather than arbitrary shared libraries. Interface files are incompatible between build and target machines (building the plugin itself might also be tricky)
  • It's still impossible to enable Template Haskell in a stage1 compiler with this, since the code paths leading up to the runMeta hook (and those for installing the plugin!) depend on GHCI being defined.

Hooks are rather low-level and can easily break the compiler, this extension to allow plugins to modify DynFlags early in the pipeline is useful by itself, but for Template Haskell, I think a different strategy should be used eventually.

I'd rename the installHooks field to something else by the way, since this is conceivably useful for anything that modifies DynFlags before the compiler pipeline is started, perhaps dependency injection, or adjusting flags for some specific purpose.

hvr edited edge metadata.EditedNov 30 2014, 4:53 AM

I would like to propose this change for 7.10 and a subsequent revision of the design for 7.12.

What do you mean by revision for 7.12? Is the API likel going to change in 7.12?

PS: Is there an associated Trac ticket and/or Wiki page? If not, this definitely deserves one. (We use Phabricator only as an extension to Trac for code-review, not as a replacement for Trac's ticket management and wiki)

Take this with a grain of salt since this is my first time looking at the Plugin facility, hooks and the new type checker plugin (TcPlugin).

Here are a few notes:

  • The idea of threading plugin state done in the type checker TcPlugin is quite useful, why not use that for the whole Plugin?
  • The Plugin API is quite useful and a natural API to extend GHC. What about making the Plugin interface the standard way of customizing GHC in both cases: GHC API and -fplugin=...? This way there is a single facility to extend GHC and extensions are all concentrated in a single API.
  • There are a few more interesting hooks described here https://www.haskell.org/pipermail/ghc-devs/2013-August/002182.html that may be worth considering.
  • I would like to use the Plugin facility to integrate Lambdachine's byte code generator instead of hardwiring it into a fork of GHC. This means more hooks.

Luite, maybe the Plugin facility should also allow loading from a shared library?

In any case, it seems that there are too many open points for discussion here and it feels a bit rushed to get this into 7.10.

luite is completely right here. The primary motivation for adding the DynFlags -> DynFlags plugin was the motivation to be able to install the hooks from plugins, so that I could sidestep recompiling ghc all the time during development. Hence the name. I also think changing the name to a better or more appropriate one would be good!

I am fully aware of the issues the out-of-proc-th approach has. But I don't think that this patch has anything to do directly with oopth. It just ended up being a useful byproduct.

On the oopth side I plan to continue with adding an OOPTH CPP define to enable the necessary code paths for oopth in stage1. This will be for non cross compiler for now. Integrating the plugin directly in ghc or even better using the pipes approach outlined by luite on GHC to reduce the complexity and dependency of ghc on the oopth driver will then have to be decided. But this is as mentioned above orthogonal to the patch discussed here.

This Patxh uses GHCI CPP due to the fact that the used function are in part only available with GHCI.

Can we get rid of the GHCI requirement for this type of plugins, even if it's just for when GHC has been linked with dynamic libs? If we can, it'd require just a few more changes to make runMeta reachable again in stage1 compilers, making this plugin thing usable in the short term for oopth for cross compilers.

In D535#14686, @hvr wrote:

I would like to propose this change for 7.10 and a subsequent revision of the design for 7.12.

What do you mean by revision for 7.12? Is the API likel going to change in 7.12?

PS: Is there an associated Trac ticket and/or Wiki page? If not, this definitely deserves one. (We use Phabricator only as an extension to Trac for code-review, not as a replacement for Trac's ticket management and wiki)

I don't think there is a trac or wiki page yet (for proposing an overhaul of the plugin system) as far as I know. @rodlogic, could you open the wiki and trac page?

rodlogic updated this revision to Diff 1813.Dec 1 2014, 11:49 AM
rodlogic edited edge metadata.

Renamed installHooks to updateDynFlags

rodlogic added a comment.EditedDec 1 2014, 11:51 AM

I renamed the new Plugin field, but there is still a build failure that I still need to investigate. It seems related to referencing a plugin that needs to be compiled from source.

fyi, this change is a bit stuck since it breaks one of the Plugin test cases. From a limited inspection, it seems that GHC supports compiling a Plugin from source before using it in the same GHC invocation. This change set assume the Plugin is compiled already and resides in the package db. On top of that I won't have much time to work on this this week.

austin requested changes to this revision.EditedMar 2 2015, 11:19 AM
austin edited edge metadata.

(Marking as Request Changes until test failures are fixed.)

This revision now requires changes to proceed.Mar 2 2015, 11:19 AM

@rodlogic, do you think we could get this unstuck for 7.12?

I have been struggling to find the spare time for GHC the past 6 months or so, but I am hopeful that will change in the near future. Let me give it a shot in the next couple of months.

@bgamari The lack of time will not change in the near future so I won't be able to pursue this further at the moment. I think it is best we get rid of this altogether as I was not the main driver behind this change.

bgamari commandeered this revision.Oct 13 2015, 2:15 PM
bgamari added a reviewer: rodlogic.

@rodlogic, thanks for the update. Feel free to commandeer this revision and continue working on it if you find yourself with an unexpected surplus of time.

bgamari abandoned this revision.Oct 13 2015, 2:16 PM
angerman commandeered this revision.Aug 17 2016, 9:38 PM
angerman edited reviewers, added: bgamari; removed: angerman.
angerman reclaimed this revision.Aug 17 2016, 9:38 PM
This revision now requires changes to proceed.Aug 17 2016, 9:38 PM
angerman updated this revision to Diff 8425.Aug 17 2016, 9:41 PM
angerman edited edge metadata.
  • Prelim plugin support
angerman planned changes to this revision.Aug 17 2016, 9:42 PM

This is work in progress. Yet I wanted to put this out there for now.

Especially all the disabling of other plugins needs to be fixed. This is mostly due to the fact that we
load plugins unconditionally and therefor end up with plugins loaded multiple times.

What is the ticket number for this? Click "Edit revision" to add it, thanks.

In D535#72117, @thomie wrote:

What is the ticket number for this? Click "Edit revision" to add it, thanks.

I don't think there is one yet. I'll certainly add it, once I'm back from vacation. However the scope of the
diff has changed considerably over time and I still expect it to evolve as needed.

The latest updates motivation has been primarily to have all the code publicly available to allow test-driving
https://github.com/angerman/data-bitcode-plugin.

angerman updated this revision to Diff 8702.Sep 10 2016, 9:04 PM
angerman edited edge metadata.
  • Allow more hooks

The way the dynflags are initialized (partition_args in Main.hs),
prior to the loading of the plugin, we can't (right now) hook
startPhase properly.

angerman planned changes to this revision.Sep 10 2016, 9:10 PM

Still work in progress.

This however contains all the parts to allow

$ ghc -fplugin Data.BitCode.Plugin HelloWorld.hs

to compile HelloWorld.hs into a working HelloWorld binary though an alternative llvm code gen[1].


[1]: https://github.com/angerman/data-bitcode-plugin

angerman updated this revision to Diff 8729.Sep 16 2016, 10:14 PM
angerman edited edge metadata.

Modifies the phase hook to be more expressive. With this it is
possible to decorate phases and selectively hook them.

angerman planned changes to this revision.Sep 16 2016, 10:23 PM

There's still some more missing. However with this ghc is sufficiently
modified to allow the Data.LLVM.Plugin to install these Hooks and therefore take over just enough of the compilation pipeline.

E.g. this

$ ghc examples/HelloWorld2.hs -fplugin Data.BitCode.Plugin

now just works[tm][1].


[1]: As the code gen doesn't cover all of cmm yet, there will be some files that can't be compiled right now.

angerman updated this revision to Diff 8864.Oct 3 2016, 4:33 AM
angerman edited edge metadata.

A stab at reorganizing the plugin infrastructure.

I hoped this would possibly also fix D2551 by accident. It did not.

angerman planned changes to this revision.Oct 3 2016, 4:34 AM

There's still a lot more to do here.

angerman updated this revision to Diff 9917.Dec 11 2016, 2:52 AM
angerman edited edge metadata.
  • rebase onto master
angerman updated this revision to Diff 9918.Dec 11 2016, 2:56 AM
angerman edited edge metadata.
  • rebase proper commit onto master
angerman planned changes to this revision.Dec 11 2016, 3:54 AM
austin resigned from this revision.Nov 6 2017, 10:07 PM
lelf added a subscriber: lelf.Apr 12 2018, 4:16 AM