code review 5440050: syscall: implement Syscall15 (issue 5440050)
alex.b...@gmail.com
Message:
Hello golan...@googlegroups.com (cc: j...@webmaster.ms),
I'd like you to review this change to
https://go.googlecode.com/hg/
Description:
syscall: implement Syscall15
Fixes issue 2251.
Please review this at http://codereview.appspot.com/5440050/
Affected files:
M src/pkg/runtime/windows/amd64/sys.s
M src/pkg/runtime/windows/syscall.goc
M src/pkg/syscall/dll_windows.go
M src/pkg/syscall/mksyscall_windows.pl
Index: src/pkg/runtime/windows/amd64/sys.s
===================================================================
--- a/src/pkg/runtime/windows/amd64/sys.s
+++ b/src/pkg/runtime/windows/amd64/sys.s
@@ -4,7 +4,7 @@
#include "amd64/asm.h"
-#define maxargs 12
+#define maxargs 15
// void runtime·asmstdcall(void *c);
TEXT runtime·asmstdcall(SB),7,$0
Index: src/pkg/runtime/windows/syscall.goc
===================================================================
--- a/src/pkg/runtime/windows/syscall.goc
+++ b/src/pkg/runtime/windows/syscall.goc
@@ -117,3 +117,29 @@
r1 = c.r1;
r2 = c.r2;
}
+
+func Syscall15(fn uintptr, nargs uintptr, a1 uintptr, a2 uintptr, a3
uintptr, a4 uintptr, a5 uintptr, a6 uintptr, a7 uintptr, a8 uintptr, a9
uintptr, a10 uintptr, a11 uintptr, a12 uintptr, a13 uintptr, a14 uintptr,
a15 uintptr) (r1 uintptr, r2 uintptr, err uintptr) {
+ WinCall c;
+
+ USED(a2);
+ USED(a3);
+ USED(a4);
+ USED(a5);
+ USED(a6);
+ USED(a7);
+ USED(a8);
+ USED(a9);
+ USED(a10);
+ USED(a11);
+ USED(a12);
+ USED(a13);
+ USED(a14);
+ USED(a15);
+ c.fn = (void*)fn;
+ c.n = nargs;
+ c.args = &a1;
+ runtime·cgocall(runtime·asmstdcall, &c);
+ err = c.err;
+ r1 = c.r1;
+ r2 = c.r2;
+}
Index: src/pkg/syscall/dll_windows.go
===================================================================
--- a/src/pkg/syscall/dll_windows.go
+++ b/src/pkg/syscall/dll_windows.go
@@ -37,6 +37,7 @@
func Syscall6(trap, nargs, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2
uintptr, err Errno)
func Syscall9(trap, nargs, a1, a2, a3, a4, a5, a6, a7, a8, a9 uintptr)
(r1, r2 uintptr, err Errno)
func Syscall12(trap, nargs, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11,
a12 uintptr) (r1, r2 uintptr, err Errno)
+func Syscall15(trap, nargs, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11,
a12, a13, a14, a15 uintptr) (r1, r2 uintptr, err Errno)
func loadlibrary(filename *uint16) (handle, err Errno)
func getprocaddress(handle uintptr, procname *uint8) (proc uintptr, err
Errno)
@@ -147,6 +148,12 @@
return Syscall12(p.Addr(), uintptr(len(a)), a[0], a[1], a[2], a[3],
a[4], a[5], a[6], a[7], a[8], a[9], a[10], 0)
case 12:
return Syscall12(p.Addr(), uintptr(len(a)), a[0], a[1], a[2], a[3],
a[4], a[5], a[6], a[7], a[8], a[9], a[10], a[11])
+ case 13:
+ return Syscall15(p.Addr(), uintptr(len(a)), a[0], a[1], a[2], a[3],
a[4], a[5], a[6], a[7], a[8], a[9], a[10], a[11], a[12], 0, 0)
+ case 14:
+ return Syscall15(p.Addr(), uintptr(len(a)), a[0], a[1], a[2], a[3],
a[4], a[5], a[6], a[7], a[8], a[9], a[10], a[11], a[12], a[13], 0)
+ case 15:
+ return Syscall15(p.Addr(), uintptr(len(a)), a[0], a[1], a[2], a[3],
a[4], a[5], a[6], a[7], a[8], a[9], a[10], a[11], a[12], a[13], a[14])
default:
panic("Call " + p.Name + " with too many arguments " + itoa(len(a))
+ ".")
}
Index: src/pkg/syscall/mksyscall_windows.pl
===================================================================
--- a/src/pkg/syscall/mksyscall_windows.pl
+++ b/src/pkg/syscall/mksyscall_windows.pl
@@ -190,6 +190,11 @@
while(@args < 12) {
push @args, "0";
}
+ } elsif(@args <= 15) {
+ $asm = "${syscalldot}Syscall15";
+ while(@args < 15) {
+ push @args, "0";
+ }
} else {
print STDERR "$ARGV:$.: too many arguments to system call\n";
}
Russ Cox
alex.b...@gmail.com
http://code.google.com/p/go/source/detail?r=9c49fd8294a5 ***
syscall: implement Syscall15
Fixes issue 2251.
R=golang-dev, rsc
CC=golang-dev, jp
http://codereview.appspot.com/5440050
alex.b...@gmail.com
> ha ha ha LGTM
Well. I really need it: http://goo.gl/H6JaD. And someone got to do it.
Alex
Rob 'Commander' Pike
-rob
alex.b...@gmail.com
> what, no hex?
Do not understand your question. What did I miss, Rob?
Alex
Rob 'Commander' Pike
a8, a9, aA, aB, aC
but please don't do that.
-rob
Evan Shaw
reason for not making Syscall variadic?
That is, if we replace various SyscallX with a single variadic Syscall
function, it will still be about as efficient, right? Especially since
Proc.Call is variadic already.
- Evan
alex.b...@gmail.com
> I might be misremembering, but wasn't lack of escape analysis the
> reason for not making Syscall variadic?
It was memory allocation that we didn't want to pay for the call.
> That is, if we replace various SyscallX with a single variadic Syscall
> function, it will still be about as efficient, right? Especially since
> Proc.Call is variadic already.
You are not suppose to use Proc.Call if you want to be "efficient". We
use it during tests. Something quick and dirty.
Alex
Evan Shaw
> It was memory allocation that we didn't want to pay for the call.
Right, but I don't think variadic calls allocate anymore.
- Evan
Evan Shaw
> Right, but I don't think variadic calls allocate anymore.
...as long as the memory doesn't escape, that is.
- Evan
Rob 'Commander' Pike
Even if they did, by the time we get to 15 arguments, you could argue variadic would do the job. The catch is the difficulty of writing a variadic function in assembler.
-rob
Evan Shaw
> Even if they did, by the time we get to 15 arguments, you could argue variadic would do the job. The catch is the difficulty of writing a variadic function in assembler.
I'm not super familiar with this code, but it looks to me like none of
the SyscallX functions are written in assembly. They're just C
wrappers around runtime·asmstdcall, which is already able to handle a
variable number of arguments.
- Evan
j...@webmaster.ms
http://codereview.appspot.com/5440050/diff/8/src/pkg/runtime/windows/amd64/sys.s
File src/pkg/runtime/windows/amd64/sys.s (right):
http://codereview.appspot.com/5440050/diff/8/src/pkg/runtime/windows/amd64/sys.s#newcode7
src/pkg/runtime/windows/amd64/sys.s:7: #define maxargs 15
Put an even number here.
16, not 15.
Stack frame of API call must be 16-byte aligned.
Although it is OK to leave it unaligned in most cases, sometimes it
causes crash (for example, on calling crypto api functions which uses
XMM registers inside counting on the stack pointer is aligned).
Or, better, make the alignment explicit by putting "AND SP, -15" after
"SUBQ $(maxargs*8), SP"
I am not 100% sure was the alignment issue broken before or after the
patch (as there is also "PUSHQ CX"), but it must be broken in one of two
cases for sure.
alex.b...@gmail.com
> Put an even number here.
> 16, not 15.
http://codereview.appspot.com/5445051
I think it is simple enough, but if you want something different, send a
CL.
Alex
j...@webmaster.ms
I haven't seen that it had been already fixed in another CL.
alex.b...@gmail.com
> Right, but I don't think variadic calls allocate anymore.
I think they do. This program
package main
func printthem(a ...uint32) {
println(a[0])
}
func main() {
printthem(1, 2)
}
compiles into
TEXT main.printthem+0(SB),$8
MOVL 44(FS),CX
MOVL (CX),CX
CMPL SP,(CX)
JHI ,401019
MOVL $0,DX
MOVL $12,AX
CALL ,4019b8+runtime.morestack
SUBL $8,SP
LEAL main.a+12(FP),BX
CMPL 4(BX),$0
JHI ,40102b
CALL ,4094e5+runtime.panicindex
MOVL (BX),BX
MOVL (BX),BP
MOVL $0,CX
MOVL BP,(SP)
MOVL CX,4(SP)
CALL ,4075b4+runtime.printint
CALL ,4077a2+runtime.printnl
ADDL $8,SP
RET ,
TEXT main.main+0(SB),$24
MOVL 44(FS),CX
MOVL (CX),CX
CMPL SP,(CX)
JHI ,40105c
MOVL $0,DX
MOVL $0,AX
CALL ,4019b8+runtime.morestack
SUBL $24,SP
MOVL $8,(SP)
CALL ,4031d9+runtime.new
MOVL 4(SP),CX
LEAL main.statictmp_0005+0(SB),SI
MOVL CX,DI
CLD ,
MOVSL ,
MOVSL ,
MOVL $2,main.autotmp_0004+16(SP)
MOVL $2,main.autotmp_0004+20(SP)
MOVL CX,main.autotmp_0004+12(SP)
LEAL main.autotmp_0004+12(SP),SI
LEAL (SP),DI
CLD ,
MOVSL ,
MOVSL ,
MOVSL ,
CALL ,401000+main.printthem
ADDL $24,SP
RET ,
As you can see, it still calls runtime.new to store printthem arguments
at call site.
Alex
Russ Cox
> Right, but I don't think variadic calls allocate anymore.
A variadic Printf call will not allocate, because the
Go compiler can see Printf's body and see that it
does not retain a reference to the slice. However,
the Go compiler cannot see the body of assembly
functions like Syscall, so that would still allocate.
Russ