code review 4977044: runtime: implement exception handling on windows/amd64

runtime: implement exception handling on windows/amd64

Fixes issue 2194.

[hector, 2011-08-29 23:03:35]

Hello rsc@golang.org, alex.brainman@gmail.com, vcc.163@gmail.com,
jp@webmaster.ms (cc: golang-dev@googlegroups.com),

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

[jp, 2011-08-29 23:46:40]

On 2011/08/29 23:03:35, hector wrote:

Does it fix http://code.google.com/p/go/issues/detail?id=2194 ?

> Hello mailto:rsc@golang.org, mailto:alex.brainman@gmail.com,
mailto:vcc.163@gmail.com,
> mailto:jp@webmaster.ms (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

[vcc, 2011-08-30 02:28:45]

Very nice, thanks for done this.

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/amd64/asm.s
File src/pkg/runtime/amd64/asm.s (right):

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/amd64/asm.s#n...
src/pkg/runtime/amd64/asm.s:451: CALL	runtime·setosstacklimits(SB)
Should be
#ifdef __WINDOWS__
	CALL	runtime·setosstacklimits(SB)
#endif
? don't bother other OS.

[hector, 2011-08-30 08:11:04]

It does.  I've updated the description that it fixes the issue.

On 30 August 2011 00:46,  <jp@webmaster.ms> wrote:
> On 2011/08/29 23:03:35, hector wrote:
>
> Does it fix http://code.google.com/p/go/issues/detail?id=2194 ?
>
>> Hello mailto:rsc@golang.org, mailto:alex.brainman@gmail.com,
>
> mailto:vcc.163@gmail.com,
>>
>> mailto:jp@webmaster.ms (cc: mailto:golang-dev@googlegroups.com),
>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>
>
>
> http://codereview.appspot.com/4977044/
>

[jp, 2011-08-30 09:31:37]

Thanks!

I would add tests with recover() after nil dereference and division to zero.
Many people is changing these files.
It would be easy to put a regression bug.

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:78: SUBQ	$2304, R10		// add on StackGuard
I would modify mkasmh.sh to have StackGuard in amd64/asm.h
Eventually 2304 here and 2304 in stack.h will go out of sync.

[hector, 2011-08-30 11:21:41]

There are some existing tests that already cover exception handling and
recover.  For example see null.go and recover*.go in the test directory.  So
I don't think it's necessary to add similar tests in runtime pkg.
On Aug 30, 2011 10:31 AM, <jp@webmaster.ms> wrote:
> Thanks!
>
> I would add tests with recover() after nil dereference and division to
> zero.
> Many people is changing these files.
> It would be easy to put a regression bug.
>
>
>
http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
> File src/pkg/runtime/windows/amd64/sys.s (right):
>
>
http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
> src/pkg/runtime/windows/amd64/sys.s:78: SUBQ $2304, R10 // add on
> StackGuard
> I would modify mkasmh.sh to have StackGuard in amd64/asm.h
> Eventually 2304 here and 2304 in stack.h will go out of sync.
>
> http://codereview.appspot.com/4977044/

[jp, 2011-08-30 20:26:52]

Nobody run the tests from test directory on windows.
Tests are skipped.
Then the programmer has an illusion of "ALL TESTS PASSED".

==== src/run.bash ====
[ "$GOHOSTOS" == windows ] ||
(xcd ../test
./run
) || exit $?

echo
echo ALL TESTS PASSED
======================


On 2011/08/30 11:21:41, hector wrote:
> There are some existing tests that already cover exception handling and
> recover.  For example see null.go and recover*.go in the test directory.  So
> I don't think it's necessary to add similar tests in runtime pkg.
> On Aug 30, 2011 10:31 AM, <mailto:jp@webmaster.ms> wrote:
> > Thanks!
> >
> > I would add tests with recover() after nil dereference and division to
> > zero.
> > Many people is changing these files.
> > It would be easy to put a regression bug.
> >
> >
> >
>
http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
> > File src/pkg/runtime/windows/amd64/sys.s (right):
> >
> >
>
http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
> > src/pkg/runtime/windows/amd64/sys.s:78: SUBQ $2304, R10 // add on
> > StackGuard
> > I would modify mkasmh.sh to have StackGuard in amd64/asm.h
> > Eventually 2304 here and 2304 in stack.h will go out of sync.
> >
> > http://codereview.appspot.com/4977044/

[hector, 2011-08-30 21:43:05]

True.  Personally I think we should enable those tests.  With the
exception handling support in place, the only test that fails in that
set is sigchld.go because syscall.Kill and syscall.SIGCHLD aren't
defined.

On 30 August 2011 21:26,  <jp@webmaster.ms> wrote:
> Nobody run the tests from test directory on windows.
> Tests are skipped.
> Then the programmer has an illusion of "ALL TESTS PASSED".
>
> ==== src/run.bash ====
> [ "$GOHOSTOS" == windows ] ||
> (xcd ../test
> ./run
> ) || exit $?
>
> echo
> echo ALL TESTS PASSED
> ======================
>
>
> On 2011/08/30 11:21:41, hector wrote:
>>
>> There are some existing tests that already cover exception handling
>
> and
>>
>> recover.  For example see null.go and recover*.go in the test
>
> directory.  So
>>
>> I don't think it's necessary to add similar tests in runtime pkg.
>> On Aug 30, 2011 10:31 AM, <mailto:jp@webmaster.ms> wrote:
>> > Thanks!
>> >
>> > I would add tests with recover() after nil dereference and division
>
> to
>>
>> > zero.
>> > Many people is changing these files.
>> > It would be easy to put a regression bug.
>> >
>> >
>> >
>
>
http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
>>
>> > File src/pkg/runtime/windows/amd64/sys.s (right):
>> >
>> >
>
>
http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
>>
>> > src/pkg/runtime/windows/amd64/sys.s:78: SUBQ $2304, R10 // add on
>> > StackGuard
>> > I would modify mkasmh.sh to have StackGuard in amd64/asm.h
>> > Eventually 2304 here and 2304 in stack.h will go out of sync.
>> >
>> > http://codereview.appspot.com/4977044/
>
>
>
> http://codereview.appspot.com/4977044/
>

[brainman, 2011-08-30 23:32:50]

On Wednesday, 31 August 2011 07:42:45 UTC+10, Hector Chu wrote:
>
> ...  Personally I think we should enable those tests.  ...


That has been discussed: http://codereview.appspot.com/4777043/

Alex

[brainman, 2011-08-30 23:56:37]

Could you, please, update your CL:

# hg par
changeset:   9601:070b7cc84e48
tag:         tip
# hg clpatch 4977044
codereview issue 4977044 is out of date: patch and recent changes conflict
(ba6daf799367->070b7cc84e48)
#

Thank you.

[jp, 2011-08-31 02:46:39]

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/signal.c (right):

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/signal.c:43: void *p = runtime·sigtramp;
USED(p);
otherwise the compiler warns

[hector, 2011-08-31 09:31:04]

PTAL

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/signal.c (right):

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/signal.c:43: void *p = runtime·sigtramp;
On 2011/08/31 02:46:39, jp wrote:
> USED(p);
> otherwise the compiler warns 

Done.

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/3001/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:78: SUBQ	$2304, R10		// add on StackGuard
On 2011/08/30 09:31:37, jp wrote:
> I would modify mkasmh.sh to have StackGuard in amd64/asm.h
> Eventually 2304 here and 2304 in stack.h will go out of sync.

Done.

[hector, 2011-08-31 18:41:41]

For anyone that has been reviewing the code, I've just uploaded a
small fix for a bug I discovered while testing exceptions inside
callback functions.  Just in case you're unfamiliar with the meaning
of the change, that number specifies the size of the arg block in
bytes, needed when the args are copied to a new split stack in
newstack().

[brainman, 2011-09-01 04:23:43]

Nice. Thank you.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/darwin/amd64...
File src/pkg/runtime/darwin/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/darwin/amd64...
src/pkg/runtime/darwin/amd64/sys.s:297: TEXT runtime·setosstacklimits(SB),7,$0
Maybe define a macro like get_tls, that would be empty for all, but
windows/amd64.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
src/pkg/runtime/windows/amd64/sys.s:73: TEXT runtime·setosstacklimits(SB),7,$0
This sets boundary for all go code OK. What about any dll code that we call?
g_stackbase and g_stackguard might not cover the stack when those functions run.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
src/pkg/runtime/windows/amd64/sys.s:100: TEXT runtime·sigtramp1(SB),0,$24-16
Just an unrelated question. What does "-16" do? I don't think it makes any
difference to the generated code.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
src/pkg/runtime/windows/amd64/sys.s:112: get_tls(CX)
Is it possible that "g" is changed between sigtramp and sigtramp1? Then you
would by updating wrong "g" with exception details. I am, probably, wrong, but I
think I have seen it happened on 386.

[brainman, 2011-09-01 04:25:05]

On 2011/08/31 18:41:41, hector wrote:
> For anyone that has been reviewing the code, I've just uploaded a
> small fix for a bug I discovered while testing exceptions inside
> callback functions.  Just in case you're unfamiliar with the meaning
> of the change, that number specifies the size of the arg block in
> bytes, needed when the args are copied to a new split stack in
> newstack().

I have no idea what you are referring to. ?

Alex

[jp, 2011-09-01 04:55:44]

On 2011/08/31 18:41:41, hector wrote:
> For anyone that has been reviewing the code, I've just uploaded a
> small fix for a bug I discovered while testing exceptions inside
> callback functions.  

Exceptions (or just panic call) inside callback functions deserve to have a
testcase.
On other platforms as well.

[brainman, 2011-09-01 05:12:26]

On 2011/09/01 04:55:44, jp wrote:
> 
> Exceptions (or just panic call) inside callback functions deserve to have a
> testcase.

I see. I have been using a variations of misc/cgo/test/callback.go to test
windows callbacks myself.

Alex

[hector, 2011-09-01 07:28:01]

PTAL

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/darwin/amd64...
File src/pkg/runtime/darwin/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/darwin/amd64...
src/pkg/runtime/darwin/amd64/sys.s:297: TEXT runtime·setosstacklimits(SB),7,$0
On 2011/09/01 04:23:44, brainman wrote:
> Maybe define a macro like get_tls, that would be empty for all, but
> windows/amd64.

Done.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
src/pkg/runtime/windows/amd64/sys.s:73: TEXT runtime·setosstacklimits(SB),7,$0
When we call dll code we go through asmstdcall, which I've modified to call
this.  g will be g0, so g_stackguard and g_stackbase should be in the right
locations and be large enough (64K).

On 2011/09/01 04:23:44, brainman wrote:
> This sets boundary for all go code OK. What about any dll code that we call?
> g_stackbase and g_stackguard might not cover the stack when those functions
run.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
src/pkg/runtime/windows/amd64/sys.s:100: TEXT runtime·sigtramp1(SB),0,$24-16
That number specifies the size of the arg block in bytes, needed when the args
are copied to a new split stack in newstack().  But anyway I've decided to
simplify it and make it nosplit, it will only consume 16 bytes more before the
first chance to split in sighandler.

On 2011/09/01 04:23:44, brainman wrote:
> Just an unrelated question. What does "-16" do? I don't think it makes any
> difference to the generated code.

http://codereview.appspot.com/4977044/diff/15001/src/pkg/runtime/windows/amd6...
src/pkg/runtime/windows/amd64/sys.s:112: get_tls(CX)
In this particular situation I can't see what could change g between sigtramp
and sigtramp1.

On 2011/09/01 04:23:44, brainman wrote:
> Is it possible that "g" is changed between sigtramp and sigtramp1? Then you
> would by updating wrong "g" with exception details. I am, probably, wrong, but
I
> think I have seen it happened on 386.

[rsc, 2011-09-01 19:22:33]

I am a little late to the party here, but is it strictly
necessary to keep setting the os stack limits after
every stack switch?  When this came up originally
(in some other thread, I think with other people),
I suggested trying setting the OS stack limits to
[0, 0xffffffffffff] (that's 48 bits of ff).  Have you tried
that?  A broad setting like that would mean never
having to change it.

[hector, 2011-09-01 20:04:53]

It does appear to work per se, but then we'll need to disable dynamic
stack growth as each time Windows grows the stack it sets the stack
limits.  In pe.c we'll set stack reserve = stack commit, and choose an
appropriate number, let's say 64K (the current reserve = 256K, commit
4K).

On 1 September 2011 20:22, Russ Cox <rsc@golang.org> wrote:
> I am a little late to the party here, but is it strictly
> necessary to keep setting the os stack limits after
> every stack switch?  When this came up originally
> (in some other thread, I think with other people),
> I suggested trying setting the OS stack limits to
> [0, 0xffffffffffff] (that's 48 bits of ff).  Have you tried
> that?  A broad setting like that would mean never
> having to change it.
>

[hector, 2011-09-01 21:28:00]

I've now reimplemented it along the lines suggested below, PTAL.

On 1 September 2011 21:04, Hector Chu <hectorchu@gmail.com> wrote:
> It does appear to work per se, but then we'll need to disable dynamic
> stack growth as each time Windows grows the stack it sets the stack
> limits.  In pe.c we'll set stack reserve = stack commit, and choose an
> appropriate number, let's say 64K (the current reserve = 256K, commit
> 4K).
>
> On 1 September 2011 20:22, Russ Cox <rsc@golang.org> wrote:
>> I am a little late to the party here, but is it strictly
>> necessary to keep setting the os stack limits after
>> every stack switch?  When this came up originally
>> (in some other thread, I think with other people),
>> I suggested trying setting the OS stack limits to
>> [0, 0xffffffffffff] (that's 48 bits of ff).  Have you tried
>> that?  A broad setting like that would mean never
>> having to change it.
>>
>

[brainman, 2011-09-02 04:21:26]

LGTM

http://codereview.appspot.com/4977044/diff/2026/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/4977044/diff/2026/src/cmd/ld/pe.c#newcode645
src/cmd/ld/pe.c:645: set(SizeOfStackReserve, 0x00010000);
Will 10K be enough for our deepest stack ever?
You probably want some warning to future readers why it is done and what the
consequences of changing it.

http://codereview.appspot.com/4977044/diff/2026/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/2026/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:74: PUSHQ	BP			// cdecl
Could put both sigtramp and sigtramp1 into one function. Now that you don't
start sigtramp1 with stack splitting code, it is unclear why these are separate.
Makes me worried, because I don't understand most of runtime code <g>.

http://codereview.appspot.com/4977044/diff/2026/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:232: SUBQ	$(64*1024), AX		// stack size
Might as well change this to ~10K to relate to how your thread stacks are.
Probably should change 386 version too. And for the first thread in
runtime/$GOARCH/asm.s.

[vcc, 2011-09-02 05:17:23]

LGTM

[hector, 2011-09-02 07:09:41]

On 2 September 2011 05:21,  <alex.brainman@gmail.com> wrote:
> http://codereview.appspot.com/4977044/diff/2026/src/cmd/ld/pe.c#newcode645
> src/cmd/ld/pe.c:645: set(SizeOfStackReserve, 0x00010000);
> Will 10K be enough for our deepest stack ever?
0x00010000 is not 10K.  It is 64K, and it should be enough.

[hector, 2011-09-02 07:34:49]

PTAL

http://codereview.appspot.com/4977044/diff/2026/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/4977044/diff/2026/src/cmd/ld/pe.c#newcode645
src/cmd/ld/pe.c:645: set(SizeOfStackReserve, 0x00010000);
On 2011/09/02 04:21:27, brainman wrote:
> Will 10K be enough for our deepest stack ever?
> You probably want some warning to future readers why it is done and what the
> consequences of changing it.

Done.

http://codereview.appspot.com/4977044/diff/2026/src/pkg/runtime/windows/amd64...
File src/pkg/runtime/windows/amd64/sys.s (right):

http://codereview.appspot.com/4977044/diff/2026/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:74: PUSHQ	BP			// cdecl
On 2011/09/02 04:21:27, brainman wrote:
> Could put both sigtramp and sigtramp1 into one function. Now that you don't
> start sigtramp1 with stack splitting code, it is unclear why these are
separate.
> Makes me worried, because I don't understand most of runtime code <g>.

Done.

http://codereview.appspot.com/4977044/diff/2026/src/pkg/runtime/windows/amd64...
src/pkg/runtime/windows/amd64/sys.s:232: SUBQ	$(64*1024), AX		// stack size
On 2011/09/02 04:21:27, brainman wrote:
> Might as well change this to ~10K to relate to how your thread stacks are.
> Probably should change 386 version too. And for the first thread in
> runtime/$GOARCH/asm.s.

0x10000 is 64K in hex

[brainman, 2011-09-03 08:26:56]

On 2011/09/02 07:34:49, hector wrote:
> 
> 0x10000 is 64K in hex

Of course it is. Thank you for your patience :-)

Alex

[brainman, 2011-09-03 08:27:27]

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

runtime: implement exception handling on windows/amd64

Fixes issue 2194.

R=rsc, alex.brainman, vcc.163, jp
CC=golang-dev
http://codereview.appspot.com/4977044

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