breaks go 1.6 cgo pointer rules

[Michael Hudson-Doyle]

Several tests fail with Go 1.6 with "Panic: runtime error: cgo argument has Go pointer to Go pointer", which is talking about the rules described here: http://tip.golang.org/cmd/cgo/#hdr-Passing_pointers

All of the panics seem to be at this call:

http://bazaar.launchpad.net/~unity-team/go-unityscopes/v2/view/70/result.go#L62

but I certainly don't know if there are more of them hiding.

(this bug is at least part of why unity-scopes-snappy ftbfs with the go 1.6 beta2 package I made, as seen at https://launchpadlibrarian.net/234161492/buildlog_ubuntu-xenial-amd64.unity-scope-snappy_0.1.0+15.10.20150827-0ubuntu1_BUILDING.txt.gz)

[James Henstridge (jamesh)]

What we want to do is pass a string down to the C/C++ wrapper without copying the data: the APIs don't take C strings, so we'd need to make yet another copy if using C.CString(), and would also need to deal with memory management of that copy.

The function doesn't hold a reference to the memory past when it returns.

[Michael Hudson-Doyle (mwhudson)]

I can't claim to know how to fix this properly. Maybe ask on golang-nuts?

On 19 January 2016 at 07:02, James Henstridge
<email address hidden> wrote:
> What we want to do is pass a string down to the C/C++ wrapper without
> copying the data: the APIs don't take C strings, so we'd need to make
> yet another copy if using C.CString(), and would also need to deal with
> memory management of that copy.
>
> The function doesn't hold a reference to the memory past when it
> returns.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1535157
>
> Title:
> breaks go 1.6 cgo pointer rules
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/go-unityscopes/+bug/1535157/+subscriptions

[James Henstridge (jamesh)]

I'll follow this up on golang-nuts. When I tried asking about this a few years ago in the Go bug tracker, they closed my bug and ignored me though:

https://github.com/golang/go/issues/6907

(with Google's bug tracker, there was no way to reopen the bug report).

In the short term the binding still works with GODEBUG=cgocheck=0 defined, but that certainly isn't a long term solution.

[James Henstridge (jamesh)]

I've posted a question about this to golang-nuts here:

https://groups.google.com/d/msg/golang-nuts/VIide5gFXho/sl9z9gJAGQAJ

[James Henstridge (jamesh)]

So the solution for the simple string cases is to convert the code to something like:

    var s string // from an argument, or wherever
    p := (*reflect.StringHeader)(&s)
    C.some_func(unsafe.Pointer(p.Data), C.int(len(s)))

Since we aren't passing the string object itself, there are no embedded Go pointers. It is important to cast p.Data back to a pointer type so that the GC knows to pin it for the duration of the function call.

There is no way to tell Go's GC to pin an array of pointers, so we'll need to work out something else there. The suggestion on golang-nuts was to concatenate the strings together and then split them again on the Go side, which is almost certainly faster than more cgo calls.

[James Henstridge (jamesh)]

I've proposed a merge that gets the test suite passing with Go 1.6. Hopefully that covers all of the problematic code paths. Any extra testing is appreciated.