code review 138250043: syscall: keep Windows syscall pointers live too

syscall: keep Windows syscall pointers live too

Like https://codereview.appspot.com/139360044

[bradfitz, 2014-09-09 00:21:46]

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

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

[rsc, 2014-09-09 00:35:10]

https://codereview.appspot.com/138250043/diff/90001/src/syscall/mksyscall_win...
File src/syscall/mksyscall_windows.go (right):

https://codereview.appspot.com/138250043/diff/90001/src/syscall/mksyscall_win...
src/syscall/mksyscall_windows.go:159: p.fn.use(p.Name)
delete

https://codereview.appspot.com/138250043/diff/90001/src/syscall/mksyscall_win...
src/syscall/mksyscall_windows.go:162: p.fn.use(p.tmpVar())
This is the important one, because the handling of string is what does an
allocation during the wrapper.

https://codereview.appspot.com/138250043/diff/90001/src/syscall/mksyscall_win...
src/syscall/mksyscall_windows.go:167: p.fn.use(p.tmpVar())
delete

https://codereview.appspot.com/138250043/diff/90001/src/syscall/zsyscall_wind...
File src/syscall/zsyscall_windows.go (right):

https://codereview.appspot.com/138250043/diff/90001/src/syscall/zsyscall_wind...
src/syscall/zsyscall_windows.go:242: use(unsafe.Pointer(_p0))
These calls are only necessary for the things that allocate in the same
function.
_p0 and args here are not referring to such allocations and should be omitted.

[bradfitz, 2014-09-09 00:37:12]

PTAL

[rsc, 2014-09-09 00:43:44]

LGTM
Thanks.

[brainman, 2014-09-09 00:44:52]

LGTM

Thank you.

Alex

[bradfitz, 2014-09-09 00:47:12]

*** Submitted as https://code.google.com/p/go/source/detail?r=4eda5e4001fd ***

syscall: keep Windows syscall pointers live too

Like https://codereview.appspot.com/139360044

LGTM=rsc, alex.brainman
R=alex.brainman, rsc
CC=golang-codereviews
https://codereview.appspot.com/138250043

[brainman, 2014-09-09 01:45:33]

Sorry, but I am trying to rebuild go.sys/windows/zsyscall_windows.go (by using
new mksyscall_windows.go) now, and it fails, because we call syscall.use
function that is not available in go.sys/windows. What would be a good
alternative to syscall.use trick, but something that will work anywhere?

Thank you.

Alex

[bradfitz, 2014-09-09 01:46:54]

atomic.StorePointer to a package global variable would work and can't be
optimized away, but it's not ideal.


On Mon, Sep 8, 2014 at 6:45 PM, <alex.brainman@gmail.com> wrote:

> Sorry, but I am trying to rebuild go.sys/windows/zsyscall_windows.go (by
> using new mksyscall_windows.go) now, and it fails, because we call
> syscall.use function that is not available in go.sys/windows. What would
> be a good alternative to syscall.use trick, but something that will work
> anywhere?
>
>
> Thank you.
>
> Alex
>
> https://codereview.appspot.com/138250043/
>

[brainman, 2014-09-09 01:49:04]

On 2014/09/09 01:46:54, bradfitz wrote:
> atomic.StorePointer to a package global variable would work and can't be
> optimized away, but it's not ideal.
> 

Are you going to update sys.go/linux syscalls? You would need some solution
there too. :-)

Alex

[bradfitz, 2014-09-09 01:54:13]

[+r]

On Mon, Sep 8, 2014 at 6:49 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/09 01:46:54, bradfitz wrote:
>
>> atomic.StorePointer to a package global variable would work and can't
>>
> be
>
>> optimized away, but it's not ideal.
>>
>
>
> Are you going to update sys.go/linux syscalls? You would need some
> solution there too. :-)
>

That's Rob's baby. Rob?

[r, 2014-09-09 02:28:14]

It may be my baby but I'm hoping for some co-parenting. If no one else
does it before then, I will push out a fix tomorrow. Should be easy.

-rob


On Mon, Sep 8, 2014 at 6:54 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> [+r]
>
> On Mon, Sep 8, 2014 at 6:49 PM, <alex.brainman@gmail.com> wrote:
>>
>> On 2014/09/09 01:46:54, bradfitz wrote:
>>>
>>> atomic.StorePointer to a package global variable would work and can't
>>
>> be
>>>
>>> optimized away, but it's not ideal.
>>
>>
>>
>> Are you going to update sys.go/linux syscalls? You would need some
>> solution there too. :-)
>
>
> That's Rob's baby. Rob?
>

[bradfitz, 2014-09-09 02:31:18]

It takes a village to curate a syscall package.


On Mon, Sep 8, 2014 at 7:27 PM, Rob Pike <r@golang.org> wrote:

> It may be my baby but I'm hoping for some co-parenting. If no one else
> does it before then, I will push out a fix tomorrow. Should be easy.
>
> -rob
>
>
> On Mon, Sep 8, 2014 at 6:54 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > [+r]
> >
> > On Mon, Sep 8, 2014 at 6:49 PM, <alex.brainman@gmail.com> wrote:
> >>
> >> On 2014/09/09 01:46:54, bradfitz wrote:
> >>>
> >>> atomic.StorePointer to a package global variable would work and can't
> >>
> >> be
> >>>
> >>> optimized away, but it's not ideal.
> >>
> >>
> >>
> >> Are you going to update sys.go/linux syscalls? You would need some
> >> solution there too. :-)
> >
> >
> > That's Rob's baby. Rob?
> >
>

[rsc, 2014-09-09 02:48:25]

Just copy src/syscall/asm.s to go.sys/*

[brainman, 2014-09-09 02:53:45]

On 2014/09/09 02:48:25, rsc wrote:
> Just copy src/syscall/asm.s to go.sys/*

I have a few public and private packages that use mksyscall_windows.go. Should I
have copy of asm.s everywhere?

Alex

[rsc, 2014-09-09 14:54:14]

On Mon, Sep 8, 2014 at 10:53 PM, <alex.brainman@gmail.com> wrote:

> On 2014/09/09 02:48:25, rsc wrote:
>
>> Just copy src/syscall/asm.s to go.sys/*
>>
>
> I have a few public and private packages that use mksyscall_windows.go.
> Should I have copy of asm.s everywhere?
>

For things other than package syscall, my suggestion is to write a typed
wrapper so that the allocation and the call to Syscall are in different
functions. For example, this is one of the current Windows syscall wrappers:

func GetHostByName(name string) (h *Hostent, err error) {
var _p0 *byte
_p0, err = BytePtrFromString(name)
...
r0, _, e1 := Syscall(procgethostbyname.Addr(), 1,
uintptr(unsafe.Pointer(_p0)), 0, 0)
...
}

If you were writing this in your own packages, I would write (or change the
generator to write):

func GetHostByName(name string) (h *Hostent, err error) {
var _p0 *byte
_p0, err = BytePtrFromString(name)
...
return getHostByName(name)
}

func getHostByName(name *byte) (h *Hostent, err error) r0, _, e1 :=
Syscall(procgethostbyname.Addr(), 1, uintptr(unsafe.Pointer(name)), 0, 0)
...
}

This avoids the problem by separating the allocation and the Syscall into
different functions.

The bigger problem here is that func Syscall has the wrong type, and worse
there is no right type. In the long term we are probably going to have to
replace direct uses of Syscall with a more automated solution, similar to
what we did for cgo.

Russ

[brainman, 2014-09-09 23:59:48]

On 2014/09/09 14:54:14, rsc wrote:
> 
> For things other than package syscall, my suggestion is to write a typed
> wrapper so that the allocation and the call to Syscall are in different
> functions. ...

I like your idea. But I don't like idea of maintaining 2 separate
mksyscall_windows.go programs. How about I change mksyscall_windows.go to reject
"string" parameters? I will rewrite 5 syscall functions that fail by hand. We
will also be able to do the same in go.sys/windows. And I will be able to use
same approach in my own packages.

How does that sounds?

Alex

[brainman, 2014-09-17 04:55:48]

On 2014/09/09 14:54:14, rsc wrote:
> ...
> This avoids the problem by separating the allocation and the Syscall into
> different functions.
> 

I have implemented your idea here https://codereview.appspot.com/143160046/

If that is acceptable, I will send undo of CL 138250043 first. Then you can
review the rest properly. Once submitted I will apply it to go.sys/windows too.

What do you think?

Alex

[brainman, 2014-10-01 07:31:15]

On 2014/09/17 04:55:48, brainman wrote:
> On 2014/09/09 14:54:14, rsc wrote:
> > ...
> > This avoids the problem by separating the allocation and the Syscall into
> > different functions.
> > 
> 
> I have implemented your idea here https://codereview.appspot.com/143160046/
> 
> If that is acceptable, I will send undo of CL 138250043 first. Then you can
> review the rest properly. Once submitted I will apply it to go.sys/windows
too.
> 
> What do you think?
> 

ping

[r, 2014-10-01 21:13:50]

Sure. Thanks.

-rob


On Wed, Oct 1, 2014 at 12:31 AM,  <alex.brainman@gmail.com> wrote:
> On 2014/09/17 04:55:48, brainman wrote:
>>
>> On 2014/09/09 14:54:14, rsc wrote:
>> > ...
>> > This avoids the problem by separating the allocation and the Syscall
>
> into
>>
>> > different functions.
>> >
>
>
>> I have implemented your idea here
>
> https://codereview.appspot.com/143160046/
>
>> If that is acceptable, I will send undo of CL 138250043 first. Then
>
> you can
>>
>> review the rest properly. Once submitted I will apply it to
>
> go.sys/windows too.
>
>> What do you think?
>
>
>
> ping
>
> https://codereview.appspot.com/138250043/

[brainman, 2014-10-02 05:44:02]

On 2014/10/01 21:13:50, r wrote:

The undo CL 147440043,

with new changes in 143160046 and go.sys changes to follow.

Alex

[brainman, 2014-10-03 02:10:55]

syscall changes (CL 143160046) are ready for review.

Alex

[brainman, 2014-10-05 23:30:09]

Last change to go.sys/windows https://codereview.appspot.com/152200043

Alex