[WIP/RFC] Pointer indirection #196
Conversation
|
I guess it makes sense to not fix the remotes code but instead wait for libgit2/libgit2#3066 to be merged, as the interface will change anyway. The rest could be merged independetly of this, though. |
| return callback(C.GoString(cPath), C.GoString(cMatchedPathspec)) | ||
| } else { | ||
| return -1 | ||
| } |
There was a problem hiding this comment.
The payload wasn't optional before, it shouldn't be now.
There was a problem hiding this comment.
Well, if you take a look at index_test.go:TestIndexAddAllNoCallback it seems as if the callback was optional after all.
There was a problem hiding this comment.
It is optional to pass, but we only tell libgit2 to call indexMatchedPathCallback if the caller did pass in the callback. This function only gets called if there is a callback.
|
I've changed the code to panic when no treewalk/submodulevisitor callback could be found. The |
|
Any comments regarding this PR? |
|
I tested "git diff" with go1.4. It solves the panic caused by GC. |
|
There's still callbacks where we check whether the pointer is valid. If we do not find the handle in the list, there is no way we can continue running the program, as the state has been corrupted. We should either assume that the callback is correct, or panic if the retrieval fails. |
|
The latest commits broke it again. |
|
@taylorchu Eh, you're correct. Seems as if I worked on an outdated branch of mine. |
As the Go runtime can move stacks at any point and the C code runs concurrently with the rest of the system, we cannot assume that the payloads we give to the C code will stay valid for any particular duration. We must therefore give the C code handles which we can then look up in our own list when the callbacks get called.
Using 0 as the first slot indice leads to not being able to differentiate between a handle to the first element or a NULL-handle. As current code may check whether the pointer is NULL, change the first indice to be 1 instead.
If we store values by uintptrs the GC may try to inspect their values when it kicks in. As the pointers are most likely invalid, this will result in an invalid pointer dereference, causing the program to panic. We can fix this by storing values by an int index value instead, returning pointers to those indices as handles instead.
|
Interestingly enough the problem was not caused by an outdated copy but by a change of the GC behavior. When the GC kicks in it inspects uintptr values, causing invalid dereferences due to us storing callback data by arbitrary uinptrs. I have no idea why the issue did not occur previously, but it's fixed now (at least for me). @taylorchu could you please verify the fix? |
|
Short bump. Any remarks left? |
[WIP/RFC] Pointer indirection
|
Hi, I believe there is a relatively minor (and fixable) problem with the code change in this PR (I got to this PR from commit 1bd338a, which touches the relevant code). I have found a reproducible test case that causes a panic because the Go garbage collector triggers and removes something that shouldn't be removed (because there are no references to it in the Go world, only in the C world). (/cc @slimsag, thanks for reviewing my findings and suggesting a fix.) I will create an issue with some steps you can follow to reproduce the issue. I also have an MVP fix for the issue, that reliably resolves the problem. There's more than one way of doing it, so I welcome review and suggestions to change it, but I believe the general idea of the fix is sound, or at least in the right direction (basically, to keep a reference to the value in Go world memory so that it doesn't get GCed). I will create the PR and reference the issue. |
This pull request fixes issues with Go callbacks that are handed over to C functions. As of Go 1.4 it is not possible to hand over Go pointers into C code as the garbage collector is allowed to copy around the stack, causing pointers to become invalid (see golang/go#8310 and golang/go#10303 for more information).
@carlosmn proposed an interface HandleList for pointer indirection which provides a global map of data that is to be tracked. Instead of handing in pointers we now instead hand over an index that points to the corresponding entry in this map.
This PR adds the HandleList implemented by @carlosmn, fixes some issues with it and adjusts code that hands over Go pointers into C code to use handles instead. Still missing is the remote.go code, as I am running into some error there ('runtime: garbage collector found invalid heap pointer *(0xc208063cd0+0x18)=0x1 s=nil') that I've not been able to fix.
This PR also fixes #163.