code review 562041: syscall: more changes to mingw version

syscall: more changes to mingw version
- mkall now generates syscall stabs automatically
- we can call unicode versions of winapi now
- introduce GetErrstr function to fetch error text given errno
- general cleanup

[brainman, 2010-03-15 04:03:44]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[rsc, 2010-03-16 19:54:16]

http://codereview.appspot.com/562041/diff/2001/3001
File src/pkg/runtime/mingw/syscall.cgo (right):

http://codereview.appspot.com/562041/diff/2001/3001#newcode10
src/pkg/runtime/mingw/syscall.cgo:10: stdcall_and_getlasterr(uintptr *lasterr,
uintptr trap, uintptr a1, uintptr a2, uintptr a3, uintptr a4, uintptr a5,
uintptr a6, uintptr a7)
Change the uintptr *lasterr to a returned value.
Taking the address of variables forces them to be
moved onto the heap, which makes the calls more expensive.

http://codereview.appspot.com/562041/diff/2001/3001#newcode10
src/pkg/runtime/mingw/syscall.cgo:10: stdcall_and_getlasterr(uintptr *lasterr,
uintptr trap, uintptr a1, uintptr a2, uintptr a3, uintptr a4, uintptr a5,
uintptr a6, uintptr a7)
this is a very long name.
how about just stdcall?
(or stdcallerr if stdcall is taken)

http://codereview.appspot.com/562041/diff/2001/3001#newcode29
src/pkg/runtime/mingw/syscall.cgo:29: func WinSyscall(trap uintptr, a1 uintptr,
a2 uintptr, a3 uintptr, a4 uintptr, a5 uintptr, a6 uintptr, a7 uintptr) (r1
uintptr, r2 uintptr, lasterr uintptr) {
Why WinSyscall?  What's the difference with normal Syscall?
If it's just the seventh argument, should be Syscall7.

http://codereview.appspot.com/562041/diff/2001/3002
File src/pkg/syscall/mkall.sh (right):

http://codereview.appspot.com/562041/diff/2001/3002#newcode160
src/pkg/syscall/mkall.sh:160: if [ -n "$mkerrors" ] ; then echo "$mkerrors
|gofmt >zerrors_$GOOSARCH.go" ; fi
drop space before semicolon, twice on this line
and on the next three lines.

http://codereview.appspot.com/562041/diff/2001/3004
File src/pkg/syscall/syscall_mingw.go (right):

http://codereview.appspot.com/562041/diff/2001/3004#newcode50
src/pkg/syscall/syscall_mingw.go:50: // StringUInt16Slice returns a
NUL-terminated slice of uint16
In general the spelling of uppercase uint is Uint, not UInt.

This function should say what it's doing, though.
This is a conversion to UTF-16 (or it should be),
so how about

// StringToUTF16 returns the UTF-16 encoding of the UTF-8 string s,
// with a terminating NUL added.
func StringToUTF16(s string) []uint16 {
    return utf16.Encode([]int(s+"\x00"))
}

// UTF16ToString returns the UTF-8 encoding of the UTF-16 sequence s,
// with a terminating NUL removed.
func UTF16ToString(s []uint16) string {
    if n := len(s); n > 0 && s[n-1] == 0 {
        s = s[0:n-1]
    }
    return string(utf16.Decode(s))
}

I'm sending the utf16 support out for review now.

[brainman, 2010-03-17 03:25:12]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-03-17 03:26:07]

http://codereview.appspot.com/562041/diff/2001/3001
File src/pkg/runtime/mingw/syscall.cgo (right):

http://codereview.appspot.com/562041/diff/2001/3001#newcode10
src/pkg/runtime/mingw/syscall.cgo:10: stdcall_and_getlasterr(uintptr *lasterr,
uintptr trap, uintptr a1, uintptr a2, uintptr a3, uintptr a4, uintptr a5,
uintptr a6, uintptr a7)
stdcallerr it is.

http://codereview.appspot.com/562041/diff/2001/3001#newcode10
src/pkg/runtime/mingw/syscall.cgo:10: stdcall_and_getlasterr(uintptr *lasterr,
uintptr trap, uintptr a1, uintptr a2, uintptr a3, uintptr a4, uintptr a5,
uintptr a6, uintptr a7)
This is "C" code, isn't it? It doesn't move nothing to heap.
And I need stdcallerr to return 2 values: stdcall return value and current
(after stdcall call) GetLastError value. How else would I return 2 values from a
"C" function?

http://codereview.appspot.com/562041/diff/2001/3001#newcode29
src/pkg/runtime/mingw/syscall.cgo:29: func WinSyscall(trap uintptr, a1 uintptr,
a2 uintptr, a3 uintptr, a4 uintptr, a5 uintptr, a6 uintptr, a7 uintptr) (r1
uintptr, r2 uintptr, lasterr uintptr) {
It is 7 argument version of Syscall. And I did think of calling it Syscall7. But
then I looked at other winapi, and some have 8 and more parameters. Until we
know which syscalls we need to implement "os" and "net" packages, we won't be
able to decide how many arguments WinSyscall should have. I didn't want to
pollute syscall name space with Syscall7, Syscall8 and so on, just have
WinSyscall with 7 arguments for now, but will extend later if needs to be.

http://codereview.appspot.com/562041/diff/2001/3002
File src/pkg/syscall/mkall.sh (right):

http://codereview.appspot.com/562041/diff/2001/3002#newcode160
src/pkg/syscall/mkall.sh:160: if [ -n "$mkerrors" ] ; then echo "$mkerrors
|gofmt >zerrors_$GOOSARCH.go" ; fi
On 2010/03/16 19:54:16, rsc wrote:
> drop space before semicolon, twice on this line
> and on the next three lines.
> 

Done.

http://codereview.appspot.com/562041/diff/2001/3004
File src/pkg/syscall/syscall_mingw.go (right):

http://codereview.appspot.com/562041/diff/2001/3004#newcode50
src/pkg/syscall/syscall_mingw.go:50: // StringUInt16Slice returns a
NUL-terminated slice of uint16
It is good with me.

[rsc, 2010-03-17 04:08:30]

> http://codereview.appspot.com/562041/diff/2001/3001#newcode10
> src/pkg/runtime/mingw/syscall.cgo:10: stdcall_and_getlasterr(uintptr
> *lasterr, uintptr trap, uintptr a1, uintptr a2, uintptr a3, uintptr a4,
> uintptr a5, uintptr a6, uintptr a7)
> This is "C" code, isn't it? It doesn't move nothing to heap.
> And I need stdcallerr to return 2 values: stdcall return value and
> current (after stdcall call) GetLastError value. How else would I return
> 2 values from a "C" function?

You just write

func stdcallerr(uintptr trap, uintptr a1, uintptr a2, etc) (uintptr
r1, uintptr lasterr) {
}

and then assign to r1 and lasterr before getting
to the bottom of the function.  When the flow of
control falls off the bottom of the function, cgo will
insert the appropriate code to return the current
values of r1 and lasterr.

> src/pkg/runtime/mingw/syscall.cgo:29: func WinSyscall(trap uintptr, a1
> uintptr, a2 uintptr, a3 uintptr, a4 uintptr, a5 uintptr, a6 uintptr, a7
> uintptr) (r1 uintptr, r2 uintptr, lasterr uintptr) {
> It is 7 argument version of Syscall. And I did think of calling it
> Syscall7. But then I looked at other winapi, and some have 8 and more
> parameters. Until we know which syscalls we need to implement "os" and
> "net" packages, we won't be able to decide how many arguments WinSyscall
> should have. I didn't want to pollute syscall name space with Syscall7,
> Syscall8 and so on, just have WinSyscall with 7 arguments for now, but
> will extend later if needs to be.

There's already Syscall (= Syscall3) and Syscall6.
Maybe make a Syscall9 and leave it at that.
You're still going to have to go back and edit
every call with the WinSyscall approach, if only
to add more zeros.

Russ

[rsc, 2010-03-17 04:09:31]

> You just write
>
> func stdcallerr(uintptr trap, uintptr a1, uintptr a2, etc) (uintptr
> r1, uintptr lasterr) {
> }
>
> and then assign to r1 and lasterr before getting
> to the bottom of the function.  When the flow of
> control falls off the bottom of the function, cgo will
> insert the appropriate code to return the current
> values of r1 and lasterr.

Never mind.  I was confused.  You're right - it's just C code.
Fine as is.

Russ

[brainman, 2010-03-17 04:13:40]

On 2010/03/17 04:08:30, rsc wrote:
> There's already Syscall (= Syscall3) and Syscall6.
> Maybe make a Syscall9 and leave it at that.

Syscall9 with 9 arguments that is.

Alex

[brainman, 2010-03-17 04:19:46]

On unrelated note, I was planning to replace syscall.Errstr() with what new
syscall.GetErrstr() is. Do you think it is OK? Should I do it as part of this
CL? What is the best way doing it, considering, I have to keep current
syscall.Errstr() for all archs, except mingw?

Thank you.

Alex

[brainman, 2010-03-17 04:30:09]

Hello rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-03-17 05:21:28]

On Tue, Mar 16, 2010 at 21:19,  <alex.brainman@gmail.com> wrote:
> On unrelated note, I was planning to replace syscall.Errstr() with what
> new syscall.GetErrstr() is.

Why change it?  What is changing exactly?
Let's leave it for a future CL so that this one can converge
and get submitted.

Russ

[brainman, 2010-03-17 05:58:38]

On 2010/03/17 05:21:28, rsc wrote:
> On Tue, Mar 16, 2010 at 21:19,  <mailto:alex.brainman@gmail.com> wrote:
> > On unrelated note, I was planning to replace syscall.Errstr() with what
> > new syscall.GetErrstr() is.
> 
> Why change it?  What is changing exactly?

Current syscall.GetErrstr() implementation uses array of hardcoded
(zerrors_$GOOS_$GOARCH.go) error messages "errors", the array needs to be
maintained, it gets carried around inside every binary. On the other hand,
syscall.GetErrstr() queries OS for that information. I understand, having making
decision not to use libc, you have no choice of calling similar strerror(3), but
on Windows we're using system dlls anyway, we might as well use what is stored
inside.

What happens if you get error that is not listed in zerrors_$GOOS_$GOARCH.go?
errstr.go returns "error " +  str(errno). syscall.GetErrstr() should always give
you proper description.

> Let's leave it for a future CL so that this one can converge
> and get submitted.

I'm fine with that.


Alex

[rsc, 2010-03-17 06:01:29]

> Current syscall.GetErrstr() implementation uses array of hardcoded
> (zerrors_$GOOS_$GOARCH.go) error messages "errors", the array needs to
> be maintained, it gets carried around inside every binary. On the other
> hand, syscall.GetErrstr() queries OS for that information. I understand,
> having making decision not to use libc, you have no choice of calling
> similar strerror(3), but on Windows we're using system dlls anyway, we
> might as well use what is stored inside.
>
> What happens if you get error that is not listed in
> zerrors_$GOOS_$GOARCH.go? errstr.go returns "error " +  str(errno).
> syscall.GetErrstr() should always give you proper description.

Sounds like what you're calling GetErrstr is just Errstr.
The map exists on Unix because the strings are hard-coded,
not served out of the kernel, but it's fine if the implementation
needs to be different on Windows.  It can still be called Errstr.

Russ

[rsc, 2010-03-17 06:03:41]

LGTM

for a future CL you might consider emitting
shorter calls (to Syscall or Syscall6) when
there are fewer arguments.

russ

[brainman, 2010-03-17 06:09:16]

On 2010/03/17 06:01:29, rsc wrote:

> Sounds like what you're calling GetErrstr is just Errstr.
> The map exists on Unix because the strings are hard-coded,
> not served out of the kernel, but it's fine if the implementation
> needs to be different on Windows.  It can still be called Errstr.

That is exactly what I want to do - change syscall.Errstr() implementation for
Windows only.

My question was, how. I do not want to move code for Errstr() from errstr.go
into every syscall_$GOOS.go. Feels like a lot of repetition.


Alex

[rsc, 2010-03-17 06:10:11]

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

syscall: more changes to mingw version
- mkall now generates syscall stabs automatically
- we can call unicode versions of winapi now
- introduce GetErrstr function to fetch error text given errno
- general cleanup

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

Committer: Russ Cox <rsc@golang.org>

[brainman, 2010-03-17 06:10:47]

> for a future CL you might consider emitting
> shorter calls (to Syscall or Syscall6) when
> there are fewer arguments.

I thought about that for a second, but couldn't work out what good would that
do.

Alex

[rsc, 2010-03-17 06:11:24]

> My question was, how. I do not want to move code for Errstr() from
> errstr.go into every syscall_$GOOS.go. Feels like a lot of repetition.

I think there will probably end up being a syscall_unix.go
that we can add to the GOFILES list for the non-Windows builds.

Russ

[rsc, 2010-03-17 06:11:54]

> I thought about that for a second, but couldn't work out what good would
> that do.

at the least it would avoid a bunch of mov instructions
in the wrappers, making the program smaller.

[brainman, 2010-03-17 06:14:31]

On 2010/03/17 06:11:24, rsc wrote:
> I think there will probably end up being a syscall_unix.go
> that we can add to the GOFILES list for the non-Windows builds.

Something to do for tomorrow <g>.

Alex

[brainman, 2010-03-17 06:15:48]

On 2010/03/17 06:11:54, rsc wrote:
> at the least it would avoid a bunch of mov instructions
> in the wrappers, making the program smaller.
> 

Another one for tomorrow.


Alex