code review 3886041: runtime/cgo: Add callbacks to support SWIG.

runtime/cgo: Add callbacks to support SWIG.

[iant, 2011-01-06 23:19:06]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-01-07 00:12:41]

LGTM

[rsc, 2011-01-07 00:14:38]

This is okay with me but I am a little surprised
that the shared library would know about crosscall2.
What the life example does now is link the gcc-compiled
stub code that connects Go with the C library
as part of the package .a file.

It is possible - likely even - that you need this because
the anemic loading support in 6l/8l cannot handle C++ .o files,
but I wanted to point this out just in case.

Russ

[iant2, 2011-01-07 01:01:52]

Russ Cox <rsc@golang.org> writes:

> This is okay with me but I am a little surprised
> that the shared library would know about crosscall2.
> What the life example does now is link the gcc-compiled
> stub code that connects Go with the C library
> as part of the package .a file.
>
> It is possible - likely even - that you need this because
> the anemic loading support in 6l/8l cannot handle C++ .o files,
> but I wanted to point this out just in case.

The issue is more complex.  SWIG is generating C++ wrapper code.  That
wrapper code has to call both ways: into Go code and into the C++
library that we are wrapping.  Also, the program that is using SWIG
expects to be able to call into the wrapper code from C++ support code
which appears in the SWIG .i file.  This is C++ code, so I don't know
the actual symbol names, since that would require mangling them.  So
SWIG can't generate the dynexport and dynimport statements which 6l
wants to see in order to make it possible to call in and out of the
wrapper code from the C++ code.

Most languages that SWIG supports can't meaningfully link the SWIG code
in, so SWIG is normally used via a shared library holding the SWIG
wrapper code.  I think it's reasonable for the Go support to work that
way too, and it solves the problems described above, avoids any concerns
about linking C++ code with 6l, and simplifies the use of SWIG.
However, this approach does mean that crosscall2 must be in dynamic
symbol table of the Go binary.  Hence this CL.

That said, the SWIG code also wants to be able to call some Go utility
routines, so I'm now planning to add an additional .c file to the cgo
package, compiled with 6c, to support that.  So I'm going to rework this
CL to do that instead of changing the iscgo.c file.  There are two
utility routines: 1) Allocate memory controlled by the Go runtime; 2)
start a Go panic.  These are needed to support operations like creating
a Go string to return to Go code and to handle C++ exceptions.

Ian

[iant, 2011-01-08 05:18:38]

Hello rsc, iant2 (cc: golang-dev@googlegroups.com),

Please take another look.

[iant, 2011-01-10 22:55:33]

Hi, I'm hoping to get some feedback on this approach in order to get the changes
into the next SWIG release which is coming up fairly soon.

I'm concerned about GC issues here--let me know what you think.

[r, 2011-01-10 23:09:23]

http://codereview.appspot.com/3886041/diff/9001/src/pkg/runtime/cgo/callbacks.c
File src/pkg/runtime/cgo/callbacks.c (right):

http://codereview.appspot.com/3886041/diff/9001/src/pkg/runtime/cgo/callbacks...
src/pkg/runtime/cgo/callbacks.c:22: // variable on the stack or accessible from
a Go variable.  The
the 'Thereore' sentence doesn't make sense to me.   what's the difference
between a local variable and a Go variable?  also there's some grammatical
glitch

http://codereview.appspot.com/3886041/diff/9001/src/pkg/runtime/cgo/callbacks...
src/pkg/runtime/cgo/callbacks.c:45: // Panic.  This effectively calls
panic(str), where str is converted
s/effectively/in effect/

but is it the effect, or is that actually what it does?

[iant2, 2011-01-10 23:42:45]

r@golang.org writes:

>
http://codereview.appspot.com/3886041/diff/9001/src/pkg/runtime/cgo/callbacks.c
> File src/pkg/runtime/cgo/callbacks.c (right):
>
>
http://codereview.appspot.com/3886041/diff/9001/src/pkg/runtime/cgo/callbacks...
> src/pkg/runtime/cgo/callbacks.c:22: // variable on the stack or
> accessible from a Go variable.  The
> the 'Thereore' sentence doesn't make sense to me.   what's the
> difference between a local variable and a Go variable?  also there's
> some grammatical glitch
>
>
http://codereview.appspot.com/3886041/diff/9001/src/pkg/runtime/cgo/callbacks...
> src/pkg/runtime/cgo/callbacks.c:45: // Panic.  This effectively calls
> panic(str), where str is converted
> s/effectively/in effect/
>
> but is it the effect, or is that actually what it does?
>
> http://codereview.appspot.com/3886041/

Comments updated and I hope more clear.

PTAL.

Ian

[r, 2011-01-10 23:46:14]

comments LGTM

http://codereview.appspot.com/3886041/diff/12002/src/pkg/runtime/cgo/callbacks.c
File src/pkg/runtime/cgo/callbacks.c (right):

http://codereview.appspot.com/3886041/diff/12002/src/pkg/runtime/cgo/callback...
src/pkg/runtime/cgo/callbacks.c:24: // pointers are in C global variable or in
memory allocated via
s/C/a C/

[rsc, 2011-01-11 16:46:29]

LGTM

Sorry for the delayed review.

http://codereview.appspot.com/3886041/diff/12002/src/pkg/runtime/cgo/callbacks.c
File src/pkg/runtime/cgo/callbacks.c (right):

http://codereview.appspot.com/3886041/diff/12002/src/pkg/runtime/cgo/callback...
src/pkg/runtime/cgo/callbacks.c:28: //   struct { int len; void *ret; } a;
size_t len

http://codereview.appspot.com/3886041/diff/12002/src/pkg/runtime/cgo/callback...
src/pkg/runtime/cgo/callbacks.c:34: _cgo_allocate_internal(int32 len, byte *ret)
should be uintptr len

[iant, 2011-01-11 17:36:50]

*** Submitted as http://code.google.com/p/go/source/detail?r=aa6aa8d5c6e6 ***

runtime/cgo: Add callbacks to support SWIG.

R=rsc, iant2, r
CC=golang-dev
http://codereview.appspot.com/3886041