code review 4058046: windows: multiple improvements and cleanups

windows: multiple improvements and cleanups

The callback mechanism has been made more flexible.
Eliminated one round of argument copying in Syscall.
Faster Get/SetLastError implemented.
Added gettime for gc perf profiling.

[hector, 2011-01-30 16:13:28]

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

I'd like you to review this change.

[rsc, 2011-01-30 17:19:21]

Sorry, but variadic Syscall is a no-go.
Calling a variadic function makes a slice of argument results
available to the callee, which right now induces a memory
allocation in case the callee keeps a reference to the slice
after it returns.  We don't want every system call to require
a memory allocation, so we're stuck with the current interface.
I plan to work on doing a little bit of cross-package analysis
to eliminate this kind of allocation where possible, and then
we could certainly look into simplifying the API as you've done.

Russ

[hector, 2011-01-30 20:03:19]

PTAL

Reinstated the Syscall<n> variants but added to each a parameter specifying how
many meaningful arguments are provided.

[rsc, 2011-01-30 22:04:10]

looks fine to me.
leaving for alex.

[mattn, 2011-01-31 00:33:52]

LGTM but it's drastic for me.

On 2011/01/30 22:04:10, rsc wrote:
> looks fine to me.
> leaving for alex.

[brainman, 2011-01-31 11:38:02]

Thanks for fixing all that. I like your new NewCallback function.

Too many things are happening at once. Please, next time put them all into
separate CLs.

This change will break some programs that use syscall.Syscall function directly.
But, I suppose, we better do it now, then later.

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (left):

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/cgocall.c#ol...
src/pkg/runtime/cgocall.c:70: runtime·startcgocallback(g1);
Shouldn't startcgocallback be #pragma textflag 7 too? And all others that are
called from here?

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/mem.c
File src/pkg/runtime/windows/mem.c (right):

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/mem....
src/pkg/runtime/windows/mem.c:23: errno = runtime·getlasterror();
I wouldn't mind if you get rid of abort function altogether. Just put a throw
with nice error message, don't need to use getlasterror here.

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/os.h
File src/pkg/runtime/windows/os.h (right):

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/os.h...
src/pkg/runtime/windows/os.h:29: void runtime·callbackasm(void);
Please, change it back. I know, it doesn't matter, but callbackasm must return
result (in AX). I would even put a comment in asm about that, so no one would
touch AX.

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

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/sysc...
src/pkg/runtime/windows/syscall.goc:10: handle =
(uint32)runtime·stdcall(runtime·LoadLibraryEx, 3, filename, nil, 0);
Shouldn't you call syscall() instead? stdcall() doesn't have entersyscall /
exitsyscall brackets.

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/sysc...
src/pkg/runtime/windows/syscall.goc:14: proc =
(uintptr)runtime·stdcall(runtime·GetProcAddress, 2, handle, procname);
Same as above.

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/windows/sysc...
src/pkg/runtime/windows/syscall.goc:24: r1 = runtime·syscall((void*)fn, nargs,
&a1, &err);
I can't see problem with stack splitting code here, but I've been burned enough
<g>. Russ?

http://codereview.appspot.com/4058046/diff/11019/src/pkg/syscall/syscall_wind...
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/4058046/diff/11019/src/pkg/syscall/syscall_wind...
src/pkg/syscall/syscall_windows.go:101: // interoperating with Windows code
requiring callbacks.
// implemented in ../pkg/runtime/windows/syscall.cgo

[rsc, 2011-01-31 17:46:33]

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/cgocall.c#ne...
src/pkg/runtime/cgocall.c:57: #pragma textflag 7
I don't think this is right.
#pragma textflag 7 is for functions that might be called
in a context where the stack overflow check is not appropriate,
either because they use next to no stack (not true here) or they
are called in contexts where the stack boundaries are not 
correct (should not be true here) or the calling convention would
be broken by a stack split (should not be true here).

This function is called from 386/asm.s's runcgocallback, which
makes sure that everything is set up right for Go code to run.

Is Windows doing something different?

[hector, 2011-01-31 22:15:09]

PTAL

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

http://codereview.appspot.com/4058046/diff/11019/src/pkg/runtime/cgocall.c#ne...
src/pkg/runtime/cgocall.c:57: #pragma textflag 7
Good point.  I had blindly copied this from the other one in thread.c without
thinking too much about it.

[brainman, 2011-01-31 23:52:31]

On 2011/01/31 17:46:33, rsc wrote:
> ... they
> are called in contexts where the stack boundaries are not 
> correct (should not be true here) ...

I think it is kind of this case, because we're running on os/scheduler stack
here, so if we try to execute morestack it will fail because it doesn't grow
scheduler stack, but our os stack is big enough so it will never (supposed to)
call morestack anyway.

> This function is called from 386/asm.s's runcgocallback, which
> makes sure that everything is set up right for Go code to run.

runtime.cgocallback is not called from runcgocallback. From life example:

#0  runtime.cgocallback (fn=void, arg=void, argsize=void)      
    at /root/hg/go/src/pkg/runtime/cgocall.c:66                
#1  0x08064ac5 in _cgoexp_da50d9b88261_GoStart (a=void, n=void)
    at /root/hg/go/misc/cgo/life/_cgo_defun.c:54               
#2  0x0807ec9a in crosscall2 ()                                
#3  0x08064ffd in GoStart ()                                   
#4  0x08064d20 in Step ()                                      
#5  0x08064eee in _cgo_da50d9b88261_Cfunc_Step ()              
#6  0x08049ae8 in runtime.runcgo (fn=void, arg=void)           
    at /root/hg/go/src/pkg/runtime/386/asm.s:364               
#7  0x00001400 in ?? ()                                        

> Is Windows doing something different?

Windows is the same, runtime.cgocallback is called by runtime·callbackasm that
just setups parameters properly, nothing else. runtime·callbackasm is called by
Windows itself.

Alex

[brainman, 2011-02-01 02:23:48]

LGTM. But I'll wait for Russ to OK again.

[rsc, 2011-02-01 13:27:07]

looks good; please make the small changes below
and I will submit.

http://codereview.appspot.com/4058046/diff/7018/src/pkg/runtime/windows/mem.c
File src/pkg/runtime/windows/mem.c (right):

http://codereview.appspot.com/4058046/diff/7018/src/pkg/runtime/windows/mem.c...
src/pkg/runtime/windows/mem.c:45: runtime·throw("runtime: failure to release
pages");
s/failure/failed/

http://codereview.appspot.com/4058046/diff/7018/src/pkg/runtime/windows/thread.c
File src/pkg/runtime/windows/thread.c (right):

http://codereview.appspot.com/4058046/diff/7018/src/pkg/runtime/windows/threa...
src/pkg/runtime/windows/thread.c:292: if(!(f =
runtime·findfunc((uintptr)fn.data)))
Please use if((f =...) == nil) for pointer comparisons.
It's harder to misread.

[rsc, 2011-02-01 13:27:28]

>> are called in contexts where the stack boundaries are not
>> correct (should not be true here) ...
>
> I think it is kind of this case, because we're running on os/scheduler
> stack here, so if we try to execute morestack it will fail because it
> doesn't grow scheduler stack, but our os stack is big enough so it will
> never (supposed to) call morestack anyway.

Okay, but that's not a reason to put #pragma textflag 7 on every
function that gets called on the scheduler stack.  The stack bound
for the scheduler stack should just be set appropriately.

>> This function is called from 386/asm.s's runcgocallback, which
>> makes sure that everything is set up right for Go code to run.
>
> runtime.cgocallback is not called from runcgocallback. From life
> example:

Oh.  Well now I can explain issue 1328.  I will fix that in a
different CL.  Thanks.

Russ

[rog, 2011-02-01 13:47:31]

On 1 February 2011 13:27, Russ Cox <rsc@golang.org> wrote:
>> runtime.cgocallback is not called from runcgocallback. From life
>> example:
>
> Oh.  Well now I can explain issue 1328.  I will fix that in a
> different CL.  Thanks.

hurrah!

[hector, 2011-02-01 15:59:44]

PTAL

[rsc, 2011-02-01 16:28:58]

LGTM

[rsc, 2011-02-01 16:49:28]

*** Submitted as 1fef4a79089a ***

windows: multiple improvements and cleanups

The callback mechanism has been made more flexible.
Eliminated one round of argument copying in Syscall.
Faster Get/SetLastError implemented.
Added gettime for gc perf profiling.

R=rsc, brainman, mattn, rog
CC=golang-dev
http://codereview.appspot.com/4058046

Committer: Russ Cox <rsc@golang.org>