Garbage Collector changes

[vfrinken]

With this PR, the RefC garbage collector behaves mores like Chez's.

Before, a pointer and a garbage collected pointer were two different
objects, which makes the following code produce a segmentation fault

1  do
2     ptr <- getExternalPointer
3     gcptr <- onCollectAny ptr free
4     putStrLn $ show ptr
5     pure()
``
It crashes because after the third line the references to gcptr are
removed, which invokes the freeing function. Line 4 therefore points
to freed memory.

Now, a each pointer has already has a reference to the garbage
collecting function, and a call to onCollect/onCollectAny simply
attaches the GC closure to it, but no new object is created.

Thus, the freeing function will only be called once all references
are removed.

[CodingCellist]

A couple of general comments:

• Could you please document what has changed in the CHANGELOG.md? This seems like it is a fairly big change and so should be documented.
• Would it be possible to add a test case so that this doesn't accidentally get reintroduced?

Since @madman-bob wrote most of the RefC code, I'll let him comment on the code changes if he has the time. (If you're reading this bob, it's just the first commit that needs checking, the rest are linter fluff.)

Speaking of: the linter unhappiness is really odd! Seems it was perfectly happy with the pre-pr refc code, so I don't know why it's suddenly upset after you changed it... (the massive number of line changes initially had me scared to look through this 😅)

[gallais]

Speaking of: the linter unhappiness is really odd! Seems it was perfectly happy with the pre-pr refc code, so I don't know why it's suddenly upset after you changed it...

The linter was probably activated after the RefC contribution and it'll only complain about
files you've touched, not existing file. So as long as you don't modify a legacy file, it won't
be linted but the moment you do... 💥

[buzden]

Probably, once the PR is ready, the first commit should be merged as a separate (not squashed) commit for future potential analysis of commits history on what was changed (e.g. if something goes wrong, e.g. on some weird architectures).

[madman-bob]

The proposed change would cause an error when we try to create two GCPointers for the same Pointer, as this would only allow one freeing function per pointer. Note that freeing functions need not free everything that is pointed to.

I would argue that your snippet is incorrect, or at least undefined behaviour.

As gcptr is unused, a backend may clean it up at any time, invoking the freeing behaviour. That the Scheme backend doesn't currently clean it up immediately should not be relied upon. Once gcptr has been created, it should be used instead of ptr in later occurences.

With regards to the linter - I recall that we disabled the C linter last year when I was doing some major RefC changes, for the same reason as here. If we are to keep the C linter, we should have a formatting commit for all .c files in the repository, to make sure that they are all in a known good state, and avoid another repeat of this.

[vfrinken]

I'm not sure I follow your argument, can you elaborate on that? With the PR, the behavior would not be undefined as the freeing function used by the last onCollect call would be used. But you you say it would be wrong. What would be the right behavior? With the current implementation, your scenario leads to a segmentation fault (removing the reference to the 2nd GCPointer would invoke the freeing function for a block of memory already freed by dereferencing the first GCPointer). What would be the scenario in which this is better?

I argue instead that the current design was instead simply based on bad design decisions I made when I wrote the RefC backend, so this is a PR to account for that.

The right approach to prevent malicious usage of the GC would be to change the signature of onCollect to not return IO (GCPtr a) but Maybe or Either or something to account for errors. Trying to register the same pointer twice to the GC could be detected with this PR, would would be undetectable the way things currently are. However, changing that would break too many things, in my opinion.

I'm happy to add test cases and copy the commit message to the Changelog.

[vfrinken]

test tests/refc/garbageCollect002/ added, CHANGELOG and CONTRIBUTORS file updated

[vfrinken]

I also took the liberty of reformatting all .c and .h files in the support/refc folder at least to adhere to appease the linter

[madman-bob]

In the following example, free1 would not be run.

doubleFree = do
    ptr <- getExternalPointer
    gcptr1 <- onCollectAny ptr free1
    gcptr2 <- onCollectAny ptr free2
    putStrLn $ show ptr

I would expect this example to be valid, and both free1 and free2 to be run.

It is correct that the current implementation causes your example to segfault. That is a flaw in your example, rather than the implementation of GCPointers. Similarly, the following example segfaults, despite not using GCPointers.

directFree = do
    ptr <- getExternalPointer
    free ptr
    putStrLn $ show ptr

The issue is the abstraction of "pointer"s, rather than the implementation of them. If you are using pointers, you acknowledge the possibility of segfaults.

I would expect a corrected version of your example to look something like this:

useGCPtr = do
    gcptr <- onCollectAny !getExternalPointer free
    putStrLn $ show gcptr.ptr

[vfrinken]

Thank you for confirming that the current version would lead to a segmentation fault. Together with it behaving differently than the other backends shows that this PR is a step forward.

To bring it two steps forward and have the garbage collector invoke both functions, free1 and free2 is a different discussion.
Such a discussion should involving runtime and complexity considerations. The garbage collector (together with trampoline) is already the slowest part of the produced code. Is it worth it to make it potentially slower for to account for exotic use-cases? Which if the freeing function should be called first? FiFo or FiLo? In my opinion, the garbage collector is the wrong place to enable multi-function freeing. Something along a new Idris primitive getFreeingFunction : GCPtr -> IO (Ptr -> IO()) would allow the end user to retrieve existing freeing functions and combine them with new freeing functions, without bothering the garbage collector logic.

I didn't understand your directFree example. There is no garbage collector involved. What am I missing? I also don't know of any actual examples that depend on registering several freeing functions with the GC for the same pointer, I would be happy if you can show me some resources where that is actually used.

On th eother hand, here is my actual use case where the necessity for this PR became clear:

There are C libraries out there, that provide data storages in various nested structs, along with their own alloc and free functions (I'm looking at you, protobuf). The user is expected to haggle their way through the structs to CRUD, but alloc and free is managed by the library for the top level struct only.

Thus, an Idris system would have a garbage collected pointer to the top struct, whose freeing function is the provided protobuf_object_free(..) function. All underlying structs would need a raw pointer (non-GC) only. Given this situation, how does read look like?
A rather verbose version would have two separate reading functions, both of which are doing the exact same thing, but one with a GCPtr a and one with a Ptr a as input. The more elegant version would have only reading function. But we cannot register the underlying structs with the garbage collector, they should remain untouched by us. But we can get a Ptr from a GCPtr without side effects. So we have the reading logic in a Ptr a -> IO (...) function together with a
extractPtr : GCPtr -> IO Ptr bound to the read.
You see where this leads to. If the last operation I do is a read or write or something, the system already dereferences the garbage collected GCPtr before the read function is invoked, with is then left hanging with already deallocated memory.

[madman-bob]

The purpose of the directFree example is to demonstrate that a segfault is not necessarily incorect behaviour. The backends behaving differently to each other does not demonstrate which one is correct. So you have not demonstrated that this PR is a step forward.

Further, before this PR, the doubleFree example works (at least in RefC), while afterwards it does not. So this PR is a step backwards.

As for your use case, the useGCPtr example would cover your needs. This requires only a (.ptr) : GCPtr t -> Ptr t function, which I'm surprised doesn't already exist.

Your concern about immediately collected GCPtrs is good. A proper solution to this issue is surprisingly complicated and subtle. Thankfully, however, it is also easily worked around, by separating the construction and usage of GCPtrs to different functions.

[vfrinken]

I tried may ways using a (.ptr) : GCPtr t -> Ptr t function, all of which result in a segmentation fault in the following scenario:
I get a packed protobuf message from an outside source in a Ptr PackedProtobuf. An elegant function along the line of
read : Ptr PackedProtobuf -> IO (Either Error t) evades me.
First I unpack the protobuf externally to a hierarchy of structs. Secondly, I register the top one with the GC: GCPtr t. Thirdly I read those. But with a (.ptr) : GCPtr t -> Ptr t function, the GCPtr t is already freed immediately.

Anyways, I won't pursue this PR and the garbage collector change any further. I see that my changes break the behavior of multiple freeing functions, but I need the new GC behavior (as well as further RefC backend changes I implemented), so it seems clear to me that best way forward is a separate and separately maintained backend.

[madman-bob]

It should be possible to get such a .ptr function to work. Creating a whole new backend seems excessive.

[vfrinken]

I don't think it is. As I mentioned, there are further changes I implemented that I care about but might be critically received. As an example, I care about creating 2 more backends for building shared and static libraries using the recently introduced %export tag. A potential problem is the global IORef variable. Assume several threads of an external program all want to invoke their own Idris code (This extra-Idris multithreading doesn't affect the GC). So I put a pthread mutex around that, which restricts the new backends to Linux/MaxOS system.

The new issue #2455 confirms my assumption that it's better to have something separately, so I can fix the memory problems without breaking current functionality for other people.

[gallais]

The new issue #2455 confirms my
assumption that it's better to have something separately, so I can fix the memory
problems without breaking current functionality for other people.

I don't understand this statement. Isn't the space leak reported in #2455 a bug that we would
all be happy to see fixed?

[vfrinken]

Yes, and I'm happy to contribute. But I don't know what would be the best way to address it, without breaking functionality for some. So I let others be the judge of that. I want to go ahead to get a working backend, even if it limits the functionality for others.
But whatever I'm doing is open source and I'm happy to integrate it into this repo if so desired.

[ohad]

Now, a each pointer has already has a reference to the garbage
collecting function, and a call to onCollect/onCollectAny simply
attaches the GC closure to it, but no new object is created.

In this case, maybe the interface to onCollect and onCollectAny should change to something like:

Deallocator : Maybe Type -> Type
Deallocator Nothing = (AnyPtr -> IO ())
Deallocator (Just t) = (Ptr t -> IO ())

onCollect : HasIO io => Ptr t -> (List (Deallocator (Just t)) -> List (Deallocator (Just t))) -> io ()

So that we don't have duplicated GCPtr's floating around?

[andrevidela]

Any progress on that?

[ohad]

I think the conclusion last time was that the interface to GCPtrs should change slightly to reflect the behavior and invariants better, and more uniformly across the backends.

I don't have a good feel as to whether that's best done in thus PR or in a new PR that starts from scratch, changes the interface, and implements the new behaviour uniformly.

[andrevidela]

let's close this for now and reopen once the interface has been clarified. @ohad @vfrinken or @madman-bob can one of you write up an issue to keep track of this?