[next] Prevent slot int variable from being GCed.

[dmitshur]

Before this change, there were no users of slot int variable in the Go world (just a pointer to it that ended up in C world only), so Go's garbage collector would free it and its value could not retrieved later (once a pointer to it comes back to Go world from C world).

Keep a pointer to it in the Go world so that does not happen.

Fixes #218. See that issue for a detailed analysis of the problem, and a reproducible test case (in a package that imports this one).

Please review for correctness and style, feedback is welcome.

There are many, many ways of doing the task of "keeping some reference to slot in Go so it's not GCed, including keeping a pointer to it as *int, or unsafe.Pointer, or interface{}. I've reused the set map since it seemed appropriate, but a new/different map can be created instead. Let me know if you prefer a different style and I can change it.

[dmitshur]

Style wise, it might be nicer to do handle := unsafe.Pointer(&slot) and later return handle - to avoid doing &slot in two places in the HandleList.Track code.

There are many, many style variations that all perform the same goal, and I don't know the most idiomatic way to solve this task, so I just went with the smallest diff PR for now. Suggestions for better style are welcome.

[carlosmn]

Thanks for looking into this. I wonder if replacing return return value to

unsafe.Pointer(slot)

would work as well. IIRC in go an int corresponds to the machine pointer size so we shouldn't have problems with the size (or we could limit it to 32 bits, having over 3 billion objects seems unlikely to be an actual limitation).

[dmitshur]

I wonder if replacing return return value to unsafe.Pointer(slot) would work as well.

That's an interesting idea. I just gave it a try. It wouldn't work exactly as you wrote it, because you can't convert from a non-pointer type to unsafe.Pointer directly, it results in a compilation error:

./handles.go:54: cannot convert slot (type int) to type unsafe.Pointer

However, it's possible to force that conversion by going through uintptr, so this will compile:

return unsafe.Pointer(uintptr(slot))

However, using that results in a fatal error:

runtime: garbage collector found invalid heap pointer *(0xc2080777e0+0x90)=0x2 s=nil
fatal error: invalid heap pointer

It would be possible to work around that by changing type of the handler from unsafe.Pointer to an integer type, but from what I can see, that may not be constrained due to libgit2 internals. Specifically, git_remote_callbacks is defined in libgit2/include/git2/remote.h:

struct git_remote_callbacks {
    ...

    /**
     * This will be passed to each of the callbacks in this struct
     * as the last parameter.
     */
    void *payload;
};

And git2go's Track and Untrack uses that field:

func populateRemoteCallbacks(ptr *C.git_remote_callbacks, callbacks *RemoteCallbacks) {
    ...
    ptr.payload = pointerHandles.Track(callbacks)
}

Unless... Is it possible to assign a Go integer type to C's void*? I would imagine C would not complain about void* having invalid pointers. (That kind of change may need an extra change so that generated slot values are never 0.)

What do you think?

[dmitshur]

Is it possible to assign a Go integer type to C's void*?

I don't think it's possible (likely for good reasons, as void* will be different size depending on architecture, and Cgo maps C's void* to Go's unsafe.Pointer). From Cgo article:

The C type void* is represented by Go's unsafe.Pointer.

With that, I conclude your suggestion is not possible without changes to libgit2's internals, unless you find another place to store the slot data that is not defined as void* in C.

Ok, it might be possible to allocate a new int in C code and have the void* payload point to it... (If you're willing to go that route.)

Let me know your thoughts before I continue forward looking for a different solution. There are many possibilities/workarounds. An alternative is to stick with the suggested solution in this PR.

[carlosmn]

I don't think it's possible (likely for good reasons, as void* will be different size depending on architecture,

So is Go's int, but we can work around that by using a int32 variable, which will fit in any arch Go runs on. That has no problem fitting in a 32-bit or 64-bit pointer type.

[dmitshur]

The issue is not the size of data, but the type.

The payload field in the C struct is defined with type void*. Cgo maps that to unsafe.Pointer. You can't assign different types in Go without a convertion, but when you try convert the slot, an int32, into unsafe.Pointer, Go will panic with:

runtime: garbage collector found invalid heap pointer *(0xc2080777e0+0x90)=0x2 s=nil
fatal error: invalid heap pointer

Can you please elaborate what you're suggesting?

[carlosmn]

void * is the only type which makes sense for libgit2 to take, as it can't know what each caller is going to need. If Go can't handle the conversion, it's not a problem with libgit2.

But anyway, let's merge this as it solves the issue and we can figure out if we want to do something else later.

[dmitshur]

But anyway, let's merge this as it solves the issue and we can figure out if we want to do something else later.

I was going to suggest doing exactly that, sounds good. Thanks for merging.