code review 10368043: runtime: do not generate code during runtime in windows...

runtime: do not generate code during runtime in windows NewCallback

Update Issue 5494

[brainman, 2013-06-18 02:44:18]

Hello golang-dev@googlegroups.com,

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

[minux1, 2013-06-18 07:58:24]

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/callback_wi...
File src/pkg/runtime/callback_windows.c (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/callback_wi...
src/pkg/runtime/callback_windows.c:31: WinCallbackContext* c;
for consistency, WinCallbackContext *c;

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows.s
File src/pkg/runtime/sys_windows.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows.s:12: TEXT runtime·callbackasm(SB),7,$0
i guess this big but mechanical source file is better generated by cmd/dist
as zcallback_windows.c??

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:173: ADDL	$4, SP
why not just combine these two as POP AX? i think it makes the intention very
clear.
(of course, you may have to adjust offset of SP below)

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:198: MULL	BX
use SHLL?

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:200: MOVL	(AX), BX
or better, delete code from SUBL $1, AX to ADDL runtime..., AX
and just use something like this:
MOVL runtime·cbctxts+-4(SB)(AX*4), BX

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
File src/pkg/runtime/sys_windows_amd64.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_amd64.s:228: MOVQ	(AX), AX
same

[brainman, 2013-06-19 07:10:13]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-06-19 07:11:25]

Thank you for review.

Alex

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/callback_wi...
File src/pkg/runtime/callback_windows.c (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/callback_wi...
src/pkg/runtime/callback_windows.c:31: WinCallbackContext* c;
On 2013/06/18 07:58:24, minux wrote:
> for consistency, WinCallbackContext *c;

Done.

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows.s
File src/pkg/runtime/sys_windows.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows.s:12: TEXT runtime·callbackasm(SB),7,$0
On 2013/06/18 07:58:24, minux wrote:
> i guess this big but mechanical source file is better generated by cmd/dist
> as zcallback_windows.c??

Done.

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:173: ADDL	$4, SP
Because it does not compile. It fails with:

runtime.callbackasm1: unbalanced PUSH/POP

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:198: MULL	BX
On 2013/06/18 07:58:24, minux wrote:
> use SHLL?

Done.

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:200: MOVL	(AX), BX
You meant "MOVL runtime·cbctxts+-4(SB)(AX*4), AX" instead, aren't you? But even
that does not work. This command looks like this

mov    0x709fdc(,%eax,4),%eax

in gdb, while

(gdb) p/x 'runtime.cbctxts'
$2 = 0x70e444

I think our asm does not work. Or we are using it wrong.

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
File src/pkg/runtime/sys_windows_amd64.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_amd64.s:228: MOVQ	(AX), AX
Used SHLQ here.

[minux1, 2013-06-19 11:03:58]

looks pretty good to me.
please let rsc or iant take a look before submit.

the "MOVL -4(BX)(AX*4), AX" is minor, feel free to ignore it.

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
File src/pkg/runtime/sys_windows_386.s (right):

https://codereview.appspot.com/10368043/diff/5001/src/pkg/runtime/sys_windows...
src/pkg/runtime/sys_windows_386.s:200: MOVL	(AX), BX
On 2013/06/19 07:11:25, brainman wrote:
> You meant "MOVL runtime·cbctxts+-4(SB)(AX*4), AX" instead, aren't you? But
even
> that does not work. This command looks like this
> 
> mov    0x709fdc(,%eax,4),%eax
> 
> in gdb, while
> 
> (gdb) p/x 'runtime.cbctxts'
> $2 = 0x70e444
> 
> I think our asm does not work. Or we are using it wrong.
ah, i see. the cbctxts is a pointer to an array.
so actually, it should be something like:
MOVL runtime·cbctxts(SB), BX
MOVL -4(BX)(AX*4), AX

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:294: if(hasprefix(goos, "windows")) {
s/hasprefix/streq/

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:298: "// license that can be found in the LICENSE
file.\n"
it seems auto generated file doesn't need copyright notices.

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:308: for(i=0; i<2001; i++) {
2001?

anyway, i think we'd better make 2000 a const, and comment
that it should keep in sync with the other 2000 in runtime
source.

or better, also generate a .h to contain that constant,
so that only the cmd/dist contains the constant.

[brainman, 2013-06-20 02:34:11]

On 2013/06/19 11:03:58, minux wrote:
> ...
> ah, i see. the cbctxts is a pointer to an array.
> so actually, it should be something like:
> MOVL runtime·cbctxts(SB), BX
> MOVL -4(BX)(AX*4), AX

Cool. I have changed the code.

Alex

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime.c
File src/cmd/dist/buildruntime.c (right):

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:294: if(hasprefix(goos, "windows")) {
On 2013/06/19 11:03:58, minux wrote:
> s/hasprefix/streq/

Done.

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:298: "// license that can be found in the LICENSE
file.\n"
On 2013/06/19 11:03:58, minux wrote:
> it seems auto generated file doesn't need copyright notices.

Removed copyright.

https://codereview.appspot.com/10368043/diff/11001/src/cmd/dist/buildruntime....
src/cmd/dist/buildruntime.c:308: for(i=0; i<2001; i++) {
On 2013/06/19 11:03:58, minux wrote:
> 2001?

I am not good at math. And I know that. I am always worried that I miscounted
something. So I always try and protected myself, when I can. :-)

> 
> or better, also generate a .h to contain that constant,
> so that only the cmd/dist contains the constant.

Done. Added it to zasm_$GOOS_$GOARCH.h. And included that file in
callback_windows.go.

[brainman, 2013-06-20 02:34:12]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com, rsc@golang.org,
iant@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2013-06-20 02:35:20]

On 2013/06/20 02:34:11, brainman wrote:
> 
> ... And included that file in
> callback_windows.go.

I meant to say callback_windows.c.

Alex

[minux1, 2013-06-20 03:51:53]

Very nice. LGTM. Thank you.
Please let rsc or iant take a look before submitting.

[brainman, 2013-06-20 03:52:33]

On 2013/06/20 03:51:53, minux wrote:
> Please let rsc or iant take a look before submitting.

Will wait. Thank you.

Alex

[rsc, 2013-06-21 18:17:42]

LGTM

Beautiful.

[brainman, 2013-06-24 07:17:54]

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

runtime: do not generate code during runtime in windows NewCallback

Update Issue 5494

R=golang-dev, minux.ma, rsc, iant
CC=golang-dev
https://codereview.appspot.com/10368043