code review 1381041: switch to os stack during windows Syscall()

switch to os stack during windows Syscall()

[brainman, 2010-05-28 04:39:54]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc1, 2010-06-03 01:00:42]

http://codereview.appspot.com/1381041/diff/2001/3001
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/1381041/diff/2001/3001#newcode16
src/pkg/runtime/windows/386/sys.s:16: // ADDED(brainman): It should be ok now as
all calls from go,
s/ADDED/NOTE/
s/,$//

same in next comment.
next line, s/c/C/

Can we just fix the calls in thread.c and mem.c too?

http://codereview.appspot.com/1381041/diff/2001/3002
File src/pkg/runtime/windows/os.h (right):

http://codereview.appspot.com/1381041/diff/2001/3002#newcode26
src/pkg/runtime/windows/os.h:26: typedef	struct	stdcall_params	stdcall_params;
In the runtime library structs have capital names:

StdcallParams

or something like that.

[brainman, 2010-06-04 03:08:21]

> Can we just fix the calls in thread.c and mem.c too?
> 

Would love to. In fact we have to now that I see my program crashing with
corrupted memory again. I have disabled write() function in thread.c and
everything is good again. So, I channeled all stcall in write() via test_stdcall
instead, like so:

#pragma textflag 7                           
void *                                       
test_stdcall(void *fn, ...)                  
{                                            
        uintptr *arg;                        
        stdcall_params p;                    
                                             
        arg = (uintptr*)(((uintptr*)&fn)+1); 
        p.f = fn;                            
        p.args[0] = arg[0];                  
        p.args[1] = arg[1];                  
        p.args[2] = arg[2];                  
        p.args[3] = arg[3];                  
        p.args[4] = arg[4];                  
        onosstack(call_stdcall, &p);         
        return (void*)(p.r);                 
}                                            

but it doesn't help - it prints for a while, and then crashes like before.

I know, I'll work it out myself in 500 years, but I'm lazy. When in go, runtime
makes sure we have enough stack to run, but what happens once you're inside
runtime C code. How does runtime make sure there is enough stack to run runtime
C code, all function calls and stack variables and stuff?


Alex

[brainman, 2010-06-04 07:06:24]

Please, ignore my last message.


Alex

[brainman, 2010-06-07 06:13:50]

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

Please take another look.

[rsc1, 2010-06-07 20:04:50]

looks pretty good.
i think the count is important.

http://codereview.appspot.com/1381041/diff/14001/15003
File src/pkg/runtime/windows/os.h (right):

http://codereview.appspot.com/1381041/diff/14001/15003#newcode25
src/pkg/runtime/windows/os.h:25: // and swtich to os stack during the call.
switch

http://codereview.appspot.com/1381041/diff/14001/15005
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1381041/diff/14001/15005#newcode268
src/pkg/runtime/windows/thread.c:268: stdcall(void *fn, ...)
I think there should be a count here.

void *fn, int count, ...

and then only copy count args.
In general it's not safe to read past the arguments:
there might not be a page mapped if you read too far,
and there are getting to be a lot of arguments.

[brainman, 2010-06-08 01:19:04]

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

Please take another look.

[brainman, 2010-06-08 01:19:23]

http://codereview.appspot.com/1381041/diff/14001/15003
File src/pkg/runtime/windows/os.h (right):

http://codereview.appspot.com/1381041/diff/14001/15003#newcode25
src/pkg/runtime/windows/os.h:25: // and swtich to os stack during the call.
On 2010/06/07 20:04:51, rsc1 wrote:
> switch
> 

Done.

http://codereview.appspot.com/1381041/diff/14001/15005
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1381041/diff/14001/15005#newcode268
src/pkg/runtime/windows/thread.c:268: stdcall(void *fn, ...)
On 2010/06/07 20:04:51, rsc1 wrote:
> I think there should be a count here.
> 
> void *fn, int count, ...
> 
> and then only copy count args.
> In general it's not safe to read past the arguments:
> there might not be a page mapped if you read too far,
> and there are getting to be a lot of arguments.
> 

Done.

[brainman, 2010-06-10 05:58:02]

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

Please take another look.

[rsc1, 2010-06-10 06:05:17]

looks good

http://codereview.appspot.com/1381041/diff/25001/26004
File src/pkg/runtime/windows/syscall.goc (right):

http://codereview.appspot.com/1381041/diff/25001/26004#newcode11
src/pkg/runtime/windows/syscall.goc:11: ·entersyscall();
Just to keep them visible, please move the entersyscall/exitsyscall
calls right next to the syscall(&p) call.
All the packing and unpacking can be done before/after.

http://codereview.appspot.com/1381041/diff/25001/26005
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1381041/diff/25001/26005#newcode274
src/pkg/runtime/windows/thread.c:274: while (count) {
no space after while (or if or for)

http://codereview.appspot.com/1381041/diff/25001/26005#newcode274
src/pkg/runtime/windows/thread.c:274: while (count) {
count > 0, just to be safe

[brainman, 2010-06-10 06:30:53]

http://codereview.appspot.com/1381041/diff/25001/26004
File src/pkg/runtime/windows/syscall.goc (right):

http://codereview.appspot.com/1381041/diff/25001/26004#newcode11
src/pkg/runtime/windows/syscall.goc:11: ·entersyscall();
On 2010/06/10 06:05:17, rsc1 wrote:
> Just to keep them visible, please move the entersyscall/exitsyscall
> calls right next to the syscall(&p) call.
> All the packing and unpacking can be done before/after.
> 

Done.

http://codereview.appspot.com/1381041/diff/25001/26005
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/1381041/diff/25001/26005#newcode274
src/pkg/runtime/windows/thread.c:274: while (count) {
On 2010/06/10 06:05:17, rsc1 wrote:
> no space after while (or if or for)
> 

Done.

[brainman, 2010-06-10 06:31:24]

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

Please take another look.

[rsc1, 2010-06-11 08:34:58]

LGTM

[rsc, 2010-06-11 08:38:22]

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

runtime: switch to OS stack during Windows syscall

R=rsc
CC=golang-dev
http://codereview.appspot.com/1381041

Committer: Russ Cox <rsc@golang.org>