code review 144130043: cgo: adjust return value location to account for stack ...

cgo: adjust return value location to account for stack copies.

During a cgo call, the stack can be copied.  This copy invalidates
the pointer that cgo has into the return value area.  To fix this
problem, pass the address of the location containing the stack
top value (which is in the G struct).  For cgo functions which
return values, read the stktop before and after the cgo call to
compute the adjustment necessary to write the return value.

Fixes issue 8771

[khr, 2014-09-18 22:57:31]

Hello iant@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://khr%40golang.org@code.google.com/p/go/

[rsc, 2014-09-18 23:31:44]

wow, ugly bug.
nice fix.

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s
File src/runtime/asm_386.s (right):

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s#new...
src/runtime/asm_386.s:684: MOVL	(g_stack+stack_hi)(DI), CX
not sure why these 3 lines changed; DI is available here

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_amd64.s
File src/runtime/asm_amd64.s (right):

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_amd64.s#n...
src/runtime/asm_amd64.s:669: MOVQ	(g_stack+stack_hi)(DI), CX
not sure why these 3 lines changed. DI is available here

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_arm.s
File src/runtime/asm_arm.s (right):

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_arm.s#new...
src/runtime/asm_arm.s:484: MOVW	fn+0(FP), R9
R9 is not available on nacl/arm, and this file is linked into nacl/arm even
though these particular routines aren't used. that's why R9 is completely unused
in this file (that and it used to be m). please use anything else (except R10,
which is g). how about R7?

https://codereview.appspot.com/144130043/diff/60001/src/runtime/stack.c
File src/runtime/stack.c (right):

https://codereview.appspot.com/144130043/diff/60001/src/runtime/stack.c#newco...
src/runtime/stack.c:830: // We can't copy the stack if we're in a syscall.  The
super pedantic but ... 
break line before "The" please.
google "semantic linefeeds".
i'm not picky about this all the time but for 1 word it's certainly better.

[khr1, 2014-09-19 00:12:55]

All fixed up.  PTAL.

On Thu, Sep 18, 2014 at 4:31 PM, <rsc@golang.org> wrote:

> wow, ugly bug.
> nice fix.
>
>
>
> https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s
> File src/runtime/asm_386.s (right):
>
> https://codereview.appspot.com/144130043/diff/60001/src/
> runtime/asm_386.s#newcode684
> src/runtime/asm_386.s:684: MOVL (g_stack+stack_hi)(DI), CX
> not sure why these 3 lines changed; DI is available here
>
> https://codereview.appspot.com/144130043/diff/60001/src/
> runtime/asm_amd64.s
> File src/runtime/asm_amd64.s (right):
>
> https://codereview.appspot.com/144130043/diff/60001/src/
> runtime/asm_amd64.s#newcode669
> src/runtime/asm_amd64.s:669: MOVQ       (g_stack+stack_hi)(DI), CX
> not sure why these 3 lines changed. DI is available here
>
> https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_arm.s
> File src/runtime/asm_arm.s (right):
>
> https://codereview.appspot.com/144130043/diff/60001/src/
> runtime/asm_arm.s#newcode484
> src/runtime/asm_arm.s:484: MOVW fn+0(FP), R9
> R9 is not available on nacl/arm, and this file is linked into nacl/arm
> even though these particular routines aren't used. that's why R9 is
> completely unused in this file (that and it used to be m). please use
> anything else (except R10, which is g). how about R7?
>
> https://codereview.appspot.com/144130043/diff/60001/src/runtime/stack.c
> File src/runtime/stack.c (right):
>
> https://codereview.appspot.com/144130043/diff/60001/src/
> runtime/stack.c#newcode830
> src/runtime/stack.c:830: // We can't copy the stack if we're in a
> syscall.  The
> super pedantic but ...
> break line before "The" please.
> google "semantic linefeeds".
> i'm not picky about this all the time but for 1 word it's certainly
> better.
>
> https://codereview.appspot.com/144130043/
>

[iant, 2014-09-19 00:15:00]

LGTM

This is clever.  Unfortunately I can't quite use this for SWIG, since SWIG
generated code should work with as many versions of Go as possible.  I guess
it's fine for the SWIG generated code to simply ignore the second argument that
Go 1.4 will pass.  SWIG generates the Go code too, and SWIG often uses two
different Go functions, so perhaps SWIG can generate Go code like

func Callable() int {
    pret := new(int)
    _wrap_Callable(pret)
    return *pret
}

func _wrap_callable(pret *int) {
    runtime.cgocall(_wrap_c_callable, uintptr(unsafe.Pointer(&pret)))
}

In C

void _wrap_c_callable(void *swig_v) {
  struct swigargs {
    intgo *pret;
  } swig_a = (struct swigargs *)swig_v;
  intgo *pret = swig_a->pret;
  int r = callable();
  *pret = r;
}

In the C function I'm copying pret off the Go stack into local memory.  So now I
should have a pointer into the heap that will not change in Go 1.4.  Of course
this approach won't work in Go 1.5, when we'll need some mechanism for asking
the GC for memory that won't move during a cgo call.

https://codereview.appspot.com/144130043/diff/60001/misc/cgo/test/callback.go
File misc/cgo/test/callback.go (right):

https://codereview.appspot.com/144130043/diff/60001/misc/cgo/test/callback.go...
misc/cgo/test/callback.go:246: println(&r)
Drop the println if you can.

https://codereview.appspot.com/144130043/diff/60001/misc/cgo/test/callback.go...
misc/cgo/test/callback.go:258: println(&r)
Drop this println too.

https://codereview.appspot.com/144130043/diff/60001/misc/cgo/test/callback_c.c
File misc/cgo/test/callback_c.c (right):

https://codereview.appspot.com/144130043/diff/60001/misc/cgo/test/callback_c....
misc/cgo/test/callback_c.c:79: int x;
Why the int x here?

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s
File src/runtime/asm_386.s (right):

https://codereview.appspot.com/144130043/diff/60001/src/runtime/asm_386.s#new...
src/runtime/asm_386.s:645: // asmcgocall(void(*fn)(void*), void *arg)
This comment is now wrong, since fn now takes two arguments.  Same for the other
asm files.

[rsc, 2014-09-19 00:26:28]

Re SWIG, what if instead of the changes here to pass a second argument we
provided in the Go library a function like char *_go_topofstack(void),
callable from gcc. Then the two instances of *stktopptr would instead be
_go_topofstack() in the cgo code, and SWIG could call it too. There must be
some magic linker trick with weak symbols that SWIG could use to supply a
dummy version (return 0 would be fine) for older Go versions. Or we could
provide an initialized char* (*_go_topofstack)(void), and SWIG could
declare a common symbol for that too, and it would just be zero if you had
an older Go.

Russ

[iant, 2014-09-19 00:34:24]

On Thu, Sep 18, 2014 at 5:26 PM, Russ Cox <rsc@golang.org> wrote:
>
> Re SWIG, what if instead of the changes here to pass a second argument we
> provided in the Go library a function like char *_go_topofstack(void),
> callable from gcc. Then the two instances of *stktopptr would instead be
> _go_topofstack() in the cgo code, and SWIG could call it too. There must be
> some magic linker trick with weak symbols that SWIG could use to supply a
> dummy version (return 0 would be fine) for older Go versions. Or we could
> provide an initialized char* (*_go_topofstack)(void), and SWIG could declare
> a common symbol for that too, and it would just be zero if you had an older
> Go.

SGTM

Just the function should be sufficient.  I was actually thinking of
calling a function but was getting hung up on the fact that old
versions wouldn't have that function.  I can't believe I didn't think
of using a stupid linker trick.

Ian

[khr1, 2014-09-19 02:33:34]

This __go_topofstack function has to be written in gcc code, but it needs
access to the G register so it can get the top of stack.  Is there any way
to do that inside gcc, or do we need to use assembly?  Would inline asm
work or might the G register be clobbered?

On Thu, Sep 18, 2014 at 5:34 PM, Ian Lance Taylor <iant@golang.org> wrote:

> On Thu, Sep 18, 2014 at 5:26 PM, Russ Cox <rsc@golang.org> wrote:
> >
> > Re SWIG, what if instead of the changes here to pass a second argument we
> > provided in the Go library a function like char *_go_topofstack(void),
> > callable from gcc. Then the two instances of *stktopptr would instead be
> > _go_topofstack() in the cgo code, and SWIG could call it too. There must
> be
> > some magic linker trick with weak symbols that SWIG could use to supply a
> > dummy version (return 0 would be fine) for older Go versions. Or we could
> > provide an initialized char* (*_go_topofstack)(void), and SWIG could
> declare
> > a common symbol for that too, and it would just be zero if you had an
> older
> > Go.
>
> SGTM
>
> Just the function should be sufficient.  I was actually thinking of
> calling a function but was getting hung up on the fact that old
> versions wouldn't have that function.  I can't believe I didn't think
> of using a stupid linker trick.
>
> Ian
>

[rsc, 2014-09-19 02:38:11]

On Thu, Sep 18, 2014 at 10:33 PM, Keith Randall <khr@google.com> wrote:

> This __go_topofstack function has to be written in gcc code, but it needs
> access to the G register so it can get the top of stack.  Is there any way
> to do that inside gcc, or do we need to use assembly?  Would inline asm
> work or might the G register be clobbered?
>

It can be written in Go assembly as long as it is NOSPLIT to turn off the
prologue and leaves the result in the right place and doesn't have a
center-dot in its name. It pretty much has to be Go assembly, or else it
won't find g the right way.

Russ

[iant, 2014-09-19 14:26:23]

On Thu, Sep 18, 2014 at 7:38 PM, Russ Cox <rsc@golang.org> wrote:
> On Thu, Sep 18, 2014 at 10:33 PM, Keith Randall <khr@google.com> wrote:
>>
>> This __go_topofstack function has to be written in gcc code, but it needs
>> access to the G register so it can get the top of stack.  Is there any way
>> to do that inside gcc, or do we need to use assembly?  Would inline asm work
>> or might the G register be clobbered?
>
>
> It can be written in Go assembly as long as it is NOSPLIT to turn off the
> prologue and leaves the result in the right place and doesn't have a
> center-dot in its name. It pretty much has to be Go assembly, or else it
> won't find g the right way.

I think it can go in runtime/cgo/asm_*.s, along with crosscall2.

Ian

[rsc, 2014-09-24 18:20:55]

Ping. Keith, are you planning to revise this CL?

[khr1, 2014-09-24 20:26:37]

Yes, I'm just fighting with the linker to make my asm stub link correctly.

On Wed, Sep 24, 2014 at 11:20 AM, Russ Cox <rsc@golang.org> wrote:

> Ping. Keith, are you planning to revise this CL?
>
>

[khr, 2014-09-24 23:07:40]

On 2014/09/24 20:26:37, khr1 wrote:
> Yes, I'm just fighting with the linker to make my asm stub link correctly.
> 
> On Wed, Sep 24, 2014 at 11:20 AM, Russ Cox <mailto:rsc@golang.org> wrote:
> 
> > Ping. Keith, are you planning to revise this CL?
> >
> >

PTAL.

Is cgo_topofstack a good name?  Does it need some extra underscores?

[rsc, 2014-09-24 23:23:28]

LGTM

https://codereview.appspot.com/144130043/diff/130001/src/cmd/cgo/out.go
File src/cmd/cgo/out.go (right):

https://codereview.appspot.com/144130043/diff/130001/src/cmd/cgo/out.go#newco...
src/cmd/cgo/out.go:47: fmt.Fprintf(fm, "char* cgo_topofstack(void) { }\n")
The others begin with a leading underscore (_cgo_allocate etc). Let's do that
here too.

https://codereview.appspot.com/144130043/diff/130001/src/runtime/asm_arm.s
File src/runtime/asm_arm.s (right):

https://codereview.appspot.com/144130043/diff/130001/src/runtime/asm_arm.s#ne...
src/runtime/asm_arm.s:1307: MOVW	g_m(g), R0
This is being called from gcc code so it needs to load the g register from TLS.
Insert above this line:

BL runtime·load_g(SB)

[iant, 2014-09-25 00:15:26]

LGTM

https://codereview.appspot.com/144130043/diff/130001/misc/cgo/test/callback.go
File misc/cgo/test/callback.go (right):

https://codereview.appspot.com/144130043/diff/130001/misc/cgo/test/callback.g...
misc/cgo/test/callback.go:256: r = C.int(f(1024))
Not important, but 1024 might be overkill here.  I just dropped it in because I
was sure it would force a stack copy.  Even with 8K stacks we should get a stack
copy if this is 64.

[khr, 2014-09-25 14:59:08]

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

cgo: adjust return value location to account for stack copies.

During a cgo call, the stack can be copied.  This copy invalidates
the pointer that cgo has into the return value area.  To fix this
problem, pass the address of the location containing the stack
top value (which is in the G struct).  For cgo functions which
return values, read the stktop before and after the cgo call to
compute the adjustment necessary to write the return value.

Fixes issue 8771

LGTM=iant, rsc
R=iant, rsc, khr
CC=golang-codereviews
https://codereview.appspot.com/144130043