code review 5440050: syscall: implement Syscall15

syscall: implement Syscall15

Fixes issue 2251.

[brainman, 2011-11-28 23:17:18]

Hello golang-dev@googlegroups.com (cc: jp@webmaster.ms),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-11-28 23:18:50]

ha ha ha LGTM

[brainman, 2011-11-28 23:24:26]

*** Submitted as 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

[brainman, 2011-11-28 23:27:35]

On 2011/11/28 23:18:50, rsc wrote:
> ha ha ha LGTM

Well. I really need it: http://goo.gl/H6JaD. And someone got to do it.

Alex

[r2, 2011-11-28 23:29:23]

what, no hex?

-rob

[brainman, 2011-11-28 23:33:44]

On 2011/11/28 23:29:23, r2 wrote:
> what, no hex?
> 

Do not understand your question. What did I miss, Rob?

Alex

[r2, 2011-11-28 23:35:47]

On Nov 28, 2011, at 3:33 PM, alex.brainman@gmail.com wrote:

> On 2011/11/28 23:29:23, r2 wrote:
>> what, no hex?
> 
> 
> Do not understand your question. What did I miss, Rob?
> 
> Alex
> 
> http://codereview.appspot.com/5440050/

a8, a9, aA, aB, aC

but please don't do that.

-rob

[eds2, 2011-11-28 23:38:29]

I might be misremembering, but wasn't lack of escape analysis the
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

[brainman, 2011-11-28 23:43:57]

On 2011/11/28 23:38:29, eds2 wrote:
> 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

[eds2, 2011-11-28 23:45:12]

On Tue, Nov 29, 2011 at 12:43 PM,  <alex.brainman@gmail.com> wrote:
> 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

[eds2, 2011-11-28 23:46:09]

On Tue, Nov 29, 2011 at 12:44 PM, Evan Shaw <edsrzf@gmail.com> wrote:
> Right, but I don't think variadic calls allocate anymore.

...as long as the memory doesn't escape, that is.

- Evan

[r2, 2011-11-28 23:46:27]

On Nov 28, 2011, at 3:44 PM, Evan Shaw wrote:

> On Tue, Nov 29, 2011 at 12:43 PM,  <alex.brainman@gmail.com> wrote:
>> It was memory allocation that we didn't want to pay for the call.
> 
> Right, but I don't think variadic calls allocate anymore.

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

[eds2, 2011-11-28 23:58:52]

On Tue, Nov 29, 2011 at 12:46 PM, Rob 'Commander' Pike <r@google.com> wrote:
> 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

[jp, 2011-11-29 05:17:39]

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/sy...
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.

[brainman, 2011-11-29 05:29:15]

On 2011/11/29 05:17:39, jp wrote:
> 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

[jp, 2011-11-29 07:56:07]

Ah, ok.
I haven't seen that it had been already fixed in another CL.

On 2011/11/29 05:29:15, brainman wrote:
> On 2011/11/29 05:17:39, jp wrote:
> > 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

[brainman, 2011-11-30 03:31:35]

On 2011/11/28 23:45:12, eds2 wrote:
> 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

[rsc, 2011-11-30 15:55:08]

On Mon, Nov 28, 2011 at 18:44, Evan Shaw <edsrzf@gmail.com> wrote:
> 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