code review 132820043: runtime: convert NewCallback and NewCallbackCDecl to Go

runtime: convert NewCallback and NewCallbackCDecl to Go

[brainman, 2014-08-22 05:41:15]

Hello khr@golang.org (cc: golang-codereviews@googlegroups.com),

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

[brainman, 2014-08-22 05:41:36]

I am not sure about two things:

1) I need address of asm runtime.callbackasm in Go. I have written
runtime.callbackasmaddr_m for that. Perhaps there is an easier way;

2) I need to allocate some memory in Go, so I have used runtime.gomallocgc. I
hope it is OK.

Alex

[remyoudompheng, 2014-08-22 06:06:50]

On 2014/08/22 05:41:36, brainman wrote:
> I am not sure about two things:
> 
> 1) I need address of asm runtime.callbackasm in Go. I have written
> runtime.callbackasmaddr_m for that. Perhaps there is an easier way;
> 
> 2) I need to allocate some memory in Go, so I have used runtime.gomallocgc. I
> hope it is OK.
> 
> Alex

For 2), do you have any reason to not use "c := new(wincallbackcontext)" ? Do
you want the allocated memory hidden to the GC ?

[brainman, 2014-08-22 06:15:02]

On 2014/08/22 06:06:50, remyoudompheng wrote:
> 
> ... do you have any reason to not use "c := new(wincallbackcontext)" ? Do
> you want the allocated memory hidden to the GC ?

I don't think so. This memory is used by Go runtime to route callbacks from
windows dlls to correct Go functions. I tried "c := &wincallbackcontext{...}"
first, but I got compiler error saying that c escapes to the heap, and that is
illegal in runtime. I don't know why ...

Alex

[remyoudompheng, 2014-08-22 06:16:29]

2014-08-22 8:15 GMT+02:00  <alex.brainman@gmail.com>:
> On 2014/08/22 06:06:50, remyoudompheng wrote:
>
>> ... do you have any reason to not use "c := new(wincallbackcontext)" ?
>
> Do
>>
>> you want the allocated memory hidden to the GC ?
>
>
> I don't think so. This memory is used by Go runtime to route callbacks
> from windows dlls to correct Go functions. I tried "c :=
> &wincallbackcontext{...}" first, but I got compiler error saying that c
> escapes to the heap, and that is illegal in runtime. I don't know why
> ...
>

The Go dialect used in runtime disallows implicit heap allocation but
IIRC explicit allocation with new or make is allowed.

Rémy.

[brainman, 2014-08-22 07:00:15]

On 2014/08/22 06:16:29, remyoudompheng wrote:
> explicit allocation with new or make is allowed.

Yes, new works. I have changed my code. Thank you.

Alex

[brainman, 2014-08-22 07:00:39]

Hello khr@golang.org, remyoudompheng@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[khr, 2014-08-25 00:39:07]

LGTM.

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/os_windows.c
File src/pkg/runtime/os_windows.c (right):

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/os_window...
src/pkg/runtime/os_windows.c:548: g->m->ptrarg[0] = runtime·callbackasm;
Get rid of this

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/syscall_w...
File src/pkg/runtime/syscall_windows.go (right):

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/syscall_w...
src/pkg/runtime/syscall_windows.go:47: return uintptr(add(addr, uintptr(i*5)))
This is overkill.  You can just do:

var callbackasm byte // type isn't really byte, it's code in runtime

    addr := add(unsafe.Pointer(&callbackasm), uintptr(i*5))

https://codereview.appspot.com/132820043/diff/50001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/132820043/diff/50001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:108: func compileCallback(fn interface{},
cleanstack bool) uintptr
Put a comment here to point a reader to the location of the implementation.

[brainman, 2014-08-25 05:59:22]

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

runtime: convert NewCallback and NewCallbackCDecl to Go

LGTM=khr
R=khr, remyoudompheng
CC=golang-codereviews
https://codereview.appspot.com/132820043

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/os_windows.c
File src/pkg/runtime/os_windows.c (right):

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/os_window...
src/pkg/runtime/os_windows.c:548: g->m->ptrarg[0] = runtime·callbackasm;
On 2014/08/25 00:39:07, khr wrote:
> Get rid of this

Done.

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/syscall_w...
File src/pkg/runtime/syscall_windows.go (right):

https://codereview.appspot.com/132820043/diff/50001/src/pkg/runtime/syscall_w...
src/pkg/runtime/syscall_windows.go:47: return uintptr(add(addr, uintptr(i*5)))
On 2014/08/25 00:39:07, khr wrote:
> This is overkill.  You can just do:
> 
> var callbackasm byte // type isn't really byte, it's code in runtime
> 
>     addr := add(unsafe.Pointer(&callbackasm), uintptr(i*5))

Done.

https://codereview.appspot.com/132820043/diff/50001/src/pkg/syscall/syscall_w...
File src/pkg/syscall/syscall_windows.go (right):

https://codereview.appspot.com/132820043/diff/50001/src/pkg/syscall/syscall_w...
src/pkg/syscall/syscall_windows.go:108: func compileCallback(fn interface{},
cleanstack bool) uintptr
On 2014/08/25 00:39:07, khr wrote:
> Put a comment here to point a reader to the location of the implementation.

Done. Implemented in asm_windows.s.