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
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.
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
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, ...
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.
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
> 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
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
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
Issue 834045: code review 834045: Library support for cgo export.
(Closed)
Created 14 years ago by iant
Modified 14 years ago
Reviewers:
Base URL:
Comments: 11
