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

Major GC hooks are no longer allowed to interact with the GC heap. #8711

Merged
merged 4 commits into from
Jun 4, 2019

Conversation

jhjourdan
Copy link
Contributor

@jhjourdan jhjourdan commented Jun 3, 2019

As discussed in #8691 (comment) (by @stedolan):

BTW, the fact that the major GC is calling hooks that are allowed to allocate and execute arbitrary OCaml code (see comment in misc.h) completely kills the point of this PR. I did not know about them!

Would it be possible to add the restriction that these functions should not call an OCaml callback? Where are these functions used, in practice?

I didn't know about these functions either!

I think it would be possible to add this restriction. I can only find two uses of this feature: one in flow and one in Jane Street's internal code. Neither allocates, performs callbacks, or indeed calls any OCaml runtime functions at all.

Since this is nominally a breaking change, I think there should be a seperate PR which just edits the comment in misc.h.

@jhjourdan jhjourdan force-pushed the major_gc_hooks_no_allocation branch from deeb79e to 78d2ebe Compare June 3, 2019 18:21
@stedolan
Copy link
Contributor

stedolan commented Jun 4, 2019

In fact, the original PR proposing these hooks (#6675) says this:

I imagine there will be constraints on the C callbacks -- for example, the callbacks must not modify or allocate OCaml memory.

I can't find any uses of this feature that don't already conform to this restriction.

Copy link
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

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

I don't think allowing allocations in these hooks was a good idea.

@xavierleroy xavierleroy merged commit 63a16e8 into ocaml:trunk Jun 4, 2019
@xavierleroy
Copy link
Contributor

I merged in trunk after light editing.

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

4 participants