code review 1696051: runtime: implementation of callback functions for windows

runtime: implementation of callback functions for windows

[brainman, 2010-07-21 04:51:36]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc1, 2010-08-26 16:07:46]

I don't know what to do about this.
Is there something this is driving toward?

[brainman, 2010-08-27 01:04:33]

On 2010/08/26 16:07:46, rsc1 wrote:
> I don't know what to do about this.
> Is there something this is driving toward?

The code is out of date, but the idea here is fine. The outcome is, we should be
able to callback from outside of Go back in. This will allow us to implement
some apis, like create and manipulate gui objects, create windows service apps
and so on.

I was hoping to settle all our existing issues with stdcalls and stacks first,
but, perhaps, I should move forward, this could push things a bit and might give
us more clues.

Alex

[brainman, 2010-09-14 01:59:47]

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

I'd like you to review this change.

[lxn, 2010-09-27 20:30:43]

On 2010/09/14 01:59:47, brainman wrote:
> Hello rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change.

Hi,

I have applied this patch to my local Go installation (release 2010-09-22) and
have started to adjust the Walk library in a separate branch to use callbacks:
http://github.com/lxn/walk/tree/callbacks

There is a drawing example app, which works fine and an image viewer example
that crashes when opening an image file.
The problem seems to be insufficient stack space and does not show up in the
master branch of Walk that uses a helper dll instead of callbacks.

It would be very nice if someone who knows the inner workings of the Go Windows
port could give it a try - and maybe come up with a fix ;)

Working callback support is a game changer for stuff like Walk and I really hope
it will make its way into one of the next Go releases.

Thanks
Alex

[alex.brainman_eventree.com, 2010-09-28 06:16:53]

Hi Alex,


> I have applied this patch to my local Go installation (release
> 2010-09-22) and have started to adjust the Walk library in a separate
> branch to use callbacks:
> http://github.com/lxn/walk/tree/callbacks

I tried to download your source, but I can't see callbacks branch (I'm
not good with git):

git clone http://github.com/lxn/walk.git
cd walk
git branch says: * master

?

Alex

[brainman, 2010-09-29 05:36:28]

On 2010/09/27 20:30:43, lxn wrote:

> The problem seems to be insufficient stack space ...

You're right on the money.

Lucky for you, I've been over this code million times, I didn't have to think
twice <g>.

Windows syscalls run on scheduler stack. The stack is working as all go stacks
do: it is set to fixed size (8k at the moment), it grows, once it reaches its
limit, it spills into new fixed size stack that is created out of malloced
memory. The special thing about scheduler stack is its first 8k section is
layout on real windows stack (it is huge). Also scheduler never uses more than
8k of stack, therefore scheduler stack never split. When windows syscall called,
it is always on real huge windows stack, it can use as much as needed, but once
back from the call, stack is back where it is started before the call.

With callbacks things becomes trickier. If callback occurs during windows
syscall execution, scheduler stack will be smaller by the amount of space used
by syscall. This, I suspect, will lead to the fact that all 8k of first fragment
will get used and scheduler will mallocate new stack section (this time on heap,
not os stack). If go makes another windows syscall now, there is a risk of
windows writing beyond new stack boundary (windows expects one huge stack). This
will write over your heap memory. Lights off.

What you need to do is tell go scheduler, that its stack is bigger then 8k. You
use real os stack here, so size doesn't matter, you could go as big as biggest
possible os stack (1 gig or something).

These changes do just that. Increase go scheduler stack for first os thread in
runtime/386/asm.s:

@@ -44,7 +44,7 @@
 	MOVL	CX, m_g0(AX)
 
 	// create istack out of the OS stack
-	LEAL	(-8192+104)(SP), AX	// TODO: 104?
+	LEAL	(-65536+104)(SP), AX	// TODO: 104?
 	MOVL	AX, g_stackguard(CX)
 	MOVL	SP, g_stackbase(CX)
 	CALL	emptyfunc(SB)	// fault if stack check is wrong

You must also do the same for every new os thread in runtime/windows/386/sys.s:

99c99
< 	SUBL	$8192, AX		// stack size
---
> 	SUBL	$65536, AX		// stack size

It works for me.

Alex

PS: It is much nicer that you have Makefiles now <g>.

[lxn, 2010-09-29 08:24:56]

On 2010/09/29 05:36:28, brainman wrote:
> On 2010/09/27 20:30:43, lxn wrote:
> 
> > The problem seems to be insufficient stack space ...
> 
> You're right on the money.
> 
> Lucky for you, I've been over this code million times, I didn't have to think
> twice <g>.
> 
> Windows syscalls run on scheduler stack. The stack is working as all go stacks
> do: it is set to fixed size (8k at the moment), it grows, once it reaches its
> limit, it spills into new fixed size stack that is created out of malloced
> memory. The special thing about scheduler stack is its first 8k section is
> layout on real windows stack (it is huge). Also scheduler never uses more than
> 8k of stack, therefore scheduler stack never split. When windows syscall
called,
> it is always on real huge windows stack, it can use as much as needed, but
once
> back from the call, stack is back where it is started before the call.
> 
> With callbacks things becomes trickier. If callback occurs during windows
> syscall execution, scheduler stack will be smaller by the amount of space used
> by syscall. This, I suspect, will lead to the fact that all 8k of first
fragment
> will get used and scheduler will mallocate new stack section (this time on
heap,
> not os stack). If go makes another windows syscall now, there is a risk of
> windows writing beyond new stack boundary (windows expects one huge stack).
This
> will write over your heap memory. Lights off.
> 
> What you need to do is tell go scheduler, that its stack is bigger then 8k.
You
> use real os stack here, so size doesn't matter, you could go as big as biggest
> possible os stack (1 gig or something).
> 
> These changes do just that. Increase go scheduler stack for first os thread in
> runtime/386/asm.s:
> 
> @@ -44,7 +44,7 @@
>  	MOVL	CX, m_g0(AX)
>  
>  	// create istack out of the OS stack
> -	LEAL	(-8192+104)(SP), AX	// TODO: 104?
> +	LEAL	(-65536+104)(SP), AX	// TODO: 104?
>  	MOVL	AX, g_stackguard(CX)
>  	MOVL	SP, g_stackbase(CX)
>  	CALL	emptyfunc(SB)	// fault if stack check is wrong
> 
> You must also do the same for every new os thread in
runtime/windows/386/sys.s:
> 
> 99c99
> < 	SUBL	$8192, AX		// stack size
> ---
> > 	SUBL	$65536, AX		// stack size
> 
> It works for me.

Works here as well. This helps me a lot, thanks. Also a nice explanation what's
going on with stacks and callbacks!

Now this just has to make its way into the Go repo :)

> 
> Alex
> 
> PS: It is much nicer that you have Makefiles now <g>.

It was about time. When I introduced them, they immediately uncovered some
errors that I didn't notice before, because some rarely used parts of the lib
weren't built for a while.

Thanks
Alex

[lxn, 2010-09-30 16:09:02]

> > It works for me.
> 
> Works here as well. This helps me a lot, thanks.

Cheered too soon.

Stack overflows still occur after slightly modifying the imageviewer example. I
tried to increase stack size to 1 meg by naively replacing the suggested 65536
with 1048576, but it seems to have no effect.

Sadly I have no experience with x86 assembly, only ever did some 6502 stuff long
time ago, so I am probably wrong, but isn't AX a 16 bit register, so it can't
work with greater values?

Hopefully someone could help with this once again.

Thanks
Alex

[dho, 2010-09-30 22:52:34]

2010/9/30  <an2048@googlemail.com>:
>> > It works for me.
>
>> Works here as well. This helps me a lot, thanks.
>
> Cheered too soon.
>
> Stack overflows still occur after slightly modifying the imageviewer
> example. I tried to increase stack size to 1 meg by naively replacing
> the suggested 65536 with 1048576, but it seems to have no effect.
>
> Sadly I have no experience with x86 assembly, only ever did some 6502
> stuff long time ago, so I am probably wrong, but isn't AX a 16 bit
> register, so it can't work with greater values?

I think the deal is that you just use AX / BX / etc as necessary, and
the "proper" register is determined based on the instruction used.
E.g. MOVL $1234, AX and MOVB $1, AX will generate different machine
code, but you don't have to think about what permutation of register
you are supposed to be using. So that's not likely the issue.

--dho

> Hopefully someone could help with this once again.
>
> Thanks
> Alex
>
> http://codereview.appspot.com/1696051/
>

[rsc, 2010-10-01 04:35:31]

> I think the deal is that you just use AX / BX / etc as necessary, and
> the "proper" register is determined based on the instruction used.
> E.g. MOVL $1234, AX and MOVB $1, AX will generate different machine
> code, but you don't have to think about what permutation of register
> you are supposed to be using. So that's not likely the issue.

Yes: the size is given in the instruction, not by a prefix on the
register name.

Russ

[brainman, 2010-10-01 07:30:46]

On 2010/09/30 16:09:02, lxn wrote:
> 
> Cheered too soon.
> 
> Stack overflows still occur after slightly modifying the imageviewer example.
I

Now that I have checked some variables in windows/thread.c/callback(), I think I
have a problem, because I'm not switching on OS stack, but small 4k stack in
m->curg. I'll just have to learn more about scheduler <g>.

Alex

[brainman, 2010-10-04 00:20:14]

On 2010/10/01 07:30:46, brainman wrote:

It looks like we need to make stack splitting code work across syscall.

Russ,

From old CL #834045:

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)

If you give me an idea of what to do, I'll have a go.


Alex

[brainman, 2011-01-11 04:14:14]

Hello rsc, lxn, alex.brainman@eventree.com, dho (cc:
golang-dev@googlegroups.com),

I'd like you to review this change.

[brainman, 2011-01-11 04:18:05]

On 2010/09/30 16:09:02, lxn wrote:
> Hopefully someone could help with this once again.
> 

We're getting nowhere, so I looked again and these small changes will let me
overcome your problem:

1) Disable check for "g stack overflow in callback" altogether. Perhaps will
come back to it, but for now I can see the stack gets beyond stackguard just by
20-40 bytes. I hope it is enough to survive.

2) Marked runtime.callback() as "#pragma textflag 7" (thanks to czaplinski),
this function is running on os stack and shouldn't attempt to allocate more
stack.

3) Increase (double) size of scheduler stack. All callbacks are happening on
scheduler stack and 8k is not enough - I can see "morestack()" gets called from
startcgocallback() and it shouldn't. Both initial and every new os thread stacks
have been increased.

Alex

[rsc, 2011-01-11 16:43:58]

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/Makefile
File src/pkg/exp/wingui/Makefile (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/Makefile#...
src/pkg/exp/wingui/Makefile:1: # Copyright 2009 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/gui.go
File src/pkg/exp/wingui/gui.go (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/gui.go#ne...
src/pkg/exp/wingui/gui.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/winapi.go
File src/pkg/exp/wingui/winapi.go (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/winapi.go...
src/pkg/exp/wingui/winapi.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
2011

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:51: // We came here from small function that
was dynamically created in
// Called from dynamic function created by ../thread.c setcallback,
// running on Windows stack (not Go stack).
// Returns straight to DLL.
// On entry to the function the stack looks like:
//
//   0(SP) - return address to callback
//   4(SP) - address of go func we need to call
//   8(SP) - total size of arguments
//   12(SP) - return address to DLL
//   16(SP) - beginning of arguments

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:64: MOVL	SP, AX			// Address to argument list
on stack.
Can simplify some of the assembly and
at the same time introduce names to avoid
some of the comments.

LEAL args+16(SP), AX
MOVL AX, 0(SP)
CLD
CALL runtime·callback(SB)

MOVL ret+12(SP), CX
MOVL size+8(SP), BX
ADDL $16, BX
ADDL BX, SP
JMP CX

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/sysc...
File src/pkg/runtime/windows/syscall.goc (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/sysc...
src/pkg/runtime/windows/syscall.goc:30: func setCallback(code *byte, fn uintptr,
argsize uint32) {
same (makeCallback, compileCallback)

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:197: runtime·setcallback(byte *code, void *fn,
uint32 argsize)
setcallback sounds like it is registering the
callback function.  This is makecallback or
compilecallback.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:223: Gobuf oldsched, oldsched_g1;
msched, g1sched

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:225: void *sp, *oldsp;
s/oldsp/gostack/

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:245: //	if(sp < g1->stackguard)
The actual check should be 

enum { StackGuard = 256 };  // defined in proc.c

if(sp < g1->stackguard - StackGuard + 4)  // +4 for return address

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:248: uint32 *p = sp;
please declare variables at top, without initialization.
also it would make more sense to do

void **p;

and then here

*p++ = arg;

and then below

return *p;

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:248: uint32 *p = sp;
declare void **p at top of function.

p = sp;
p[0] = arg;

below

return p[1];

[brainman, 2011-01-12 04:26:37]

Hello rsc, lxn, alex.brainman@eventree.com, dho (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-01-12 04:26:47]

Thanks for reveiw.

Just want to make sure, you're OK with increase size of initial scheduler stack
to 16K. I'm surprused, no one had problems overflowing it with CGO callbacks.
All other scheduler stacks are huge with CGO, but not first.

Alex

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/Makefile
File src/pkg/exp/wingui/Makefile (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/Makefile#...
src/pkg/exp/wingui/Makefile:1: # Copyright 2009 The Go Authors. All rights
reserved.
On 2011/01/11 16:43:59, rsc wrote:
> 2011

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/gui.go
File src/pkg/exp/wingui/gui.go (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/gui.go#ne...
src/pkg/exp/wingui/gui.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
On 2011/01/11 16:43:59, rsc wrote:
> 2011

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/winapi.go
File src/pkg/exp/wingui/winapi.go (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/exp/wingui/winapi.go...
src/pkg/exp/wingui/winapi.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
On 2011/01/11 16:43:59, rsc wrote:
> 2011

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:51: // We came here from small function that
was dynamically created in
On 2011/01/11 16:43:59, rsc wrote:
> // Called from dynamic function created by ../thread.c setcallback,
> // running on Windows stack (not Go stack).
> // Returns straight to DLL.
> // On entry to the function the stack looks like:
> //
> //   0(SP) - return address to callback
> //   4(SP) - address of go func we need to call
> //   8(SP) - total size of arguments
> //   12(SP) - return address to DLL
> //   16(SP) - beginning of arguments

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:64: MOVL	SP, AX			// Address to argument list
on stack.
On 2011/01/11 16:43:59, rsc wrote:
> Can simplify some of the assembly and
> at the same time introduce names to avoid
> some of the comments.
> 
> LEAL args+16(SP), AX
> MOVL AX, 0(SP)
> CLD
> CALL runtime·callback(SB)
> 
> MOVL ret+12(SP), CX
> MOVL size+8(SP), BX
> ADDL $16, BX
> ADDL BX, SP
> JMP CX
> 

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/sysc...
File src/pkg/runtime/windows/syscall.goc (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/sysc...
src/pkg/runtime/windows/syscall.goc:30: func setCallback(code *byte, fn uintptr,
argsize uint32) {
On 2011/01/11 16:43:59, rsc wrote:
> same (makeCallback, compileCallback)

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:197: runtime·setcallback(byte *code, void *fn,
uint32 argsize)
On 2011/01/11 16:43:59, rsc wrote:
> setcallback sounds like it is registering the
> callback function.  This is makecallback or
> compilecallback.

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:223: Gobuf oldsched, oldsched_g1;
On 2011/01/11 16:43:59, rsc wrote:
> msched, g1sched

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:225: void *sp, *oldsp;
On 2011/01/11 16:43:59, rsc wrote:
> s/oldsp/gostack/

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:245: //	if(sp < g1->stackguard)
On 2011/01/11 16:43:59, rsc wrote:
> The actual check should be 
> 
> enum { StackGuard = 256 };  // defined in proc.c
> 
> if(sp < g1->stackguard - StackGuard + 4)  // +4 for return address

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:248: uint32 *p = sp;
On 2011/01/11 16:43:59, rsc wrote:
> declare void **p at top of function.
> 
> p = sp;
> p[0] = arg;
> 
> below
> 
> return p[1];

Done.

http://codereview.appspot.com/1696051/diff/43001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:248: uint32 *p = sp;
On 2011/01/11 16:43:59, rsc wrote:
> please declare variables at top, without initialization.
> also it would make more sense to do
> 
> void **p;
> 
> and then here
> 
> *p++ = arg;
> 
> and then below
> 
> return *p;
> 
> 

Done.

[rsc, 2011-01-13 18:21:44]

Looks pretty good.  I think you should address the sys.s TODO now.

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:62: // TODO(brainman): preserve and restore
EBX, ESI, EDI and
I missed this TODO before.
This is pretty important: I think you'll get very
hard to debug crashes otherwise.

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:235: byte *p = code;
byte *p;

p = code;

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:279: sp = g1->sched.sp - 8;		// our go function
will have one input param (pointer to args) and one return value
can shorten this by writing

sp = g1->sched.sp - 4 - 4;  // one input, one output

[brainman, 2011-01-20 11:45:29]

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:62: // TODO(brainman): preserve and restore
EBX, ESI, EDI and
On 2011/01/13 18:21:44, rsc wrote:
> I missed this TODO before.

You didn't. It wasn't here until last change. I put it in the last minute, after
looking at http://codereview.appspot.com/3822041/.

> This is pretty important: I think you'll get very
> hard to debug crashes otherwise.

I've implemented it now.

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:235: byte *p = code;
On 2011/01/13 18:21:44, rsc wrote:
> byte *p;
> 
> p = code;

Done.

[brainman, 2011-01-20 11:45:51]

Hello rsc, lxn, alex.brainman@eventree.com, dho (cc:
golang-dev@googlegroups.com),

I'd like you to review this change.

[lxn, 2011-01-21 11:28:38]

I applied your latest changes to release.2011-01-20 and checked the examples
from the Walk[1] library and the only problem I still see is a crasher when the
WebView widget is involved, which is the case in the filebrowser and webbrowser
examples.

The WebView widget embeds the IE control which requires the use of COM. I
wouldn't be surprised if the problem is in the COM handling code of Walk, but
with a Go runtime patch from Mateusz Czapliński the WebView worked. Only when I
later added code for WebView event handling, it stopped working with his patch.

Maybe this has to be dealt with in Walk, so don't let it hold up the commit ;)

[1] https://github.com/lxn/walk

Cheers
Alex

[rsc, 2011-01-21 16:35:41]

LGTM

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:279: sp = g1->sched.sp - 8;		// our go function
will have one input param (pointer to args) and one return value
On 2011/01/13 18:21:44, rsc wrote:
> can shorten this by writing
> 
> sp = g1->sched.sp - 4 - 4;  // one input, one output

missed this?

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:107: // 24(SP) - return address to DLL
This used to be 12(SP).  How did you change it?

[hector, 2011-01-22 00:39:38]

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:301: enum { StackGuard = 256 }; // defined in
proc.c
StackGuard is now equal to 2048. Not sure how this extra space affects the
tradeoffs, if any.

[brainman, 2011-01-22 02:31:10]

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/55001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:279: sp = g1->sched.sp - 8;		// our go function
will have one input param (pointer to args) and one return value
On 2011/01/21 16:35:42, rsc wrote:
> missed this?

I did indeed. Done.

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/386/...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/386/...
src/pkg/runtime/windows/386/sys.s:107: // 24(SP) - return address to DLL
On 2011/01/21 16:35:42, rsc wrote:
> This used to be 12(SP).  How did you change it?

I subtracted 12 from SP in thread.c/compilecallback to leave room to save BX, SI
and DI. Therefore everything beyond needs to be reference by +12:

12(SP) - return address to DLL + 12 = 24(SP)
16(SP) - beginning of arguments + 12 = 28(SP)

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/thre...
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1696051/diff/72001/src/pkg/runtime/windows/thre...
src/pkg/runtime/windows/thread.c:301: enum { StackGuard = 256 }; // defined in
proc.c
On 2011/01/22 00:39:39, hector wrote:
> StackGuard is now equal to 2048. Not sure how this extra space affects the
> tradeoffs, if any.

Set StackGuard to 2048. I think we should get rid of duplicate StackGuard
definitions. Perhaps in another CL, it is not important here. On the other hand,
I didn't look at your change, but I don't like to have StackGuard different for
windows. I'll look at your change when I have a chance.

Thanks for review.

[brainman, 2011-01-22 02:31:25]

Hello rsc, lxn, alex.brainman@eventree.com, dho (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-01-22 02:56:05]

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

runtime: implementation of callback functions for windows

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

Committer: Alex Brainman <alex.brainman@gmail.com>