Issue 3759042: code review 3759042: runtime: windows/amd64 port (Closed)
|
Created:
14 years, 2 months ago by vcc Modified:
13 years, 8 months ago Reviewers:
CC:
rsc, brainman, hector, r2, golang-dev Visibility:
Public. |
Descriptionruntime: windows/amd64 port
Patch Set 1 #Patch Set 2 : code review 3759042: runtime: Windows X64 port. #
Total comments: 12
Patch Set 3 : code review 3759042: runtime: windows/amd64 port #
Total comments: 7
Patch Set 4 : code review 3759042: runtime: windows/amd64 port #
Total comments: 18
Patch Set 5 : code review 3759042: runtime: windows/amd64 port #
Total comments: 12
Patch Set 6 : code review 3759042: runtime: windows/amd64 port #
Total comments: 12
Patch Set 7 : code review 3759042: runtime: windows/amd64 port #Patch Set 8 : code review 3759042: runtime: windows/amd64 port #
Total comments: 4
Patch Set 9 : code review 3759042: runtime: windows/amd64 port #Patch Set 10 : code review 3759042: runtime: windows/amd64 port #Patch Set 11 : code review 3759042: runtime: windows/amd64 port #Patch Set 12 : code review 3759042: runtime: windows/amd64 port #Patch Set 13 : code review 3759042: runtime: windows/amd64 port #
Total comments: 13
Patch Set 14 : diff -r 632f57713423 https://go.googlecode.com/hg/ #Patch Set 15 : diff -r 632f57713423 https://go.googlecode.com/hg/ #Patch Set 16 : diff -r 632f57713423 https://go.googlecode.com/hg/ #
Total comments: 1
MessagesTotal messages: 30
Hello golang-dev@googlegroups.com, I'd like you to review this change.
Sign in to reply to this message.
please change CL description to runtime: windows/amd64 port http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/cgo/windows_a... File src/pkg/runtime/cgo/windows_amd64.c (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/cgo/windows_a... src/pkg/runtime/cgo/windows_amd64.c:39: /* please use tabs for indenting, like in the rest of the file http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/amd64... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/amd64... src/pkg/runtime/windows/amd64/sys.s:92: CMPXCHGL CX, 0(BX) probably CMPXCHGQ http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/amd64... src/pkg/runtime/windows/amd64/sys.s:100: TEXT stdcall_raw00(SB),7,$8 Why not use what the 386 does and pass in a count? MOVQ count+16(FP), R12 SUBQ $0x60, SP CMPQ R12, $1 JLT call MOVQ arg0+24(FP), CX CMPQ R12, $2 JLT call MOVQ arg1+32(FP), DX CMPQ R12, $3 JLT call MOVQ arg2+40(FP), R8 ... You will need to change the call sites to cast their arguments to (uintptr) but that is okay and portable. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/mem.c File src/pkg/runtime/windows/mem.c (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/mem.c... src/pkg/runtime/windows/mem.c:23: #ifdef _64BIT Then there wouldn't be these #ifdefs, which I would like to avoid anyway. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/os.h File src/pkg/runtime/windows/os.h (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/os.h#... src/pkg/runtime/windows/os.h:37: #ifdef _64BIT And this one. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/threa... src/pkg/runtime/windows/thread.c:45: #ifdef _64BIT etc
Sign in to reply to this message.
PTAL http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/cgo/windows_a... File src/pkg/runtime/cgo/windows_amd64.c (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/cgo/windows_a... src/pkg/runtime/cgo/windows_amd64.c:39: /* On 2011/01/19 19:06:57, rsc wrote: > please use tabs for indenting, like in the rest of the file Done. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/amd64... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/amd64... src/pkg/runtime/windows/amd64/sys.s:92: CMPXCHGL CX, 0(BX) On 2011/01/19 19:06:57, rsc wrote: > probably CMPXCHGQ Done. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/amd64... src/pkg/runtime/windows/amd64/sys.s:100: TEXT stdcall_raw00(SB),7,$8 On 2011/01/19 19:06:57, rsc wrote: > Why not use what the 386 does and pass in a count? > > MOVQ count+16(FP), R12 > SUBQ $0x60, SP > > CMPQ R12, $1 > JLT call > MOVQ arg0+24(FP), CX > CMPQ R12, $2 > JLT call > MOVQ arg1+32(FP), DX > CMPQ R12, $3 > JLT call > MOVQ arg2+40(FP), R8 > ... > > You will need to change the call sites to cast > their arguments to (uintptr) but that is okay > and portable. Done. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/mem.c File src/pkg/runtime/windows/mem.c (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/mem.c... src/pkg/runtime/windows/mem.c:23: #ifdef _64BIT On 2011/01/19 19:06:57, rsc wrote: > Then there wouldn't be these #ifdefs, which I would like to avoid anyway. Done. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/os.h File src/pkg/runtime/windows/os.h (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/os.h#... src/pkg/runtime/windows/os.h:37: #ifdef _64BIT On 2011/01/19 19:06:57, rsc wrote: > And this one. Done. http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/thread.c File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/2001/src/pkg/runtime/windows/threa... src/pkg/runtime/windows/thread.c:45: #ifdef _64BIT On 2011/01/19 19:06:57, rsc wrote: > etc Done.
Sign in to reply to this message.
It runs OK here! Please, update to the tip. Thank you. Alex http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:18: SUBQ $8192, AX // stack size s/8192/64*1024/ http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:87: TEXT runtime·casp(SB), 7, $0 386 version of this function lives on runtime/386/asm.s. Please, be consistent - put them both in runtime/$GOARCH/asm.s or both in runtime/windows/$GOARCH/sys.s. http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/thre... File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:112: event = runtime·stdcall(runtime·CreateEvent, 4, (uintptr)0, (uintptr)0, (uintptr)0, (uintptr)0); Russ, (uintptr)0, (uintptr)0, (uintptr)0, (uintptr)0 doesn't look good, and very easy to trip if we forget to cast. Perhaps we better go back to fixed number of args function instead (like syscall package): stdcall(void *fn, void *a1, void *a2, void *a3) and stdcall6(void *fn, void *a1, void *a2, void *a3, void *a4, void *a5, void *a6) http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:198: #define runtime·stdcall_raw runtime·stdcall This is wrong stdcall_raw and stdcall are not equivalent. Their parameter list is different: stdcall_raw has 2, with second parameter being pointer to array of stdcall arguments. You're not calling stdcall_raw yet, so it doesn't break.
Sign in to reply to this message.
PTAL http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:18: SUBQ $8192, AX // stack size On 2011/01/28 06:16:08, brainman wrote: > s/8192/64*1024/ Done. http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:87: TEXT runtime·casp(SB), 7, $0 On 2011/01/28 06:16:08, brainman wrote: > 386 version of this function lives on runtime/386/asm.s. Please, be consistent - > put them both in runtime/$GOARCH/asm.s or both in runtime/windows/$GOARCH/sys.s. move to runtime/amd64/asm.s. http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/thre... File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/12001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:198: #define runtime·stdcall_raw runtime·stdcall On 2011/01/28 06:16:08, brainman wrote: > This is wrong stdcall_raw and stdcall are not equivalent. Their parameter list > is different: stdcall_raw has 2, with second parameter being pointer to array of > stdcall arguments. You're not calling stdcall_raw yet, so it doesn't break. Done.
Sign in to reply to this message.
looks good; please update to incorporate hector's most recent changes. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/signal.c (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/signal.c:59: gp->sigcode1 = info->ExceptionInformation[1]; add gp->sigpc
Sign in to reply to this message.
http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/defs.h (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/defs.h:50: typedef struct Context Context; Was this file really regenerated? These are the x86 registers! http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/signal.c (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/signal.c:8: This is an x86 implementation of exception handling. Please adapt an implementation from e.g. linux/amd64/signal.c or else remove all this code. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:46: MOVQ CX, BX // stdcall frist arg in RCX s/frist/first http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:62: TEXT runtime·gettime(SB),7,$0 I don't think this is quite right... It might be okay if the MOVLs were changed to MOVQs. Better yet, copy over the implementation from x86, it actually does stuff. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:70: TEXT runtime·notok(SB),7,$0 What is this function for? Maybe add a comment. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:77: MOVQ DI, 0x58(GS) fix indent http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:80: // void *stdcall(void *fn, uintptr count, uintptr *args) Hmm, shouldn't it be stdcall(fn, count, ...) http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:100: JLT call I think all these branches are more expensive than unconditionally loading the 4 argument registers. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:114: MOVQ R10, 32(SP) Is there no REP MOVS on x64? http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:241: TEXT runtime·sigtramp(SB),7,$0 Does all this exception handling code actually work? There are significant differences between x86 and x64 exception handling. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/thre... File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:59: env = runtime·stdcall(runtime·GetEnvironmentStringsW, (uintptr)0); Can we not use 0LL?
Sign in to reply to this message.
http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/thre... File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:59: env = runtime·stdcall(runtime·GetEnvironmentStringsW, (uintptr)0); Sorry, disregard my previous suggestion. Another idea would be to get rid of stdcall(fn, nargs, ...) and always use the other one. For example: uintptr args[] = { arg1, arg2 }; stdcall(fn, 2, args); Maybe we could turn this idiom into a macro? You could even calculate the number of args automatically by sizeof(args)/sizeof(uintptr).
Sign in to reply to this message.
On 2011/02/06 16:07:33, hector wrote:
> Another idea would be to get rid of stdcall(fn, nargs, ...) and always use the
> other one. For example:
> uintptr args[] = { arg1, arg2 };
> stdcall(fn, 2, args);
> Maybe we could turn this idiom into a macro? You could even calculate the
> number of args automatically by sizeof(args)/sizeof(uintptr).
Perhaps this http://codereview.appspot.com/4167042/ will be enough.
Alex
Sign in to reply to this message.
PTAL http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/defs.h (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/defs.h:50: typedef struct Context Context; On 2011/02/06 14:34:29, hector wrote: > Was this file really regenerated? These are the x86 registers! removed. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:46: MOVQ CX, BX // stdcall frist arg in RCX On 2011/02/06 14:34:29, hector wrote: > s/frist/first Done. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:62: TEXT runtime·gettime(SB),7,$0 On 2011/02/06 14:34:29, hector wrote: > I don't think this is quite right... > It might be okay if the MOVLs were changed to MOVQs. > Better yet, copy over the implementation from x86, it actually does stuff. I saw you implement gettime in thread.c for x86, I think should work on x64 too, so remove this code. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:77: MOVQ DI, 0x58(GS) On 2011/02/06 14:34:29, hector wrote: > fix indent Done. http://codereview.appspot.com/3759042/diff/24001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:241: TEXT runtime·sigtramp(SB),7,$0 On 2011/02/06 14:34:29, hector wrote: > Does all this exception handling code actually work? > There are significant differences between x86 and x64 exception handling. x64 need rewrite exception handle, remove these code.
Sign in to reply to this message.
http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:72: TEXT runtime·stdcall(SB),7,$8 Shouldn't need this function. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:171: CMPQ R12, $1 Please, don't do conditionals here, just load registers as suggested by Hector. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:185: MOVQ 32(R11), R10 I think, you should be able to use REP; MOVSQ here. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:226: MOVQ m_gostack(DX), SP // restore SP See 386 version of stdcall_raw stack switching code. Recent change by Hector will provide correct number of stdcall parameters, so you should be able to manage stack without saving its position to m.gostack. Please, change accordingly. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/thre... File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:219: // X64 runtime·stdcall and runtime·stdcall_raw define in windows/amd64/sys.s You don't need to have "special" asm 64 bit version of runtime·stdcall. Please, get rid of it. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:225: runtime·stdcall(void *fn, uintptr count, ...) Please, try to implement some of our suggestions for stdcall: Hectors: Another idea would be to get rid of stdcall(fn, nargs, ...) and always use the other one. For example: uintptr args[] = { arg1, arg2 }; stdcall(fn, 2, args); Maybe we could turn this idiom into a macro? You could even calculate the number of args automatically by sizeof(args)/sizeof(uintptr). Or mine: Perhaps this http://codereview.appspot.com/4167042/ will be enough. or explain why you prefer to have this r = (uintptr)runtime·stdcall(runtime·VirtualFree, (uintptr)3, v, (uintptr)0, (uintptr)MEM_RELEASE); or event = runtime·stdcall(runtime·CreateEvent, 4, (uintptr)0, (uintptr)0, (uintptr)0, (uintptr)0); instead.
Sign in to reply to this message.
PTAL Note: for test it, need apply CL :http://codereview.appspot.com/4182061/ http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:72: TEXT runtime·stdcall(SB),7,$8 On 2011/02/16 06:30:07, brainman wrote: > Shouldn't need this function. Done. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:171: CMPQ R12, $1 On 2011/02/16 06:30:07, brainman wrote: > Please, don't do conditionals here, just load registers as suggested by Hector. Done. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:185: MOVQ 32(R11), R10 On 2011/02/16 06:30:07, brainman wrote: > I think, you should be able to use REP; MOVSQ here. Done. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:226: MOVQ m_gostack(DX), SP // restore SP On 2011/02/16 06:30:07, brainman wrote: > See 386 version of stdcall_raw stack switching code. Recent change by Hector > will provide correct number of stdcall parameters, so you should be able to > manage stack without saving its position to m.gostack. Please, change > accordingly. I think X64 SEH different from X86, may not use same mechanism. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/thre... File src/pkg/runtime/windows/thread.c (right): http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:219: // X64 runtime·stdcall and runtime·stdcall_raw define in windows/amd64/sys.s On 2011/02/16 06:30:07, brainman wrote: > You don't need to have "special" asm 64 bit version of runtime·stdcall. Please, > get rid of it. Done. http://codereview.appspot.com/3759042/diff/34001/src/pkg/runtime/windows/thre... src/pkg/runtime/windows/thread.c:225: runtime·stdcall(void *fn, uintptr count, ...) On 2011/02/16 06:30:07, brainman wrote: > Please, try to implement some of our suggestions for stdcall: > > Hectors: > > Another idea would be to get rid of stdcall(fn, nargs, ...) and always use the > other one. For example: > uintptr args[] = { arg1, arg2 }; > stdcall(fn, 2, args); > Maybe we could turn this idiom into a macro? You could even calculate the > number of args automatically by sizeof(args)/sizeof(uintptr). > > > Or mine: > > Perhaps this http://codereview.appspot.com/4167042/ will be enough. > > or explain why you prefer to have this > > r = (uintptr)runtime·stdcall(runtime·VirtualFree, (uintptr)3, v, (uintptr)0, > (uintptr)MEM_RELEASE); > > or > > event = runtime·stdcall(runtime·CreateEvent, 4, (uintptr)0, (uintptr)0, > (uintptr)0, (uintptr)0); > > instead. It's simple, don't need do extra works. we only need take care two files (thread.c and mem.c), so I think is ok now.
Sign in to reply to this message.
On 2011/02/16 16:26:33, vcc wrote: > It's simple, don't need do extra works. we only need take care two files > (thread.c and mem.c), so I think is ok now. It is certainly not ok with me - you have no control / checking of how big your parameters are. Very easy to make mistake that will be impossible to find. In fact, you already had these problems. Alex http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:16: SUBQ $256, AX // just some space for ourselves Shouldn't need that. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:28: MOVQ g_stackbase(DX), SP Shouldn't need this line either. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:89: SUBQ $0x60, SP Shouldn't need this. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:90: MOVQ 0(R11), CX Load these 4 registers last. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:97: SUBQ $4, R12 This is what 386 does here: // Copy args to new stack. MOVL CX, BX SALL $2, BX SUBL BX, SP MOVL SP, DI MOVL BP, SI CLD REP; MOVSL So, I suspect, you should be able to do: SUBQ $4, R12 MOVQ R12, CX MOVQ CX, BX SALQ $3, BX SUBQ BX, SP MOVQ SP, DI MOVQ R11, SI CLD REP; MOVSQ http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:112: get_tls(DI) Should be able to just: get_tls(DI) POPQ g(DI) POPQ SP http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:128: TEXT runtime·getlasterror(SB),7,$0 Where did you get this code?
Sign in to reply to this message.
To help us along: http://blogs.msdn.com/b/oldnewthing/archive/2004/01/14/58579.aspx Alex
Sign in to reply to this message.
PTAL. sorry for delay reply, I'm busy in these days. I found 6c only push two near const int var in one 8 byte, so just only need make (uintptr) in tow near const var, for example: change event = runtime·stdcall(runtime·CreateEvent, 4, 0, 0, 0, 0); to event = runtime·stdcall(runtime·CreateEvent, 4, 0, (uintptr)0, 0, (uintptr)0); works. I think it's ok now, maybe we should change 6c in the future. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:16: SUBQ $256, AX // just some space for ourselves On 2011/02/17 04:52:10, brainman wrote: > Shouldn't need that. Done. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:28: MOVQ g_stackbase(DX), SP On 2011/02/17 04:52:10, brainman wrote: > Shouldn't need this line either. Done. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:90: MOVQ 0(R11), CX On 2011/02/17 04:52:10, brainman wrote: > Load these 4 registers last. Done. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:97: SUBQ $4, R12 On 2011/02/17 04:52:10, brainman wrote: > This is what 386 does here: > > // Copy args to new stack. > MOVL CX, BX > SALL $2, BX > SUBL BX, SP > MOVL SP, DI > MOVL BP, SI > CLD > REP; MOVSL > > So, I suspect, you should be able to do: > > SUBQ $4, R12 > MOVQ R12, CX > MOVQ CX, BX > SALQ $3, BX > SUBQ BX, SP > MOVQ SP, DI > MOVQ R11, SI > CLD > REP; MOVSQ Just copy all args now, sub $4 has problem if args count < 4. http://codereview.appspot.com/3759042/diff/41001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:128: TEXT runtime·getlasterror(SB),7,$0 On 2011/02/17 04:52:10, brainman wrote: > Where did you get this code? Get from debug in GetLastError.
Sign in to reply to this message.
Just some quick notes. I'll look properly later. Alex http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/runtime.h#ne... src/pkg/runtime/runtime.h:247: void* gostack; I still think we do not need that. Let's try and do what 386 version does. http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/signal.c (right): http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/signal.c:1: // Copyright 2009 The Go Authors. All rights reserved. s/2009/2011/ in all new files. http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/windows/os.h File src/pkg/runtime/windows/os.h (right): http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/windows/os.h... src/pkg/runtime/windows/os.h:11: void *runtime·stdcall(void *fn, uintptr count, ...); I'm concerned that this call could interpret variables as 4 bytes or 8 bytes, without us realising which one it is. It would be very hard to find mistakes like these. Let's find something safer.
Sign in to reply to this message.
PTAL need CL <http://codereview.appspot.com/4628064/>
Sign in to reply to this message.
http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/signal.c (right): http://codereview.appspot.com/3759042/diff/45001/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/signal.c:1: // Copyright 2009 The Go Authors. All rights reserved. On 2011/05/23 12:38:50, brainman wrote: > s/2009/2011/ > in all new files. Done.
Sign in to reply to this message.
I'm very happy to see this port moving forward. We need to figure out how to handle the variadic stdcall_raw. Changing the C compiler isn't an option. The two options are: (1) Make it take a fixed number of parameters all with type uintptr. (2) Find every call site and make sure everything that's not a pointer is casted to (uintptr).
Sign in to reply to this message.
On 2011/06/27 20:24:43, rsc wrote: > The two options are: > > (1) Make it take a fixed number of parameters > all with type uintptr. > http://codereview.appspot.com/4648057/ or http://codereview.appspot.com/4167042/ (this one is old, but I can update it) > (2) Find every call site and make sure everything > that's not a pointer is casted to (uintptr). I don't like that option, the "make sure" bit. I always make mistakes and prefer when compiler holds my hand. I feel, it would be very easy to make mistake here, which would be very difficult to find. Alex
Sign in to reply to this message.
>> (1) Make it take a fixed number of parameters >> all with type uintptr. > > http://codereview.appspot.com/4648057/ > or > http://codereview.appspot.com/4167042/ > (this one is old, but I can update it) Hmm. Both of these are pretty ugly. >> (2) Find every call site and make sure everything >> that's not a pointer is casted to (uintptr). > > I don't like that option, the "make sure" bit. I always make mistakes > and prefer when compiler holds my hand. I feel, it would be very easy to > make mistake here, which would be very difficult to find. I agree. Maybe it makes sense to tell the compiler the rules. http://codereview.appspot.com/4634103 Russ
Sign in to reply to this message.
On 28/06/2011, at 9:49 AM, Russ Cox wrote: >>> (1) Make it take a fixed number of parameters >>> all with type uintptr. >> >> http://codereview.appspot.com/4648057/ >> or >> http://codereview.appspot.com/4167042/ >> (this one is old, but I can update it) > > Hmm. Both of these are pretty ugly. > >>> (2) Find every call site and make sure everything >>> that's not a pointer is casted to (uintptr). >> >> I don't like that option, the "make sure" bit. I always make mistakes >> and prefer when compiler holds my hand. I feel, it would be very easy to >> make mistake here, which would be very difficult to find. > > I agree. Maybe it makes sense to tell the compiler the rules. > http://codereview.appspot.com/4634103 sure beats changing character constants to 64 bits. -rob
Sign in to reply to this message.
On 2011/06/27 23:49:45, rsc wrote: > > ... Maybe it makes sense to tell the compiler the rules. > http://codereview.appspot.com/4634103 > Let me try it. Sorry, I'm a slow one here <g>. I'm still not sure, if it is worth complication in compiler to implement one funcion in runtime. Alex
Sign in to reply to this message.
PTAL
Sign in to reply to this message.
http://codereview.appspot.com/3759042/diff/59002/src/cmd/cc/dpchk.c File src/cmd/cc/dpchk.c (right): http://codereview.appspot.com/3759042/diff/59002/src/cmd/cc/dpchk.c#newcode455 src/cmd/cc/dpchk.c:455: if(a->op != OCAST && (a->op != OCONST || !typechl[a->type->etype])) { This shouldn't be necessary. What line of code is triggering this? http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:7: // void tstart(M *newm); Could you please reorder the functions in this file to match the order in windows/386/sys.s? That will make comparing the files easier later. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:13: What about SEH? http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:73: // Switch to m->g0 if needed. Any SEH? http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:85: SUBQ $0x60, SP // Copy args to new stack. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:96: CALL AX // Call stdcall function. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/os.h File src/pkg/runtime/windows/os.h (left): http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/os.h... src/pkg/runtime/windows/os.h:14: void *runtime·stdcall(void *fn, int32 count, ...); I'm not sure why this changed from int32 to uintptr. int32 seems fine - are you expecting to write a call with more than 2 billion arguments?
Sign in to reply to this message.
http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:13: There is no more SEH on 64 bit. Something to start you: http://blogs.msdn.com/b/freik/archive/2006/01/04/509372.aspx For general calling convention on 64 bit: http://blogs.msdn.com/b/oldnewthing/archive/2004/01/14/58579.aspx
Sign in to reply to this message.
PTAL http://codereview.appspot.com/3759042/diff/59002/src/cmd/cc/dpchk.c File src/cmd/cc/dpchk.c (right): http://codereview.appspot.com/3759042/diff/59002/src/cmd/cc/dpchk.c#newcode455 src/cmd/cc/dpchk.c:455: if(a->op != OCAST && (a->op != OCONST || !typechl[a->type->etype])) { On 2011/06/28 12:25:06, rsc wrote: > This shouldn't be necessary. > What line of code is triggering this? if change void *runtime·stdcall(void *fn, int32 count, ...); to void *runtime·stdcall(void *fn, uintptr count, ...); , need this. reverted, don't need. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... File src/pkg/runtime/windows/amd64/sys.s (right): http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:7: // void tstart(M *newm); On 2011/06/28 12:25:06, rsc wrote: > Could you please reorder the functions in this file to > match the order in windows/386/sys.s? That will make > comparing the files easier later. Done. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:85: SUBQ $0x60, SP On 2011/06/28 12:25:06, rsc wrote: > // Copy args to new stack. Done. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/amd6... src/pkg/runtime/windows/amd64/sys.s:96: CALL AX On 2011/06/28 12:25:06, rsc wrote: > // Call stdcall function. Done. http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/os.h File src/pkg/runtime/windows/os.h (left): http://codereview.appspot.com/3759042/diff/59002/src/pkg/runtime/windows/os.h... src/pkg/runtime/windows/os.h:14: void *runtime·stdcall(void *fn, int32 count, ...); On 2011/06/28 12:25:06, rsc wrote: > I'm not sure why this changed from int32 to uintptr. > int32 seems fine - are you expecting to write a call with > more than 2 billion arguments? reverted.
Sign in to reply to this message.
LGTM Leaving for Alex to review + submit. Glad it turned out so simple.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/3759042/diff/58011/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): http://codereview.appspot.com/3759042/diff/58011/src/pkg/runtime/runtime.h#ne... src/pkg/runtime/runtime.h:247: void* gostack; I'm sure we could do without it. But let's leave it for now.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=dd7479dd252a *** runtime: windows/amd64 port R=rsc, alex.brainman, hectorchu, r CC=golang-dev http://codereview.appspot.com/3759042 Committer: Alex Brainman <alex.brainman@gmail.com>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
