code review 4961045: windows/386: clean stack after syscall (it is necessary...

windows/386: clean stack after syscall (it is necessary after call cdecl functions and does not have an effect after stdcall)

Result of discussion here: http://groups.google.com/group/golang-nuts/browse_thread/thread/357c806cbb57ca62

[jp, 2011-08-26 05:57:43]

windows/386: clean stack after syscall (it is necessary after call cdecl
functions and does not have an effect after stdcall)

Result of discussion here:
http://groups.google.com/group/golang-nuts/browse_thread/thread/357c806cbb57ca62

All old tests and examples work.

New testcase (does not work before patch and works after):

package main

import "syscall"
import "unsafe"

func main() {
  h, e := syscall.LoadLibrary("user32.dll")
  if e!=0 { panic(""); }
  p, e := syscall.GetProcAddress(h, "wsprintfA")
  if e!=0 { panic(""); }

  var buf [50]byte
  a, _, _ := syscall.Syscall6(uintptr(p),
                             6,
                             uintptr(unsafe.Pointer(&buf[0])),
                             uintptr(unsafe.Pointer(syscall.StringBytePtr("%d
%I64d %d"))),
                             1000, 2000, 3000, 4000)
  if string(buf[:a]) != "1000 12884901890000 4000" {
    panic(buf[:a]);
  }
}

[bradfitz, 2011-08-26 06:00:49]

Why not add that test to a *_test.go file somewhere?

func TestCDecl(t *testing.T) {
....
}

On Fri, Aug 26, 2011 at 9:57 AM, <jp@webmaster.ms> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> windows/386: clean stack after syscall (it is necessary after call cdecl
> functions and does not have an effect after stdcall)
>
> Result of discussion here:
> http://groups.google.com/**group/golang-nuts/browse_**
>
thread/thread/357c806cbb57ca62<http://groups.google.com/group/golang-nuts/browse_thread/thread/357c806cbb57ca62>
>
> All old tests and examples work.
>
> New testcase (does not work before patch and works after):
>
> package main
>
> import "syscall"
> import "unsafe"
>
> func main() {
>  h, e := syscall.LoadLibrary("user32.**dll")
>  if e!=0 { panic(""); }
>  p, e := syscall.GetProcAddress(h, "wsprintfA")
>  if e!=0 { panic(""); }
>
>  var buf [50]byte
>  a, _, _ := syscall.Syscall6(uintptr(p),
>                             6,
>                             uintptr(unsafe.Pointer(&buf[0]**)),
>
> uintptr(unsafe.Pointer(**syscall.StringBytePtr("%d
> %I64d %d"))),
>                             1000, 2000, 3000, 4000)
>  if string(buf[:a]) != "1000 12884901890000 4000" {
>    panic(buf[:a]);
>  }
> }
>
> Description:
> windows/386: clean stack after syscall (it is necessary after call cdecl
> functions and does not have an effect after stdcall)
>
> Result of discussion here:
> http://groups.google.com/**group/golang-nuts/browse_**
>
thread/thread/357c806cbb57ca62<http://groups.google.com/group/golang-nuts/browse_thread/thread/357c806cbb57ca62>
>
> All old tests and examples work.
>
> New testcase (does not work before patch and works after):
>
> package main
>
> import "syscall"
> import "unsafe"
>
> func main() {
>  h, e := syscall.LoadLibrary("user32.**dll")
>  if e!=0 { panic(""); }
>  p, e := syscall.GetProcAddress(h, "wsprintfA")
>  if e!=0 { panic(""); }
>
>  var buf [50]byte
>  a, _, _ := syscall.Syscall6(uintptr(p),
>                             6,
>                             uintptr(unsafe.Pointer(&buf[0]**)),
>
> uintptr(unsafe.Pointer(**syscall.StringBytePtr("%d %I64d %d"))),
>                             1000, 2000, 3000, 4000)
>  if string(buf[:a]) != "1000 12884901890000 4000" {
>    panic(buf[:a]);
>  }
> }
>
> Please review this at
http://codereview.appspot.com/**4961045/<http://codereview.appspot.com/4961045/>
>
> Affected files:
>  M src/pkg/runtime/windows/386/**sys.s
>
>
> Index: src/pkg/runtime/windows/386/**sys.s
> ==============================**==============================**=======
> --- a/src/pkg/runtime/windows/386/**sys.s
> +++ b/src/pkg/runtime/windows/386/**sys.s
> @@ -9,36 +9,32 @@
>        // Copy arguments from stack.
>        MOVL    fn+0(FP), AX
>        MOVL    count+4(FP), CX         // words
> -       MOVL    args+8(FP), BP
> +       MOVL    args+8(FP), DX
>
>        // Switch to m->g0 if needed.
>        get_tls(DI)
> -       MOVL    m(DI), DX
> -       MOVL    m_g0(DX), SI
> +       MOVL    m(DI), BX
> +       MOVL    m_g0(BX), SI
>        CMPL    g(DI), SI
> -       MOVL    SP, BX
> +       MOVL    SP, BX
>        JEQ     2(PC)
>        MOVL    (g_sched+gobuf_sp)(SI), SP
> -       PUSHL   BX
> -       PUSHL   g(DI)
> -       MOVL    SI, g(DI)
> +       XCHGL   SI, g(DI)
>
>        // Copy args to new stack.
> -       MOVL    CX, BX
> -       SALL    $2, BX
> -       SUBL    BX, SP                  // room for args
> -       MOVL    SP, DI
> -       MOVL    BP, SI
> -       CLD
> -       REP; MOVSL
> +       // workaround 6a bug (JCXZ) and "unbalanced PUSH/POP" warning
> +       BYTE $0xe3; BYTE $0x06                         // JCXZ  3(PC)
> +       BYTE $0xff; BYTE $0x74; BYTE $0x8a; BYTE $0xfc // PUSHL
> (-4)(DX)(CX*4)
> +       BYTE $0xe2; BYTE $0xfa;                        // LOOP  -1(PC)
>
> -       // Call stdcall function.
> +       // Call stdcall or cdecl function.
> +       // DI SI BP BX are preserved
>        CALL    AX
>
>        // Restore original SP, g.
> -       get_tls(DI)
> -       POPL    g(DI)
> -       POPL    SP
> +       MOVL    BX, SP
> +       get_tls(BX)
> +       MOVL    SI, g(BX)
>
>        // Someday the convention will be D is always cleared.
>        CLD
>
>
>

[brainman, 2011-08-26 06:30:34]

Why would we need to use user32.dll wsprintfA?

Alex

http://codereview.appspot.com/4961045/diff/2001/src/pkg/runtime/windows/386/s...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/2001/src/pkg/runtime/windows/386/s...
src/pkg/runtime/windows/386/sys.s:31: // DI SI BP BX are preserved
What happens if BX is not preserved by the callee?

[jp, 2011-08-26 06:47:53]

On 2011/08/26 06:30:34, brainman wrote:
> Why would we need to use user32.dll wsprintfA?

Only for the test purposes.
ShellMessageBox() shows pop up
creating own dll adds files.

[jp, 2011-08-26 06:58:24]

http://codereview.appspot.com/4961045/diff/2001/src/pkg/runtime/windows/386/s...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/2001/src/pkg/runtime/windows/386/s...
src/pkg/runtime/windows/386/sys.s:31: // DI SI BP BX are preserved
On 2011/08/26 06:30:34, brainman wrote:
> What happens if BX is not preserved by the callee?

It has to be. With all 386 calling conventions.
http://blogs.msdn.com/b/oldnewthing/archive/2004/01/08/48616.aspx

[jp, 2011-08-26 06:59:38]

On 2011/08/26 06:00:49, bradfitz wrote:
> Why not add that test to a *_test.go file somewhere?
Sure.

[bradfitz, 2011-08-26 08:25:53]

http://codereview.appspot.com/4961045/diff/16007/src/pkg/syscall/syscall_test.go
File src/pkg/syscall/syscall_test.go (right):

http://codereview.appspot.com/4961045/diff/16007/src/pkg/syscall/syscall_test...
src/pkg/syscall/syscall_test.go:21: h, e := syscall.LoadLibrary("user32.dll")
This won't work on Linux because it won't even compile.

You'll need to be put this in a test that only compiles on Windows, named like
syscall_windows_test.go

Then you don't need the runtime.GOOS check, either.

[jp, 2011-08-26 08:45:13]

On 2011/08/26 08:25:53, bradfitz wrote:
>
http://codereview.appspot.com/4961045/diff/16007/src/pkg/syscall/syscall_test.go
> File src/pkg/syscall/syscall_test.go (right):
> 
>
http://codereview.appspot.com/4961045/diff/16007/src/pkg/syscall/syscall_test...
> src/pkg/syscall/syscall_test.go:21: h, e := syscall.LoadLibrary("user32.dll")
> This won't work on Linux because it won't even compile.
> 
> You'll need to be put this in a test that only compiles on Windows, named like
> syscall_windows_test.go
> 
> Then you don't need the runtime.GOOS check, either.

fixed

[hector, 2011-08-26 11:23:15]

http://codereview.appspot.com/4961045/diff/7/src/pkg/runtime/windows/386/sys.s
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/7/src/pkg/runtime/windows/386/sys....
src/pkg/runtime/windows/386/sys.s:28: BYTE $0xe2; BYTE $0xfa;                   
    // LOOP	-1(PC)
Why does this loop need to be rewritten?  I prefer the faster REP MOVS.
Also please no workarounds for assembler bugs and warnings - either write
correct code or fix the assembler.

[jp, 2011-08-26 16:54:54]

http://codereview.appspot.com/4961045/diff/7/src/pkg/runtime/windows/386/sys.s
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/7/src/pkg/runtime/windows/386/sys....
src/pkg/runtime/windows/386/sys.s:28: BYTE $0xe2; BYTE $0xfa;                   
    // LOOP	-1(PC)
> Also please no workarounds for assembler bugs and warnings - either write
correct code or fix the assembler.
Well, it looks pretty easy to fix JCXZ bug (I am going to have a look at 6a),
but how to disable warnings only at the spots where they have to be disabled? 

Do you like the idea of introducing #pragma warning on / #pragma warning off
syntax ?

It is not only spot when I have to work around warnings. 6a (or 6l?) calcs stack
usage (number of PUSH/POP) for all the functions even if they use virtually
unlimited OS thread stack. 
There should be a better way to work around it than BYTE.
Here
http://codereview.appspot.com/4958042/diff/6033/src/pkg/runtime/windows/amd64...
is cannot be easily solved by changing to REP MOVS.

http://codereview.appspot.com/4961045/diff/7/src/pkg/runtime/windows/386/sys....
src/pkg/runtime/windows/386/sys.s:28: BYTE $0xe2; BYTE $0xfa;                   
    // LOOP	-1(PC)
> Why does this loop need to be rewritten?  I prefer the faster REP MOVS.

I tried to use as less registers as possible (windows/amd64 must preserve SI and
DI and here I had PUSHL SI DI BX BP at the beginning and POPL BP BX DI SI at the
end of function, but then removed as the tests passed ok, even with reentering
syscall from callback context).

I am not sure that REP MOVS is faster (especially when we talk about the typical
moving of 1-3 words of call parameters) but required their parameters in SI/DI
registers while we have them in DX/SP. 
Also PUSH/LOOP solves 2 problems at once - stack allocating of CX words (4 extra
commands and extra register usage on the left side) and copying CX words.

So we have 3 cpu commands here in place of former 8 and also do not change SI DI
BX

[rsc, 2011-08-26 17:08:47]

This CL is attempting a bug fix and a cleanup
at the same time.  That's usually a recipe for
making things hard to understand.  Can you please
make this one only about cdecl vs stdcall
(that is, put the REP MOVSL back, don't
rename the registers, etc) and then save
the cleanup for a separate CL + discussion?
Being able to talk about one thing at a time
should speed things up.

Thanks.
Russ

[jp, 2011-08-26 17:36:44]

On 2011/08/26 17:08:47, rsc wrote:
> This CL is attempting a bug fix and a cleanup
> at the same time.  That's usually a recipe for
> making things hard to understand.  Can you please
> make this one only about cdecl vs stdcall
> (that is, put the REP MOVSL back, don't
> rename the registers, etc) and then save
> the cleanup for a separate CL + discussion?
> Being able to talk about one thing at a time
> should speed things up.

You are right, that separate CL depends on 6a fixes while the cdecl issue does
not.

[jp, 2011-08-27 16:46:02]

Reflected changes http://codereview.appspot.com/4926042/

[brainman, 2011-08-29 00:23:40]

http://codereview.appspot.com/4961045/diff/2011/src/pkg/runtime/windows/386/s...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/2011/src/pkg/runtime/windows/386/s...
src/pkg/runtime/windows/386/sys.s:26: // DI SI BP BX are preserved, SP is not
SP will be set appropriately by stdcall function. Can you be certain about BP?

[jp, 2011-08-29 01:06:34]

http://codereview.appspot.com/4961045/diff/2011/src/pkg/runtime/windows/386/s...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/2011/src/pkg/runtime/windows/386/s...
src/pkg/runtime/windows/386/sys.s:26: // DI SI BP BX are preserved, SP is not
On 2011/08/29 00:23:40, brainman wrote:
> SP will be set appropriately by stdcall function. Can you be certain about BP?

SP is preserved only by stdcall.
DI SI BP BX are preserved by both cdecl and stdcall.

Not sure if there is a ISO standard on calling conventions.
For me, blogs on msdn.com and personal experience is enough prove.

[brainman, 2011-08-29 01:55:22]

http://codereview.appspot.com/4961045/diff/2011/src/pkg/runtime/windows/386/s...
File src/pkg/runtime/windows/386/sys.s (right):

http://codereview.appspot.com/4961045/diff/2011/src/pkg/runtime/windows/386/s...
src/pkg/runtime/windows/386/sys.s:26: // DI SI BP BX are preserved, SP is not
On 2011/08/29 01:06:34, jp wrote:
> 
> SP is preserved only by stdcall.
> DI SI BP BX are preserved by both cdecl and stdcall.
> 
> Not sure if there is a ISO standard on calling conventions.
> For me, blogs on http://msdn.com and personal experience is enough prove. 

I am worried about our stdcall callee messing with BP, and then our stack will
be busted. But, fair enough, let's go ahead with this until we find things
different. Please, move your test into runtime, like in your other CL. Thank
you.

[jp, 2011-08-30 00:14:58]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, alex.brainman@gmail.com,
hectorchu@gmail.com, rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[brainman, 2011-08-30 00:57:12]

On 2011/08/30 00:14:58, jp wrote:
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

Your new test fails on my Win2k:

G:\src\pkg\runtime>8.out.exe -test.v
=== RUN  runtime_test.TestSideEffectOrder
--- PASS: runtime_test.TestSideEffectOrder (0.00 seconds)
=== RUN  runtime_test.TestChanSendInterface
--- PASS: runtime_test.TestChanSendInterface (0.00 seconds)
=== RUN  runtime_test.TestPseudoRandomSend
--- PASS: runtime_test.TestPseudoRandomSend (0.00 seconds)
=== RUN  runtime_test.TestStopTheWorldDeadlock
--- PASS: runtime_test.TestStopTheWorldDeadlock (1.30 seconds)
=== RUN  runtime_test.TestFloat64
--- PASS: runtime_test.TestFloat64 (0.45 seconds)
=== RUN  runtime_test.TestCaller
--- PASS: runtime_test.TestCaller (0.00 seconds)
=== RUN  runtime_test.TestStdCall
--- PASS: runtime_test.TestStdCall (0.00 seconds)
=== RUN  runtime_test.TestCDecl
--- FAIL: runtime_test.TestCDecl (0.00 seconds)
        cdecl USER32.wsprintfA returns 14 buf= [49 48 48 48 32 73 54 52 100 32 5
0 48 48 48]
FAIL

[jp, 2011-08-30 01:18:39]

seems %I64d was introduced after win2k
PTAL

On 2011/08/30 00:57:12, brainman wrote:
> On 2011/08/30 00:14:58, jp wrote:
> > 
> > I'd like you to review this change to
> > https://go.googlecode.com/hg/
> 
> Your new test fails on my Win2k:
> 
> G:\src\pkg\runtime>8.out.exe -test.v
> === RUN  runtime_test.TestSideEffectOrder
> --- PASS: runtime_test.TestSideEffectOrder (0.00 seconds)
> === RUN  runtime_test.TestChanSendInterface
> --- PASS: runtime_test.TestChanSendInterface (0.00 seconds)
> === RUN  runtime_test.TestPseudoRandomSend
> --- PASS: runtime_test.TestPseudoRandomSend (0.00 seconds)
> === RUN  runtime_test.TestStopTheWorldDeadlock
> --- PASS: runtime_test.TestStopTheWorldDeadlock (1.30 seconds)
> === RUN  runtime_test.TestFloat64
> --- PASS: runtime_test.TestFloat64 (0.45 seconds)
> === RUN  runtime_test.TestCaller
> --- PASS: runtime_test.TestCaller (0.00 seconds)
> === RUN  runtime_test.TestStdCall
> --- PASS: runtime_test.TestStdCall (0.00 seconds)
> === RUN  runtime_test.TestCDecl
> --- FAIL: runtime_test.TestCDecl (0.00 seconds)
>         cdecl USER32.wsprintfA returns 14 buf= [49 48 48 48 32 73 54 52 100 32
5
> 0 48 48 48]
> FAIL

[brainman, 2011-08-30 04:43:38]

LGTM

[brainman, 2011-08-30 04:44:04]

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

windows/386: clean stack after syscall (it is necessary after call cdecl
functions and does not have an effect after stdcall)

Result of discussion here:
http://groups.google.com/group/golang-nuts/browse_thread/thread/357c806cbb57ca62

R=golang-dev, bradfitz, alex.brainman, hectorchu, rsc
CC=golang-dev
http://codereview.appspot.com/4961045

Committer: Alex Brainman <alex.brainman@gmail.com>