[next] Prevent slot int variable from being GCed. #219

2015-07-06T21:33:02-05:00

dmitshur commented

2015-07-06 21:33:02 -05:00

(Migrated from github.com)

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.

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 commented

2015-07-06 21:37:55 -05:00

(Migrated from github.com)

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.

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 commented

2015-07-10 08:02:51 -05:00

(Migrated from github.com)

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).

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 commented

2015-07-10 19:53:32 -05:00

(Migrated from github.com)

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?

> 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: ``` Go 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`: ``` C 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: ``` Go 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 commented

2015-07-10 20:01:43 -05:00

(Migrated from github.com)

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.

> 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](https://golang.org/cmd/cgo/#hdr-Go_references_to_C): > 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 commented

2015-07-15 07:25:43 -05:00

(Migrated from github.com)

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.

> 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 commented

2015-07-15 13:33:01 -05:00

(Migrated from github.com)

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?

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 commented

2015-07-24 06:43:16 -05:00

(Migrated from github.com)

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.

`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 commented

2015-07-24 18:51:06 -05:00

(Migrated from github.com)

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.

> 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.

Sign in to join this conversation.

No reviewers