code review 176066: Ported runtime to Windows.

Ported runtime to Windows.

[hector, 2009-12-13 13:33:17]

Hello rsc,

I'd like you to review the following change.

[rsc, 2009-12-13 22:09:52]

Thanks.

I'm glad you understand all this stuff!
What little I knew once, I had forgotten...

http://codereview.appspot.com/176066/diff/1020/1025
File src/pkg/runtime/mgc0.c (right):

http://codereview.appspot.com/176066/diff/1020/1025#newcode77
src/pkg/runtime/mgc0.c:77: G *gg = g, *g = gg;
It looks like this line is just making a copy of g,
but g is only used once below, so why make a copy?

http://codereview.appspot.com/176066/diff/1020/1025#newcode97
src/pkg/runtime/mgc0.c:97: G *gg = g, *g = gg;
This one too.  If the idea is to avoid the repeated
lookup of g, then I'd rather drop the #ifdef __MINGW__
and say

    G *gg;

    gg = g;

and below change gp != g to gp != gg.

But I'd also like to understand why this is
necessary.  Even though it's in a loop, the
if statement below only executes a single
time for each call to mark.

http://codereview.appspot.com/176066/diff/1020/1029
File src/pkg/runtime/mingw/386/sys.s (right):

http://codereview.appspot.com/176066/diff/1020/1029#newcode15
src/pkg/runtime/mingw/386/sys.s:15: TEXT stdcall(SB),7,$0
Both here and in stdcall_nolock, I'm worried about
the called code assuming it has more stack available
than it really does.  Do you know how much stack
these routines might assume?  Go only guarantees
something like 128 or 256 bytes, which is pretty
tiny by most standards.

In the Unix ports, when runcgo (see 386/asm.s)
must call a standard ELF function, it switches to
the m's stack, which was created by the OS and
is presumably big enough for such calls.

I think that something like that probably needs
to happen here too.

http://codereview.appspot.com/176066/diff/1020/1029#newcode63
src/pkg/runtime/mingw/386/sys.s:63: CALL	CX			// fn()
Before CALL CX, please insert

CALL stackcheck(SB)  // trashes AX

which just double-checks that m and g and SP
all made it to the new goroutine intact.  
(There have been some bugs recently involving
this on FreeBSD and OS X.)

http://codereview.appspot.com/176066/diff/1020/1033
File src/pkg/runtime/mingw/thread.c (right):

http://codereview.appspot.com/176066/diff/1020/1033#newcode20
src/pkg/runtime/mingw/thread.c:20: static void *get_proc_addr2(byte *base, byte
*name)
There are a handful of local C formatting conventions
that aren't followed in this file.
(I'm sorry; I wish we had a gofmt for C.)

1. Newlines before the function name.

    static void*
    get_proc_addr2(...


2. Declarations do not initialize, are all
at the top of the function, and have a
blank line separating them from code.

    byte *pe_header, *exports;
    uint32 entries, *addr, *names, *ordinals, i;

    pe_header = ...
    exports = ...

3. Drop space after keyword for, if, switch.

    for(i=0; i<entries; i++) {

4. The "then" body of an if or else always starts on a new line.

5. Pointer conditions in if are p != nil or p == nil, not p or !p.
(Harder to misread.)

I know it's annoying, but please change the code to
follow these conventions.  This makes it easier for us
to maintain, because it doesn't look so different from
the rest of the runtime C.

Thanks.

http://codereview.appspot.com/176066/diff/1020/1033#newcode127
src/pkg/runtime/mingw/thread.c:127: int32 /*flags*/, int32 /*fd*/, uint32
/*off*/)
If you'd like to keep the names, another way to silence the
warning is to put a line of code below

   USED(prot, flags, fd, off);

http://codereview.appspot.com/176066/diff/1020/1033#newcode167
src/pkg/runtime/mingw/thread.c:167: void lock(Lock *l)
This implementation of lock always does a stdcall.
The implementations on the other systems pick off
the uncontended case with a simple cas.

I'm a little worried about the overhead of doing the
dll switch on every call to lock.  Can we do something
more like the Darwin implementation, which uses
cas for the uncontended case and then falls back on
a semaphore when there is contention?

http://codereview.appspot.com/176066/diff/1020/1033#newcode170
src/pkg/runtime/mingw/thread.c:170: if (!ecs) ecs =
get_proc_addr("kernel32.dll", "EnterCriticalSection");
All these statics should probably be global variables instead.
They can be initialized during osinit.

http://codereview.appspot.com/176066/diff/1020/1033#newcode196
src/pkg/runtime/mingw/thread.c:196: if (!n->event)
I'd like to avoid the kernel here too, when possible.
See ../darwin/thread.c for a semaphore-based implementation.

http://codereview.appspot.com/176066/diff/1020/1034
File src/pkg/runtime/mkasmh.sh (right):

http://codereview.appspot.com/176066/diff/1020/1034#newcode21
src/pkg/runtime/mkasmh.sh:21: case "$GOOS" in
Nice

http://codereview.appspot.com/176066/diff/1020/1035
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/176066/diff/1020/1035#newcode522
src/pkg/runtime/proc.c:522: G *gg = g, *g = gg;
This line still bothers me.

http://codereview.appspot.com/176066/diff/1020/1037
File src/pkg/runtime/string.cgo (right):

http://codereview.appspot.com/176066/diff/1020/1037#newcode72
src/pkg/runtime/string.cgo:72: while (l--) *p++ = *str++;
Usual formatting nits (sorry):

   while(l--)
      *p++ = *str++;

http://codereview.appspot.com/176066/diff/1020/1037#newcode72
src/pkg/runtime/string.cgo:72: while (l--) *p++ = *str++;
There's a bigger point, though.  Since this is a W string,
it should be converted to proper UTF-8 instead of assuming
it only contains ASCII characters.

You should be able to copy and adapt the body of
sliceinttostring below.

[hector, 2009-12-13 22:47:30]

Hi Russ,
As you might have gathered, I've been pretty busy these past couple of
weeks.  Mainly because I started a new job and have a long commute
from Cambridge to London everyday, hence not much time for hacking
during the evenings.
Anyway, that G *gg = g, *g = gg; thing is fairly interesting.  The
patch in 8l was not enough for the case when an extern register
variable was directly accessed without being loaded into a register
first.  It's harder to patch these cases in the linker as I'm not sure
if I'm going to clobber a register when loading the TLS pointer.  It
was easier to patch the C code so the extern register variable was
loaded into a register first.  That's what that funny looking
statement is trying to do.  So yeah, probably best to just write G *gg
= g; and change g to gg.
The point you bring up about stack size when calling Windows DLL
functions is interesting.  I can't seem to find out any info on this
at the moment.  Anyway it makes sense to switch to m's stack just in
case.
Finally, I can probably rework the locking and notification to use cas
and semaphores, I guess the stdcall is too expensive for the common
case.
Anyway, until I'm able to move closer to London I probably won't be
able to do this stuff until next weekend.  So don't think that I've
lost interest in this language!
Thanks,
Hector

[hector, 2009-12-30 21:40:23]

Hi Russ,
Hope you've had a good Christmas!
I've done most of what you've asked, just a bit stuck on what to do
with the stdcall stack issue.  I'm finding it difficult to switch
stacks as I don't know the size of the stdcall arg stack (making it
difficult to switch stacks within the stdcall function and copy the
args over) and it's a bit of a pain to have to manually change the
stack before and after each call to stdcall (as there appears to be no
way to use the preprocessor to automate such a change).  Can we just
assume that the Windows DLL functions don't consume a lot of stack
space?
Thanks,
Hector

2009/12/13 Hector Chu <hectorchu@gmail.com>:
> Hi Russ,
> As you might have gathered, I've been pretty busy these past couple of
> weeks.  Mainly because I started a new job and have a long commute
> from Cambridge to London everyday, hence not much time for hacking
> during the evenings.
> Anyway, that G *gg = g, *g = gg; thing is fairly interesting.  The
> patch in 8l was not enough for the case when an extern register
> variable was directly accessed without being loaded into a register
> first.  It's harder to patch these cases in the linker as I'm not sure
> if I'm going to clobber a register when loading the TLS pointer.  It
> was easier to patch the C code so the extern register variable was
> loaded into a register first.  That's what that funny looking
> statement is trying to do.  So yeah, probably best to just write G *gg
> = g; and change g to gg.
> The point you bring up about stack size when calling Windows DLL
> functions is interesting.  I can't seem to find out any info on this
> at the moment.  Anyway it makes sense to switch to m's stack just in
> case.
> Finally, I can probably rework the locking and notification to use cas
> and semaphores, I guess the stdcall is too expensive for the common
> case.
> Anyway, until I'm able to move closer to London I probably won't be
> able to do this stuff until next weekend.  So don't think that I've
> lost interest in this language!
> Thanks,
> Hector
>

[hector, 2010-01-05 23:13:55]

Hi Russ, PTAL. Thanks

[rsc, 2010-01-06 00:58:21]

looks great
a bunch of small things.

i'm glad you found a way to switch stacks
during stdcall.

http://andi.latest.codereview.appspot.com/176066/diff/2002/2006
File src/pkg/runtime/mgc0.c (right):

http://andi.latest.codereview.appspot.com/176066/diff/2002/2006#newcode76
src/pkg/runtime/mgc0.c:76: G* gg = g;

// TODO(rsc): Change 8g not to assume that extern register
// variables are not directly addressable.  Declaring the
// local variable here works around the bug.
G *gg = g;

http://andi.latest.codereview.appspot.com/176066/diff/2002/2006#newcode93
src/pkg/runtime/mgc0.c:93: G *gp, *gg = g;
same comment (and move G *gg = g to a new line)

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013
File src/pkg/runtime/mingw/thread.c (right):

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode12
src/pkg/runtime/mingw/thread.c:12: void *CloseHandle;
if these are only used in this file please make them static

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode51
src/pkg/runtime/mingw/thread.c:51: void *base = get_kernel_module();
please separate decl and init
and move inits all to top.

void *base;

base = get_kernel_module();

i've marked other instances in this file
with the 1-word comment "style".
sorry about the nits but it keeps the
code consistent across the different
architectures.

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode70
src/pkg/runtime/mingw/thread.c:70: void *base = stdcall(LoadLibraryEx, library,
0, 0);
style

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode85
src/pkg/runtime/mingw/thread.c:85: void *gcl = get_proc_addr("kernel32.dll",
"GetCommandLineW");
style

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode89
src/pkg/runtime/mingw/thread.c:89: uint16 *cmd = stdcall(gcl);
style

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode124
src/pkg/runtime/mingw/thread.c:124: uint32 written = 0;
style

http://andi.latest.codereview.appspot.com/176066/diff/2002/2013#newcode170
src/pkg/runtime/mingw/thread.c:170: void *event = stdcall(CreateEvent, 0, 0, 0,
0);
style

http://andi.latest.codereview.appspot.com/176066/diff/2002/2015
File src/pkg/runtime/proc.c (right):

http://andi.latest.codereview.appspot.com/176066/diff/2002/2015#newcode390
src/pkg/runtime/proc.c:390: G* gg = g;
blank line after this one (style)

http://andi.latest.codereview.appspot.com/176066/diff/2002/2015#newcode522
src/pkg/runtime/proc.c:522: G* gg = g;
style

http://andi.latest.codereview.appspot.com/176066/diff/2002/2018
File src/pkg/runtime/string.cgo (right):

http://andi.latest.codereview.appspot.com/176066/diff/2002/2018#newcode69
src/pkg/runtime/string.cgo:69: for(n=i=0; str[i]; i++)
please move the n=0 above the loop.
same below.

[rsc, 2010-01-06 15:30:27]

> I've done most of what you've asked, just a bit stuck on what to do
> with the stdcall stack issue.  I'm finding it difficult to switch
> stacks as I don't know the size of the stdcall arg stack (making it
> difficult to switch stacks within the stdcall function and copy the
> args over) and it's a bit of a pain to have to manually change the
> stack before and after each call to stdcall (as there appears to be no
> way to use the preprocessor to automate such a change).  Can we just
> assume that the Windows DLL functions don't consume a lot of stack
> space?

I misread your latest code.  I thought you'd added a stack switch
but it was other stack manipulation that I was seeing.
It's fine to get this review done without doing the stack
switch but I think the stack switch is ultimately important.
There could be as little as 128 bytes left on the stack
when the call happens, and I think even Windows DLL
functions are going to consume that much.  The default
stack size on Windows, even for fibers, seems to be 1 MB,
so I think some DLL function is going to exceed the
128 bytes and it's going to be very hard to figure out.

Please add a

// TODO(rsc,hectorchu): Switch to m stack before call.

and we can do it in a separate change.

Russ

[hector, 2010-01-06 21:21:50]

PTAL

[rsc, 2010-01-07 00:18:48]

LGTM

Thanks again for all the effort and good code.

[rsc, 2010-01-07 01:59:03]

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

Ported runtime to Windows.

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

Committer: Russ Cox <rsc@golang.org>