code review 834045: Library support for cgo export.

Library support for cgo export.

These functions are used to call from a C function back to a
Go function.  This only includes 386 support.

[iant, 2010-04-02 04:05:56]

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

I'd like you to review this change.

[rsc1, 2010-04-02 06:31:25]

LGTM

I'm glad you're writing this and not me!

http://codereview.appspot.com/834045/diff/2001/3001
File src/libcgo/386.S (right):

http://codereview.appspot.com/834045/diff/2001/3001#newcode39
src/libcgo/386.S:39: * void crosscall2(void (*fn)(void *, int32), void*, int32)
s/void */void*/

http://codereview.appspot.com/834045/diff/2001/3002
File src/pkg/runtime/386/asm.s (right):

http://codereview.appspot.com/834045/diff/2001/3002#newcode360
src/pkg/runtime/386/asm.s:360: // runcgocallback(void* sp, void (*fn)())
s/void* sp/void *sp/
s/()/(void)/

i know i'm a pain, sorry

http://codereview.appspot.com/834045/diff/2001/3002#newcode362
src/pkg/runtime/386/asm.s:362: TEXT	runcgocallback(SB),7,$0
s/$0/$32/ and then you can delete
the SUBL and ADDL lines below.

http://codereview.appspot.com/834045/diff/2001/3002#newcode363
src/pkg/runtime/386/asm.s:363: MOVL	4(SP), AX	// new SP
s/4(SP)/sp+0(FP)/  
s/8(SP)/fn+4(FP)/

- args begin at 0(FP) always, avoids mental math. 
- adding sp+ tells the debug info the name of the argument.

http://codereview.appspot.com/834045/diff/2001/3002#newcode368
src/pkg/runtime/386/asm.s:368: // clobber our stack.
ouch.  nice catch

http://codereview.appspot.com/834045/diff/2001/3002#newcode378
src/pkg/runtime/386/asm.s:378: // Restore old SP, return
Seems like this section should just be RET.
Hopefully the SP didn't change across the call.

http://codereview.appspot.com/834045/diff/2001/3003
File src/pkg/runtime/cgocall.c (right):

http://codereview.appspot.com/834045/diff/2001/3003#newcode54
src/pkg/runtime/cgocall.c:54: cgocallback(void (*fn)(), void *arg, int32
argsize)
s/()/(void)/

http://codereview.appspot.com/834045/diff/2001/3003#newcode66
src/pkg/runtime/cgocall.c:66: sp = g->sched.sp - argsize;
if(sp < g->stackguard)
    throw("g stack overflow in cgocallback");

(and eventually we can do the right thing
but adding a stack split to this code seems
like a good thing for a separate CL)

http://codereview.appspot.com/834045/diff/2001/3004
File src/pkg/runtime/cgocall.h (right):

http://codereview.appspot.com/834045/diff/2001/3004#newcode10
src/pkg/runtime/cgocall.h:10: void cgocallback(void (*fn)(), void*, int32);
s/()/(void)/

[iant2, 2010-04-03 02:16:16]

rsc@google.com writes:

> http://codereview.appspot.com/834045/diff/2001/3002#newcode360
> src/pkg/runtime/386/asm.s:360: // runcgocallback(void* sp, void (*fn)())
> s/void* sp/void *sp/
> s/()/(void)/

(void) isn't really right since the function takes some unknown number
of arguments.  I changed this to

// runcgocallback(void* sp, void (*fn)(...))
// Switch to sp, call fn, switch back.  fn's arguments are on the
// new stack.

> http://codereview.appspot.com/834045/diff/2001/3002#newcode368
> src/pkg/runtime/386/asm.s:368: // clobber our stack.
> ouch.  nice catch

Hah.  Took me hours to work that out.

> http://codereview.appspot.com/834045/diff/2001/3002#newcode378
> src/pkg/runtime/386/asm.s:378: // Restore old SP, return
> Seems like this section should just be RET.
> Hopefully the SP didn't change across the call.

We changed the SP ourselves.  We need to restore it, but we can't save
it on the new stack because we don't know how many arguments we are
passing to FN.

> http://codereview.appspot.com/834045/diff/2001/3003#newcode54
> src/pkg/runtime/cgocall.c:54: cgocallback(void (*fn)(), void *arg, int32
> argsize)
> s/()/(void)/

The function does take arguments, so (void) seems odd, we just don't
know it's real signature.  I can write (void) here if it seems better,
of course.

> http://codereview.appspot.com/834045/diff/2001/3004#newcode10
> src/pkg/runtime/cgocall.h:10: void cgocallback(void (*fn)(), void*,
> int32);
> s/()/(void)/

Same here.

Ian

[rsc, 2010-04-06 05:19:23]

LGTM

[rsc, 2010-04-06 05:19:28]

Let's just use void (*fn)(void) everywhere.
Most of the current instances of that really
mean ... already.

[rsc1, 2010-04-06 05:28:55]

On closer examination, --LGTM.

I think there's something funny going on with the stack
pointer and the current goroutine pointer.

The comment inside runcgocallback(sp, fn) says that
it is running on m->g0's stack, but if so then
g should be == m->g0, yet in the function that
calls runcgocallback (cgocallback), there is an explicit check
that g == m->curg.  Both can't be true.

In general, any time SP is changed, g should be changed
too, because g carries with it the appropriate bounds for SP.
As a sanity check, you should be able to insert

    CALL stackcheck(SB)

before any function call instruction and not have it crash,
yet I believe that if you insert it before runcgocallback's

    CALL    BX

it will crash.  (Beware that stackcheck smashes AX, CX on 386.)

http://codereview.appspot.com/834045/diff/2001/3002
File src/pkg/runtime/386/asm.s (right):

http://codereview.appspot.com/834045/diff/2001/3002#newcode368
src/pkg/runtime/386/asm.s:368: // clobber our stack.
// clobber our stack, and also so that we can restore
// the SP when the call finishes.  Reusing m->sched.sp
// for this purpose depends on the fact that there is only
// one possible gosave of m->sched.

[iant2, 2010-04-06 13:59:10]

rsc@google.com writes:

> I think there's something funny going on with the stack
> pointer and the current goroutine pointer.
>
> The comment inside runcgocallback(sp, fn) says that
> it is running on m->g0's stack, but if so then
> g should be == m->g0, yet in the function that
> calls runcgocallback (cgocallback), there is an explicit check
> that g == m->curg.  Both can't be true.

Yes, they can.  The runcgocallback function is always called via some
series of calls which started with runcgo.  The runcgo function
switches to the scheduler stack without changing m->curg.  This is all
correct: the scheduler stack is always used to run non-Go code, but
that does not imply that we have changed g.

I updated the (void) and the comments.


I have the 64-bit version working on GNU/Linux, by the way, but it
needs another small change to cgo/out.go so I may have to fold in that
change.  I also need to test Mac before I commit to at least make sure
that I don't break the build.

Ian

[rsc1, 2010-04-06 21:11:50]

http://codereview.appspot.com/834045/diff/10002/18003
File src/pkg/runtime/cgocall.c (right):

http://codereview.appspot.com/834045/diff/10002/18003#newcode48
src/pkg/runtime/cgocall.c:48: // When a C function calls back into Go, the
wrapper function will
I don't see where this gets called, and maybe that's part of my confusion.
But with that caveat....

Because this is a C function, the SP needs to match the g register
or else the stack growth prologue will do the wrong thing.
That means that on entry to this function, if the stack pointer
is on m->g0's stack, then g needs to be m->g0 and not m->curg.  

That invariant is maintained by all stack switches in the go tree
(they always change SP and g at the same time) except one:
the one in runcgo, because it's calling ELF code that doesn't
care what g is and might smash it anyway.  Calling back into
gc code from the ELF code is making that assumption invalid.

On the 386 it should suffice to change g in runcgo and then
change back after the call (use m->curg unless m->curg == nil).

On the amd64 it is harder: I don't know how one can find out
what m and g are for the call, because the ELF code in the middle
might have smashed all the registers.  We may need to introduce
the same kind of TLS code that the 386 has.

[iant2, 2010-04-06 22:00:29]

rsc@google.com writes:

> http://codereview.appspot.com/834045/diff/10002/18003
> File src/pkg/runtime/cgocall.c (right):
>
> http://codereview.appspot.com/834045/diff/10002/18003#newcode48
> src/pkg/runtime/cgocall.c:48: // When a C function calls back into Go,
> the wrapper function will
> I don't see where this gets called, and maybe that's part of my
> confusion.

The cgocallback function is called by a wrapper generated by cgo.  The
wrapper for an exported function looks like this:

#pragma dynexport _cgoexp_NAME _cgoexp_NAME
void
_cgoexp_NAME(void *a, int32 n)
{
	cgocallback(NAME, a, n);
}


> Because this is a C function, the SP needs to match the g register
> or else the stack growth prologue will do the wrong thing.
> That means that on entry to this function, if the stack pointer
> is on m->g0's stack, then g needs to be m->g0 and not m->curg.
>
> That invariant is maintained by all stack switches in the go tree
> (they always change SP and g at the same time) except one:
> the one in runcgo, because it's calling ELF code that doesn't
> care what g is and might smash it anyway.  Calling back into
> gc code from the ELF code is making that assumption invalid.

Hmmm.  I suppose we could use

#pragma textflag 7

but having runcgo change g should also suffice.


> On the amd64 it is harder: I don't know how one can find out
> what m and g are for the call, because the ELF code in the middle
> might have smashed all the registers.  We may need to introduce
> the same kind of TLS code that the 386 has.

I've already done something similar for the 64-bit code.  I just
created CL 857045 so you can see what I'm doing.

Ian

[rsc, 2010-04-06 23:33:09]

> Hmmm.  I suppose we could use
>
> #pragma textflag 7
>
> but having runcgo change g should also suffice.

Right, I'd rather change g since otherwise everything
this function calls has to be 7 too (mcpy is the tough one).

Russ

[iant2, 2010-04-06 23:57:21]

rsc@google.com writes:

> Because this is a C function, the SP needs to match the g register
> or else the stack growth prologue will do the wrong thing.
> That means that on entry to this function, if the stack pointer
> is on m->g0's stack, then g needs to be m->g0 and not m->curg.
>
> That invariant is maintained by all stack switches in the go tree
> (they always change SP and g at the same time) except one:
> the one in runcgo, because it's calling ELF code that doesn't
> care what g is and might smash it anyway.  Calling back into
> gc code from the ELF code is making that assumption invalid.
>
> On the 386 it should suffice to change g in runcgo and then
> change back after the call (use m->curg unless m->curg == nil).

Thanks for pointing that out.  I uploaded a new version of the 386
code which I think keeps g and SP in synch.  I'll do the amd64 version
later.

Ian

[iant2, 2010-04-07 00:02:09]

Russ Cox <rsc@golang.org> writes:

>> Hmmm.  I suppose we could use
>>
>> #pragma textflag 7
>>
>> but having runcgo change g should also suffice.
>
> Right, I'd rather change g since otherwise everything
> this function calls has to be 7 too (mcpy is the tough one).

Yeah, I was imagining using another magic function to change g before
calling mcpy, but the other way is better.

Ian

[rsc1, 2010-04-07 00:11:34]

LGTM

[iant, 2010-04-09 20:30:40]

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

Library support for cgo export.

These functions are used to call from a C function back to a
Go function.  This only includes 386 support.

R=rsc
CC=golang-dev
http://codereview.appspot.com/834045