Keep an unsafe pointer to a context, passed to C, alive

[noxiouz]

Helllo!

I use cgo to implement a binding for C++ library (https://github.com/noxiouz/elliptics-go/blob/master/elliptics/callback.go). To get results from asyncronous methods of library, channels are used. It works that way:

1. Call go function (https://github.com/noxiouz/elliptics-go/blob/master/elliptics/session.go#L232), which returns channel.
2. unsafe.pointer is passed to C code as context
3. After C and C++ magic, this pointer will pass to go function, which is called from C (https://github.com/noxiouz/elliptics-go/blob/master/elliptics/session.cpp#L94, https://github.com/noxiouz/elliptics-go/blob/master/elliptics/session.cpp#L85)
4. Data is sent to the channel by calling context

Seems unsafe.Pointer to context is collected by gc since go1.3. Which is a proper way to keep this pointer alive?

I really don't want to save this pointer in global map or smth like this unless there is no other way.

Thanks!

[Tamás Gulácsi]

AFAIK you have to save that unsafe.Pointer to prevent collection.

[Ian Lance Taylor]

There is no other way. If the Go garbage collector can't see the
pointer, it will collect it. The Go garbage collector can't see
pointers in C code.

Moreover in the future it is possible that the Go garbage collector
will move memory allocations around, meaning that you may need to do
more work to safely pass pointers in and out of C. See
http://golang.org/issue/8310 .

Ian

[Alexey Borzenkov]

On Tue, Aug 12, 2014 at 5:54 PM, Ian Lance Taylor <ia...@golang.org> wrote:
> There is no other way. If the Go garbage collector can't see the
> pointer, it will collect it. The Go garbage collector can't see
> pointers in C code.

I accidentally replied off the list (happens every time I'm using my
phone), but I think there is (no a very good one, but still), using a
goroutine, e.g.

type context struct {
done chan struct{}
// ...
}

func keepalive(c *context) {
<-c.done
}

And the code that needs to keep context alive would do:

c := &context{
done: make(chan struct{}),
// ...
}
go keepalive(c)

And later when context is no longer needed (e.g. callback was
unregistered) you would do close(c.done). This would work until
optimizer smart enough to realize there's never any real write on done
and do <-c.done doesn't have any side effects.

> Moreover in the future it is possible that the Go garbage collector
> will move memory allocations around, meaning that you may need to do
> more work to safely pass pointers in and out of C. See
> http://golang.org/issue/8310 .

Microsoft's .NET (for example, not sure about others) had concurrent
gc for many years now, and it supports interfacing with C++ with
C++/CLI. The way you pass pointers around using pin_ptr, which makes
sure gc cannot move objects it points while it's in scope. Which is
actually cool since it's entirely implicit.

Since gc would already need to know all pointers to actually move them
wouldn't it be possible to use unsafe.Pointer effectively as a pinned
pointer, i.e. unsafe.Pointer should never be moved due to gc, except
when making a bigger stack and they point into it.

This would also make cgo and syscall work with less surprises and
without unnecessary copying. The price would be that memory might
become fragmented (since not everything would potentially be moved),
but I don't think it would be such a big problem. I wonder what would
Dmitry think though.

[Wes Freeman]

I'm using a library that had similar pain with 1.3, passing a local channel variable as a pointer to C to block on in Go. I submitted an experimental PR that seemed pretty hacky (but seemed to work) by returning the address of the channel, but was wondering if someone could comment on it--maybe there's a better practice for the desired behavior?

https://github.com/FoundationDB/fdb-go/pull/6

Wes

[Alexey Borzenkov]

But why would returning that pointer make a difference? Wouldn't it be
easier just storing that pointer in a local variable?

[Wes Freeman]

It is in a local variable already. https://github.com/FoundationDB/fdb-go/blob/master/fdb/futures.go#L89

I'm not sure why it doesn't work, and I'm not sure why returning it makes it work.

[Wes Freeman]

Oh, I see what you mean. If I make it a local pointer, like:

func fdb_future_block_until_ready(f *C.FDBFuture) {

|  if C.fdb_future_is_ready(f) != 0 {

|  |  return

|  }

|  ch := make(chan struct{}, 1)

|  chptr := &ch

|  C.go_set_callback(unsafe.Pointer(f), unsafe.Pointer(chptr))

|  <-ch

}

it doesn't work. I get an ugly mach 15 error, same as if it's not a pointer.

https://gist.github.com/wfreeman/675651027509b8232757

Wes

[Alexey Borzenkov]

On Tue, Aug 12, 2014 at 10:08 PM, Wes Freeman <freem...@gmail.com> wrote:
> Oh, I see what you mean. If I make it a local pointer, like:
>
> func fdb_future_block_until_ready(f *C.FDBFuture) {
> | if C.fdb_future_is_ready(f) != 0 {
> | | return
> | }
>
> | ch := make(chan struct{}, 1)
> | chptr := &ch
> | C.go_set_callback(unsafe.Pointer(f), unsafe.Pointer(chptr))
> | <-ch
> }
>
> it doesn't work. I get an ugly mach 15 error, same as if it's not a pointer.
>
> https://gist.github.com/wfreeman/675651027509b8232757

Hmm. Interesting. Does ch escape to heap in your case? I wonder if go
thinks ch doesn't escape in your case and allocates it on the stack,
and then channel receive causes stack to grow and thus ch to move.
Can't quite reproduce it yet.

[Wes Freeman]

Hmm. Interesting. Does ch escape to heap in your case? I wonder if go

thinks ch doesn't escape in your case and allocates it on the stack,
and then channel receive causes stack to grow and thus ch to move.
Can't quite reproduce it yet.

I guess it must escape to heap, if the address is being returned (although it isn't being used elsewhere). Is there some way to confirm? I just tested with 1.3.1 and the same behavior.

[Alexey Borzenkov]

Yes, you can look at the output of go build -gcflags '-m'. It would be various escapes or does not escape. Assembly output with gcflags -S is better if you can read assembly though.

[Wes Freeman]

Hmm, looks like it escapes to heap both ways, but twice with the return.

With the return &ch (works):

 ./futures.go:89: moved to heap: ch

 ./futures.go:90: &ch escapes to heap

 ./futures.go:89: make(chan struct {}, 1) escapes to heap

 ./futures.go:92: &ch escapes to heap

 ./futures.go:89: make(chan struct {}, 1) escapes to heap

Without the return (fails):

./futures.go:89: moved to heap: ch

./futures.go:90: &ch escapes to heap

./futures.go:89: make(chan struct {}, 1) escapes to heap

full output of -S and -m here--that was educational, thanks:
https://gist.github.com/wfreeman/83bfb365e6810d2a8829

Wes

[Alexey Borzenkov]

On Wed, Aug 13, 2014 at 9:11 PM, Wes Freeman <freem...@gmail.com> wrote:
> Hmm, looks like it escapes to heap both ways, but twice with the return.

[...]

> full output of -S and -m here--that was educational, thanks:
> https://gist.github.com/wfreeman/83bfb365e6810d2a8829

Can't see anything wrong in assembly either. :-/ Another idea though,
are you sure that fdb_future_set_callback doesn't cause its callback
to somehow be called more than once? Could you add something like
printf("go_callback(%p, %p)\n", f, ch); before notifyChannel, maybe it
would shed a light?

Reproducible example, OS and software versions would be nice too.

[Wes Freeman]

Yeah, the problem with the reproducible example is that it requires running the foundationdb service (I'm running 2.0.7) in order to test. 

I'm running OSX 10.9.4; also tested in Centos 6.4 and got similar failures.

I tried putting the printf in there, and added an fflush() as well, since it wasn't printing before crashing, I get one before it crashes:

go_callback(0x49004c0, 0xc208032068)

If I put the hacky return &ch in, it seems to work fine:
https://gist.github.com/wfreeman/c9f55ab73d2de6d97e84

Wes

[Alexey Borzenkov]

On Wed, Aug 13, 2014 at 11:32 PM, Wes Freeman <freem...@gmail.com> wrote:
> Yeah, the problem with the reproducible example is that it requires running
> the foundationdb service (I'm running 2.0.7) in order to test.
> I'm running OSX 10.9.4; also tested in Centos 6.4 and got similar failures.

I tried on my OS X 10.9.4 with FoundationDB 2.0.8 and finally
reproduced it after adding runtime.GC() call to notifyChannel and
various fprintf(stderr, ...) calls. Now running under GOTRACEBACK=2
gives a very interesting result:

https://gist.github.com/snaury/2c4154d86c3c2f6fecf0

There you would see that fdb_future_block_until_ready(0x4c00250) is
parked inside chanrecv1(0x4103e00, 0xc208004120, 0x0), where
0xc208004120 is the actual channel pointer. However in the closeNotify
it is in closechan(0xc2080340a8, 0x40b018f), notice that the pointer
is different. What's more, it is visible that the pointer got changed
between setting the callback and its execution. At first I thought
this was some weird moving of heap pointers in Go 1.3.1, but running
with StackDebug=2 didn't reveal any suspicious adjustments at all, so
I was at a loss why that might be.

And then it hit me. Values of chan T are already pointer. What you
pass into callback is a pointer to a pointer, but since it's actually
nowehere in locals there's nothing that could prevent it from being
collected. That's why adding it to return values helps, because it
just so happens it really needs to keep that until function returns
(that's of course until optimizer is smart enough to optimize that
return value away).

Interestingly local variables don't help, even something like this:

pch := new(chan struct{})
*pch = make(chan struct{}, 1)
C.go_set_callback(f, unsafe.Pointer(pch))
<-*pch

Doesn't make any difference. So, how is one expected to interact with
cgo in these situations, when even a local variable is not guaranteed
to keep a reference to the object, if the variable itself may be freed
after it stops being used?

[Tamás Gulácsi]

Why not

ch := make(chan struct{}, 1)
pch := unsafe.Pointer(&ch)
C.go_callback(pch)
<-ch

[Alexey Borzenkov]

Because it doesn't make a difference. Making pch an unsafe.Pointer
doesn't magically make it stay alive until the end of the function
(would be nice if it did, and also if it pinned memory in the future),
it seems it's only alive until the return from C.go_callback(), and
then it can be garbage collected.

[Wes Freeman]

Thanks for chasing it down! So, at least we know that the return &ch will fix it for 1.3.1--knowing what to do in a more future-proof way would be great.

Wes

[ian.p...@foundationdb.com]

Hey guys,

I'm the primary author of the FoundationDB Go interface (as well as a few of our other language bindings). I wanted to jump in and say thanks for looking into this -- Wes has been chasing this for a bit, and Alexey's insight that chan T is already a pointer type finally helped me understand what's going on here.

I've put together a much simpler example that doesn't require FoundationDB to be installed and running to demonstrate the issue: https://gist.github.com/ipeters/4bc867c5e093679ab145

As Alexey reasoned, the problem is that we are handing to C the address of a channel that is itself a pointer type. By the time we try to dereference the channel, that stack space may already have been reused. The channel itself (pointed to by the chan struct{}) is still alive (we're trying to read from it), but the dereference is invalid.

So here's the fix we're looking at: https://gist.github.com/ipeters/d68202a0433bb3566f6e

We got here after eventually stumbling across https://golang.org/misc/cgo/test/callback.go while searching for some discussion of this issue. This obviously relies upon an assumption of the representation of chan T, which makes me a little uncomfortable.

This will also fall apart if/when Go starts moving things in the collector, so hopefully if Go ever gets to that point we'll also have a way to explicitly pin memory (or else this kind of Go/C integration with callbacks seems to no longer be feasible).

Unless I hear a better idea, I think this is the approach we're going to have to go with to keep the fdb-go package working with Go 1.3 and beyond.

[Julian Phillips]

On 15/08/2014 16:44, ian.p...@foundationdb.com wrote:
> Hey guys,
>
> I'm the primary author of the FoundationDB Go interface (as well as a
> few
> of our other language bindings). I wanted to jump in and say thanks for
> looking into this -- Wes has been chasing this for a bit, and Alexey's
> insight that chan T is already a pointer type finally helped me
> understand
> what's going on here.
>
> I've put together a much simpler example that doesn't require
> FoundationDB
> to be installed and running to demonstrate the
> issue: https://gist.github.com/ipeters/4bc867c5e093679ab145
>
> As Alexey reasoned, the problem is that we are handing to C the address
> of
> a channel that is itself a pointer type. By the time we try to
> dereference
> the channel, that stack space may already have been reused. The channel
> itself (pointed to by the chan struct{}) is still alive (we're trying
> to
> read from it), but the dereference is invalid.
>
> So here's the fix we're looking
> at: https://gist.github.com/ipeters/d68202a0433bb3566f6e

why not just use a mutex?

The code below should be safe until heap pointers can be moved by GC,
without having to make any assumptions:

func block_until_ready() {
m := &sync.Mutex{}
m.Lock()
C.callback(unsafe.Pointer(m))
m.Lock()
}

func goCallback(p unsafe.Pointer) {
m := (*sync.Mutex)(p)
m.Unlock()
}

--
Julian

[Wes Freeman]

Yeah, I was thinking that might work after Alexey's realization about chan being a pointer. If anything, it would reduce the number of weird double pointer casts/dereferences.

Wes

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.