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

Remove support for compiler plugins #2276

Merged
merged 6 commits into from Mar 13, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 1, 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:

  1. They are a potential security risk.
  2. 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).
  3. Many applications of plugins should be able to be expressed by building custom compiler drivers that link against compilerlibs.

It is not clear there are actually any extant uses of this feature (save for one from @trefis that can be implemented in a different way).

debugger/Makefile Outdated Show resolved Hide resolved
debugger/Makefile Show resolved Hide resolved
@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

This PR also removes the "hooks" (Misc.MakeHooks et al) machinery which is not directly related to the plugin mechanism. Is this intended?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@nojb I thought the majority of the "hooks" code---exactly those parts I deleted---were introduced along with the plugins mechanism. (I deduced that by looking at some of the documentation, which described their use in conjunction with plugins, if I remember correctly.) I did leave some "hooks" in place, in particular in the toplevel code.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@nojb I thought the majority of the "hooks" code---exactly those parts I deleted---were introduced along with the plugins mechanism. (I deduced that by looking at some of the documentation, which described their use in conjunction with plugins, if I remember correctly.) I did leave some "hooks" in place, in particular in the toplevel code.

Yes, but the hook mechanism could still be used when building standalone compiler drivers. I don't have any use case in mind, so am not arguing to keep them. But maybe worth asking about it in caml-devel.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

I do have a light use case for hooks: I have at least one toy projects, https://github.com/Octachron/more_ocaml_warnings, which use the Typemod's hook in order to add a warning for the beginner's mistake like module M: sig type t end = ....

Going from a compiler plugin to a statistically-linked driver does not change much in term of usability (it may even improve it), but removing hooks would make harder to maintain such toy projects.

This is certainly not a very strong point for keeping those hooks. However, since the maintenance cost of such hooks is far lighter than the plugin mechanism, it might make sense to keep it?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

  • I think the general notion of hooks is useful, as they allow to implement language/compiler extensions by creating custom drivers, without having to fork the compiler entirely. I'm in favor of keeping existing hooks (and possibly adding more, if the need arises).

  • I initially thought that removing support for dynamic plugins in the compiler would be a no-brainer, since users could always create their own custom driver that would link with Dynlink to provide the same feature. But now I remember that it's probably not possible, since Dynlink would clash with internal data structures from the compiler; is that right? Static linking of "plugins" in custom drivers is a possibility, but this might be much less convenient that drivers with support for dynamic plugins. Wouldn't it be possible to find a way to allow a program to link with both compilerlibs and dynlink, even if the "official" compiler doesn't link with dynlink itself?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@alainfrisch #2268 will provide what you want, I think, although I'm still working out some tricky details with @xclerc .

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Regarding hooks, it doesn't seem to me that these are worth keeping. I think the main reason is as follows: they just exist in a small collection of rather arbitrary locations. As such it seems to me that someone's future development work changing the behaviour of the compiler will, with some reasonable probability, not coincide with a place where one of these hooks exist---and it seems messy to add hooks everywhere. The whole thing seems like a poor-man's version of aspect-oriented programming (in which case it must be quite poor).

It also isn't difficult to rebuild the compiler libraries if you want to implement functionality that isn't provided by any hook. This might also, perhaps, encourage people to contribute such functionality upstream.

@xavierleroy and @damiendoligez : Could you please offer an opinion?

@trefis

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

FTR: my use of a plugin was indeed just lazyness. For my use case it would have been better, for various other reasons, to write my own driver.

Personally my plugin didn't use any fancy "hook" mechanism. It just updated some references already present in the typechecker; but I assume that the hook mechanism is just a nicer interface, which covers a different set of references than the one I happened to need?
In any case, it was useful, and so I'd tend to agree with Alain. It is possible to do with them, but it becomes tedious.

@mshinwell mshinwell force-pushed the mshinwell:remove_compiler_plugins branch from 3499765 to 098bcd4 Mar 6, 2019
@Drup

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

On hooks: Some of them are quite essential for alternate toplevels (like jsoo's toplevel or sketch.ml). In particular the hook to redefine .cmi loading. Maintaining a fork of the typechecker just for these is not reasonable.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

The .cmi loading hook and the hooks in the toplevel have not been removed by this patch.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

There is a debugger test that fails on Windows.

toplevel/opttoploop.ml Outdated Show resolved Hide resolved
@mshinwell mshinwell force-pushed the mshinwell:remove_compiler_plugins branch from 53a96a9 to cb03285 Mar 7, 2019
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

I've pushed a single changeset containing everything so far, since there was some trouble rebasing.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

CI green! The only objection was about not removing the toplevel hooks and indeed they are not. Unless someone else objects, this looks good to merge.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Has it been through precheck? Precheck #211

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

One small spanner to throw into the works - is it definitely the case that the -plugin command line flag should be deleted? It would seem better to update the text and display either a warning that it's ignored or a proper error message that the functionality was only available until 4.08.0?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

I think deleting the flag is fine, given the data we collected about the (non-)use of plugins.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

That's not the point, though - the issue is how we deal with changes in the CLI - there are other flags for which this has happened and it should be consistentm

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Unrelated to that, otherlibs/dynlink/nodynlink.ml should be deleted as part of this GPR.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@dra27 Given that the work has been done to remove the flag now, and with the lack of use of plugins, I really think it is fine as it is. There is a clear Changes entry saying what has been removed. We're just making work for ourselves otherwise, both now and in the future.

I have removed nodynlink.ml, well spotted.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Come on, the "work" we're talking here is one-line alterations in driver/main_args.ml and utils/clflags.ml* to change plugin to be internally a boolean flag followed by an error message in driver/main.ml below the vmthreads deprecation warning.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

dra27@eb1ab30 is what I was meaning which gives:

dra@cosmic:~/ocaml-$ ./ocamlc.opt -plugin foo
-plugin is only supported up to OCaml 4.08.0
dra@cosmic:~/ocaml$ echo $?
2

This seems to have the additional desirable side-effect that -prefix can't be re-added in a driver which tries to extend the default one.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

I could use an explanation of Dynlink.unsafe_get_global_value, a function that this PR adds.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@xavierleroy This function is needed to support existing use cases where clients of the Dynlink library look up global values by name, instead of what I understand to be the more usual situation, where the module initialisers in the shared library make various registrations with the main program.

The two existing use cases, which now both go via this function, are:

  1. Printers in the debugger. This is bytecode-only at present and used Symtable.get_global_value. Since this pull request makes the Dynlink library linked into the debugger use the compilerlibs modules from the dynlink-specific pack (the Compdynlink machinery has been removed) then the relevant Symtable module is no longer accessible from the debugger code itself [*]. As such, some interface in Dynlink is needed here.

  2. The native toplevel. This currently binds one of the external C functions in the natdynlink code, which is ugly.

The Dynlink.unsafe_get_global_value provides a uniform interface for both use cases. (I would like to improve the type of its argument to something better than string, but that should wait until after my other patches relating to symbol types.)

[*] This seems good, because Dynlink now can't interfere with other clients of Symtable, as used to happen in the debugger when a single Symtable was shared between Dynlink and the debugger code itself. Special-case code was needed to work around this, which can now be removed.

@mshinwell mshinwell force-pushed the mshinwell:remove_compiler_plugins branch from 8684f51 to 0985d56 Mar 12, 2019
@dra27
dra27 approved these changes Mar 12, 2019
Copy link
Contributor

left a comment

Dynlink.unsafe_get_global_variable is hidden from the documentation, as being an internal function.

LGTM, pending @xavierleroy's agreement

@@ -428,8 +426,6 @@ let read_one_param ppf position name v =
let if_on = if name = "timings" then [ `Time ] else Profile.all_columns in
profile_columns := if check_bool ppf name v then if_on else []

| "plugin" -> !load_plugin v

This comment has been minimized.

Copy link
@dra27

dra27 Mar 12, 2019

Contributor

My feeling is that where -plugin wants to remain in the CLI, leaving handling here could be annoying (on the basis that OCAMLRUNPARAM comes from your shell, not the specific invocation of the driver binary.

Arguably we could emit a warning instead? (doesn't have to be in this GPR)

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 12, 2019

Author Contributor

I think this is fine, at least for the moment.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

@xavierleroy If you are happy with my explanation above, please merge.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Thank you @mshinwell for the detailed explanations. With them I could review the dynlink + debugger part of this PR. It looks good to me and I agree the debugger's use of Symtable is nicer and clearer.

About Dynlink + Symtable: the following is probably obvious for @mshinwell but perhaps not clear to everyone. Even though proper packing of Dynlink hides the copy of Symtable that it uses, making it possible to link it along Symtable, there is still only one table of global values in the bytecode VM and changes to this table are NOT synchronized between Dynlink and the Symtable functions that change the global value table (update_global_table, assign_global_value). That's why we can't use Dynlink from the toplevel interactive loop, in particular. I think this GPR doesn't run into this trap, since the debugger doesn't seem to use those Dynlink functions.

Coming back to this GPR: I think it's in good shape and will merge soon. I still encourage @dra27 to submit a PR that will 1- remove Clflags.plugin entirely, and 2- print a specific error message if -plugin is given on the command line, in the style of #2312.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@xavierleroy I was not fully clear on all the details of that actually, so thanks for the explanation. Let me just add a comment based on your text before we merge this.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I was writing the log entry for the squashed merge :-) Ping me when you're done.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

@xavierleroy - no problem to follow it up with a Clflags.plugin removal.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

@xavierleroy Please see the comment, which is a slightly expanded version of your text. That's all from me.

@xavierleroy xavierleroy merged commit 36c1632 into ocaml:trunk Mar 13, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@damiendoligez

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@dra27 don't forget to open a PR for the removal of Clflags.plugin.

@OvermindDL1

This comment has been minimized.

Copy link

commented Jun 6, 2019

I was advised from https://discuss.ocaml.org/t/compiler-plugins/3898/2?u=overminddl1 to voice my issues upon the compiler plugins being removed, it's essentially killing my codebase and preventing me from upgrading past 4.8.0 (which I just completed doing).

I'm not finding any documentation on any migration path or anything to be able to keep the workflow this uses in a backwards compatible way using the stock installed compiler, otherwise it seems I'll need to essentially fork the compiler, which I'm exceptionally not wanting to do...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.