Ensure that C allocation functions do not trigger callbacks#8897
Ensure that C allocation functions do not trigger callbacks#8897dra27 merged 2 commits intoocaml:trunkfrom
Conversation
83c617b to
e87f03d
Compare
runtime/minor_gc.c
Outdated
|
|
||
| /* Called by [Alloc_small] when [caml_young_ptr] reaches [caml_young_limit]. | ||
| We have to either call memprof or the gc. */ | ||
| We may have to either call memprof or the gc. */ |
There was a problem hiding this comment.
If a finaliser (or other async callback) is pending, then in native code this function will be called on each minor allocation even when the heap is not exhausted. The code of the function will correctly do nothing in this case, but the comment suggested otherwise.
dra27
left a comment
There was a problem hiding this comment.
Is it necessary (or possible) to consider doing anything clever to try to enforce the correct function being called? i.e. does the interface of caml_check_urgent_gc want to change instead?
|
I think the thing that enforces the correct function being called is testing! (The unit test in this patch fails if any of the |
3c4d9d4 to
711db11
Compare
|
Well spotted. The fix seems good to me. Sorry for having forgotten these calls when submitting #8691. |
|
Seems like MSVC is not happy with your tests, @stedolan ! |
|
Hmm, it’s not happy with that tweak either... I have another test running locally... |
|
OK, my tweak should stay as that restores compatibility with older C compilers (really must update configure to add The problem is that you have a C identifier called |
|
Thanks, @dra27, for investigating. I pushed a fix. Once CI passes, we will be able to squash+merge. |
|
As it happens, on the OCaml side you can call the stub whatever you like - the issue was only with the C naming. Further investigation reveals even |
Fixes a bug in ocaml#8691 and adds a test.
dra27
left a comment
There was a problem hiding this comment.
I've squashed my commit into Stephen's original commit and simplified the Windows fix as outlined above (and tested it...!). I'll let CI pass again for the sake of it, but I'd like merge rather than squash to keep those two commits as if/when #8901 gets fixed I'll revert the second commit as a simple regression test.
|
Many Thanks, @dra27! |
Despite reviewing and merging #8691, I didn't notice a serious bug in that patch: it fails to prevent callbacks during allocation functions in the codepaths that allocate directly to the major heap (for large allocations).
The fix is trivial. Most of the code in this PR is to add some testing for this invariant. The test works by setting up a finaliser, then doing lots of allocation from C and verifying that the finaliser doesn't actually run until control returns to OCaml.