code review 4681049: runtime: stdcall_raw stack 16byte align for Win64

runtime: stdcall_raw stack 16byte align for Win64

[vcc, 2011-07-11 14:33:43]

Hello golang-dev@googlegroups.com,

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

[vcc, 2011-07-11 14:39:33]

I think windows 386 should also need this change, m->g0 not set
g_sched+gobuf_sp, only set g_stackbase, please check _rt0_amd64 and
_rt0_386.

2011/7/11  <vcc.163@gmail.com>:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> runtime: stdcall_raw use m->g0->g_stackbase in windows amd64
>
> Please review this at http://codereview.appspot.com/4681049/
>
> Affected files:
>  M src/pkg/runtime/windows/amd64/sys.s
>
>
> Index: src/pkg/runtime/windows/amd64/sys.s
> ===================================================================
> --- a/src/pkg/runtime/windows/amd64/sys.s
> +++ b/src/pkg/runtime/windows/amd64/sys.s
> @@ -19,7 +19,7 @@
>        MOVQ    m_g0(DX), SI
>        CMPQ    g(DI), SI
>        JEQ 3(PC)
> -       MOVQ    (g_sched+gobuf_sp)(SI), SP
> +       MOVQ    g_stackbase(SI), SP
>        MOVQ    SI, g(DI)
>
>        SUBQ    $0x60, SP
>
>
>

[brainman, 2011-07-12 04:44:15]

http://codereview.appspot.com/4681049/diff/5001/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4681049/diff/5001/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:22: MOVQ	g_stackbase(SI), SP
On the other hand, while comparing 386/asm.s 

// create istack out of the OS stack
LEAL    (-64*1024+104)(SP), AX  // TODO: 104?
MOVL    AX, g_stackguard(CX)
MOVL    SP, g_stackbase(CX)

with amd64/asm.s

// create istack out of the given (operating system) stack
LEAQ    (-8192+104)(SP), AX
MOVQ    AX, g_stackguard(CX)
MOVQ    SP, g_stackbase(CX)

I can see that they set different stack sizes.

I think we want amd64 to be 64k too. But I would expect this to fail only if you
use windows callbacks.

http://codereview.appspot.com/4681049/diff/5001/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:22: MOVQ	g_stackbase(SI), SP
I'm sure you're wrong here.

g_stackbase points to the start of our stack, See this line:

MOVL    SP, g_stackbase(CX)

in amd64/asm.s.

Would you like to tell us about the problem you're trying to fix?

[vcc, 2011-07-12 05:23:44]

2011/7/12  <alex.brainman@gmail.com>:
>
>
http://codereview.appspot.com/4681049/diff/5001/src/pkg/runtime/windows/amd64...
> File src/pkg/runtime/windows/amd64/sys.s (right):
>
>
http://codereview.appspot.com/4681049/diff/5001/src/pkg/runtime/windows/amd64...
> src/pkg/runtime/windows/amd64/sys.s:22: MOVQ    g_stackbase(SI), SP
> On the other hand, while comparing 386/asm.s
>
> // create istack out of the OS stack
> LEAL    (-64*1024+104)(SP), AX  // TODO: 104?
> MOVL    AX, g_stackguard(CX)
> MOVL    SP, g_stackbase(CX)
>
> with amd64/asm.s
>
> // create istack out of the given (operating system) stack
> LEAQ    (-8192+104)(SP), AX
> MOVQ    AX, g_stackguard(CX)
> MOVQ    SP, g_stackbase(CX)
>
> I can see that they set different stack sizes.
>
> I think we want amd64 to be 64k too. But I would expect this to fail
> only if you use windows callbacks.
>
>
http://codereview.appspot.com/4681049/diff/5001/src/pkg/runtime/windows/amd64...
> src/pkg/runtime/windows/amd64/sys.s:22: MOVQ    g_stackbase(SI), SP
> I'm sure you're wrong here.
>
> g_stackbase points to the start of our stack, See this line:
>
> MOVL    SP, g_stackbase(CX)
>
> in amd64/asm.s.
>
> Would you like to tell us about the problem you're trying to fix?

I try to fix problem is some syscall fail on windows amd64. for example:
package main
import ( "net")
func main() {
	port, err := net.LookupPort("tcp", "echo")
	println(port, err)
}

net.LookupPort fail.

after debug it, I found these syscall not call on os stack, so I try
set SP to g_stackbase, and run all is ok.

>
> http://codereview.appspot.com/4681049/
>

[brainman, 2011-07-12 05:27:49]

On 2011/07/12 05:23:44, vcc wrote:
> 
> after debug it, I found these syscall not call on os stack, ...

What did you found exactly?

Alex

[vcc, 2011-07-12 05:53:00]

2011/7/12  <alex.brainman@gmail.com>:
> On 2011/07/12 05:23:44, vcc wrote:
>
>> after debug it, I found these syscall not call on os stack, ...
>
> What did you found exactly?

call syscall.GetServByName Segmentation fault:

Program received signal SIGSEGV, Segmentation fault.
0x000007fefbe01a90 in NAPINSP!DllMain () from C:\Windows\system32\NapiNSP.dll
(gdb) info reg
rax            0x7fefbe11000    8791728918528
rbx            0x0      0
rcx            0x7fefbe0e3e0    8791728907232
rdx            0x1      1
rsi            0x2a4ed0 2772688
rdi            0x0      0
rbp            0x3e254b0        0x3e254b0
rsp            0x6f6b8  0x6f6b8
r8             0x6f788  456584
r9             0x0      0
r10            0xcc     204
r11            0x1312c0 1249984
r12            0x7fefbe0e3d0    8791728907216
r13            0x7fefbe11070    8791728918640
r14            0x7fefbe0e2d0    8791728906960
r15            0x3e271a0        65171872
rip            0x7fefbe01a90    0x7fefbe01a90 <NAPINSP!DllMain+1772>
eflags         0x10216  [ PF AF IF RF ]
cs             0x33     51
ss             0x216002b        34996267
ds             0x0      0
es             0x0      0
fs             0x0      0
gs             0x2b0000 2818048
(gdb)

rsp is 0x6f6b8, I don't know why is 0x6f6b8 and where to set this.

> http://codereview.appspot.com/4681049/
>

[brainman, 2011-07-12 06:25:40]

On 2011/07/12 05:53:00, vcc wrote:
> 
> rsp is 0x6f6b8, I don't know why is 0x6f6b8 and where to set this.
> 

It is hard for me to tell now. I don't have 64 bit Windows. You're welcome to
create an issue, someone might help. If you do, please supply a stack trace if
you can get one. Thank you.

Alex

[vcc, 2011-07-12 14:55:03]

PTAL

finally, I found the problem is stack align.

change issue to: runtime: make gosave stack  16 byte align for amd64

[rsc, 2011-07-12 15:47:15]

> finally, I found the problem is stack align.

This can't possibly be correct.  It means that
after a gosave+gogo, the next time through will
have a different SP, which means that none of
the local variables will be in the right stack
position anymore.

I think we need to figure out why SP is wrong on
the call to gosave and fix whatever is causing
that instead.

Russ

[brainman, 2011-07-13 00:22:53]

On 2011/07/12 14:55:03, vcc wrote:
> 
> finally, I found the problem is stack align.
> 

I agree with Russ. Even if it is an alignment problem, gosave is not the right
place to fix it. Perhaps, you could align SP right before you copy windows api
call parameters in stdcall_raw.

Alex

[vcc, 2011-07-13 06:32:45]

2011/7/12 Russ Cox <rsc@golang.org>:
>> finally, I found the problem is stack align.
>
> This can't possibly be correct.  It means that
> after a gosave+gogo, the next time through will
> have a different SP, which means that none of
> the local variables will be in the right stack
> position anymore.
>
> I think we need to figure out why SP is wrong on
> the call to gosave and fix whatever is causing
> that instead.

in _rt_amd64:

        ...

	// start this M
	CALL	runtime·mstart(SB)

only push 8byte return address, so mstart call gosave to save sp is
not 16byte align.

so we can change to:
 	// start this M
+	PUSHQ	$0
 	CALL	runtime·mstart(SB)
+	POPQ	AX

or as Alex suggest make align in stdcall_raw:
--- a/src/pkg/runtime/windows/amd64/sys.s
+++ b/src/pkg/runtime/windows/amd64/sys.s
@@ -20,6 +20,7 @@
 	CMPQ	g(DI), SI
 	JEQ 3(PC)
 	MOVQ	(g_sched+gobuf_sp)(SI), SP
+	ANDQ	$~15, SP
 	MOVQ	SI, g(DI)
 	
 	SUBQ	$0x60, SP

I like the second one.

Wei guangjing

[vcc, 2011-07-13 13:51:47]

PTAL

[rsc, 2011-07-13 18:04:23]

> in _rt_amd64:
>
>        ...
>
>        // start this M
>        CALL    runtime·mstart(SB)
>
> only push 8byte return address, so mstart call gosave to save sp is
> not 16byte align.

This is just the first of many.
The stack frames of individual functions
are not necessarily the right size to ensure
16-byte alignment either.

> or as Alex suggest make align in stdcall_raw:
> --- a/src/pkg/runtime/windows/amd64/sys.s
> +++ b/src/pkg/runtime/windows/amd64/sys.s
> @@ -20,6 +20,7 @@
>        CMPQ    g(DI), SI
>        JEQ 3(PC)
>        MOVQ    (g_sched+gobuf_sp)(SI), SP
> +       ANDQ    $~15, SP
>        MOVQ    SI, g(DI)
>
>        SUBQ    $0x60, SP

Sounds good.  We do this for cgo calls too.

Thanks.
Russ

[rsc, 2011-07-13 18:44:35]

LGTM

[rsc, 2011-07-13 18:44:47]

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

runtime: stdcall_raw stack 16byte align for Win64

R=alex.brainman, rsc
CC=golang-dev
http://codereview.appspot.com/4681049

Committer: Russ Cox <rsc@golang.org>