Anticipating a reprimand:

— Was it really necessary to merge asmrun/startup.c into byterun/startup.c?
— No, it was not. I had to put caml_shutdown somewhere, and putting a copy of it in both places seemed silly. As a positive side effect, certain functionality is no longer duplicated (notably, the OCAMLRUNPARAM parser).

If this seems like an overly intrusive change, I will revert it.

I have no opinion on the PR but I can confirm that the memory leak is a glibc issue. glibc stores a pointer to heap-allocated memory in thread-local storage. The memory isn't reclaimed until the thread exits. Fortunately the allocation is small so it's pretty harmless.

Wow, the CI bot builds and tests each PR automatically? So XXI century... Pure awesome!

The Travis CI build failed:

Summary:
  441 test(s) passed
   12 test(s) failed
    1 unexpected error(s)

List of failed tests:
  tests/lib-systhreads/testfork.ml
  tests/lib-threads/close.ml
  tests/lib-threads/test1.ml
  tests/lib-threads/test2.ml
  tests/lib-threads/test8.ml
  tests/lib-threads/test9.ml
  tests/lib-threads/testA.ml
  tests/lib-threads/testexit.ml
  tests/lib-threads/testio.ml
  tests/lib-threads/testsieve.ml
  tests/lib-threads/testsocket.ml
  tests/lib-threads/token2.ml

List of unexpected errors:
  tests/lib-dynlink-native/main

Oh well. Guess I messed up something thread-related, which does not sound particularly exciting. The following assertion, which was logged by CI, fires when a memory block is first allocated with regular malloc, then manipulated with caml_stat_*:

halloc.c:223: halloc: Assertion `p->magic == 0x20040518L' failed.

For the time being, Halloc is compiled in debug mode, and each block is marked with a magic number, which is checked by assertions in order to detect certain kinds of fishy situations, like the ones mentioned by me previously:

Care must be taken to ensure that each block allocated with caml_stat_alloc is further operated on using only the caml_stat_* functions, otherwise we're asking for segfaults and memory corruption, since "normal" blocks do not start with a header.

Investigating...

The issue with merging asmrun/startup.c into byterun/startup.c is that it introduces native code into byterun, which hasn't been the case before. All of the native code is based off the byterun/ directory. It would perhaps be cleaner to split up the environment variable parsing into a separate module.

All tests should pass now, with my latest changes (they do pass on my machine at least). The changes have been rebased (squashed into relevant commits) for reviewing convenience.

Do you really have to call finalizers? This seems to add a lot of complexity to the proposal, yet there is no guarantee (nor there should be) that finalizers are called at shutdown. They won't be if you just let your application exit.

Do you really have to call finalizers?

Yes, it is necessary to call finalisers, because failing to do so can result in leaking of:

• File descriptors.
• Compiled regular expressions (pcre-ocaml).
• ImageMagick bitmaps.
• Systems of nonlinear equations (nlopt-ocaml).

... and this is only what a single particular program of mine leaks, unless I enable finalisation in caml_shutdown. So, of course it is necessary, no doubts about that.

This seems to add a lot of complexity to the proposal, yet there is no guarantee (nor there should be) that finalizers are called at shutdown.

There is a 100% guarantee for those finalisers that are associated with Custom blocks. There is no guarantee indeed for those that are registered with Gc.finalise. It could be argued that it is not necessary to handle the latter kind of finalisers, because we can never guarantee to trigger them all. However, many libraries use Gc.finalise to keep track of external resources. See ocamlnet for examples.

But do we really need a 100% guarantee, if the finalisation semantics of caml_shutdown is well documented and simple enough to comprehend? In typical situations, all the values get finalised anyway. If they don't, odds are that a programmer did something wrong. I've studied the source code of several projects that use Gc.finalise, and never stumbled upon the case when running a finaliser results in allocation of yet more finalisable values.

The proposed logic will work well enough for most (if not all) of the real world use cases. The potential users of the feature (all three of us) will be immediately liberated, in certain conditions, from using inferior tools which happen to be better suitable for embedding (say, C++).

They won't be if you just let your application exit.

They won't, because they don't need to, if the process exits anyway. Not so simple is the case of embedding OCaml into a presumably unloadable shared library, which usually is the one and only sane way to interface with proprietary software packages.

By the way, I have a (yet) unimplemented addition to my proposal: why don't we add a new flag to OCAMLRUNPARAM that would trigger caml_shutdown on process exit? It would simplify detecting memory and descriptor leaks inside applications. Currently, I use a modification of the runtime which does exactly that — calls caml_shutdown at the end of main. Before that, I had to tell Valgrind about a myriad of exceptional cases, studying the source in each particular case in order to separate the "runtime leaks" and the "finalisation leaks" from the most curious "genuine application leaks".

To clarify: my applications are often written in a mix of languages, one of which happens to be OCaml. Sometimes, OCaml is the main language of implementation, and this is the case when the proposed "shutdown" flag in OCAMLRUNPARAM would come handy, at least during development, when the application is standalone and not yet used as a dynamic library by a foreign (non-OCaml) application.

Yes, I was completely wrong. You could rephrase the above as "you can avoid running finalizers if the OS does the cleanup; when you unload a shared object, it does not."

After discussing it at the consortium meeting, it was considered that this patch address two different (related) issues that should be considered separately:

• The core team seams quite convince that being able to correctly free the runtime is desirable. But some are not convinced by the halloc part and would prefer explicit deallocation.
• The semantics of when, how and in what order should the OCaml finalizers be run is far from obvious and the relation with at_exit is not clear. This is something for a later patch when the core team has a clear idea of the expected behavior.

By the way, thank you for steping up for such a change.

This PR have been discussed during the last developers' meeting, where I had the pleasure to be. The discussion has been long and interesting, but the conclusion is that more thinking is needed. Mainly about which semantic we want for ocaml finalizer:

• It is clear that C finalizer must be run
• It is unclear if ocaml finalizer should be run, for example they are not run during at_exit
• ocaml finalizer can create new value with finalizers.
• Are ocaml finalizers used for releasing external ressources? (ctypes?)

Some people suggested to propose a smaller patch focused on exposing malloc and free, but everybody agree that the first thing to do is understand what we want to ensure to the finalizer users. (Could @planar look at this proposition?)

A few comments:

• if all you need it to keep track of malloc block to be able to free them bulk, a few lines of code should do the trick and there's no need to pull two whole files from halloc.
• about explicit vs bulk deallocation, I don't have a firm opinion at this point.
• it seems clear to me that at_exit should be executed before any of the finalisation functions
• for the OCaml-level finalisers, you should have a loop to execute any new finalisers set during the execution of other finalisers.
• An OCaml-level finaliser might not terminate, but call Gc.finalise_release instead. We should find a way to deal with this case, or document the problem.

This patch looks good. The sooner it gets merged, the better, in my opinion.

@cakeplus what do you think of the previous comments? Thanks.

@cakeplus what do you think of the previous comments? Thanks.

Hello.

The previous comments provide valuable and much appreciated feedback. Unfortunately, I had not much time lately to allocate to the matter at hand -- sorry about that. Expect an updated proposal in a couple of days, as well as a thorough analysis of the mentioned issues.

By the way, I actually took the risk of using the patched runtime in production, across dozens of Windows installations, for almost a year -- and it works great (no leaks and no crashes) in my particular and very narrow case. However, I'm still eager to push the changes upstream, so expect an update soon.

The unloadable runtime, reloaded

As promised, I have updated the patch, rebasing it against the trunk, as well as making improvements that are described below. The old patch is available here.

Clarifying the scope

At first, I've tried to better understand the meaning of the proposed change and came to the conclusion that, truly, a distinction must be made between two separate issues (as pointed by @chambart):

1. Freeing the operating system resources (memory, file and shared library descriptors).
2. Ensuring the correct shutdown of a user-supplied algorithm -- executing its finalisation logic that was expressed by the user by means of the documented semantics of at_exit and Gc.finalise.

The first issue is the main one that we're trying to solve with caml_shutdown -- the showstopper of not being able to shut the runtime down without leaking lots of runtime-internal resources. This clearly is a showstopper, because at now there is no possible way at all to solve the problem from the user-space. Also, the proposed semantics for this part of caml_shutdown is quite clear: all Custom-block finalisers must be run, all caml_stat_* blocks must be freed, all the dynlinked shared libraries must be unloaded.

The second issue is somewhat related to the first one because it is certainly possible for the user to call malloc directly (e.g. through the dynamic FFI like ctypes) and expect the memory block to be freed by a direct call to free in the OCaml-level finaliser. However, having studied lots of use cases of Gc.finalise that I found on OPAM (including ctypes), I've never found a single such case.

Now, if we wish to solve the first issue without drowning in the complexities of the second one, a simple solution is possible -- prescribe in the manual the correct way of writing unloadable code:

1. Using caml_stat_* functions for all heap-allocations inside the C stubs.
2. Wrapping all handles to the OS resources into finalisable Custom blocks.

An educated guess: most code in the wild does already meet these requirements. Those libraries that do not (e.g. sometimes there are direct calls to malloc in C stubs) can be fixed by the user if the need arises.

I have thus removed the part of caml_shutdown that dealt with running the OCaml-level finalisers (caml_final_do_all_calls), as the implementation was fishy and probably incorrect. Also, dealing with these finalisers is unrequired if we take the simple path of prescribing the correct way of writing unloadable code.

Dealing with allocations in the C heap

@chambart

The core team seams quite convince that being able to correctly free the runtime is desirable. But some are not convinced by the halloc part

@planar

if all you need it to keep track of malloc block to be able to free them bulk, a few lines of code should do the trick and there's no need to pull two whole files from halloc.

Since people expressed concern over the undesirable complexity of Halloc, I've come up with a much simpler solution -- a trivial ring-style data structure that allows to add and remove memory blocks in constant time, still allowing for calls to malloc, realloc, free to be intertwined, conserving compatibility. The implementation is compact enough to be inlined right into memory.h. Also, I believe it to be simple enough to understand at a glance (unlike Halloc). I'd be surprised if it could be any simpler.

@chambart

The core team seams quite convince that being able to correctly free the runtime is desirable. But some are not convinced by the halloc part and would prefer explicit deallocation.

@planar

about explicit vs bulk deallocation, I don't have a firm opinion at this point.

Regarding the benefits of bulk allocation: please take a look at my first unfinished attempt at implementing the unloadable runtime. In describing the taken approach, I am quoting myself:

An ugly way to solve the problem involves locating all points of unresolved allocation inside the runtime's source code, figuring out how to free the memory correctly in each particular case, meticulously writing a lot of stupid boilerplate code, and eventually polluting the namespace with numerous "cleanup" functions — not nice at all.

To say the truth, "not nice at all" was an understatement. I quickly got bored and frustrated with figuring out the not-so-trivial logic of how various memory blocks were allocated and deallocated (in order to figure out how to free them in bulk), not even touching the important case of a user (the library author) performing allocations by means of the public caml_stat_* functions. Starting from a certain point it felt very wrong doing it this way.

In contrast, the approach with embedding a pool allocator proved promising in the very first tests. It is simpler, more abstract and general. I have absolutely no doubts about it.

Summing it up

I firmly believe that the latest version of the patch is good enough to be merged upstream. It robustly solves the main issue outlined in the proposal (being able to free the OS resources). I would shamelessly rate the quality of the implementation as a "solid A".

The only thing that waits to be done is a proper update to the manual (the section on interfacing with C). I am waiting for the official blessing before contributing that last missing piece.

The new implementation seems very clean to me -- but then I have few clues about memory-allocation code, and could not comment on eg. the alignment field in the structure.

This latest version looks very good. I notice that you removed the RTLD_NODELETE flag from the dlopen call. Can you explain the consequences of this change?

@gasche

The new implementation seems very clean to me -- but then I have few clues about memory-allocation code, and could not comment on eg. the alignment field in the structure.

That trick with max_align is not needed at all for correctness (void* would do just fine as far as correctness goes); however, it is supposed to increase the chances of getting a more favourable alignment of data in terms of performance (it is important, since data is going to be accessed much more often than the header of the block).

That said, I have no quantifiable evidence to back my claim. The trick was taken from Halloc, because I thought it made perfect sense. @planar, do you have an opinion on it?

This latest version looks very good. I notice that you removed the RTLD_NODELETE flag from the dlopen call. Can you explain the consequences of this change?

The positive consequence is that now it is possible to unload shared libraries on Unix (as it was always possible on Windows). The important question to ask is why that flag was there in the first place. Quoting myself:

A strange thing: on Unix (and Unix only), caml_dlopen uses the RTLD_NODELETE flag, which disables unloading completely, leading to unavoidable leaks. The flag was added 12 years ago by Jacques Garrigue as part of a huge commit, which, apart from adding that flag, doesn't seem to be anyhow related to handling shared libraries.

@garrigue, could you please clarify that issue for us, if you still remember anything about it?

2015/06/25 6:20 "Max Mouratov" [email protected]:

This latest version looks very good. I notice that you removed the RTLD_NODELETE flag from the dlopen call. Can you explain the consequences of this change?

The positive consequence is that now it is possible to unload shared libraries on Unix (as it was always possible on Windows). The important question to ask is why that flag was there in the first place. Quoting myself:

A strange thing: on Unix (and Unix only), caml_dlopen uses the RTLD_NODELETE flag, which disables unloading completely, leading to unavoidable leaks. The flag was added 12 years ago by Jacques Garrigue as part of a huge commit, which, apart from adding that flag, doesn't seem to
be anyhow related to handling shared libraries.

@garrigue, could you please clarify that issue for us, if you still remember anything about it?

This is a very old story, so it is hard to remember the details, but IIRC I
was trying to make dynamic loading work on Sun Solaris, and fighting a
number of bugs in the OS. I suppose that this flag was necessary at that
time to have dynamic loading work at all.
Solaris is not used that much anymore, and the bugs have certainly been
corrected long ago.

Jacques

@planar

This latest version looks very good. I notice that you removed the RTLD_NODELETE flag from the dlopen call. Can you explain the consequences of this change?

One probable negative consequence I can think of is: once we allow to unload shared libraries, care must be taken to avoid situations when a C-level finaliser residing in a shared library is run after said library is unloaded -- this typically results in a segfault. In my own implementation of a certain proprietary language (the runtime of which draws inspiration from ocamlrun) the problem is solved trivially: the user has no way to unload the dynlinked module, and the unloading of those modules is only performed automatically on the runtime shutdown, once the GC heap is swiped, in order to avoid the issue of "dangling finalisers". It is a design compromise that trades flexibilty for safety and simplicity.

The very same scheme is applicable to OCaml. My patch adds the missing piece -- bulk automatic unloading.

I reviewed the code of the MR partially to understand a bit more what you mean by bulk automatic unloading. In caml_shutdown (in native code), you call caml_free_shared_libs, which in turn performs a caml_dlclose on all the shared libraries.

In the current version of the code, those shared libraries were loaded with the RTLD_NODELETE flag: the code was never unloaded, and C level finalizers are still available. (Btw, typical C level finalizers include finalizers such as routines with the __attribute__(destructor), or handlers registered with pthread_atfork and atexit. )

Removing the RTLD_NODELETE flag implies that there might be dangling handlers. However, according to the doc of atexit or the __attribute__(destructor), these finalizers should be run prior to unloading, so we are safe. It is not necessarily the cases for the finalizers registered with pthread_atfork though (but this is a known issue of the pthread API).

A few remarks:

• I would have expected shared libs to be unloaded from most recently added to least recently added.
• I would have expected to see things related to the thread machinery (e.g., a call to caml_thread_cleanup) in caml_shutdown.

@braibant

Removing the RTLD_NODELETE flag implies that there might be dangling handlers. However, according to the doc of atexit or the attribute(destructor), these finalizers should be run prior to unloading, so we are safe. It is not necessarily the cases for the finalizers registered with pthread_atfork though (but this is a known issue of the pthread API).

By "C level finalizers" I meant those C functions that are registered as Custom block finalisers; аnd those are purely GC-driven. In caml_shutdown we avoid the problem by forcefully sweeping the heap (thus running the finalisers) before the libraries are unloaded -- so we are safe, yes. There would be a problem if the user had a way to unload libraries on his own (by means of some API function). Fortunately, there is no such way now: the caml_dynlink_close_lib primitive is undocumented and used only internally, by the compiler.

I would have expected shared libs to be unloaded from most recently added to least recently added.

Makes sense. Fixed it. Thank you!

I would have expected to see things related to the thread machinery (e.g., a call to caml_thread_cleanup) in caml_shutdown.

Good point. I will investigate into this.

The trick was taken from Halloc, because I thought it made perfect sense. @planar, do you have an opinion on it?

The trick looks good, and it should force alignment in a portable way. More portable than some other parts of the runtime, in fact.

@cakeplus what is the status of the current implementation? Is it ready for merging?

@alainfrisch, your point is very well taken, and I agree it would be nice to use plain malloc and free. (There is already enough space overheads in them...) @cakeplus, thanks for looking into this.

Done: #1121.

Commit a6cad36 in this PR leads to at_exit functions being called twice under some circumstances, see MPR#7796.

See also #1783