Changes to shared library and reloading

[yav]

Currently there is a slight "gotcha" with the FFI, in that if we have a loaded module that uses the FFI, then we change the external library (i.e., the shared object) and reload the module, the changes are not visible to Cryptol.

The current work around is to either quite Cryptol, or force unload everything (e.g., by typing :m Cryptol) and then reload everything. This works because unloading everything lets go of references to the library and so it gets unloaded, so when we load the module again, we dkopen will now open the newly modified external library.

I am not exactly sure what's the best way to fix that, but it seems that it could lead to some confusing situations while working on
a Cryptol spec and external code at the same time.

[qsctr]

Hmm this is interesting because I do remember explicitly testing this in the past and it did work correctly. I just did some further testing and it turns out that on macOS it did work in an old commit and doesn't work on the current head of the ffi branch, and on Linux it doesn't work at all on either the old commit or right now. I'll do some further digging to see what I did to break it and if there's somehow a way to get the right behavior to work on Linux too.

[qsctr]

Ok, I've narrowed it down to 9efd822. In that commit I changed the path I pass to dlopen from a relative path to an absolute path. I did that because, while both macOS and linux support passing relative paths containing / to dlopen, only macOS supports relative paths without /, because when the linux dlopen sees just a filename without / it only searches in certain system lib directories and not the current working directory. On the other hand, macOS always searches in the working directory no matter if the path contains / or not. This problem showed up in the FFI test, which would pass on macOS and fail on linux since the Cryptol file in the FFI test was not in a subdirectory relative to the .icry file.

The reason it has to do with this issue is that it seems like on macOS absolute paths passed to dlopen are somehow cached so a second dlopen call to the same path returns the old dylib, while relative paths containing / are not cached so a second call returns the new dylib. I have also found that, for whatever reason, relative paths not containing / are cached as well, so it fails on cases when you're trying to reload a module in the current directory too. Meanwhile, on Linux the shared objects are always cached no matter whether the path is absolute or not or contains a / or not, so that's why it never worked.

[qsctr]

In any case, what is happening now is we call dlopen a second time before the dlclose is called by the GC in the ForeignPtr finalizer, and it seems like dlopen's supposed behavior is that all it does when this happens is increment then decrement an internal reference count, so I'm not sure why it actually reloads it in certain cases on macOS. But what we would want to do is always call dlclose first so the refcount drops to 0 and then do dlopen again. I think that just discarding the old environment before loading the new one won't actually achieve it because the finalizer is called a fair bit after it's actually discarded; when I test it by printing to stdout when dlclose is called, the output appears after a noticeable delay after the :r command is completed, and prints out after the next prompt is already displayed.

I think there are several options for what we could do:

1. Keep the ForeignSrc around in the LoadedModule object and pass any existing ones along to loadForeignLib when loading a new module, which will call finalizeForeignPtr on any existing old version of the same library to do dlclose before dlopening the new library. What I am not sure about is whether or not it runs the finalizer in a separate thread; the description in Foreign.Concurrent seems to say so but briefly looking at the source of finalizeForeignPtr it seems like it's just run directly when explicitly finalized. This would also remove the need to keep the ForeignPtr around in the ForeignImpls even though it's not actually used.
2. dlopen has a RTLD_NOLOAD flag which can be used to check if a library is already in memory. So we call dlopen with that, then if it returns null we just do a normal dlopen, but if it returns an existing handle then we dlclose it twice (to get the refcount to 0, since the NOLOAD dlopen also adds to the refcount), then call dlopen again on it twice, and after a bit the finalizer will call dlclose dropping the refcount back to 1. Feels kind of hacky but I think it would work.
3. Give up on using the finalizer and explicitly call dlclose whenever it's required.

[yav]

@qsctr what do you think about an approach where we modify ForeginSrc to basicaly support "force unload". I think this could work as follows:

• When we create a create new ForeignSrc, we allocate a piece of state (IORef, or probably to be safe we should use MVar) that stores a boolean, indicating if the library is loaded. This thing is stored in the ForeginSrc together with the ForeignPtr.
• Then we can add a function unloadForeginSrc that checks if the library is still loaded, and if so unloads it and sets the variable to False (this should happen atomically)
• The function unloadForeignSrc applied to the MVar can be used as the finalizer for the foreign pointer
• We modify the LoadedModule to have a Maybe ForeignSrc field, so that when we unload the module we can use unloadForeignSrc to unload the library.
• If we wanted to be extra safe, we could modify callForeignImpl to check the ForeignSrc inside (so store the whole ForeignSrc, not just the ForeignPtr), and raise a civilized exception if the library got unloaded somehow.

[qsctr]

That sounds good to me, I'll try that out.

[qsctr]

Fixed in 6cc5cec.