code review 6490056: runtime: increase stack frame during cgo call on window...

runtime: increase stack frame during cgo call on windows/amd64

Fixes issue 3945.

[brainman, 2012-08-30 05:11:23]

Hello golang-dev@googlegroups.com (cc: minux.ma@gmail.com, vcc.163@gmail.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2012-08-30 05:14:25]

While debugging:

package main

// #include <stdio.h>
//
// void say() {
//    printf("%s from C\n", "hello");
// }
//
import "C"

func main() {
	C.say()
}

on windows/amd64, I have discovered that stack is getting smashed during cgo
call.

What I see is:
- we allocate 16 bytes on stack before calling gcc compiled say function;
- say function is in-lined and does not use stack, just fiddles with some
registers and makes couple of jumps;
- then we end-up in printf msvcrt.dll;
- first thing ms printf does is saves 4 "fast-call" registers in the
stack-backed area that printf caller is supposed to reserve for it:

(gdb) x/10i $rip
=> 0x7fefe6a79b8 <printf>:      mov    %rsp,%rax
   0x7fefe6a79bb <printf+3>:    mov    %rcx,0x8(%rax)
   0x7fefe6a79bf <printf+7>:    mov    %rdx,0x10(%rax)
   0x7fefe6a79c3 <printf+11>:   mov    %r8,0x18(%rax)
   0x7fefe6a79c7 <printf+15>:   mov    %r9,0x20(%rax)
   0x7fefe6a79cb <printf+19>:   push   %rbx
   0x7fefe6a79cc <printf+20>:   push   %rsi
   0x7fefe6a79cd <printf+21>:   sub    $0x38,%rsp
   0x7fefe6a79d1 <printf+25>:   xor    %eax,%eax
   0x7fefe6a79d3 <printf+27>:   test   %rcx,%rcx
(gdb)

- we reserved room for 2 registers only, so printf overwrites our stack where
return SP is stored, so that is the end of road for our process.

I am not sure what is the right way to fix it. I suppose, since printf is called
by say function, it should be up to gcc to arrange stack. On the other hand I do
not know what call convention is for gcc compiled functions. So, perhaps our
call into that is wrong too. Looking at our runtime.asmcgocall, I do not
understand why we reserve 16 bytes (http://codereview.appspot.com/4826041/), not
more, not less.

Meanwhile, I have decided to increase this stack space to fit all 4 registers.
This solves printf issue. I can't see it hurt anywhere.

Looking for comments.

Alex

[minux1, 2012-08-30 09:36:24]

http://codereview.appspot.com/6490056/diff/3/misc/cgo/test/issue3945.go
File misc/cgo/test/issue3945.go (right):

http://codereview.appspot.com/6490056/diff/3/misc/cgo/test/issue3945.go#newcode5
misc/cgo/test/issue3945.go:5: package cgotest
add a comment mention issue 3945?

http://codereview.appspot.com/6490056/diff/3/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

http://codereview.appspot.com/6490056/diff/3/src/pkg/runtime/asm_amd64.s#newc...
src/pkg/runtime/asm_amd64.s:487: // Now on a scheduling stack (a pthread-created
stack).
add a comment explaining the stack layout?

[minux1, 2012-08-30 09:41:27]

On Thu, Aug 30, 2012 at 1:14 PM, <alex.brainman@gmail.com> wrote:
>
> What I see is:
> - we allocate 16 bytes on stack before calling gcc compiled say
> function;
> - say function is in-lined and does not use stack, just fiddles with
> some registers and makes couple of jumps;
> - then we end-up in printf msvcrt.dll;
> - first thing ms printf does is saves 4 "fast-call" registers in the
> stack-backed area that printf caller is supposed to reserve for it:
> - we reserved room for 2 registers only, so printf overwrites our stack
> where return SP is stored, so that is the end of road for our process.
>
> I am not sure what is the right way to fix it. I suppose, since printf
> is called by say function, it should be up to gcc to arrange stack. On
>
this is the correct fix.
windows amd64 calling convention demands that caller leave 4*8 byte
of space (no need to initialize them however) on stack for callee to save
the 4 arguments passed in registers.

> the other hand I do not know what call convention is for gcc compiled
>
mingw gcc should use native windows amd64 calling convention.

> functions. So, perhaps our call into that is wrong too. Looking at our
> runtime.asmcgocall, I do not understand why we reserve 16 bytes
>
(http://codereview.appspot.**com/4826041/<http://codereview.appspot.com/4826041/>),
> not more, not less.
>

[brainman, 2012-08-31 02:36:08]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com, vcc.163@gmail.com),

Please take another look.

[brainman, 2012-08-31 02:36:15]

Thank you for comments.

http://codereview.appspot.com/6490056/diff/3/misc/cgo/test/issue3945.go
File misc/cgo/test/issue3945.go (right):

http://codereview.appspot.com/6490056/diff/3/misc/cgo/test/issue3945.go#newcode5
misc/cgo/test/issue3945.go:5: package cgotest
On 2012/08/30 09:36:24, minux wrote:
> add a comment mention issue 3945?

Done.

http://codereview.appspot.com/6490056/diff/3/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

http://codereview.appspot.com/6490056/diff/3/src/pkg/runtime/asm_amd64.s#newc...
src/pkg/runtime/asm_amd64.s:487: // Now on a scheduling stack (a pthread-created
stack).
On 2012/08/30 09:36:24, minux wrote:
> add a comment explaining the stack layout?

Done.

[minux1, 2012-08-31 07:15:50]

LGTM.

strangely, i can't find official document on this issue
(x64 convention page only mentions the so-called shadow space once,)

but it seems our code does have mentioned this fact:
http://code.google.com/p/go/source/browse/src/pkg/runtime/windows/amd64/sys.s...

http://codereview.appspot.com/6490056/diff/9001/misc/cgo/test/issue3945.go
File misc/cgo/test/issue3945.go (right):

http://codereview.appspot.com/6490056/diff/9001/misc/cgo/test/issue3945.go#ne...
misc/cgo/test/issue3945.go:8: // See
http://code.google.com/p/go/issues/detail?id=3945 for details.
we can just use http://golang.org/issue/3945

[brainman, 2012-09-03 00:50:56]

On 2012/08/31 07:15:50, minux wrote:
> 
> strangely, i can't find official document on this issue
> (x64 convention page only mentions the so-called shadow space once,)
> 

Some of my bookmarks on these:

http://blogs.msdn.com/b/oldnewthing/archive/2004/01/14/58579.aspx

http://blogs.msdn.com/b/freik/archive/2005/03/17/398200.aspx

http://www.ntcore.com/files/vista_x64.htm

http://msdn.microsoft.com/en-us/magazine/cc300794.aspx

Alex

[brainman, 2012-09-03 02:12:58]

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

runtime: increase stack frame during cgo call on windows/amd64

Fixes issue 3945.

R=golang-dev, minux.ma
CC=golang-dev, vcc.163
http://codereview.appspot.com/6490056

http://codereview.appspot.com/6490056/diff/9001/misc/cgo/test/issue3945.go
File misc/cgo/test/issue3945.go (right):

http://codereview.appspot.com/6490056/diff/9001/misc/cgo/test/issue3945.go#ne...
misc/cgo/test/issue3945.go:8: // See
http://code.google.com/p/go/issues/detail?id=3945 for details.
On 2012/08/31 07:15:50, minux wrote:
> we can just use http://golang.org/issue/3945

Done.